[IDE] Implement completion-like cursor info for ValueDecls #62362

ahoppen · 2022-12-02T11:42:35Z

This brings up the ability to compute cursor info results using the completion-like type checking paradigm, which an reuse ASTContexts and doesn’t need to type check the entire file.

For now, the new implementation only supports cursor info on ValueDecls (not on references) because they were easiest to implement. More cursor info kinds are coming soon.

At the moment, we only run the new implementation in a verification mode: It is only invoked in assert toolchains and when run, we check that the results are equivalent to the old implementation. Once more cursor info kinds are implemented and if the SourceKit stress tester doesn’t find any verification issues, we can enable the new implementation, falling back to the old implementation if the new one didn’t produce any results.

ahoppen · 2022-12-06T12:03:53Z

@swift-ci Please smoke test macOS

ahoppen · 2022-12-06T12:03:59Z

@swift-ci Please SourceKit stress test

ahoppen · 2022-12-07T14:33:41Z

@swift-ci Please smoke test macOS

ahoppen · 2022-12-07T14:33:48Z

@swift-ci Please SourceKit stress test

rintaro · 2022-12-07T17:35:08Z

tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp

-  return Impl.getMemoryBuffer(Filename, FileSystem, Error);
+  return Impl.getFileContent(Filename, /*IsPrimary=*/false, FileSystem, Error)
+      .Buffer;


Implementation has both getMemoryBuffer and getFileContent, but the public interface only has getMemoryBuffer and it calls Implementation::getFileContent. How confusing 😬

Yes, I know. But 🤷

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

rintaro · 2022-12-07T17:46:56Z

lib/IDE/CursorInfo.cpp

+
+/// Walks the AST, looking for a node at \c LocToResolves. While walking the
+/// AST, also gathers information about shorthand shadows.
+class NodeFinder : ASTWalker {


I feel we have too many similar "finder" like this. E.g. ContextFinder in Refactoring.cpp. It'd be great we can consolidate them. But that doesn't block this PR.

I’ll see what I can do in a follow-up PR.

Yeah, IMO ideally we'd do what Clang does here - ie. build up a hierarchy to the deepest node and pass that around everywhere we need it (rather than continually performing the same ASTWalk skipping outside-of-loc ranges in basically every refactoring + cursor info).

I filed rdar://103178329 so we don’t forget about it and hope that I’ll get to it at some point.

lib/IDE/CursorInfo.cpp

test/SourceKit/CursorInfo/completion_like_cursor_with_edit.swift

test/SourceKit/CursorInfo/cursor_in_global_closure.swift

test/SourceKit/CursorInfo/cursor_uses_swiftsourceinfo.swift

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

bnbarham · 2022-12-08T00:29:35Z

lib/IDETool/CompletionInstance.cpp

@@ -794,3 +794,70 @@ void swift::ide::CompletionInstance::conformingMethodList(
            Callback);
      });
 }
+
+void swift::ide::CompletionInstance::cursorInfo(


Little weird it's a part of CompletionInstance 😅.

Yes, we should rename CompletionInstance. But I didn’t know how else to name it.

lib/IDE/CursorInfo.cpp

bnbarham · 2022-12-08T00:44:03Z

lib/IDE/CursorInfo.cpp

+
+/// Walks the AST, looking for a node at \c LocToResolves. While walking the
+/// AST, also gathers information about shorthand shadows.
+class NodeFinder : ASTWalker {


Yeah, IMO ideally we'd do what Clang does here - ie. build up a hierarchy to the deepest node and pass that around everywhere we need it (rather than continually performing the same ASTWalk skipping outside-of-loc ranges in basically every refactoring + cursor info).

bnbarham · 2022-12-08T00:48:02Z

test/SourceKit/CursorInfo/cursor_in_pound_if.swift

+  let xxx = "hello"
+#endif
+}
+// TODO: Once we switch to use the solver-based cursor info implementation, we also receive results for the int case


Does it handle unparsed #if regions?

Yes, it just fell out of the implementation that it does. I think we just type check whatever decl the cursor info points to. I’m not entirely sure how well it works if the decl you invoke cursor info on references other variables in a #if though.

bnbarham · 2022-12-08T00:53:27Z

test/SourceKit/CursorInfo/cursor_uses_swiftsourceinfo.swift

+// RUN: %{python} %utils/split_file.py -o %t/split %s
+// RUN: %empty-directory(%t/build)
+// RUN: %target-swift-frontend -emit-module -module-name MyModule -emit-module-path %t/build/MyModule.swiftmodule -emit-module-source-info-path %t/build/MyModule.swiftsourceinfo %t/split/Action.swift
+// RUN: %sourcekitd-test -req=cursor -req-opts=retrieve_symbol_graph=1 -pos=5:14 %t/split/test.swift -- %t/split/test.swift -I %t/build | %FileCheck %s


Any reason to include the symbol graph here rather than just doing cursor info on Action and checking the file/line/col? I would have thought we have a check for that already though, do we really not?

Because AFAICT symbol graph causes REFERENCED DECLS to be returned and that’s what references the source file from .swiftsourceinfo.

Doing a cursor info on Action should also end up using the .swiftsourceinfo. REFERENCED DECLS does as well because the decl is actually the type of the decl you're doing cursor info on. But as I said, I'm very surprised we don't already have a test for that.

For reasons I don’t fully understand (I haven’t really done any investigation though) we report the correct source location of Action even when we’re not using .swiftsourceinfo.

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

This brings up the ability to compute cursor info results using the completion-like type checking paradigm, which an reuse ASTContexts and doesn’t need to type check the entire file. For now, the new implementation only supports cursor info on `ValueDecl`s (not on references) because they were easiest to implement. More cursor info kinds are coming soon. At the moment, we only run the new implementation in a verification mode: It is only invoked in assert toolchains and when run, we check that the results are equivalent to the old implementation. Once more cursor info kinds are implemented and if the SourceKit stress tester doesn’t find any verification issues, we can enable the new implementation, falling back to the old implementation if the new one didn’t produce any results.

…ming the second code completion pass Otherwise, the closure discriminator will be incremented by one when the closure witht he code completion token is parsed a second time during the second pass, and thus it would receive a different discriminator during the second pass.

Cursor infor needs source info for referenced decls.

ahoppen · 2022-12-08T14:13:18Z

Note to self: The stress tester failure at offset 300 of modification concurrent-359 of rows/MovieRow.swift has the following reduce reproducer:

// RUN: %sourcekitd-test -req=cursor -req-opts=retrieve_symbol_graph=1 -pos=5:12 %s -- %s

let formatter: DateFormatter = {
  struct MovieRow {
    struct Props

The solver-based implementation reports Props as conforming to Sendable in symbol graph, the AST-based implementation does not report this. I think this change is acceptable and I’ll just mark this discrepancy as an XFail in the stress tester.

ahoppen · 2022-12-08T14:40:10Z

@swift-ci Please SourceKit stress test

ahoppen · 2022-12-09T10:38:13Z

@swift-ci Please SourceKit stress test

ahoppen · 2022-12-09T10:38:18Z

@swift-ci Please smoke test

ahoppen · 2022-12-09T17:35:27Z

@swift-ci Please smoke test macOS

…ursorInfo The lexer will be responsible for knowing whether we have a code completion token, everything else will also work for other IDE inspection features. The changes start to really make sense once I rename CodeCompletion -> IDEInspection in a lot of places.

ahoppen · 2022-12-10T11:19:54Z

@swift-ci Please SourceKit stress test

ahoppen · 2022-12-10T11:20:03Z

@swift-ci Please smoke test

ahoppen requested a review from rintaro December 2, 2022 11:42

ahoppen force-pushed the ahoppen/completion-like-cursor-info branch 2 times, most recently from 99ca5d0 to 66ae9bd Compare December 6, 2022 12:03

ahoppen force-pushed the ahoppen/completion-like-cursor-info branch from 66ae9bd to 2820645 Compare December 6, 2022 23:26

ahoppen requested review from bnbarham, CodaFi and DougGregor as code owners December 7, 2022 14:33

rintaro reviewed Dec 7, 2022

View reviewed changes

bnbarham reviewed Dec 8, 2022

View reviewed changes

ahoppen added 4 commits December 8, 2022 14:39

[IDE] Run RecontextualizeClosures in TypeCheckASTNodeAtLocRequest

0dfcf59

[SourceKit] Don't ignore swiftsourceinfo for solver-based cursor info

e777d37

Cursor infor needs source info for referenced decls.

ahoppen mentioned this pull request Dec 8, 2022

[SR-8898] CodeCompletion: Forward references do not show up in top-level module scope #51404

Open

ahoppen force-pushed the ahoppen/completion-like-cursor-info branch from 3b52924 to e777d37 Compare December 8, 2022 14:40

This was referenced Dec 9, 2022

[CursorInfo] Implement a few expression references as solver-based #62478

Merged

[IDE] Skip more nodes in CursorInfo’s NodeFinder #62479

Draft

ahoppen force-pushed the ahoppen/completion-like-cursor-info branch from 2e9ebf9 to 9a0a772 Compare December 10, 2022 11:19

ahoppen merged commit d9382b1 into swiftlang:main Dec 12, 2022

Azoy mentioned this pull request Dec 12, 2022

Revert "[IDE] Implement completion-like cursor info for ValueDecls" #62534

Closed

[IDE] Implement completion-like cursor info for ValueDecls #62362

[IDE] Implement completion-like cursor info for ValueDecls #62362

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ahoppen commented Dec 6, 2022

Uh oh!

ahoppen commented Dec 6, 2022

Uh oh!

ahoppen commented Dec 7, 2022

Uh oh!

ahoppen commented Dec 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented Dec 8, 2022

Uh oh!

ahoppen commented Dec 8, 2022

Uh oh!

ahoppen commented Dec 9, 2022

Uh oh!

ahoppen commented Dec 9, 2022

Uh oh!

ahoppen commented Dec 9, 2022

Uh oh!

ahoppen commented Dec 10, 2022

Uh oh!

ahoppen commented Dec 10, 2022

Uh oh!

Uh oh!

ahoppen commented Dec 2, 2022 •

edited

Loading