[SR-1052][Parser] Warning + FixIt for @warn_unused_result #2760

tanadeau · 2016-05-28T15:13:27Z

What's in this pull request?

Part 6 (final part) of SR-1052: Support for the now-useless @warn_unused_result is removed with an error and fixit.

Resolved bug number: (SR-1052)

Before merging this pull request to apple/swift repository:

Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

tanadeau · 2016-06-03T00:18:25Z

I rebased and fixed the conflicts.

gribozavr · 2016-06-06T06:16:34Z

CC @DougGregor @jopamer @jrose-apple

@tanadeau If you could submit test changes Removed last uses of @warn_unused_result as a separate PR, I'd merge them right away.

tanadeau · 2016-06-06T12:04:44Z

@gribozavr I'm in the process of rebasing and retesting now.

Do you want me to split that commit off into another PR because it's big enough to be its own PR or because it's the commit tending to cause conflicts? If it's the latter, it won't help as the problematic file is for the newly introduced fixit to remove @warn_unused_result which is tied to the rest of this PR.

tanadeau · 2016-06-06T12:14:34Z

Rebased.

gribozavr · 2016-06-06T16:24:58Z

@tanadeau Because we can land those changes and stop worrying about them, while making the patch smaller to review.

jrose-apple · 2016-06-06T17:12:55Z

include/swift/AST/DiagnosticsParse.def

-      "expected a string following ':' for '%0' parameter", (StringRef))
-WARNING(attr_warn_unused_result_unknown_parameter,none,
-        "unknown parameter '%0' in 'warn_unused_result' attribute", (StringRef))
+ERROR(attr_warn_unused_result_removed,none,


This should be a warning rather than an error for now, so that people can update their code at their leisure.

I made it an error as that was what @lattner put in the JIRA ticket. I can change that if still desired though.

I think any diagnostic that says "x has no effect and the right fix is to remove it" should be a warning rather than an error.

That makes sense to me. I based it off the ticket, and an error seemed to follow the pattern for "renamed" attributes since there were not already any that were completely removed. I'll make that change.

Diagnostic changed to warning.

jrose-apple · 2016-06-06T17:18:04Z

Other than those two comments the compiler changes look good to me.

tanadeau · 2016-06-06T23:17:14Z

@gribozavr I split out the commit you requested into a new PR (#2923).

tanadeau · 2016-06-07T02:13:47Z

I rebased against latest master (including merge of #2923). This PR is now much smaller.

tanadeau · 2016-06-08T01:58:37Z

I believe I've addressed all of the comments. I changed the name of the PR since the diagnostic is now a warning instead of an error.

jrose-apple · 2016-06-08T02:06:40Z

Thanks, Trent! Let's do one last CI build.

@swift-ci Please test

tanadeau · 2016-06-08T03:06:07Z

The Linux test failure appears to be unrelated to this PR.

jrose-apple · 2016-06-08T17:09:22Z

Yep, I'm willing to call this good enough. Merging!

tanadeau · 2016-06-08T17:12:26Z

Thanks!

gribozavr · 2016-06-09T07:03:13Z

@tanadeau I'm afraid there's an issue:

import Foundation
class X {
  @warn_unused_result
  @objc
  func foo() -> Int {}
}

$ xcrun $(pwd)/bin/swiftc b.swift
b.swift:3:3: warning: 'warn_unused_result' attribute behavior is now the default
  @warn_unused_result
  ^~~~~~~~~~~~~~~~~~~

b.swift:4:3: error: expected declaration
  @objc

Why does it produce an error?

tanadeau · 2016-06-09T12:43:42Z

@gribozavr, I think I have a fix for that issue. The return value in my if is not correct. It just mattered that I was returning early if there were no other attributes. I'm going to have a PR shortly.

tanadeau · 2016-06-09T13:14:25Z

@gribozavr I created #2970 with the fix and a new test.

tanadeau force-pushed the sr-1052-remove-old-attr branch 2 times, most recently from 9155608 to a49dcad Compare May 28, 2016 16:31

This was referenced May 29, 2016

Removed uses of deprecated @warn_unused_result attr swiftlang/swift-corelibs-foundation#393

Closed

Removed uses of deprecated @warn_unused_result attr swiftlang/swift-corelibs-libdispatch#75

Closed

tanadeau force-pushed the sr-1052-remove-old-attr branch from a49dcad to 600c6e9 Compare June 3, 2016 00:18

tanadeau force-pushed the sr-1052-remove-old-attr branch from 600c6e9 to be5e811 Compare June 6, 2016 12:14

jrose-apple reviewed Jun 6, 2016
View reviewed changes

tanadeau mentioned this pull request Jun 6, 2016

Removed last uses of @warn_unused_result #2923

Merged

1 task

tanadeau force-pushed the sr-1052-remove-old-attr branch from be5e811 to 3324b81 Compare June 7, 2016 02:12

Made use of @warn_unused_result a warning with a fixit

f0c65dc

tanadeau force-pushed the sr-1052-remove-old-attr branch from 3324b81 to f0c65dc Compare June 8, 2016 01:57

tanadeau changed the title ~~[SR-1052][Parser] Error + FixIt for @warn_unused_result~~ [SR-1052][Parser] Warning + FixIt for @warn_unused_result Jun 8, 2016

jrose-apple merged commit 78a420d into swiftlang:master Jun 8, 2016

tanadeau deleted the sr-1052-remove-old-attr branch June 8, 2016 17:11

tanadeau mentioned this pull request Jun 9, 2016

Fix diagnostics when @warn_unused_result is used with other attrs #2970

Merged

1 task

ikesyo mentioned this pull request Jun 28, 2016

[5.0] Swift 3.0 Groundwork ReactiveCocoa/ReactiveCocoa#3001

Merged

swift-ci mentioned this pull request May 28, 2016

[SR-1052] Implement SE-0047 - Defaulting non-Void functions so they warn on unused results #43664

Closed

[SR-1052][Parser] Warning + FixIt for @warn_unused_result #2760

[SR-1052][Parser] Warning + FixIt for @warn_unused_result #2760

Uh oh!

Conversation

tanadeau commented May 28, 2016

What's in this pull request?

Resolved bug number: (SR-1052)

Uh oh!

tanadeau commented Jun 3, 2016

Uh oh!

gribozavr commented Jun 6, 2016

Uh oh!

tanadeau commented Jun 6, 2016

Uh oh!

tanadeau commented Jun 6, 2016

Uh oh!

gribozavr commented Jun 6, 2016

Uh oh!

jrose-apple Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

tanadeau Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

tanadeau Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

tanadeau Jun 8, 2016

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jun 6, 2016

Uh oh!

tanadeau commented Jun 6, 2016

Uh oh!

tanadeau commented Jun 7, 2016

Uh oh!

tanadeau commented Jun 8, 2016

Uh oh!

jrose-apple commented Jun 8, 2016

Uh oh!

tanadeau commented Jun 8, 2016

Uh oh!

jrose-apple commented Jun 8, 2016

Uh oh!

tanadeau commented Jun 8, 2016

Uh oh!

gribozavr commented Jun 9, 2016

Uh oh!

tanadeau commented Jun 9, 2016

Uh oh!

tanadeau commented Jun 9, 2016

Uh oh!

Uh oh!