[Review] SE-0047 Defaulting non-Void functions so they warn on unused results

+1 but:

I'm somewhat hesistant about not marking the plausibly discarable ones
@discardableResult.
- I rarely use the result value of removeLast() and I don't see how
requiring it here adds any safety. It is obvious this call has side-effects
and doesn't just return the last element.
- I would say the canonical way to clean-up observers would be to call
removeObservers(self) in deinit to remove all of them at once. Though
arguably not marking it @discardable might have some small safety benefit
here.

I've been told that this is automatically disabled in playgrounds (waves hi to Jordan R) since results are always used by
displaying them in the sidebar.

"Playgrounds don't give any "unused result" warnings because there aren't any unused results—
they all get displayed in the output column." -- SR-945

-- E

···

On Mar 17, 2016, at 1:09 PM, Dan Raviv via swift-evolution <swift-evolution@swift.org> wrote:

-1 for
this behavior in playgrounds.

What is your evaluation of the proposal?

-0.5 if the annotation is verbose (much longer than @discardable).
+0.5 if the annotation is pleasant and concise, like @discardable

Is the problem being addressed significant enough to warrant a change to Swift?

The warn-by-default behavior is mostly useless. The only reason to
make the change is because “if we were designing Swift from scratch,
this would probably be a slightly better default”.

Most non-void functions are called for their result, and nobody ever
forgets to use that result; if they do, it's like if they forgot to
call the function altogether — trivial to find, not useful as a
compiler diagnostic at all.

The new default is better for:

- (A) classes that provide both mutating and non-mutating methods;
- (B) methods where forgetting to use the result produces a bug (a
download task that needs to be resumed, an alert that needs to be
displayed, a listener that needs to be stored somewhere, etc).

To be clear, the mistake this warning prevents is the unintentional call
to a non-mutating method when one thinks one is mutating the receiver.
This scenario can arise independently of A or B.

···

on Wed Mar 23 2016, Andrey Tarantsov <swift-evolution@swift.org> wrote:

The old default is better for:

- (C) fluid APIs and other similar DSL scenarios;
- (D) methods that are mainly invoked for their side effect and return a value “just in case”, like removing/adding elements, scheduling tasks, ...

I've just scanned the entire Swift codebase I wrote since that fateful
WWDC'14 (~20 kLOC excluding comments and blanks). I only have a
handful of classes/methods in each category above, and annotating them
one way or another is a trivial matter. Some of them *are not*
currently annotated with @warn_unused_result, which is a point in
favor of this proposal.

Does this proposal fit well with the feel and direction of Swift?

I think the safe-by-default, explicit-opt-out behavior is a better default for Swift, although, like I've said, in practice it doesn't really matter much.

Perhaps it's most useful for newcomers; you can easily skip over @warn_unused_result when learning Swift, but you won't be able ignore @discardable.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

Haven't ever seen this in a language.

Golang had a (not very informative) discussion on this topic at Redirecting to Google Groups

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Closer to an in-depth study; looked at my Swift codebase, and read through all the discussions.

On a side note, let me once again point to a dangerous trend in this
mailing list: not looking at (or reporting on) how potential changes
affect actual, specific, production code bases. We need a lot more of
that in our reviews.

Thanks,

A.

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

--
Dave

• What is your evaluation of the proposal?
+1 on proposal overall. I agree that @discardable is sufficient to describe what it does. If there is questions it is easy to search it on the web. Also the context of where the @discardable modifier is on the line will reduce ambiguity. Conciseness is important and I don’t think it significantly detracts from the clarity to leave off the “Return” part.

  • Is the problem being addressed significant enough to warrant a change to Swift?
Yes

  • Does this proposal fit well with the feel and direction of Swift?
Yes

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
NA

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Been following and contributing to discussion and read the proposal.

···

On Mar 16, 2016, at 5:57 PM, Erica Sadun via swift-evolution <swift-evolution@swift.org> wrote:

On Mar 16, 2016, at 5:27 PM, Brent Royal-Gordon via swift-evolution <swift-evolution@swift.org> wrote:

  • What is your evaluation of the proposal?

I am in favor of the semantic, but I don't like `@discardableResult`; it's long and unwieldy. I would prefer that we either use a shorter word than "discardable", attach the keyword to the return type as discussed in "Future Directions", or both.

1. The keyword will be used rarely. I don't mind if it's slightly hard to type.
2. When used, it should be as clear as possible, both in understanding what it does and visually standing out.
3. The most popular keyword requested was actually @allowUnusedResult followed by @suppressUnusedResultWarning.
  Both of these are longer. I felt @discardableResult was more descriptive than @allowUnusedResult. I wanted to avoid
  the word "suppress" as it is appears on many frequently misspelled words lists.

  I believe discardable (the number one choice by *far* for a type annotation, with ignorable as its second) perfectly describes
  how the return value/result should be treated. When included, the behavior mimics:

       let _ = resultReturningFunctionOfSomeType()

  which basically discards the result (an active decision) rather than ignores it (a passive one).

I also don't like that this proposal doesn't include an "Impact on existing code" section. We ought to decide whether the migrator will add `@discardableResult` to existing symbols or not.

This is a good point and Adrian and I will be happy to add this in. I do not believe it should, although I'll let Adrian answer
on his own. The entire point of this exercise is to reduce likely error points. Simply changing the behavior without fixits
should help accomplish that in existing code. If you add @discardableResult, we mask the advantage this behavior
should address.

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes. I should use `@warn_unused_result`, but never bother because it's just too much of a hassle. My code will be safer with this change.

And that's why I don't think the migrator should make any code changes.

-- E

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

I also don't like that this proposal doesn't include an "Impact on existing code" section. We ought to decide whether the migrator will add `@discardableResult` to existing symbols or not.

I totally agree with Erica – the migrator should only remove existing @warn_unused_result attributes. @discardableResult should be added manually and deliberately to functions in stdlib and your own code. We'll be happy to clarify this behavior under "Impact on existing code" section.

I agree in general, but I'm sure there are at least one or two codebases out there which really *have* been thoroughly audited and their authors will be highly displeased to have to update them manually. We might want to provide an option to invert the existing annotations, or to invert things automatically if the code meets some heuristic, like having more than half of the functions in a given file marked with `@warn_unused_result`.

···

--
Brent Royal-Gordon
Architechies

@Patrick,

I rarely use the result value of removeLast() and I don't see how requiring it here adds any safety. It is obvious this call has side-effects and doesn't just return the last element.

Actually, removeLast() and other pop()-like functions are examples of functions that will most probably be marked with @discartableResult attribute.

···

————————————————————

@Brent,

I would prefer that we either use a shorter word than "discardable", attach the keyword to the return type as discussed in "Future Directions", or both.

That was the first idea, but it has been changed to function declaration attribute, per core team design discussion <https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160314/012823.html&gt;\.

I also don't like that this proposal doesn't include an "Impact on existing code" section. We ought to decide whether the migrator will add `@discardableResult` to existing symbols or not.

I totally agree with Erica – the migrator should only remove existing @warn_unused_result attributes. @discardableResult should be added manually and deliberately to functions in stdlib and your own code. We'll be happy to clarify this behavior under "Impact on existing code" section.

————————————————————

@Jed,

That said, the problem isn’t necessarily difficult to solve — it’s just that it’s important that it actually be solved at the same time the feature is rolled out.

I cannot speak for Apple, but judging from last Swift releases where SDKs were always up-to-date with latest Swift changes, I believe this change will be reflected as well.

The only place that inconsistencies may appear, are third-party libraries, but this has always been the case with all language changes.

Regards,
Adrian Kashivskyy

1. The keyword will be used rarely. I don't mind if it's slightly hard to type.

The first sentence might be true for many developers, but you can't predict how other people will design their libraries:
Have a look at FluentInterface for a good reason to keep "results" discardable.

Sadly, the second sentence is definitely true — "@discardableResult" doesn't "feel" right, and would discourage techniques like fluent interfaces.

OS SDKs don't turn on a dime so this is a reasonable concern. We don't want to ship an Xcode package with Swift and SDKs that don't play well together. (Those of you in bleeding-edge open source land are not so lucky…) For this feature there are feasible adoption paths even if the SDK updates slowly so we should be okay.

If we can't get enough of the SDK to adapt in time then we can make the change in stages, deferring complete adoption until the SDK can catch up. For this feature one interim measure would be to temporarily accept both @warn_unused_result and @discardable, add a temporary compiler flag to choose what the default is for unmarked functions, and set the flag to the old way by default. Then the new SDK could update their code while the old SDK still works. Once the SDK was ready we could remove @warn_unused_result and the compiler flag.

A simpler staged adoption would be to temporarily accept both @warn_unused_default and @discardable, but ignore them both. The downside there is that we would temporarily get no unused result warnings at all, which sounds bad.

In any case the problem of SDK malleability is not a show-stopper for acceptance of this proposal. It's not hard to make it work.

···

On Mar 16, 2016, at 11:46 PM, Adrian Kashivskyy via swift-evolution <swift-evolution@swift.org> wrote:

@Jed,

That said, the problem isn’t necessarily difficult to solve — it’s just that it’s important that it actually be solved at the same time the feature is rolled out.

I cannot speak for Apple, but judging from last Swift releases where SDKs were always up-to-date with latest Swift changes, I believe this change will be reflected as well.

--
Greg Parker gparker@apple.com <mailto:gparker@apple.com> Runtime Wrangler

The new default is better for:

- (A) classes that provide both mutating and non-mutating methods;
- (B) methods where forgetting to use the result produces a bug (a
download task that needs to be resumed, an alert that needs to be
displayed, a listener that needs to be stored somewhere, etc).

To be clear, the mistake this warning prevents is the unintentional call
to a non-mutating method when one thinks one is mutating the receiver.
This scenario can arise independently of A or B.

Sure. Although if a type only has mutating or non-mutating methods, but not both, the mistake will probably be immediately apparent, so the diagnostic doesn't win you much (except maybe in a newbie learning environment, which is an important use case as well).

A.

Somehow I completely forgot about the issue around naming the replacement attribute; I agree that it doesn’t feel right, but then @warn_unused_result doesn’t feel that right to me either.

I’m very much more in favour of decorating the return type itself as mentioned in alternatives. In fact, I’d be in favour of delaying this change until that can be done, rather than using something else in the mean time; I do want this change, but it’s not such a big deal to me that I’m unwilling to wait a little bit longer to get what I feel is the better form of it.

If it does go ahead though, I think that in the short term @ignore_unused_result is more consistent with what we have now, and can be re-evaluated depending upon what we want to do with the attribute naming convention (or has that been decided already?). While it’s longer, I think that any variation of "ignore unused” is a bit clearer than discardable, at least to me.

···

On 17 Mar 2016, at 09:27, Tino Heth via swift-evolution <swift-evolution@swift.org> wrote:

Sadly, the second sentence is definitely true — "@discardableResult" doesn't "feel" right, and would discourage techniques like fluent interfaces.

Ack, hit send before quoting this part to come back to it.

I assume your point here is that having to add @discardableResult (or whatever) will lead to fluent interfaces becoming prone to the same mistakes that non-fluent interfaces currently have with @warn_unused_result.

Perhaps we could add some kind of attribute to the type itself to allow for selection between the two behaviours?

For example, we could use a different attribute in the style of:

@unusedResult(ignore)
@unusedResult(warn)

Add this to a function and it affects that specifically, add it to a type and it affects all methods of that type (unless they have their own attribute overriding it). So for a fluent interface I could do:

  @unusedResult(ignore)
  protocol MyFluentType {
    func something() -> Self
    func somethignElse() -> Self

    @unusedResult(warn)
    func notAFluentMethod() -> Self
  }

If we do get return type attributes then the return type form could become @unused(ignore) to make it a bit shorter?

···

On 17 Mar 2016, at 09:27, Tino Heth via swift-evolution <swift-evolution@swift.org> wrote:
Have a look at FluentInterface for a good reason to keep "results" discardable.

This is the last official day of SE-0047 review. I wanted to throw this in here before things wrapped up.
Hopefully this addresses concerns brought up during the review period.

Thanks, -- Erica

Migration and Impact On Existing Code

Flipping Swift's default behavior to automatically warn on unused results should expose overlooked errors in existing code. Community review has convinced us that simply removing the existing attributes from Swift code will not be the best solution for all developers.

For many developers, removing @warn_unused_result will be sufficient, enabling warnings to drive code audits. Deprecating @warn_unused_result instead of remove-on-migration, would better allow inspection of any code that is not @warn_unused_result.

Some developers have requested an on-demand inversion (marking un-marked functions) for code they believe is well audited and will resist any issues with masking inherent problems. Brent Royal-Gordan wrote, "If you have done a reasonably thorough audit, deleting the annotations instead of inverting them is a destructive change. When Swift 2 changed the documentation format, the migrator didn't delete all the old doc comments. Deleting @warn_unused_result in audited codebases is akin to that."

Concerns were raised over imported functions written in discardable-by-default environments, especially for C-language sources where community members felt imported functions should match the default behavior of the C-language. Global functions that return scalar values from Objective-C sources might be treated in the same way. We believe any Objective-C method or function that returns an NSObject should warn on ignored result unless explicitly marked. removeLast() and other pop()-like functions are examples of functions that will most probably be marked with @discartableResult attribute.

<disc.md · GitHub concerns:

In Swift Evolution discussions, the term @discardable was mildly preferred over @discardableResult.

Some community members requested a new attribute enabling exceptional imported functions to be properly annotated from Objective-C source.

Dany St-Amant requested two levels of compiler response: @discardableResult(warn) for simple warnings and @discardableResult(critical) which generates an error with unused results, such as when returning allocated resources.

The new default is better for:

- (A) classes that provide both mutating and non-mutating methods;
- (B) methods where forgetting to use the result produces a bug (a
download task that needs to be resumed, an alert that needs to be
displayed, a listener that needs to be stored somewhere, etc).

To be clear, the mistake this warning prevents is the unintentional call
to a non-mutating method when one thinks one is mutating the receiver.
This scenario can arise independently of A or B.

Sure. Although if a type only has mutating or non-mutating methods,
but not both, the mistake will probably be immediately apparent,

Why do you think so?

···

on Thu Mar 24 2016, Andrey Tarantsov <swift-evolution@swift.org> wrote:

so the diagnostic doesn't win you much (except maybe in a newbie
learning environment, which is an important use case as well).

A.

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

--
Dave

In the proposal there is no mention of generic functions.
Take a map function of a Wrapper:

Wrapper<T> {
        func map<U>(transform: T -> U) -> U { ... }
}

let intWrapper = Wrapper(1)
intWrapper.map{ print($0) } // this returns Void

Should the last line be inferred to have a discardable result?

In my opinion a function which returns a generic result should warn for unused results in every case. Here, "map" gets a closure with side effects which is very unusual and should be handled with another function like "apply".

- Maximilian

Perhaps we could add some kind of attribute to the type itself to allow for selection between the two behaviours?

imho that's a good direction — it's a pity that linking to other posts is so tedious ;-): I voted for something similar (change the setting for a whole module).

If we go with something like this, we may want to also have a:

@unusedResult(critical)

Which generate error for unused result, and a warning when assigning the result to _.
This could be useful for function which return allocated resources, which is a return code that should never be ignored even explicitly.

Dany

···

Le 17 mars 2016 à 06:50, Haravikk via swift-evolution <swift-evolution@swift.org> a écrit :

On 17 Mar 2016, at 09:27, Tino Heth via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:
Have a look at FluentInterface for a good reason to keep "results" discardable.

Ack, hit send before quoting this part to come back to it.

I assume your point here is that having to add @discardableResult (or whatever) will lead to fluent interfaces becoming prone to the same mistakes that non-fluent interfaces currently have with @warn_unused_result.

Perhaps we could add some kind of attribute to the type itself to allow for selection between the two behaviours?

For example, we could use a different attribute in the style of:

@unusedResult(ignore)
@unusedResult(warn)

This looks good, I think that @discardableResult() makes sense until we can get @discardable on the return type itself. I think that if warn/critical is added then there should also be a corresponding “allow” or similar (which is what the default is), i.e- @discardableResult on its own implies @discardResult(allow) to issue no warnings of any kind.

I also like the idea of being able to add this attribute to types as well as functions, allowing the default behaviour to be changed on a per-type basis, as this would be useful for types that are specifically designed with method chaining for example (where most results are discardable as standard). While the choice of default will never satisfy everyone, this would make it easy to tweak for your own needs.

Allow us to do something like the following, assuming that @discardableResult(warn) is now the default:

  @discardableResult(allow)
  class MyChainableType {
    func chainableMethod() -> Self { … }
    func anotherChainableMethod() -> Self { … }

    @discardableResult(critical)
    func getResult() -> Dictionary { … }
  }

Of course, if we can get attributes on return types now or in future, then the function form of the attribute should be @discardable(warn) for .getResult().

···

On 21 Mar 2016, at 20:35, Erica Sadun via swift-evolution <swift-evolution@swift.org> wrote:
In Swift Evolution discussions, the term @discardable was mildly preferred over @discardableResult.

Some community members requested a new attribute enabling exceptional imported functions to be properly annotated from Objective-C source.

Dany St-Amant requested two levels of compiler response: @discardableResult(warn) for simple warnings and @discardableResult(critical) which generates an error with unused results, such as when returning allocated resources

Yes, this will be inferred to return Void, so no warning will be produced.

-Chris

···

On Mar 27, 2016, at 6:33 AM, Maximilian Hünenberger via swift-evolution <swift-evolution@swift.org> wrote:

In the proposal there is no mention of generic functions.
Take a map function of a Wrapper:

Wrapper<T> {
       func map<U>(transform: T -> U) -> U { ... }
}

let intWrapper = Wrapper(1)
intWrapper.map{ print($0) } // this returns Void
Should the last line be inferred to have a discardable result?

Sorry for how informal this review is; I'm generally in favor of it, although I would recommend that imported C functions are by default tagged with @discardableResult.

Félix

···

Le 17 mars 2016 à 07:33:10, Tino Heth via swift-evolution <swift-evolution@swift.org> a écrit :

Perhaps we could add some kind of attribute to the type itself to allow for selection between the two behaviours?

imho that's a good direction — it's a pity that linking to other posts is so tedious ;-): I voted for something similar (change the setting for a whole module).
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

  • All my code has been thoroughly pre-audited and the transition will be painful.

If adopted, this proposal will incur negative costs for some developers. I believe the benefits for the language and the wider community outweigh the negatives.

I think this was in response to a statement I made, so I'd like to make two clarifications:

* I do not personally have a fully audited codebase. Actually, I'm not sure I've ever used @warn_unused_result in anger—it's just too much work. Nevertheless, some people have, and I feel bad for them.

* I emphatically do *not* think that means we should keep the current behavior. I think the people who have already audited would be the first to endorse this change. I merely think we should offer them better migration support.

Please understand: If you have done a reasonably thorough audit, deleting the annotations instead of inverting them is a *destructive* change. When Swift 2 changed the documentation format, the migrator didn't delete all the old doc comments. Deleting @warn_unused_result in audited codebases is akin to that.

···

--
Brent Royal-Gordon
Architechies