Convert `lit`-based syntax classification test to XCTest #1953

StevenWong12 · 2023-07-27T15:07:56Z

We should use the assertClassification to make these test cases look nicer and get rid of the dependency on FileCheck eventually.

I converted a subset of the original lit-tests since I think most of them are mutually covered. There are some tests not producing the expected results and I wrote down the reasons in XCTExpectFailure. Maybe we should determine whether changing the test cases or marking these as bugs to fix.

Tests/SwiftIDEUtilsTest/Assertions.swift

Tests/SwiftIDEUtilsTest/ClassificationTests.swift

ahoppen · 2023-07-27T20:07:26Z

Tests/SwiftIDEUtilsTest/ClassificationTests.swift

+    XCTExpectFailure(
+      """
+      '@available' is classified as @ and a typeIdentifier
+      'iOS' is classified as a typeIdentifier
+      '8.0' and '10.10' are classified as integerLiteral
+      """
+    )


Hmm, this is expected by the way the syntax tree gets represented because by now we always model the name of an attribute as a type. Also the lines between builtin attributes (like @availablable) with completely custom syntax, user-defined attributes that are types (like result builders and property wrappers) and user-defined types that aren’t types (attached macros) are blurring. So, what I would expect is available to be classified as an attribute.

You should be able to achieve that by changing \AttributeSyntax.attributeName to return (.attribute, false) in SyntaxClassification.classify(_ keyPath:)

Same for testAttribute2 where objc and IBOutlet should be classified as attribute.

You should be able to achieve that by changing \AttributeSyntax.attributeName to return (.attribute, false)

Do you mean we changing this to (.attribute, true)? Since \AttributeSyntax.attributeName already returns (.attribute, false) now.

If so, we may need to make some changes on handleLayout (as my new commits) since \AttributeSyntax.attributeName is not a TokenSyntax and the second return value of SyntaxClassification.classify(_ keyPath:) only has effects in handleToken.

But I can't find a proper workaround for classifying the trivia pieces in a layout node for now though.

On a second thought, what do you think about refactor ClassificationVisitor as a subclass of SyntaxAnyVisitor like ParseDiagnosticsGenerator. That should make this case handled easier. And the 8.0 and 10.10 can be specified as floatingLiteral rather that some integerLiterals in a version tuple.

Do you mean we changing this to (.attribute, true)? Since \AttributeSyntax.attributeName already returns (.attribute, false) now.

If so, we may need to make some changes on handleLayout (as my new commits) since \AttributeSyntax.attributeName is not a TokenSyntax and the second return value of SyntaxClassification.classify(_ keyPath:) only has effects in handleToken.

Yes, that’s what I meant. But I didn’t realize it only had an effect on tokens 😢

On a second thought, what do you think about refactor ClassificationVisitor as a subclass of SyntaxAnyVisitor like ParseDiagnosticsGenerator. That should make this case handled easier. And the 8.0 and 10.10 can be specified as floatingLiteral rather that some integerLiterals in a version tuple.

I think that would be a good idea and should make the classifier easier to maintain. If the SyntaxAnyVisitor-based implementation is significantly slower than the current one, we should look into why that is and see if we can make SyntaxAnyVisitor faster.

In any case, I would prefer to do this in a follow-up PR. We can just keep the test case disabled for now. One note though: XCTExpectFailure is not supported on Linux IIRC, so you would need to do try XCTSkipIf(true, <message>)

Tests/SwiftIDEUtilsTest/ClassificationTests.swift

Sources/SwiftIDEUtils/SyntaxClassification.swift

ahoppen · 2023-07-29T17:36:13Z

Sources/SwiftIDEUtils/SyntaxClassifier.swift

+
+      if let classification, classification.force == true {
+        let range = SyntaxClassifiedRange(
+          kind: classification.classification,
+          range: ByteSourceRange(offset: byteOffset, length: child.byteLength - child.leadingTriviaByteLength - child.trailingTriviaByteLength)
+        )
+        report(range: range)
+        byteOffset += child.byteLength
+        continue
+      }
+


Oh, did you find a solution to #1953 (comment) after all? Or doesn’t this work?

That could be a solution if we don't refactorClassificationVisitor, and that works.😉

I think that’s a fine solution. Good idea 👍🏽

Sources/SwiftIDEUtils/SyntaxClassifier.swift

Tests/SwiftIDEUtilsTest/ClassificationTests.swift

ahoppen

Thank you. Your idea with forcing the classification of an entire subtree sounds like a good solution for now. We would still be classifying the comment in the following as an attribute but that’s something we can fix in a rewrite of SyntaxClassifier.

@MyType /* abc */ . NestedResultBuilder
func foo() {}

ahoppen · 2023-07-31T19:33:06Z

Sources/SwiftIDEUtils/SyntaxClassifier.swift

+        // Leading trivia.
+        report(range: SyntaxClassifiedRange(kind: .none, range: ByteSourceRange(offset: byteOffset, length: child.leadingTriviaByteLength)))


We should also classify the trivia for doc comments (both leading and trailing). A test case to test that would be.

@MyAttribute // some comment func foo() {}

To do that, I think you should factor out the following and call it to classify the trivia.

https://github.com/apple/swift-syntax/blob/e04c5c117bdf5a92ddc309ac4ce8e33fe302db41/Sources/SwiftIDEUtils/SyntaxClassifier.swift#L168-L173

Ah, yeah. But we also have to add two public properties leadingTriviaPieces and trailingTriviaPieces in RawSyntax to get the trivia pieces as in my new commit.

Oh, I thought we could have a function to classify the trivia pieces so we don’t need to repeat this loop everywhere.

Something like

/// Classifies `trivia` and returns the number of bytes the trivia took up in the source func classify(trivia: [RawTriviaPiece]) -> Int { var classifiedBytes = 0 for triviaPiece in triviaPieces { let range = triviaPiece.classify(offset: byteOffset) report(range: range) classifiedBytes += triviaPiece.byteLength } return classifiedBytes }

And then here you can just do

if let triviaPieces = child.leadingTriviaPieces { byteOffset += classify(trivia: triviaPieces) }

And you can also use the function in handleToken.

My thinking is that repeating an implementation twice is still OK but as soon as we reach 3 or more copies, we should really factor it out.

Ah, I think that looks nice. 👍

ahoppen · 2023-07-31T19:33:28Z

Sources/SwiftIDEUtils/SyntaxClassifier.swift

+
+      if let classification, classification.force == true {
+        let range = SyntaxClassifiedRange(
+          kind: classification.classification,
+          range: ByteSourceRange(offset: byteOffset, length: child.byteLength - child.leadingTriviaByteLength - child.trailingTriviaByteLength)
+        )
+        report(range: range)
+        byteOffset += child.byteLength
+        continue
+      }
+


I think that’s a fine solution. Good idea 👍🏽

Sources/SwiftIDEUtils/SyntaxClassifier.swift

ahoppen

Looks good 👍🏽 Thanks

ahoppen · 2023-08-02T17:22:50Z

@swift-ci Please test

ahoppen · 2023-08-02T23:47:06Z

This breaks sourcekit-lsp because you are removing SyntaxClassification.objectLiteral. Could you create a corresponding sourcekit-lsp PR?

StevenWong12 · 2023-08-03T01:16:20Z

Sure, here is the link swiftlang/sourcekit-lsp#788

ahoppen · 2023-08-03T14:06:29Z

swiftlang/sourcekit-lsp#788

@swift-ci Please test

ahoppen reviewed Jul 27, 2023

View reviewed changes

ahoppen reviewed Jul 29, 2023

View reviewed changes

StevenWong12 force-pushed the convert_coloring_lit_test branch 2 times, most recently from 6f9c15b to 9d7c597 Compare July 30, 2023 15:39

StevenWong12 marked this pull request as ready for review July 30, 2023 15:40

StevenWong12 force-pushed the convert_coloring_lit_test branch from 9d7c597 to 54d3de5 Compare July 30, 2023 16:01

StevenWong12 mentioned this pull request Jul 31, 2023

Remove syntax classification lit-based tests #1966

Merged

ahoppen mentioned this pull request Jul 31, 2023

(5.9) attributes incorrectly highlighted as identifiers swiftlang/sourcekit-lsp#785

Closed

ahoppen reviewed Jul 31, 2023

View reviewed changes

StevenWong12 force-pushed the convert_coloring_lit_test branch from 54d3de5 to ad6358a Compare August 1, 2023 11:29

ahoppen reviewed Aug 1, 2023

View reviewed changes

Sources/SwiftIDEUtils/SyntaxClassifier.swift Outdated Show resolved Hide resolved

Convert lit-based syntax classification tests to XCTests

462b69b

StevenWong12 force-pushed the convert_coloring_lit_test branch from ad6358a to 462b69b Compare August 2, 2023 16:41

ahoppen approved these changes Aug 2, 2023

View reviewed changes

StevenWong12 mentioned this pull request Aug 3, 2023

Remove SyntaxClassification.objectLiteral swiftlang/sourcekit-lsp#788

Merged

ahoppen merged commit 1cd6c22 into swiftlang:main Aug 3, 2023

		// Leading trivia.
		report(range: SyntaxClassifiedRange(kind: .none, range: ByteSourceRange(offset: byteOffset, length: child.leadingTriviaByteLength)))

Convert lit-based syntax classification test to XCTest #1953

Convert lit-based syntax classification test to XCTest #1953

Uh oh!

Conversation

StevenWong12 commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StevenWong12 Jul 29, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StevenWong12 Jul 30, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen left a comment

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 left a comment

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Aug 2, 2023

Uh oh!

ahoppen commented Aug 2, 2023

Uh oh!

StevenWong12 commented Aug 3, 2023

Uh oh!

ahoppen commented Aug 3, 2023

Uh oh!

Uh oh!

Convert `lit`-based syntax classification test to XCTest #1953

Convert `lit`-based syntax classification test to XCTest #1953

StevenWong12 commented Jul 27, 2023 •

edited

Loading

StevenWong12 Jul 29, 2023 •

edited

Loading

StevenWong12 Jul 30, 2023 •

edited

Loading