dart-lang / linter

Linter for Dart.
https://dart.dev/tools/linter-rules
BSD 3-Clause "New" or "Revised" License
627 stars 172 forks source link

discarded_futures false positive? #4866

Open wujek-srujek opened 5 months ago

wujek-srujek commented 5 months ago

Describe the issue A possible false positive for the discarded_futures lint.

To Reproduce

Stream<int> foo() {
  return Stream.fromFuture(futureInt()); // <== lint
}

Future<int> futureInt() => Future.value(17);

Expected behavior I think the lint shouldn't be triggered, the Future isn't discarded, it is consumed by Stream.fromFuture.

lrhn commented 5 months ago

There are other similar reports.

The documentation for the lint states that the thing it warns about is calling an asynchronous function in a non-async function.

You do that, calling futureInt() which returns a Future from foo which is not async, so the warning matches what the lint states that it will warn against.

(The lint is confusingly named, which is probably one of the reasons for the many reports of it not doing what people think it should be doing.)

wujek-srujek commented 5 months ago

Yes, I understand what it's supposed to do, but for some reason I was under the impression that it has built-in exceptions where it doesn't report it (because it knows the 'bigger picture'), and framework functions could be among the exceptions. Especially because I have cases where the lint isn't triggered, but I am calling a function returning a Future in a non-async function. I will try to post some examples after I extract a small sample.

wujek-srujek commented 5 months ago

Ok, I know now what the difference is in my code snippet that doesn't trigger the lint, I have a getter instead of a function calll:

Stream<int> foo() {
  return Stream.fromFuture(futureInt); // <== no lint
}

Future<int> get futureInt => Future.value(17);

I guess this is also expected, because the lint doc say 'Making asynchronous calls in non-async functions', and a getter returning a Future is not an async call?

eernstg commented 5 months ago

I can't see why we wouldn't consider a getter invocation to be a function invocation in this context. A future which is obtained by calling a getter would, presumably, be just as worthy of a lint message as a future returned by a method invocation—if the latter is dangerous/error-prone then the former would be equally so.

I've added the label 'false-negative' because this behavior is very likely to be considered as such.

The behavior which was reported initially (and mentioned in the issue title) was a false positive. I'm not so sure it is a false positive, based on the description of the lint, but I added the label 'false-positive' as well because it was described as such.

lrhn commented 5 months ago

and a getter returning a Future is not an async call?

That's one interpretation. My more cyncical interpretation is that the lint is vaguely specified and therefore inconsistently implemented.

I'd assume the intent of the lint to be captured by "never have an expression with a type of Future or FutureOr in a non-async/async* function". Calling an asynchronous funcition is one way to start an asynchronous computation, but anything that produces a future does that.