Generalize handling of error-prone situations where an object is discarded

[eernstg]

The lint unawaited_futures is concerned with situations where a future is computed and then discarded (for example, a statement is foo(); rather than await foo();, and foo returns a future). This situation is considered error-prone because it is likely to cause some computations to occur "too late". The situation is hard to detect without tool support, because it is an implicit and perfectly normal thing to discard an object (of any type).

However, #57769 makes the point that this lint is currently ignoring many situations where a future is discarded: If the future is assigned to a variable or passed to a parameter whose declared type is void or Object or a top type, any static knowledge that there exists a future that may need to be awaited is lost, and in the case where the static type of the future will be void we even have support to ensure that the object is discarded.

Another incompleteness in the checking performed by unawaited_futures is the fact that higher order occurrences are ignored:

• If, for any T, we assign a function of type Future<T> Function() to a variable/parameter of type void Function() or Object Function() then it is likely that every invocation of said function will discard a future, which is just as error-prone as the first-order case.

• Another higher-order case arises when Future<...> is used as a type argument; for instance, assigning a List<Future<T>> to a variable/parameter of type List<void> or List<Object> will destroy the information that each element in that list might need to be awaited.

Issue #57653 notes that all the above lints will ignore functions whose body isn't marked async. This means that developers will not get the heads up in the situation where a function foo is not marked async, but it calls bar, and bar has now been updated to return a future.

Future<void> bar() => ...; // Used to return `void`, was then updated.

void foo() {
  ...
  bar(); // No lint, but discards a `Future<void>`.
  ...
}

We have a similar situation with return statements, for arbitrary types (not just futures, but anything which doesn't have type void or dynamic): void foo() { return 1; } is an error (not just a lint or hint!), because it is considered error-prone to make it look like a value of type int is being delivered to a caller, but it is effectively discarded because of the return type void. Similar rules kick in when a non-void expression is returned from an async function whose return type is void or Future<void>.

Finally, we have an object named useResult in package 'meta', and this object is used as metadata on functions whose returned value should not be discarded. In this case we would also have higher-order variants of the situations which are currently linted, and those higher-order variants are not detected.

Could we generalize the lint support for flagging error-prone situations where an object is discarded, such that it is at least possible to detect higher-order cases, and the mechanism can be used for other types than Future and/or void? Is it just a matter of making useResult more powerful, and perhaps adding support for a useType annotation such that we can do @useType on the class Future, or implicitly add @useResult on all async functions whose return type isn't void?

Cf. #57653, #57769, #58622.

[bwilkerson]

Yes, we could definitely generalize the lint support for these cases, and I like the idea of doing so.

But I think this raises a very interesting design question about how best to support such checks. I haven't formed any strong opinions yet, but I'll share a couple of thoughts.

We've had discussions several times about the appropriate scope for lint rules. We haven't formulated any concrete principles other than the obvious one of trying to do what's best for users. For example, we currently have three similar lints around returning null from various kinds of functions (avoid_returning_null, avoid_returning_null_for_future, and avoid_returning_null_for_void). (They might be obsolete under null safety, but I'll use them as an example anyway.) At first glance they're all dealing with the same question: when is it OK to return null? Should they be a single lint, or is it better for them to be separate lints so that users can decide just how strict they want their code base to be on this topic?

A similar question arises here. Should we have a single lint that catches all of the places where a Future is discarded? A single rule for places where any kind of object is discarded? Multiple rules so that users can have more control? I don't have a good answer yet, but I think it's worth thinking about.

And then, of course, we have another mechanism that might be more appropriate if we think we should consolidate all of this checking into one, or a small number, of lints. We have support in the analysis options file to specify when we should effectively be using a stricter definition of the language. Users can currently enable strict-casts, strict-inference, and strict-raw-types. Is support for detecting when objects are discarded better thought of as a new kind of strictness check?

@leafpetersen @srawlins

[eernstg]

We might be able to have lints that amount to sets of "smaller" lints. A developer could then enable each of the small lints separately for better control, or they could enable the "big" lint that simply enables every lint that is a member of that set.

[leafpetersen]

My broad take on this is that: 1) this seems like a useful extension to the lint (or possibly a different lint, but in general keeping things simpler seems better) and 2) that this is inherently going to be very heuristic and specific to Futures (or else extraordinarily complicated), so I'm skeptical about generalizing this too much.

On 2, note that if you want to generalize this fully, things like generics are going to make your life complicated. You can't in general know statically whether a type variable T has been instantiated with Future<Something>, and so you either need to disallow dropping generics, or have constraints, or... something. And once you get into things like assigning to List<Object> you're really getting heuristic.

@bwilkerson

Should we have a single lint that catches all of the places where a Future is discarded?

This seems most reasonable to me, but other folks involved in the original discussion probably have better context, perhaps they should be looped in here?

A single rule for places where any kind of object is discarded?

My initial response is, "no". Futures are very special here. While I can imagine a coding style that forbids implicitly discarding any value, that feels draconian to me.

Is support for detecting when objects are discarded better thought of as a new kind of strictness check?

My initial take is no, though perhaps I could be convinced otherwise? Certainly not all objects in general, that feels, as I say above, really draconian. Even the Future case depends on the details. My read here is that for this lint, folks are willing to tolerate more false positives than I would be comfortable having in something like the strict checks - I'd really like the strict checks to be something that I can pretty much unreservedly recommend everyone turn on.

[bwilkerson]

... perhaps they should be looped in here?

The "we" is primarily Phil and I, so I can't think of anyone else to loop in at the moment.

My read here is that for this lint, folks are willing to tolerate more false positives than I would be comfortable having in something like the strict checks ...

I completely agree that the strict checks should have no false positives, and that's a compelling argument for leaving it as a lint.

... this seems like a useful extension to the lint (or possibly a different lint, but in general keeping things simpler seems better) ...

Unless we hear from users that they need to have more granularity, I agree, having fewer lints is generally better.

The one potentially mitigating factor is that changing a lint to start producing diagnostics in more cases can be a breaking change (if code that previously built cleanly starts failing). We'll need to measure the impact of such a change in order to make a decision.

[leafpetersen]

... perhaps they should be looped in here?

The "we" is primarily Phil and I, so I can't think of anyone else to loop in at the moment.

There was an internal discussion thread among a number of people that I believe inspired this issue.

[parren-google]

I was one of the folks raising this issue as a source of errors. From this perspective, a single lint that catches all places where a Future is silently discarded (so not via unawaited) sounds very reasonable to me.