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

Move unawaited to package:meta? #2522

Closed
goderbauer opened this issue Mar 17, 2021 · 14 comments
Closed

Move unawaited to package:meta? #2522

goderbauer opened this issue Mar 17, 2021 · 14 comments
Labels
type-enhancement A request for a change that isn't a bug

Comments

@goderbauer
Copy link
Contributor

The unawaited_futures lint is part of the core.yaml set of lints [1] that will be distributed as part of a new package. It is a little odd that in order to fully use it and get access to the unawaited function recommended by the lint you have to also depend on package:pedantic, which is yet another unrelated package bundling opinionated linter rules.

Maybe the unawaited function recommended by the lint should live somewhere else, like the meta package? That package is home to most other annotations that modify how lint rules and the analyzer behave.

/cc @pq

[1] https://github.com/dart-lang/linter/blob/bf95e79bccaedaa17824e364fad8c291a1543bd6/tool/canonical/core.yaml

@goderbauer
Copy link
Contributor Author

When we turn on unawaited_futures for Flutter, we would want to avoid adding an extra dependency to package:pedantic because that will lock our users into a particular version of that package. Moving unawaited to meta (which we already depend on) would help with that. If that's not possible, we'd probably have to look into backing the unawaited functionality right into the Flutter framework.

@bwilkerson
Copy link
Member

We attempted to move the unawaited() function from pedantic to meta awhile back, but ran into some migration issues that we didn't have time to solve, so we temporarily abandoned the effort.

Since then a proposal was made to add an extension method (getter) to Future<T>? to have the same effect as the function. The discussion is in dart-lang/sdk#45284 if you're interested. This has the advantage that it avoids the migration issue that we ran into earlier.

@pq
Copy link
Member

pq commented Mar 17, 2021

Taking it further, I wonder if we might consider landing this in the SDK proper?

/cc @lrhn

@lrhn
Copy link
Member

lrhn commented Mar 18, 2021

No reason to put it into the SDK. It doesn't do anything.

The unawaited function/extension getter is just a way placate the linter with the unawaited_futures lint on.
It belongs with other linter/analyzer affecting things.

@pq
Copy link
Member

pq commented Mar 18, 2021

It doesn't do anything.

Thanks @lrhn! I guess my thinking is that an increasing number of people are depending on it and more still if unawaited_futures becomes part of the core set of "scoring" lints. Other folks have suggested this should actual graduate to a keyword but I can see how that may be a bridge too far. Adding a function is just a compromise.

Alternatively, I'm drawn to the idea of fire-and-forget annotations on the declaration side (#2513); curious how that sits with you. (And @davidmorgan, if that would make unawaited unnecessary in a significant percentage of internal cases?)

@lrhn
Copy link
Member

lrhn commented Mar 18, 2021

Putting more annotations in the platform libraries sits badly with me, unsurprisingly. The fewer the better, and with @deprecated, @Since and @pragma, I think we're covered.

I don't think annotations on the declarations is the way to go. It assumes that the declaration point knows how the result will be used, which is a stretch. It's probably sometimes true that an async function can say that it's return value is FYI only, and only if it can guarantee that the future will never complete with an error, but most such async functions are written in user libraries. The platform library async functions are generally so generic and user-data dependent that they it's hard to say anything universal about them.
If we want to do it anyway, and want to annotate platform library declarations, the pragma annotation could be used, say, as @pragma("analyzer:allow_unawaited"). It's a tool-facing, not user-facing, annotation, so I'd still add some other annotation to package:meta that users can use instead (even if it's just a nicer name for the same pragma).

Since we are recommending this lint, we might want to expose the same tool in other places as well, possibly re-exporting it from whichever package we distribute the recommended lints using. I still think package:meta is the right origin for the feature.

@mit-mit
Copy link
Member

mit-mit commented Mar 19, 2021

It's probably sometimes true that an async function can say that it's return value is FYI only, and only if it can guarantee that the future will never complete with an error, but most such async functions are written in user libraries.

I believe @goderbauer had examples of framework async functions that do know if the return value is FYI or not?

@lrhn
Copy link
Member

lrhn commented Mar 19, 2021

Ack. To me, framework libraries are also "user libraries" 😁. I meant it as non-platform libraries.

@bwilkerson
Copy link
Member

Maybe the unawaited function recommended by the lint should live somewhere else, like the meta package?

Two additional thoughts.

What if we stopped recommending unawaited in the lint. It is equally valid to use an ignore comment to silence the lint. The function was only added to make it possible for internal code to silence the lint without allowing the use of the ignore comment (because the comment was seen as being too general).

Note that in some other issue there is discussion of adding an annotation to mark functions whose returned future can safely be ignored. I have to wonder whether that would cover most or all of the places where this lint is currently being ignored (using either mechanism) and hence largely negate the need for a way to silence the lint, making the function even less useful/necessary.

@srawlins
Copy link
Member

I have to wonder whether that would cover most or all of the places where this lint is currently being ignored (using either mechanism) and hence largely negate the need for a way to silence the lint, making the function even less useful/necessary.

I hypothesize that something like this is true. Something like: 80% of unawaited calls are made against 20% of Future-returning functions that are ever "unawaited". I would strongly recommend a little study in google3 / pub via surveyor (or something like it).

@davidmorgan
Copy link
Contributor

For google3 we have gone pretty strongly for never advising use of '// ignore' comments; I don't think that will change for 'unawaited' even if it's only 20% or 5% of uses.

I don't mind if the external lint message / doc changes, though, since we can now set a google3-specific one if we want.

@bwilkerson
Copy link
Member

To be clear, I wasn't suggesting changes to internal usage, only changing the messaging externally. And external users would still be able to use unawaited if they want to, we just wouldn't recommend it any longer.

Although if my hypothesis is true, and we could annotate the appropriate internal APIs as not requiring an await, then we could reduce the number of dependencies that some packages have, which would have some benefit.

@subzero911
Copy link

subzero911 commented Nov 15, 2021

After deprecating of pedantic and introducing flutter_lints, how about moving unawaited to some core library?

Upd. Already done https://api.dart.dev/stable/2.14.2/dart-async/unawaited.html

@srawlins
Copy link
Member

srawlins commented Nov 15, 2021

@subzero911 you may be looking for dart:async's unawaited.

I think this issue can be closed.

Edit: Oops I missed your update. 😄

@srawlins srawlins added the type-enhancement A request for a change that isn't a bug label Aug 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

8 participants