I think that in this case the warning telling you to write _ = set.insert(4) is fine, personally. I agree that the fact that the closure has a return value is non-obvious, but to me that's a strike against implicit return in closures, not a strike against having a warning for failing to read the value you're returning.
Almost every instance of unmanaged Task { ... } represents a real bug. Even in the <Void, Never> case! The only reason I suggest the carve-out is because I think designing an API to cover the legitimate uses for a one-shot task, without admitting the cases that result in bugs, is impossible within the language as it stands.
But for the sake of discussion, it would probably look something like:
/// calling these `async` by analogy to `DispatchQueue.async`
extension GlobalActor {
/// Queue a self-isolated (hypothetical syntax) synchronous closure
/// to run asap.
/// You could probably do this without modifying the language if
/// you only want to provide it for the `@MainActor`, but I think
/// in general it's something global actors should be able to
/// support generically.
func async(_ work: sending @Self () -> Void)
}
extension Actor {
/// Queue a self-isolated (hypothetical syntax) synchronous closure
/// to run asap.
/// not sure quite how close to valid this syntax might already be?
/// it should be possible to call instance-isolated methods
/// *synchronously* from this closure.
func async(_ work: sending (`self`: isolated Self) -> Void)
}
Let me comment on that right away: We explicitly do not want such APIs on actors. This should be done with Task{} and isolation control using isolated closure capture lists once we finally get to that ( Closure isolation control ).
Let’s stick to discussing the specific pitch about handling the result of Task please.
I’m not convinced “every Task{} without _= is a bug”, that’s very much an overgeneralization… It is tempting to look into that though, but I’m not sure the churn this might cause wouldn’t add to the already quite annoyed developers who are facing many warnings when moving to Swift concurrency hm. So that’s a tradeoff we need to consider here, ultimately this’ll end up being an LSG discussion in formed by discussions in the review thread tbh.
Essentially, a Task { ... } where the value is discarded, cannot be cancelled. It's a little awkward to define clearly the situations under which that causes "actual problems", but in my experience, if it captures a reference type (thereby extending its lifetime), and contains anything which can be "long-running", it can & will eventually cause real problems.
Since the Task handle hasn't been kept, the only way a discarded task can communicate with the outside world is by performing I/O, or by capturing a reference type. So effectively any task which is useful to discard must fall into this first category.
"Long-running" is also very vague, but I think it's fair to say that if it's not synchronous after an initial actor hop, it's probably in the category of being long-running enough to cause problems in practice.
(This is where my suggestion above came from: I think we might be able to define a category of things that are more-or-less risk-free to fire & forget, and then distinguish that category from general tasks that should always be managed. Happy to take discussion of that elsewhere, but I'd very much like to hear more about the vehemence with which it got shot down!)
Anyway, that's my justification for my hard line on this proposal. If the only way to do a one-shot that falls into my "probably safe" bucket is Task { ... } then the carve-out for <Void, Never> makes some kind of sense, but I truly think that it's silencing warnings for real problems, and I would rather have to write _ = Task { on the rare occasions that it's necessary, than not see those warnings in all the other cases.
I'm also in favour of @discardableResult being present only where Success == Void, Failure == Never. I don't expect case of _ = set.insert(...) to be very common, and adding _ = is an acceptable cost.
On the other hand, both (inserted: Bool, memberAfterInsert: Int) and Void are valid solutions for T in Task<T, Never> { () -> T in set.insert(4) }. If overload with @discardableResult is preferred, then there won't be need for _ = . I'm not sure if it is worth to update constraint solver to take @discardableResult into account, but in the short term marking generic overload with @_disfavoredOverload seems to do the trick:
struct T<ValueType, ErrorType: Error> {
@_disfavoredOverload
init(_ op: () throws(ErrorType) -> ValueType) {}
}
extension T where ValueType == Void, ErrorType == Never {
@discardableResult
init( _ op: () -> Void) {}
}
func tets() {
var set: Set<String> = []
T {
set.insert("abc")
}
}
+1 for the pitch; seems like an improvement to me.
i am also at least in philosophical agreement that discarding the task handle implicitly should either only be allowed on Task<Void, Never> or that it should be banned from all tasks in favor of some more explicit fire-and-forget API. but i'm sympathetic to the concern that the volume of warnings a broader change would introduce may not be worth the developer annoyance.
on the matter of 'warnings as errors' that Jon brought up – i agree that introducing new warnings in non-major compiler version bumps is fine (and generally desirable). in this case though i wonder if either some specific new diagnostic should somehow supersede the existing 'no usage' one (as Raúl suggested) and include a diagnostic group, or if the existing warnings produced by the proposed changes should be added to a diagnostic group (they don't appear to have one today). this way the diagnostic behavior can be more directly controlled by teams using -warning-as-errors, and i think it fits with the spirit of the statement here that new warnings should be associated with a diagnostic group.
Small update: I'll do a revision of this proposal this week probably, I'll explore improving the warning messages special-tailored to Task.initializers, explaining the problem a bit more with them.
OTOH, this could be done in one of those diagnostic options like "Thread Sanitiser", "Thread Performance Checker", etc., which is probably out of scope of this pitch and might not even require a formal pitch.
+1 for the pitch, it's a small step in the right direction.
I have reworked the warning messages a bit and also added a warning group which then has further documentation and examples around the issue. You can see the additional docs in the PR linked from the proposal, the docs are not subject to evolution discussion, but we can keep working on improving them separately.
On a related idea of performing runtime checks to ensure task results or errors are never lost, I did some experiments with attaching a class reference to tasks (via thin wrappers) to perform checks upon deallocation, and saw some promising results. However, this requires source changes at task creation sites, whilst ideally, this would be done in a non-invasive manner and built into the diagnostic machinery, similar to Thread Sanitizer or the runtime concurrency checker. Is this forum the right place for this idea, or should it be a mere Radar/Feedback Assistant feature request?
