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

[tanadeau]

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]

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.

[lattner]

Not sure either, but please keep them consistent.

[tanadeau]

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.

[gribozavr]

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

I tend to agree.

[tanadeau]

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.

[lattner]

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

[dabrahams]

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

[dabrahams]

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]

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

[tanadeau]

@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]

@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]

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

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

[tanadeau]

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]

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

[tanadeau]

@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]

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

[tanadeau]

I'm happy if you're happy 😄

[gribozavr]

@swift-ci Please test and merge

[tanadeau]

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]

@tanadeau Thank you! Could you resolve merge conflicts?

[tanadeau]

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

[gribozavr]

@swift-ci Please test and merge

[tanadeau]

@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.