[WIP][Stdlib][SR-1052] Added @discardableResult attr to mutating functions #2044

tanadeau · 2016-04-03T21:42:43Z

What's in this pull request?

Part 2 for SR-1052: Add @discardableResult attr to mutating functions in stdlib that return results

Resolved bug number: Part 2 of (SR-1052)

I tried to find all of the mutating functions with non-Void return values and that were not marked with @warn_unused_result. I then marked them with the new @discardableResult attribute. These will stay around after the @warn_unused_result attribute is removed and warning on unused results becomes the default.

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
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
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

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

tanadeau · 2016-04-03T21:44:09Z

stdlib/public/core/Arrays.swift.gyb

@@ -1412,6 +1416,7 @@ extension Array {
  ///
  /// - Complexity: O(`self.count`) if the array is bridged,
  ///   otherwise O(1).
+  @discardableResult
  public mutating func popLast() -> Element? {


I'm not sure why, but the popLast here and later in this same file differ in "discardability" from the popLast in Collection.swift, where it has @warn_unused_result.

If these need to be reconciled, I'm happy to do that under this PR.

Not sure either, but please keep them consistent.

Any idea which is preferable for the consistent solution? They seem to be so close to remove and removeLast that they should have the same "discardability", meaning that all of the pop* functions should have @discardableResult.

They seem to be so close to remove and removeLast that they should have the same "discardability"

I tend to agree.

Actually, I think I see why the pop* functions should warn on unused results. Unlike the remove and removeLast functions, the pop* functions return an optional (nil indicating the collection was empty). This is a success/failure indicator and shouldn't be ignored.

Good point, I'd appreciate @dabrahams thoughts on that.

LGTM.

Sent from my moss-covered three-handled family gradunza

On Apr 3, 2016, at 6:18 PM, Chris Lattner notifications@github.com wrote:

In stdlib/public/core/Arrays.swift.gyb:

@@ -1412,6 +1416,7 @@ extension Array {
///
/// - Complexity: O(self.count) if the array is bridged,
/// otherwise O(1).

@discardableResult
public mutating func popLast() -> Element? {
Good point, I'd appreciate @dabrahams thoughts on that.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub

on Sun Apr 03 2016, Chris Lattner <notifications-AT-github.com> wrote:

In stdlib/public/core/Arrays.swift.gyb:

@@ -1412,6 +1416,7 @@ extension Array {
///
/// - Complexity: O(self.count) if the array is bridged,
/// otherwise O(1).

@discardableResult
public mutating func popLast() -> Element? {

Good point, I'd appreciate @dabrahams thoughts on that.

On second thought, I think we should say, “use removeLast if you don't
want the result.”

Dave

gribozavr · 2016-04-03T22:21:30Z

Did you audit the underscore-land? I see a few cases in stdlib/public/core/Runtime.swift.gyb.

tanadeau · 2016-04-03T22:27:34Z

@gribozavr, I was looking at mutating functions in particular as they have side-effects and the result may be unneeded. I don't see any mutating functions in stdlib/public/core/Runtime.swift.gyb, and most of the functions there have @warn_unused_result or return Void.

Do you have an example of where it should be added in that file?

gribozavr · 2016-04-03T22:46:59Z

@tanadeau I was thinking about _swift_stdlib_atomicFetch${operation}, in C these functions are typically used just for side-effects. But I see what you mean -- they have fetch in their name, which sounds like the primary operation, and the result should not be discarded. We can add functions without a result that don't have fetch in the name and don't have a result to make semantics more clear.

But there seem to be other cases -- for example what do you think about _StringBuffer.grow()?

tanadeau · 2016-04-03T22:48:57Z

I believe I've addressed all the comments with the exception of Dmitri's dealing with Runtime.swift.gyb.

I made all of the pop* methods have @warn_unused_result for consistency and correctness (to force checking of failure).
I removed the @discardableResult attr from the with* methods that take closures that can return something.
I removed the @discardableResult attr from the native* and _custom methods that are implementation details.
I removed the @discardableResult attr from a removeFirst that returned Void.

tanadeau · 2016-04-03T23:11:26Z

I added @discardableResult to _StringBuffer.grow(). That method appears to be used once with no use of the return value. Also the meaning of the return value is not documented.

gribozavr · 2016-04-04T01:04:57Z

Your patch LGTM to the extent of the changes that you made, but I'm still a little concerned about errors of omission.

tanadeau · 2016-04-04T01:09:42Z

@gribozavr, understood. One thing to keep in mind with this PR is that the @discardableResult attribute doesn't actually do anything yet. As part of the switch to make warning on unused results the default and enabling the new attribute, I wouldn't be surprised if some new warnings show up, but that would be taken care of in that future PR.

gribozavr · 2016-04-04T01:11:22Z

@tanadeau OK. If you are happy with the current state of the patch, we can merge it.

tanadeau · 2016-04-04T01:14:04Z

I'm happy if you're happy 😄

gribozavr · 2016-04-04T01:14:46Z

@swift-ci Please test and merge

tanadeau · 2016-04-05T01:19:05Z

I think I fixed the test failure that CI found. The expected output for the serialized XML version of the remove(_:) call was missing the new @discardableResult attribute. I also rebased against latest master.

I'm not sure why this test doesn't run for me. I'm testing in an Ubuntu VM using build-script -R -T.

gribozavr · 2016-04-07T17:48:17Z

@tanadeau Thank you! Could you resolve merge conflicts?

tanadeau · 2016-04-08T00:24:09Z

@gribozavr I rebased and fixed the conflicts. I believe this is ready for a CI test-and-merge.

gribozavr · 2016-04-08T00:27:45Z

@swift-ci Please test and merge

tanadeau · 2016-04-08T04:21:04Z

@gribozavr The OS X build passed. The Linux build failed due to a mismatch between a change in swift-corelibs-xctest and swift-integration-tests.

/tmp/swift-package-tests/test-xctest-package/Output/test-xctest-package.py.tmp.dir/tool/main.swift:9:17: error: missing argument for parameter 'name' in call
let x = MyXCTest()
                ^
<unknown>:0: error: build had 1 command failures

The change at swiftlang/swift-corelibs-xctest@ce992f5 added a new required initializer to XCTest, but swift-integration-tests wasn't updated for subclasses of XCTest, specifically the one at https://github.com/apple/swift-integration-tests/blob/master/test-xctest-package/main.swift#L3.

None of the above was due to this PR.

tanadeau reviewed Apr 3, 2016
View reviewed changes

tanadeau force-pushed the sr-1052-stdlib-add-attr branch from 032b819 to dd0475e Compare April 3, 2016 22:43

tanadeau force-pushed the sr-1052-stdlib-add-attr branch from dd0475e to f266a7b Compare April 3, 2016 23:09

tanadeau force-pushed the sr-1052-stdlib-add-attr branch from f266a7b to 515df05 Compare April 5, 2016 01:15

Added @discardableResult attr to mutating functions

f96ad26

tanadeau force-pushed the sr-1052-stdlib-add-attr branch from 515df05 to f96ad26 Compare April 8, 2016 00:19

gribozavr merged commit 086328d into swiftlang:master Apr 8, 2016

tanadeau deleted the sr-1052-stdlib-add-attr branch April 8, 2016 05:21

swift-ci mentioned this pull request Apr 3, 2016

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

Closed

[WIP][Stdlib][SR-1052] Added @discardableResult attr to mutating functions #2044

[WIP][Stdlib][SR-1052] Added @discardableResult attr to mutating functions #2044

Uh oh!

Conversation

tanadeau commented Apr 3, 2016

What's in this pull request?

Resolved bug number: Part 2 of (SR-1052)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gribozavr commented Apr 3, 2016

Uh oh!

tanadeau commented Apr 3, 2016

Uh oh!

gribozavr commented Apr 3, 2016

Uh oh!

tanadeau commented Apr 3, 2016

Uh oh!

tanadeau commented Apr 3, 2016

Uh oh!

gribozavr commented Apr 4, 2016

Uh oh!

tanadeau commented Apr 4, 2016

Uh oh!

gribozavr commented Apr 4, 2016

Uh oh!

tanadeau commented Apr 4, 2016

Uh oh!

gribozavr commented Apr 4, 2016

Uh oh!

tanadeau commented Apr 5, 2016

Uh oh!

gribozavr commented Apr 7, 2016

Uh oh!

tanadeau commented Apr 8, 2016

Uh oh!

gribozavr commented Apr 8, 2016

Uh oh!

tanadeau commented Apr 8, 2016

Uh oh!

Uh oh!