dart-lint icon indicating copy to clipboard operation
dart-lint copied to clipboard

add in unawaited_futures

Open bsutton opened this issue 3 years ago • 5 comments

# Too many false positives.
# Using the pedantic package for the unawaited function doesn't make code better readable
#
# pedantic: enabled
# https://dart-lang.github.io/linter/lints/unawaited_futures.html
# - unawaited_futures

As above the doco suggest this does produce false positive.

The converse issues is that diagnosing a missing await can be a very complex issue.

I personally am happy to wear the burden of having to unawait methods to avoid bugs especially bugs that are hard to solve.

I really think this should be part of the default package.

I just added this to a small project (6000 lines) and it fixed three bugs and returned no false positives.

bsutton avatar Dec 15 '20 23:12 bsutton

Thanks for opening this issue and starting a discussion 👍

I enabled it in few bigger apps I have access to, to check if something has changed since I disabled the rule. I found 16/72 cases where the check was correct and an await was missing. 77% of the time it is a false positive though.

Most false positives are button presses that start async operations (intended) or fire and forget calls like analytics event tracking.

Another reason this check is disabled, which I haven't documented, is that the unawaited function is part of the pedantic package. To use it, lint would have to depend on pedantic and export it. It would be a bit weird to depend on something this project replaces.

I'm happy to add it as default, given proof that it is correct in ~80% of cases and enough demand from the community (Upvote the issue if you read this). For the time being, enable it manually for your project:

include: package:lint/analysis_options.yaml

linter:
  rules:
    # Always await, intentionally not await
    unawaited_futures: true

Don't forget to also add the pedantic dependency to actually be able to use the unawaited function.

passsy avatar Dec 16 '20 02:12 passsy

We could always just copy the unawait function from pedantic.

On the flip side I really don't see an issue with depending on a package that you are trying to improve on.

I do however think you are asking the wrong question.

Whilst the false positive rate is a factor the most important questions is how damaging are these types of errors.

I would suggest that they are highly significant and perhaps more importantly extremely hard to find. I've been burnt quite badly by these types of problems on multiple occasions.

I would opinion that these factors outweigh the slight pain of the false positives.

On Wed, 16 Dec 2020 at 13:02, Pascal Welsch [email protected] wrote:

Thanks for opening this issue and starting a discussion 👍

I enabled it in few bigger apps I have access to, to check if something has changed since I disabled the rule. I found 16/72 cases where the check was correct and an await was missing. 77% of the time it is a false positive though.

Most false positives are button presses that start async operations (intended) or fire and forget calls like analytics event tracking.

Another reason this check is disabled, which I haven't documented, is that the unawaited function is part of the pedantic package. To use it, lint would have to depend on pedantic and export it. It would be a bit weird to depend on something this project replaces.

I'm happy to add it as default, given proof that it is correct in ~80% of cases and enough demand from the community (Upvote the issue if you read this). For the time being, enable it manually for your project:

include: package:lint/analysis_options.yaml

linter:

rules:

# Always await, intentionally not await

unawaited_futures: true

Don't forget to also add the pedantic dependency to actually be able to use the unawaited function.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/passsy/dart-lint/issues/26#issuecomment-745714203, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG32OFVGZENTE7FYMGJZZDSVAIKPANCNFSM4U5EHVPA .

bsutton avatar Dec 16 '20 03:12 bsutton

Was this ever solved? Do we have to include lint and pendantic to get the unawaited() function?

sgehrman avatar Apr 04 '21 21:04 sgehrman

I let this issue sit for a while because the unawaited function was moved from pedantic to meta. I don't mind depending on meta but unfortunately, it was reverted DOH!

Screen-Shot-2021-04-05-15-36-54 60

I then checked the linter code and discovered - to my surprise - that the check is not tied to the unawaited function of pedantic. Instead, any wrapper function and extensions work! This means lint can ship its own unawaited function.

Please check the PR for the new fireAndFoget function https://github.com/passsy/dart-lint/pull/33

passsy avatar Apr 05 '21 17:04 passsy

I spent a lot of time exploring unawaited_futures and the new discarded_futures lints. There are many false positives and at this point not good enough to be included.

  • https://github.com/dart-lang/lints/issues/25
  • https://github.com/dart-lang/linter/issues/3739
  • https://github.com/dart-lang/linter/issues/3801
  • https://github.com/dart-lang/linter/issues/2513
  • https://github.com/dart-lang/linter/issues/338

passsy avatar Nov 30 '22 11:11 passsy