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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
73ff1c5
[Parse] Add diagnostic for mixed syntax availability attribute
d-ronnqvist Apr 28, 2017
71fac57
[Parse] Add fix-it for single mixed availability syntax
d-ronnqvist Apr 29, 2017
bb16afc
Automatic formatting using git-clang-format
d-ronnqvist Apr 29, 2017
336022a
Fix typos in test code and language in comment
d-ronnqvist Apr 29, 2017
c54dce0
[Parse] Add note to explain the mixed availability syntax fix-it inse…
d-ronnqvist Apr 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions include/swift/AST/DiagnosticsParse.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1472,6 +1472,13 @@ ERROR(avail_query_unrecognized_platform_name,
ERROR(avail_query_disallowed_operator, PointsToFirstBadToken,
"'%0' cannot be used in an availability condition", (StringRef))

ERROR(avail_query_argument_and_shorthand_mix_not_allowed, PointsToFirstBadToken,
"'%0' can't be combined with shorthand specification '%1'",
(StringRef, StringRef))

NOTE(avail_query_meant_introduced,PointsToFirstBadToken,
"did you mean to use '%0, introduced: %1'?", (StringRef, StringRef))

ERROR(avail_query_version_comparison_not_needed,
none,"version comparison not needed", ())

Expand Down
49 changes: 48 additions & 1 deletion lib/Parse/ParseStmt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1103,7 +1103,54 @@ 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 provide a more specific diagnostic if
// that's not the case.
if (Tok.isIdentifierOrUnderscore() &&
!peekToken().isAny(tok::integer_literal, tok::floating_literal)) {
auto Text = Tok.getText();
if (Text == "deprecated" || Text == "renamed" || Text == "introduced" ||
Text == "message" || Text == "obsoleted" || Text == "unavailable") {
auto *Previous = Specs.back();
auto &SourceManager = Context.SourceMgr;
auto PreviousSpecText =
SourceManager.extractText(L->getCharSourceRangeFromSourceRange(
SourceManager, Previous->getSourceRange()));

diagnose(Tok,
diag::avail_query_argument_and_shorthand_mix_not_allowed,
Text, PreviousSpecText);

// If this was preceded by a single platform version constraint, we
// can guess that the intention was to treat it as 'introduced' and
// suggest a fix-it to combine them.
if (Specs.size() == 1 &&
PlatformVersionConstraintAvailabilitySpec::classof(Previous) &&
Text != "introduced") {
auto *PlatformSpec =
cast<PlatformVersionConstraintAvailabilitySpec>(Previous);

auto PlatformName = platformString(PlatformSpec->getPlatform());
auto PlatformNameEndLoc =
PlatformSpec->getPlatformLoc().getAdvancedLoc(
PlatformName.size());

StringRef VersionName = PlatformSpec->getVersion().getAsString();

diagnose(PlatformSpec->getPlatformLoc(),
diag::avail_query_meant_introduced, PlatformName,
VersionName)
.fixItInsert(PlatformNameEndLoc, ", introduced:");
}

Status.setIsParseError();
break;
}
}

// Otherwise, keep going.
} else {
break;
}
Expand Down
28 changes: 28 additions & 0 deletions test/Parse/diagnose_availability.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// RUN: %target-typecheck-verify-swift

// SR-4231: Misleading/wrong error message for malformed @available

@available(OSX 10.6, *) // no error
func availableSince10_6() {}

@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 •
edited
Loading

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.

// expected-error@-1 {{'deprecated' can't be combined with shorthand specification 'OSX 10.0'}}
// expected-note@-2 {{did you mean to use 'OSX, introduced: 10.0'?}} {{15-15=, introduced:}}
// expected-error@-3 {{expected declaration}}
func shorthandFollowedByDeprecated() {}

@available(OSX 10.0, introduced: 10.12)
// expected-error@-1 {{'introduced' can't be combined with shorthand specification 'OSX 10.0'}}
// expected-error@-2 {{expected declaration}}
func shorthandFollowedByIntroduced() {}

@available(iOS 6.0, OSX 10.8, *) // no error
func availableOnMultiplePlatforms() {}

@available(iOS 6.0, OSX 10.0, deprecated: 10.12)
// expected-error@-1 {{'deprecated' can't be combined with shorthand specification 'OSX 10.0'}}
// expected-error@-2 {{expected declaration}}
func twoShorthandsFollowedByDeprecated() {}