Improve DiagnosticEngine's handling of ValueDecl arguments #67075
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
beccadax
commented
Jul 1, 2023
beccadax
force-pushed
the
kinder-diagnostics
branch
from
71bca16 to
78587c4
Compare
beccadax
force-pushed
the
kinder-diagnostics
branch
from
78587c4 to
4178c61
Compare
beccadax
requested review from
hborla,
slavapestov,
xedin,
ktoso,
kavon,
AnthonyLatsis,
tshortli,
zoecarver,
hyp,
egorzhdan,
ahoppen,
bnbarham,
rintaro and
hamishknight
as code owners
|
@swift-ci please test |
kavon
approved these changes
Jul 19, 2023
Implements several enhancements to DiagnosticEngine’s handling of Decl arguments: • All Decl classes, not just ValueDecls, are now valid to use as arguments. • There is built-in logic to handle accessors by printing a phrase like `getter for 'foo'`, so this no longer has to be special-cased for each diagnostic. • `%kind` can be used to insert the descriptive kind before a name; `%kindonly` inserts only the descriptive kind. This can eliminate kind/name argument pairs. • `%base` can insert only the base name, leaving out any argument labels; `%kindbase` combines `%kind` and `%base`. This PR is marked NFC because there are no intentional uses of these changes yet.
These particular changes slightly alter some diagnostics.
Causes minor changes in diagnostic text.
It was used in one place to handle accessors, which happily is no longer necessary.
Causes slight changes to notes on certain `init`s.
…warn This changes the wording of some diagnostics in Swift 4.2 and Swift 4 modes.
…and also adopt new DiagnosticEngine features.
Allows the removal of a helper function.
Slightly alters wording of a diagnostic.
beccadax
force-pushed
the
kinder-diagnostics
branch
from
4178c61 to
4981654
Compare
|
@swift-ci smoke test and merge |
beccadax
force-pushed
the
kinder-diagnostics
branch
from
4981654 to
fe67534
Compare
|
@swift-ci smoke test and merge |
tshortli
approved these changes
Jul 20, 2023
| @@ -7,8 +7,8 @@ public protocol PublicProtoWithReqs { | |||
| } | |||
|
|
|||
| @usableFromInline struct UFIAdopter<T> : PublicProtoWithReqs {} | |||
| // expected-warning@-1 {{type alias 'Assoc' should be declared '@usableFromInline' because it matches a requirement in protocol 'PublicProtoWithReqs'}} {{none}} | |||
| // expected-warning@-2 {{instance method 'foo()' should be declared '@usableFromInline' because it matches a requirement in protocol 'PublicProtoWithReqs'}} {{none}} | |||
| // expected-warning@-1 {{type alias 'Assoc' must be declared '@usableFromInline' because it matches a requirement in protocol 'PublicProtoWithReqs'; this is an error in Swift 5}} {{none}} | |||
There was a problem hiding this comment.
Looks probably intentional and fine to me but I do want to flag this just in case - are the wording changes here a drive by improvement coming from making other changes in the area?
There was a problem hiding this comment.
Yes. When I was updating this diagnostic, I noticed that it had a separate warn_ variant because it predated the introduction of warnUntilSwiftVersion(), so I merged them together to simplify things overall.
|
Just used this, awesome improvement @beccadax 😍! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR improves the DiagnosticEngine's handling of
Decl *diagnostic arguments:Decl, not justValueDecl, can now be used. This is mainly meant for decls likeOperatorDeclwhich have a name but aren’tValueDecls, but it works for allDecls.getter for 'someProperty’; if it’s an extension, the diagnostic engine will describe it using a phrase likeextension of ’SomeType’.%kindmodifier is used, the diagnostic engine will prefix the name with the decl's descriptive kind; this lets you eliminate pairs of arguments.%basemodifier is used, only the base name will be printed.%kindbasemodifier is used, the effects of the previous two modifiers are combined.%kindonlymodifier is used, only the kind will be inserted. This is useful when the kind and name aren’t paired together in the usual way.I believe these changes will simplify a lot of diagnostic code that currently special-cases accessors, passes
DescriptiveDeclKind/ValueDeclargument pairs that need to be kept matched up, and looks up declaration names by hand. AdoptingDeclmore broadly may also open up opportunities for future improvements to diagnostics, like turning decl names into links in clients that would support it.I've tried to keep this PR close to NFC, but I've kept a few obvious wins where this change handled accessors better or made kinds more specific.
I’ve also removed some diagnostics that are unused and replaced some
warn_variants of diagnostics with more modern features likelimitBehaviororwarnUntilSwiftVersion.