Skip to content

[Diagnostics] Use imperative msg for protocol conformance & switch-case fixits #67909

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 1 commit into from
Aug 21, 2023

Conversation

nishithshah2211
Copy link
Contributor

@nishithshah2211 nishithshah2211 commented Aug 14, 2023

This commit changes fixit messages from a question/suggestion to an imperative message for protocol conformances and switch-case. The only source change is in DiagnosticsSema.def. Rest of the changes are in tests to get them to pass.

Resolves #67510.

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Yeah given the discussion in the issue I think moving to imperative form is good

@ktoso
Copy link
Contributor

ktoso commented Aug 14, 2023

Let's also give @xedin and @ahoppen a chance to review 👍

@ktoso
Copy link
Contributor

ktoso commented Aug 14, 2023

@swift-ci please smoke test

…se fixits

This commit changes fixit messages from a question/suggestion to an
imperative message for protocol conformances and switch-case. Addresses
swiftlang#67510.
@nishithshah2211
Copy link
Contributor Author

Previous CI run had some failing tests - addressed them with a new commit. @ktoso

@ktoso
Copy link
Contributor

ktoso commented Aug 14, 2023

Thanks, let's give this a full run to be sure that's everything covered

@ktoso
Copy link
Contributor

ktoso commented Aug 14, 2023

@swift-ci please test

@nishithshah2211
Copy link
Contributor Author

@ktoso - Linux build is failing because of this - https://github.com/apple/sourcekit-lsp/blob/6d3db6d0850d53d9521374c1c18212d6834ae1f9/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift#L121. I can open a PR for that repo in parallel.

@nishithshah2211
Copy link
Contributor Author

And the macOS build is failing on FAIL: test_tsan_swift_dwarf (TestTsanSwiftAccessRace.TsanSwiftAccessRaceTestCase). Not sure how any changes in this PR should affect that.

@hborla
Copy link
Member

hborla commented Aug 17, 2023

@swift-ci please test

@nishithshah2211
Copy link
Contributor Author

The windows had succeeded earlier but timed out this time 😞

I think Linux and macOS builds are expected to fail because they are running tests that are in a different repo - https://github.com/apple/sourcekit-lsp/blob/6d3db6d0850d53d9521374c1c18212d6834ae1f9/Tests/SourceKitLSPTests/PullDiagnosticsTests.swift#L121.

What is the best way to address this? One way is to create a parallel PR that changes the SourceKitLSPTests test and we can try to time the PR merges. But it feels hacky.

@bnbarham
Copy link
Contributor

What is the best way to address this? One way is to create a parallel PR that changes the SourceKitLSPTests test and we can try to time the PR merges. But it feels hacky.

That is indeed the way right now 😅. We can do cross-PR testing here to check the combined PRs.

nishithshah2211 pushed a commit to nishithshah2211/sourcekit-lsp that referenced this pull request Aug 17, 2023
This commit corresponds to changes introduced in
swiftlang/swift#67909.
@nishithshah2211
Copy link
Contributor Author

We can do cross-PR testing here to check the combined PRs.

How can that be done? I created swiftlang/sourcekit-lsp#799 in parallel but both need to be built together.

Another way is to avoid running the SourceKitLSP tests for PRs inside of this repo but I think there is likely a good reason why SourceKit-LSP tests get run for PRs in this repo.

@ahoppen
Copy link
Member

ahoppen commented Aug 17, 2023

swiftlang/sourcekit-lsp#799

@swift-ci please test

@nishithshah2211
Copy link
Contributor Author

Can the windows build be re-triggered? All other builds succeeded.

@bnbarham
Copy link
Contributor

swiftlang/sourcekit-lsp#799

@swift-ci please test Windows platform

@nishithshah2211
Copy link
Contributor Author

@bnbarham Thanks for triggering the CI. The builds succeeded - please merge both the PRs when you get a chance, thanks!

@ahoppen ahoppen merged commit baa20ae into swiftlang:main Aug 21, 2023
@ahoppen
Copy link
Member

ahoppen commented Aug 21, 2023

Thank you for your contribution @nishithshah2211

@nishithshah2211
Copy link
Contributor Author

Thanks for the merge!

@nishithshah2211 nishithshah2211 deleted the imperative-fixits branch August 21, 2023 21:58
ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Oct 10, 2023
The Fix-It message that we are testing here has changed in swiftlang/swift#67909. Accept both the old and the new message to make sure SourceKit-LSP tests also pass with older toolchains (e.g. if are using Xcode 15.0, which doesn’t contain the change yet).

rdar://116706363
ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Oct 10, 2023
The Fix-It message that we are testing here has changed in swiftlang/swift#67909. Accept both the old and the new message to make sure SourceKit-LSP tests also pass with older toolchains (e.g. if are using Xcode 15.0, which doesn’t contain the change yet).
ahoppen added a commit to ahoppen/sourcekit-lsp that referenced this pull request Oct 12, 2023
The Fix-It message that we are testing here has changed in swiftlang/swift#67909. Accept both the old and the new message to make sure SourceKit-LSP tests also pass with older toolchains (e.g. if are using Xcode 15.0, which doesn’t contain the change yet).

rdar://116706363
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.

Fix-Its should be phrased as imperative actions rather than questions
6 participants