Pitch: Remove discardableResult from throwing task initializers

Evolution Pitches
1

Hi everyone,

continuing the effort to improve small yet valuable things in the Concurrency library, I’d like to propose an often discussed but never finished change to Task.init and friends: removing the @discardableResult when the operation is throwing.

As a side note, we’ve already adopted typed throws in all these APIs, that change was no behavioral change so we did so without a specific SE proposal. This change however does cause more warnings to be emitted, so we wanted to go through the proper Swift Evolution path here.

Discardable result use in Task initializers

Summary of changes

Previously, @discardableResult was applied universally to all Task initializers, making it too easy to accidentally miss that a thrown error was ignored. We propose to only apply discardable result treatment to non-throwing Task initializers.

Motivation

Currently the following code, in which a function inside an unstructured task is throwing an error results in no warnings being emitted:

Task { // no warning
  try boom() 
}
print("Yay!")

In simple snippets like this it may be obvious that we missed to handle the error, however real codebases often have more noise and it is possible to miss the fact that we silently ignored the error.

This issue has been raised multiple times in the Swift community, however none wound up resulting in a complete proposal to actually make the change:

The origin of this semantic is that originally this Task {} syntax was primarily intended for fire-and-forget semantics, and we did not account enough for the possibility of accounting for the thrown errors. This behavior however is inconsistent with Swift's error handling model and leads to hard-to-find bugs where errors are silently swallowed.

Detailed design

The concurrency library has now adopted typed throws in Task.init, Task.detached, Task.immediate, Task.immediateDetached, so the only change needed for this proposal is to remove the @discardableResult annotation:

extension Task {
  @discardableResult // REMOVE
  public init(...)
}

extension Task where Failure == Never {
  @discardableResult // keep
  public init(...)
}

Which will result in the following warning behaviors:

Task {  }
Task { throws in ... } // warning: result of call to 'init(name:priority:operation:)' is unused
Task { throws(Boom) in ... } // warning: result of call to 'init(name:priority:operation:)' is unused

Task.detached {  }
Task.detached { throws in ... } // warning: result of call to 'detached(name:priority:operation:)' is unused
Task.detached { throws(Boom) in ... } // warning: result of call to 'detached(name:priority:operation:)' is unused

// etc.

And it is possible to silence the issue by explicitly ignoring the value:

_ = Task { throws in ... }

The same change will be applied to all other unstructured task initializers.

Source compatibility

The change is source compatible, however may cause new warnings to be emitted.

Those warnings can be silenced by explicitly discarding the task value: _ = Task { try boom() }

ABI compatibility

This proposal does not affect ABI as it only changes compile-time diagnostic behavior.

Alternatives considered

Do nothing

We feel the community has voiced this as a problem for long enough, and we should improve the situation here.

Also remove discardableResult from non-throwing initializers

We believe that non-throwing values which return either Void or non-Void values are usually intentionally fire and forget -- otherwise you would be interested in using the resulting value, and be forced to store the task (or await it immediately). Therefore we do not propose to remove this attribute from Task initializers which do not throw.

Acknowledgments

Everyone in the previous pitch threads, thank you for raising the issue.

Please send typo fixes directly to the pull request: SE-NNNN: Add proposal to remove discardableResult from throwing Task inits by ktoso · Pull Request #3133 · swiftlang/swift-evolution · GitHub

31 Likes
Should Swift warn about ineffective cancellation checks inside unstructured tasks?
Should Swift warn about ineffective cancellation checks inside unstructured tasks?
2

I still think that it should be removed from all initializers except Task<Void, Never>. Discarding a task that either returns or throws is almost certainly a mistake.

(I'd be quite happy to remove it from all Task initializers; I think almost every situation where you discard a task is a mistake; _ = Task { in the few cases that you really intend to would be a worthwhile bit of friction IMO)

19 Likes
3

Unless @discardableResult can bubble through the Task, that seems rather unergonomic.

This could be worth it, though I have to wonder how impactful the new warning might be. And it's not really source compatible for anyone compiling with warnings as errors, so this may need to be an upcoming feature.

1 Like
4

If you're using warnings-as-errors you've explicitly made that decision; no, adding warnings is not source breaking and we're allowed to do this in incremental language versions. Otherwise none of Swift concurrency would have ever been able to be staged in -- we periodically add warnings about future deprecations or "will be error in swift 6 mode" etc.

18 Likes
5

I'll add a bit of discussion / detective work for further context:

I've searched some large codebases and it seems Task {} is used a lot in UIs like Button { Task {, and things like that... There is quite a number of Task{} just in the middle of some normal code, and those indeed may be not ideal, but it's somewhat hard to draw the line between those. Very often you want a "fire-and-forget"... we don't have a different way to spell this, and the result type gets inferred from the value: Task { bla() } may be intended as fire and forget however since bla() returns Bla this would then have to be _ = Task.

We could always revisit in the future if this is seen more common and helpful. It is true that technically speaking we'd like dissuade people from just doing Task{} randomly everywhere, but I'm not sure just making it more annoying is the best way to do that.

My personal bias which isn't backed by any team plans or anything would be to really embrace some real way to "fire and forget" that isn't Task{} that could play into the equation then as well :thinking:

8 Likes
6

I'd be neat if we could add a note specifically pointing out that errors thrown in the task are ignored, on top of the default diagnostic.

Today, discarding the task handle is way more popular than keeping it[1], to the point that makes me question how familiar some devs are with its return type (other than knowing a task of some sort is returned), much less understand the nuanced change of the API becoming @discardableResult only when Failure == Never.

There's no harm in clarifying the rules and pointing out what's missing, and it could help promote this warning from "just another concurrency warning" to "oh, this is actually useful" in the minds of some.

Speaking of which: yes, this is certainly useful. I'm sure I've missed handling thrown errors in this way myself, so big +1 to the proposal, I'm excited to see this coming! :slight_smile: Seems like everywhere it triggers it's going to encourage writing better code.

  1. I actually checked on a big project: I found 2600+ uses of Task { ... }, but only ~300 were preceded by an assignment operator. ↩︎

6 Likes
7

Yeah, I was pondering this but didn't act on it yet... It would be simple enough to add a note about this, which perhaps is the best way here, without introducing a new error category just "explain it more". I'd personally quite like that, thanks for poking on this idea.

5 Likes
8

On a side note, I often wish that @discardableResult would bubble through generic functions as well, as well as being accepted by the type system as a Void-returning function.

That is, whenever something expects a () -> Void it would accept @discardableResult () -> T as a valid parameter.

I have e.g a Logger.time function, like this:

func time<T>(_ message: String, _ action: () async throws -> T) async rethrows -> T {
    let start = ContinuousClock.now
    defer {
        let time = start.duration(to: .now)
        let humanTime = time.formatted(.units(
            allowed: [.seconds, .milliseconds, .microseconds],
            maximumUnitCount: 1
        ))
        self.log(.info, "⏱️ \(message) took \(humanTime)")
    }
    return try await action()
}

It produces warnings when called with a @discardableResult function:

@discardableResult
func someOperationThatReturnsSuccess() async -> Bool { ... }

await time("something", someOperationThatReturnsSuccess)
// warning: result of call to 'time' is unused
7 Likes
9

Definitely! But this is a lie:

That does not meet the criteria to be called "unused", so this change does not meet the criteria to be called an improvement. It's well-meaning, but the problem is unrelated to discardability, so nothing should be done until the conflation can be properly addressed.

10

You’ll have to formulate an actual argument other than just state something is a lie while that’s how the compiler would report the code in question.

3 Likes
11

No, the warning message is not for the compiler.

@discardableResult means that a function has side effects.

You can't take away a Task's side effects by removing this decoration. A Task is not its own value or error. It is a nonsensical proposition.

I hate that there's no warning or error associated with the problem, but this proposal is just not the way to go.

Alternatively, the proposal could be amended to require the further change that these become synonymous for throwing procedure calls:

try?
_ =

I don't think that makes any sense, but it would at least add consistency.

1 Like
12

I agree with @KeithBauerANZ here. Many places where Task's return a non-Void value and aren't stored and subsequently the value is awaited are often a bug.

7 Likes
13

This is not correct. @discardableResult means the function returns a non-Void value that the API author considers likely to be discarded by callers.

It says nothing at all about side-effects.

6 Likes
14

One issue is that Success might not be Void in cases where that is not obvious. For example:

var set = Set([1, 2, 3])

Task {
    set.insert(4)
}

This creates a task of type Task<(inserted: Bool, memberAfterInsert: Int), Never>.

3 Likes
15

It effectively means that it has side effects because otherwise it would be useless to call it and discard the result

6 Likes
16

Naïve question.. but why is this even allowed to begin with?

One could write try? boom() if they want to ignore errors, or do { ... } catch { ... } if they want to do something about errors.. having bare throwing things inside seem too risky – too easy to silence those errors and miss them.

2 Likes
17

The error would be rethrown when you await the Task’s .value. Therefore you’d need to prefix that await with try. The danger is that you’re not required to store the Task anywhere, which makes it easier to forget to await its value. This pitch fixes that. You’d still be able to do _ = Task { try foo() }, but the assignment to _ more strongly indicates the error will be dropped.

You can alternatively await the Task’s .result without a try, but that’s considered a benefit by the people who use Result<Value, Error>.

1 Like
18

But 99% of Task { ... } in the field are fire and forget, so I wonder would that actually help..

Is the following possible?

Task { try foo() } // error

try Task { try foo() } // fix

(at which point if I wanted to ignore the error I'd put try? either on the inner try or on the outer try)

No, that would not fly.

How about this (in broad strokes):
For Tasks with "throwing closures" (also for tasks having a non Void values) implement some magic inside standard library equivalent to having some internal object's deinit:

deinit {
    precondition(..., message: "Tasks `.value` has not been called")
}

So if I didn't call task's value (for those "throwing closure" Tasks, or tasks with non void values) – I'd at least get a guaranteed runtime error. And not just in cases when the closure did actually throw, but always, to ease testing. Non ideal, still, as it's runtime check instead of compile time – but it will actually work.

19

Strongly in favour here, I’d go as far as remove it from all initializers except Task<Void, Never> as was suggested somewhere.

I don’t believe these extra warnings are annoying because they have value: if you want to ignore the value you can make it clear to anyone reading, both compiler and colleagues, that you are indeed ignoring it.

Without the warning, accidentally ignoring thrown errors is a common footgun that got me too many times.

If you hate warnings so much you can disable them.

5 Likes
20

That’s an interesting idea! For example, on Darwin, it may be possible to show a purple warning/runtime issue in Xcode on the line where the task was initialized when it is deinitialized an uncaught error that was never accessed. Or an uncaught error could do a function call to a function with a well-known name that could have a default breakpoint in LLDB (although the backtrace at that moment may not be useful)

5 Likes