Skip to content

[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

Merged
merged 5 commits into from
Apr 30, 2017
Merged

[SR-4231] Add diagnostic (& fix-it) for mixed syntax availability attribute #9122

merged 5 commits into from
Apr 30, 2017

Conversation

d-ronnqvist
Copy link
Contributor

@d-ronnqvist d-ronnqvist commented Apr 29, 2017

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:

@available(OSX 10.0, deprecated: 10.12)
//                             ^
// error: expected version number
// error: expected declaration

New error:

@available(OSX 10.0, deprecated: 10.12)
//                   ^
// error: 'deprecated' can't be combined with shorthand specification 'OSX 10.0'
// error: expected declaration

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:

@available(OSX 10.0, deprecated: 10.12)

into:

@available(OSX, introduced: 10.0, deprecated: 10.12)
// insert:    ^-----------^

Resolves SR-4231.

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
@d-ronnqvist
Copy link
Contributor Author

I've tried to keep the comments focused on why something is done, but I'm not sure if I ended up
with a good comment to code ratio or if it's too much.


@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}}
Copy link
Contributor

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.
Copy link
Contributor

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.

// 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
Copy link
Collaborator

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() {}
Copy link
Collaborator

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)
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@CodaFi CodaFi Apr 29, 2017

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.

@CodaFi
Copy link
Contributor

CodaFi commented Apr 29, 2017

@swift-ci please smoke test

PlatformSpec->getPlatformLoc().getAdvancedLoc(
PlatformName.size());

diag.fixItInsert(PlatformNameEndLoc, ", introduced:");
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@CodaFi
Copy link
Contributor

CodaFi commented Apr 30, 2017

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.
@CodaFi
Copy link
Contributor

CodaFi commented Apr 30, 2017

@swift-ci please smoke test

@CodaFi
Copy link
Contributor

CodaFi commented Apr 30, 2017

Thanks @d-ronnqvist!

⛵️

@CodaFi CodaFi merged commit f45196d into swiftlang:master Apr 30, 2017
@d-ronnqvist d-ronnqvist deleted the SR-4231 branch May 2, 2017 18:29
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