Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review unawaited to accept FutureOr #59877

Open
FMorschel opened this issue Jan 10, 2025 · 7 comments
Open

Review unawaited to accept FutureOr #59877

FMorschel opened this issue Jan 10, 2025 · 7 comments
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion.

Comments

@FMorschel
Copy link
Contributor

FMorschel commented Jan 10, 2025

The unawaited function is suggested as a fix for discarded_futures and currently takes only Future as a parameter so this is not a valid fix for FutureOr.

It should only show the fix to make the scope async.

@FMorschel FMorschel added the area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. label Jan 10, 2025
@lrhn lrhn added the analyzer-linter Issues with the analyzer's support for the linter package label Jan 10, 2025
@FMorschel
Copy link
Contributor Author

As a reference, this is coming from working on a fix for #59331.

@bwilkerson
Copy link
Member

The other possible solution is to change unawaited to take a FutureOr. I'm not sure why we wouldn't want to be able to flag a possible Future value as being ok to not await.

@FMorschel
Copy link
Contributor Author

FMorschel commented Jan 10, 2025

I agree that would be better, yes. That could also be a start for solving #59105

@FMorschel
Copy link
Contributor Author

In that case I'd also like to request that we reviewew the docs for unawaited mentioning discarded_futures too.

I see a mention to FutureExtensions.ignore on the docs, is there a similar thing for FutureOr? If we don't than maybe we could have?

@FMorschel FMorschel changed the title discarded_futures shows unawaited as fix for FutureOr Review unawaited to accept FutureOr Jan 10, 2025
@lrhn
Copy link
Member

lrhn commented Jan 10, 2025

We won't make unawaited accept FutureOr<Object?> because that type of just a long way to write Object?, and if the function accepts any type, it's too easy to use it incorrectly.

There are no extension methods on FutureOr<T> for any T for the same reason - it would be a method on every type, also those completely unrelated to futures. Like Stream.

In general, don't work with FutureOrs. Don't have APIs that return them.
Be async or don't, there is no good middle ground, that's just more work for everybody to have two code paths.

(I have no plan to mention discarded_futures until it's heavily revised to become a much more useful lint. There is a wish to make unawaited_futures a recommended lint. The current specified and implemented behavior for discarded_futures is not something I want to recommend to anyone.)

@FMorschel
Copy link
Contributor Author

About discarded_futures, I have a change open and waiting for review to make it more useful: https://dart-review.googlesource.com/c/sdk/+/403901. The change is that if the user typed Future as the type for something and we are assigning that, it should not warn. I'm assuming the user knows what has done and will handle that correctly in this case. But cases like var/final solely or no assignment would trigger the lint as it does today.

@lrhn if you disagree that this should be changed this way, maybe we could add an asFuture getter for it similar to FutureExtensions.ignore in the sense that we could then assign it there. WDYT?

@lrhn
Copy link
Member

lrhn commented Jan 11, 2025

I've commented on the CL. Not saying it's not an improvement, it's just not far-reaching enough for me :)

I don't think I'd want to add an asFuture getter on Object?.
If you have a FutureOr<Object>, await it. If you're in a non-async function, do a type test, then you have either a Future<Object> or just an Object, and we know how to handle those.
If you want to ignore it, maybe use // ignore: discarded_futures. But it's better to just not have a FutureOr to begin with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzer-linter Issues with the analyzer's support for the linter package area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion.
Projects
None yet
Development

No branches or pull requests

3 participants