Skip to content

[Sema/SourceKit] Emit same diagnostics for missing protocol requirements on the command line and in SourceKit #75667

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 8, 2024

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Aug 3, 2024

Some editors use diagnostics from SourceKit to replace build issues. This causes issues if the diagnostics from SourceKit are formatted differently than the build issues. Make sure they are rendered the same way, removing most uses of DiagnosticsEditorMode.

To do so, always emit the add stubs for conformance note (which previously was only emitted in editor mode) and remove all ; add <something> suffixes from notes that state which requirements are missing.

rdar://129283608

@ahoppen
Copy link
Member Author

ahoppen commented Aug 3, 2024

@swift-ci Please smoke test

@ahoppen ahoppen force-pushed the build-issues-live-issues branch from 564a3b6 to 6d879cf Compare August 5, 2024 20:15
@ahoppen
Copy link
Member Author

ahoppen commented Aug 5, 2024

@swift-ci Please smoke test

@ahoppen ahoppen force-pushed the build-issues-live-issues branch from 6d879cf to 6577a60 Compare August 7, 2024 16:58
@ahoppen
Copy link
Member Author

ahoppen commented Aug 7, 2024

@swift-ci Please smoke test

@@ -3474,11 +3474,6 @@ bool ContextualFailure::tryProtocolConformanceFixIt(
conformanceDiag.fixItInsert(nameEndLoc, ": " + protoString);
}

// Emit fix-its to insert requirement stubs if we're in editor mode.
if (!getASTContext().LangOpts.DiagnosticsEditorMode) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we fully remove this LangOpt?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two more uses (about switch IIRC) that I want to remove in a follow-up PR. This one is big enough on its own.

Copy link
Contributor

@bnbarham bnbarham left a comment

Choose a reason for hiding this comment

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

Thanks @ahoppen!

…nts on the command line and in SourceKit

Some editors use diagnostics from SourceKit to replace build issues. This causes issues if the diagnostics from SourceKit are formatted differently than the build issues. Make sure they are rendered the same way, removing most uses of `DiagnosticsEditorMode`.

To do so, always emit the `add stubs for conformance` note (which previously was only emitted in editor mode) and remove all `; add <something>` suffixes from notes that state which requirements are missing.

rdar://129283608
@ahoppen ahoppen force-pushed the build-issues-live-issues branch from 6577a60 to 6610439 Compare August 7, 2024 21:01
@ahoppen ahoppen requested a review from xymus as a code owner August 7, 2024 21:01
@ahoppen
Copy link
Member Author

ahoppen commented Aug 7, 2024

@swift-ci Please smoke test

@ahoppen ahoppen merged commit 8edb57c into swiftlang:main Aug 8, 2024
3 checks passed
@ahoppen ahoppen deleted the build-issues-live-issues branch August 8, 2024 00:24
@@ -3,12 +3,11 @@
// FIXME: this test only passes on platforms which have Float80.
// <rdar://problem/19508460> Floating point enum raw values are not portable

// REQUIRES: CPU=i386 || CPU=x86_64
Copy link
Contributor

@asl asl Aug 8, 2024

Choose a reason for hiding this comment

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

The test fails on arm platforms due to this change. See FIXME above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Already fixing it in #75784. This change as unintentional.

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.

5 participants