Skip to content

Improve unexpected tokens diagnostics #2811

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

Conversation

mateusrodriguesxyz
Copy link
Contributor

While investigating #1373 I noticed that replacing a break with a continue in canRecoverTo improves diagnostic in a number of recovery tests.
I've also added a little logic to call skipSingle so we achieve desired diagnostic in testRecovery105(), but only if possible at the same line, so cases like testRecovery100() is not messed up.

@mateusrodriguesxyz mateusrodriguesxyz force-pushed the improve-unexpected-tokens-diagnostics branch from 64ee567 to 4f3f7f8 Compare August 20, 2024 00:53
Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you @mateusrodriguesxyz!

@ahoppen
Copy link
Member

ahoppen commented Aug 20, 2024

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge August 20, 2024 14:20
@mateusrodriguesxyz
Copy link
Contributor Author

Couldn't figure out what caused the tests to fail 🤔

@ahoppen
Copy link
Member

ahoppen commented Aug 20, 2024

Looks like BasicFormatTest.testClosureParameter failed. That test did look unrelated to your other changes anyway.

/Users/ec2-user/jenkins/workspace/swift-syntax-PR-macOS/branch-main/swift-syntax/Tests/SwiftBasicFormatTest/BasicFormatTests.swift:62: error: -[SwiftBasicFormatTest.BasicFormatTest testClosureParameter] : failed - Actual output does not match the expected
 myFunc({
     return true
–})
+    })

auto-merge was automatically disabled August 21, 2024 00:32

Head branch was pushed to by a user without write access

@mateusrodriguesxyz mateusrodriguesxyz force-pushed the improve-unexpected-tokens-diagnostics branch from 4f3f7f8 to b02a7f1 Compare August 21, 2024 00:32
@ahoppen
Copy link
Member

ahoppen commented Aug 21, 2024

@swift-ci Please test

@ahoppen ahoppen enabled auto-merge August 21, 2024 16:27
@ahoppen
Copy link
Member

ahoppen commented Aug 21, 2024

@swift-ci Please test Windows

Copy link
Member

@ahoppen ahoppen left a comment

Choose a reason for hiding this comment

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

Looks like a few files that used to parse correctly now no longer do.

Failed Tests (9):
  Swift(macosx-x86_64) :: ModuleInterface/features.swift
  Swift(macosx-x86_64) :: Parse/ConditionalCompilation/compiler.swift
  Swift(macosx-x86_64) :: Parse/ConditionalCompilation/language_version_explicit.swift
  Swift(macosx-x86_64) :: Parse/ConditionalCompilation/sequence_version.swift
  Swift(macosx-x86_64) :: Parse/features.swift
  Swift(macosx-x86_64) :: SILGen/specialize_attr.swift
  Swift(macosx-x86_64) :: SILOptimizer/pre_specialize-macos.swift
  Swift(macosx-x86_64) :: SILOptimizer/pre_specialize.swift
  Swift(macosx-x86_64) :: SILOptimizer/pre_specialize_layouts.swift

These files are located in https://github.com/swiftlang/swift/tree/main/test

@ahoppen
Copy link
Member

ahoppen commented Aug 22, 2024

Oh, looks like the errors are unrelated to your PR, they also seem to fail in #2814.

@mateusrodriguesxyz
Copy link
Contributor Author

mateusrodriguesxyz commented Aug 22, 2024

These failed in #2814 too.

@DougGregor
Copy link
Member

I think swiftlang/swift#76040 will fix this issue

@mateusrodriguesxyz
Copy link
Contributor Author

mateusrodriguesxyz commented Aug 26, 2024

@ahoppen I think this should pass now that swiftlang/swift#76040 has been merged. Could you test again, please?

@DougGregor
Copy link
Member

@swift-ci please test

@ahoppen ahoppen merged commit f0d2d35 into swiftlang:main Aug 26, 2024
3 checks passed
@mateusrodriguesxyz
Copy link
Contributor Author

Thanks @DougGregor !

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