[Parser] Improve diagnostics for special platforms in available attribute. #18895

dingobye · 2018-08-22T10:20:31Z

This patch adds warnings when a version number is used on the non-specific * platform. For example, it warns unexpected version number in 'available' attribute for non-specific platform '*' in the following case:

@available(*, deprecated: 4.2)

In addition, it fixes some misleading warning messages on swift platform as mentioned by @airspeedswift , including the following cases:

@available(swift, unavailable)
 ^
- warning: unknown platform 'swift' for attribute 'available'
+ warning: 'unavailable' cannot be used in 'available' attribute for platform 'swift'


@available(swift, deprecated)
 ^
- warning: unknown platform 'swift' for attribute 'available'
+ warning: expected version number with 'deprecated' in 'available' attribute for platform 'swift'


@available(swift, message: "missing valid option")
 ^
- warning: unknown platform 'swift' for attribute 'available'
+ warning: expected 'introduced', 'deprecated', or 'obsoleted' in 'available' attribute for platform 'swift'

Resolves SR-8598.

airspeedswift · 2018-08-22T15:18:20Z

@swift-ci please test

jrose-apple

Thanks for picking this up!

jrose-apple · 2018-08-22T15:40:57Z

include/swift/AST/DiagnosticsParse.def

@@ -1316,6 +1316,20 @@ ERROR(attr_availability_expected_equal,none,
 ERROR(attr_availability_expected_version,none,
      "expected version number in '%0' attribute", (StringRef))

+WARNING(attr_availability_swift_platform_expected_option,none,
+      "expected '%0' option with a version number for swift platform, "


Nitpick: I suggest using this as "platform 'swift'" (like "platform '*'"). It seems a little clearer to me.

After seeing it in context, I also think it might just be better to simplify to something like "expected 'introduced', 'deprecated', or 'obsoleted' in '%0' attribute for platform 'swift'". It does mean we have to keep it up to date, but it's less circumlocution.

jrose-apple · 2018-08-22T15:41:16Z

include/swift/AST/DiagnosticsParse.def

+      "expected '%0' option with a version number for swift platform, "
+      "such as 'introduced', 'deprecated', or 'obsoleted'", (StringRef))
+WARNING(attr_availability_swift_platform_deprecated,none,
+      "'deprecated' must have a version number for swift platform in '%0' "


This could also apply to obsoleted or introduced, right?

In general, obsoleted and introduced must always have a version number alongside, while version for deprecated is optional. In this particular case (i.e., platform 'swift'), however, deprecated cannot be used alone and must followed by a version. So, the motivation of such warning here is for deprecated only.

jrose-apple · 2018-08-22T15:41:49Z

include/swift/AST/DiagnosticsParse.def

+      "'deprecated' must have a version number for swift platform in '%0' "
+      "attribute", (StringRef))
+WARNING(attr_availability_swift_platform_infeasible,none,
+      "argument '%0' is infeasible for swift platform in '%1' attribute",


Nitpick: To match the tone of other diagnostics, I would prefer something more like "cannot be used with" instead of "is infeasible for". (I'm also not sure we call them "arguments" in attributes, but in this case I think just leading with '%0' is fine.)

jrose-apple · 2018-08-22T15:42:58Z

lib/Parse/ParseDecl.cpp

@@ -528,6 +547,14 @@ ParserResult<AvailableAttr> Parser::parseExtendedAvailabilitySpecList(
    return nullptr;
  }

+  // Warn if any version is specified for non-specific '*' platforms.
+  if (Platform == "*" && SomeVersion) {
+    diagnose(AttrLoc,


Is it possible to add a fix-it for this one?

Good idea! Fix-it added.

dingobye · 2018-08-23T01:24:56Z

Thanks for the review, Jordan! I updated the code to address your comments.

jrose-apple

Thanks, this looks great!

jrose-apple · 2018-08-23T01:33:53Z

@swift-ci Please test

swift-ci · 2018-08-23T01:34:23Z

Build failed
Swift Test OS X Platform
Git Sha - 153b57924af970fed8c92646d6bbee11dae75297

swift-ci · 2018-08-23T01:36:16Z

Build failed
Swift Test Linux Platform
Git Sha - 153b57924af970fed8c92646d6bbee11dae75297

rintaro

Looks great! thank you!

dingobye · 2018-08-23T06:46:45Z

Oh, just noticed some warnings are raised from stdlib. Please let me fix those before merging this PR.

adapted to swiftlang/swift#18895.

dingobye · 2018-08-23T08:08:36Z

I updated the code to fix the warnings in stdlib.

In addition, I checked other projects and found corelibs-libdispatch affected. I created #391 there.

jrose-apple · 2018-08-23T15:58:40Z

Whoops. Thank you for checking. cc @airspeedswift

airspeedswift

LGTM. Thanks!

airspeedswift · 2018-08-23T17:46:52Z

@swift-ci please test

jrose-apple · 2018-08-23T17:58:51Z

Let's wait for the SCD changes before merging.

…bute. This patch adds warnings when a version number is used on the non-specific '*' platform. In addition, it fixes some misleading warning messages on 'swift' platform. Resolves: SR-8598.

dingobye · 2018-08-30T22:48:07Z

Hi Jordan, thank you for taking care of the dependency PR #391 in swift-corelibs-libdispatch repo.

I just updated the code here to make sure it is conflict-free.

jrose-apple · 2018-08-31T00:42:27Z

Let's do it!

@swift-ci Please test

swiftlang/swift#18895. Signed-off-by: Kim Topley <[email protected]>

airspeedswift requested a review from jrose-apple August 22, 2018 15:18

jrose-apple reviewed Aug 22, 2018

View reviewed changes

jrose-apple requested a review from rintaro August 22, 2018 15:48

jrose-apple approved these changes Aug 23, 2018

View reviewed changes

jrose-apple self-assigned this Aug 23, 2018

rintaro approved these changes Aug 23, 2018

View reviewed changes

dingobye added a commit to dingobye/swift-corelibs-libdispatch that referenced this pull request Aug 23, 2018

Fix warnings in attribute to make swift-corelibs-libdispatch

c328c0f

adapted to swiftlang/swift#18895.

dingobye mentioned this pull request Aug 23, 2018

Remove version number used in 'available' attribute for non-specific platform '*'. swiftlang/swift-corelibs-libdispatch#391

Merged

airspeedswift approved these changes Aug 23, 2018

View reviewed changes

dingobye added 3 commits August 31, 2018 08:40

[Parser] Improve diagnostics for special platforms in available attri…

b790eb5

…bute. This patch adds warnings when a version number is used on the non-specific '*' platform. In addition, it fixes some misleading warning messages on 'swift' platform. Resolves: SR-8598.

Add a fix-it and rephrase some warning messages.

3dca923

Fix warnings in stdlib

033f6ac

dingobye force-pushed the sr8598 branch from 00f6848 to 033f6ac Compare August 30, 2018 22:44

jrose-apple merged commit 78560c7 into swiftlang:master Sep 1, 2018

ktopley-apple pushed a commit to swiftlang/swift-corelibs-libdispatch that referenced this pull request Dec 6, 2018

Fix warnings in attribute to make swift-corelibs-libdispatch adapted to

01e11bb

swiftlang/swift#18895. Signed-off-by: Kim Topley <[email protected]>

[Parser] Improve diagnostics for special platforms in available attribute. #18895

[Parser] Improve diagnostics for special platforms in available attribute. #18895

Uh oh!

Conversation

dingobye commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

airspeedswift commented Aug 22, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

dingobye Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

jrose-apple Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

dingobye Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

dingobye Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

jrose-apple Aug 22, 2018

Choose a reason for hiding this comment

Uh oh!

dingobye Aug 23, 2018

Choose a reason for hiding this comment

Uh oh!

dingobye commented Aug 23, 2018

Uh oh!

jrose-apple left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Aug 23, 2018

Uh oh!

swift-ci commented Aug 23, 2018

Uh oh!

swift-ci commented Aug 23, 2018

Uh oh!

rintaro left a comment

Choose a reason for hiding this comment

Uh oh!

dingobye commented Aug 23, 2018

Uh oh!

dingobye commented Aug 23, 2018

Uh oh!

jrose-apple commented Aug 23, 2018

Uh oh!

airspeedswift left a comment

Choose a reason for hiding this comment

Uh oh!

airspeedswift commented Aug 23, 2018

Uh oh!

jrose-apple commented Aug 23, 2018

Uh oh!

dingobye commented Aug 30, 2018

Uh oh!

jrose-apple commented Aug 31, 2018

Uh oh!

Uh oh!

dingobye commented Aug 22, 2018 •

edited

Loading

dingobye Aug 23, 2018 •

edited

Loading