[SourceKit] Make sure we reuse ASTContext in function bodies for solver-based cursor info #62574

ahoppen · 2022-12-14T11:21:43Z

The main problem that prevented us from reusing the ASTContext was that we weren’t remapping the LocToResolve in the temporary buffer that only contains the re-parsed function back to the original buffer. Thus NodeFinder couldn’t find the node that we want to get cursor info for.

Getting AST reuse to work for top-level items is harder because it currently heavily relies on the HasCodeCompletion state being set on the parser result. I’ll try that in a follow-up PR.

rdar://103251263

ahoppen · 2022-12-15T20:21:03Z

The SourceKit failure ResultTests.swift:concurrent-834:710 is an improvement. In the following reduced reproducer the solver-based implementation is correctly reporting the shorthand shadow. The AST-based implementation is not

extension Invalid {
  func tryIsSuccess(_ text: String?) {
    // RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):15 %s -- %s
    guard let text else {}
  }
}

ahoppen · 2022-12-15T22:21:00Z

@swift-ci Please SourceKit stress test

ahoppen · 2022-12-16T10:22:02Z

@swift-ci Please SourceKit stress test

rintaro

The divergence between "cursor info" and other requests in IDEInspectionInstance is getting bigger.

Existence of code completion token
Callbacks requirement
IgnoreSwiftSourceInfo requirement
SourceLoc requirements

I feel passing different constant arguments depending on the request kind is not great for code readability.

We might want to reconsider whether we should continue to use a IDEInspectionInstance for everything. Maybe it would be another IDEInspectionInstance instance with different option, or different "cursor info instance" sharing some code (e.g. dependency checking) with IDEInspectionInstance.

rintaro · 2022-12-21T03:05:29Z

lib/IDETool/IDEInspectionInstance.cpp

+    MapSourceLocIntoOriginalBuffer = [&SM, startOffset, tmpBufferID,
+                                      newBufferID](SourceLoc Loc) -> SourceLoc {
+      if (SM.findBufferContainingLoc(Loc) == newBufferID) {
+        return SM.getLocForOffset(tmpBufferID,
+                                  SM.getLocOffsetInBuffer(Loc, newBufferID) +
+                                      startOffset);
+      } else {
+        return Loc;
+      }
+    };
+


AFAICT, tmpBufferID is a buffer ID in tmpSM, not in SM. So I believe SM.getLocForOffset(tmpBufferID, ...) is not correct. This PR currently happens to work probably because the bufferID is the same 1 and the content is the same because there's no edits between the requests.

Also, I think this whole SourceLoc conversion mechanism should be handled in SourceManager.
I think we need to:
a) get offset, line, and column in the original buffer
b) compare source loc between beyond buffer IDs.

For (a) line and columns can be retrieved by getPresumedLineAndColumnForLoc(). For offset, I would add an optional parameter to SourceManager::openVirtualFile() like offsetOffset and remember it in VirtualFiles, then create SourceManager::getPresumedLocOffsetInBuffer() to retrieve the adjusted "offset".

For (b), SourceManager knows the replaced source ranges, in ReplacedRanges, so it should be able to "compare" the locations beyond source buffer if the loc is a part of ReplacedRanges (like ASTScope does).

Could you explore this option? I feel passing around this MapSourceLocIntoOriginalBuffer is a too much burden for this purpose.

This assures stable sort order when comparing solver-based and AST-based results.

…quest` The self nominal type decl needs to be passed as a parameter in here so this request doesn't tigger self nominal type computation, which results in an assertion failure.

…ursor info

…entations

…ed cursor info

…mmand Otherwise the order of sourcekitd-test’s output and the shell command is undefined.

If we cache the buffers between multiple requests we don’t pick up if the source file has changed on disk since we last read it.

…er-based cursor info The main problem that prevented us from reusing the ASTContext was that we weren’t remapping the `LocToResolve` in the temporary buffer that only contains the re-parsed function back to the original buffer. Thus `NodeFinder` couldn’t find the node that we want to get cursor info for. Getting AST reuse to work for top-level items is harder because it currently heavily relies on the `HasCodeCompletion` state being set on the parser result. I’ll try that in a follow-up PR. rdar://103251263

ahoppen · 2023-02-03T20:11:49Z

@swift-ci Please smoke test

ahoppen · 2023-02-03T20:15:40Z

@swift-ci Please SourceKit stress test

ahoppen · 2023-02-03T20:18:19Z

@swift-ci Please SourceKit stress test

ahoppen · 2023-02-04T07:51:27Z

Note to self: All cursor info verification failures in the stress tester are improvement. Most of them are because we now correctly return an optional and a non-optional result for an incomplete if let. Others are cases where the USR previously contained error types but no longer does.

…igned to values in completion-like cursor info

ahoppen · 2023-02-04T07:52:04Z

@swift-ci Please smoke test

ahoppen requested a review from rintaro December 14, 2022 11:21

ahoppen force-pushed the ahoppen/reuse-ast-context branch from daa2704 to b204f2f Compare December 14, 2022 11:22

ahoppen force-pushed the ahoppen/reuse-ast-context branch 3 times, most recently from 48821d8 to 65fc90f Compare December 15, 2022 22:20

This was referenced Dec 16, 2022

[SourceKit] Retrieve swiftsourcedocinfo for code completion request #62636

Merged

[Sema] Teach typeCheckASTNodeAtLoc to check StmtConditionElement #62637

Merged

rintaro requested changes Dec 21, 2022

View reviewed changes

ahoppen force-pushed the ahoppen/reuse-ast-context branch from 92b18ea to f48c64f Compare January 30, 2023 16:06

ahoppen requested review from hborla, slavapestov and xedin as code owners January 30, 2023 16:06

ahoppen force-pushed the ahoppen/reuse-ast-context branch from f48c64f to be0b18d Compare January 30, 2023 22:48

rintaro approved these changes Jan 30, 2023

View reviewed changes

xedin removed their request for review February 1, 2023 21:51

ahoppen force-pushed the ahoppen/reuse-ast-context branch from 44bd218 to 9e745ed Compare February 2, 2023 12:13

ahoppen added 9 commits February 2, 2023 22:03

[SourceKit] Sort ReferencedDecls when printing them

04d44bf

This assures stable sort order when comparing solver-based and AST-based results.

[SourceKit] Disable completion-like cursor info for ParamDecls

e47aea4

[Sema] Pass self nominal type as a parameter to `CheckRedeclarationRe…

ec1ca7f

…quest` The self nominal type decl needs to be passed as a parameter in here so this request doesn't tigger self nominal type computation, which results in an assertion failure.

[IDE] Type check property wrappers so their USRs can be reported in c…

846a39d

…ursor info

[SymbolGraph] Sort edges to get consistent symbol graph string repres…

2fb487f

…entations

[SourceKit] Trigger generation of synthesized accessors in solver-bas…

1ed94ff

…ed cursor info

[sourcekitd-test] Flush stdout and stderr before executing a shell co…

d87700d

…mmand Otherwise the order of sourcekitd-test’s output and the shell command is undefined.

[sourcekitd-test] Don't cache memory buffers

ac3f9f8

If we cache the buffers between multiple requests we don’t pick up if the source file has changed on disk since we last read it.

ahoppen force-pushed the ahoppen/reuse-ast-context branch from 9e745ed to d18aca8 Compare February 3, 2023 20:11

[CursorInfo] Fix a bug that caused incorrect discriminators to be ass…

9dfeb0a

…igned to values in completion-like cursor info

ahoppen force-pushed the ahoppen/reuse-ast-context branch from d18aca8 to 9dfeb0a Compare February 4, 2023 07:51

ahoppen merged commit 3c67b06 into swiftlang:main Feb 6, 2023

ahoppen deleted the ahoppen/reuse-ast-context branch February 6, 2023 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SourceKit] Make sure we reuse ASTContext in function bodies for solver-based cursor info #62574

[SourceKit] Make sure we reuse ASTContext in function bodies for solver-based cursor info #62574

Uh oh!

ahoppen commented Dec 14, 2022 •

edited

Loading

Uh oh!

ahoppen commented Dec 15, 2022

Uh oh!

ahoppen commented Dec 15, 2022

Uh oh!

ahoppen commented Dec 16, 2022

Uh oh!

rintaro left a comment •

edited

Loading

Uh oh!

rintaro Dec 21, 2022 •

edited

Loading

Uh oh!

ahoppen commented Feb 3, 2023

Uh oh!

ahoppen commented Feb 3, 2023

Uh oh!

ahoppen commented Feb 3, 2023

Uh oh!

ahoppen commented Feb 4, 2023

Uh oh!

ahoppen commented Feb 4, 2023

Uh oh!

Uh oh!

[SourceKit] Make sure we reuse ASTContext in function bodies for solver-based cursor info #62574

[SourceKit] Make sure we reuse ASTContext in function bodies for solver-based cursor info #62574

Uh oh!

Conversation

ahoppen commented Dec 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Dec 15, 2022

Uh oh!

ahoppen commented Dec 15, 2022

Uh oh!

ahoppen commented Dec 16, 2022

Uh oh!

rintaro left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rintaro Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Feb 3, 2023

Uh oh!

ahoppen commented Feb 3, 2023

Uh oh!

ahoppen commented Feb 3, 2023

Uh oh!

ahoppen commented Feb 4, 2023

Uh oh!

ahoppen commented Feb 4, 2023

Uh oh!

Uh oh!

ahoppen commented Dec 14, 2022 •

edited

Loading

rintaro left a comment •

edited

Loading

rintaro Dec 21, 2022 •

edited

Loading