Skip to content

[SourceKit] Add a request to get the semantic tokens of a file #69294

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 24, 2023

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Oct 19, 2023

We need this request for semantic highlighting in LSP. Previously, we were getting the semantic tokens using a 0,0 edit after a document update notification but that will no longer be possible if we open the documents in syntactic only mode.

@ahoppen
Copy link
Member Author

ahoppen commented Oct 19, 2023

@swift-ci Please smoke test

@ahoppen
Copy link
Member Author

ahoppen commented Oct 19, 2023

@swift-ci Please build toolchain macOS

@ahoppen
Copy link
Member Author

ahoppen commented Oct 20, 2023

@swift-ci Please smoke test

@ahoppen ahoppen force-pushed the ahoppen/semantic-tokens branch from 8441396 to e543961 Compare October 20, 2023 17:36
We need this request for semantic highlighting in LSP. Previously, we were getting the semantic tokens using a 0,0 edit after a document update notification but that will no longer be possible if we open the documents in syntactic only mode.
@ahoppen ahoppen force-pushed the ahoppen/semantic-tokens branch from e543961 to 4bc03f8 Compare October 20, 2023 17:38
@ahoppen
Copy link
Member Author

ahoppen commented Oct 20, 2023

@swift-ci Please smoke test

@@ -0,0 +1,17 @@
// RUN: %sourcekitd-test -req=semantic-tokens %s -- %s | %FileCheck %s

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we get semantic tokens of buffers within a primary file?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do now. Added 1776e37

@ahoppen
Copy link
Member Author

ahoppen commented Oct 21, 2023

@swift-ci Please smoke test

// IN_BUFFER: source.lang.swift.ref.struct
// IN_BUFFER: key.kind: source.lang.swift.ref.class

// Check that we don't get semantic tokens inside the buffer when requesting semantic tokens for the outer type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Check that we don't get semantic tokens inside the buffer when requesting semantic tokens for the outer type
// Check that we don't get semantic tokens inside the buffer when requesting semantic tokens for the outer file

Comment on lines +2564 to +2566
if (!InvocationError.empty()) {
LOG_WARN_FUNC("error creating ASTInvocation: " << InvocationError);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this actually show up? Is it ever more obvious than just the actual error response?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@hamishknight hamishknight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo the below comment

Comment on lines 611 to 620
} else if (auto ME = dyn_cast<MacroExpansionExpr>(E)) {
// Add a reference to the macro
auto macroRef = ME->getMacroRef();
if (auto *macroDecl = dyn_cast_or_null<MacroDecl>(macroRef.getDecl())) {
auto macroRefType = macroDecl->getDeclaredInterfaceType();
if (!passReference(
macroDecl, macroRefType, ME->getMacroNameLoc(),
ReferenceMetaData(SemaReferenceKind::DeclRef, llvm::None)))
return Action::Stop();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why remove this? Wouldn't this impact things like cursor info?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption was that we end up passing the reference through some other means. But based on the failing test, seems like that isn't the case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were passing a reference twice and I thought removing this fixed it. But looks like I didn’t rebuild before running tests or something along those lines. I need to look into it again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, we needed to check that the MacroExpansionExpr didn’t have a SubstituteDecl. Could you double-check that the comment I wrote in my new version of this PR makes sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@ahoppen ahoppen force-pushed the ahoppen/semantic-tokens branch from 1776e37 to 2507e0e Compare October 23, 2023 18:07
@ahoppen
Copy link
Member Author

ahoppen commented Oct 23, 2023

@swift-ci Please smoke test

@ahoppen ahoppen merged commit fdc8e7d into swiftlang:main Oct 24, 2023
@ahoppen ahoppen deleted the ahoppen/semantic-tokens branch October 24, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants