-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[SR-4231] Add diagnostic (& fix-it) for mixed syntax availability attribute #9122
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
When parsing a list of availablity specifications in the shorthand syntax, check to see that the next specification is also in shorthand syntax. If the next specification looks like an explicit “deprecated” (or similad) attribute, then emit a specific diagnostic about it to help the developer understand the problem. https://bugs.swift.org/browse/SR-4231
For the scenario that’s described in SR-4231, when there is one shorthard syntax followed by ‘deprecated’ (or similar), then we can guess that the intention was to treat the shorthand as ‘introduced’ so that the two of them work together. This guess is only made if there is one platform version constrain, that is followed by ‘deprecated’, ‘renamed’, etc. but not ‘introduced’. https://bugs.swift.org/browse/SR-4231
I've tried to keep the comments focused on why something is done, but I'm not sure if I ended up |
|
||
@available(OSX 10.0, deprecated: 10.12) | ||
// expected-error@-1 {{'deprecated' can't be combined with shorthand specification 'OSX 10.0'}} {{15-15=, introduced:}} | ||
// expected-error@-2 {{expected declaration}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reasonable recovery path here?
@@ -1103,7 +1103,47 @@ Parser::parseAvailabilitySpecList(SmallVectorImpl<AvailabilitySpec *> &Specs) { | |||
consumeToken(); | |||
Status.setIsParseError(); | |||
} else if (consumeIf(tok::comma)) { | |||
// keep going. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, can you run git-clang-format on this patch. There's like 4 or 5 places with some interesting indentation.
lib/Parse/ParseStmt.cpp
Outdated
// There is more to parse in this list. | ||
|
||
// Before continuing to parse the next specification, we check that it's | ||
// also in the shorthand syntax and try to and provide a more specific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drive-by nit: "and try to and provide"?
func availableSince10_6() {} | ||
|
||
@available(OSX, introduced: 10.0, deprecated: 10.12) // no error | ||
func introducedFollowedByDepreaction() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another nit: here and below (not that it matters for your test), typo "Depreaction".
Also, consistently names test functions as “deprecated” and "introduced"
@available(OSX, introduced: 10.0, deprecated: 10.12) // no error | ||
func introducedFollowedByDeprecated() {} | ||
|
||
@available(OSX 10.0, deprecated: 10.12) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For visibility (got folded by the force-pushes): Can you make this recover? There's a decl down there, but this second diagnostic makes it seems like there isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that it probably could be made to recover, but I don't know how to do it. (I’m assuming that recovery means being able to emit the right attribute and continue parsing.)
As far as I understand how this works parseAvailabilitySpecList
(this code) parses a comma separated list of short for availability specifications, and returns it to the DAK_Available
case of parseNewDeclAttribute
which turns each specification into their full attribute.
I think that in order to recover, it would need to pass some additional information back so that an AvailableAttr
could be created with both an introduced and deprecated parameter, or so that the current token could be parsed differently. In either case, I need to consume tokens until the closing parenthesis.
Not that it would be a lot of work, but I don’t know if it would add much on top of the diagnostic and fix-it. That said; if you think it's worth doing, I would see it as a good learning opportunity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recovery could be as simple as re-forming the attribute list as though the user had accepted your fixit, then just sending things on downstream. It's not a pressing thing and could definitely be addressed in another pull request.
@swift-ci please smoke test |
lib/Parse/ParseStmt.cpp
Outdated
PlatformSpec->getPlatformLoc().getAdvancedLoc( | ||
PlatformName.size()); | ||
|
||
diag.fixItInsert(PlatformNameEndLoc, ", introduced:"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing: Can you motivate this fixit insertion with either a separate note or a dependent clause in the error text that mentions your assumption here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the same error message is used both when there is and isn't a fix-it, I think it would be better to add a separate note and also move the fix-it to that note.
Thanks @d-ronnqvist! Don't worry yourself with the recovery thing for now. I've got one last request then I think this can be put in. |
…rtion This change also moves the fix-it from the error to the note.
@swift-ci please smoke test |
Thanks @d-ronnqvist! ⛵️ |
This adds a specific diagnostic when both shorthand and non-shorthand specifications are mixed in the same availability attribute. For example:
@available(OSX 10.0, deprecated: 10.12)
.Current error:
New error:
If (and only if) there is only a single platform version constraint, that is followed by ‘deprecated’, ‘renamed’, etc. but not ‘introduced’, then it also offers a fix-it to insert
, introduced:
in the platform version constraint, turning:into:
Resolves SR-4231.