Add unawaited to package:meta.

[davidmorgan]

Adding a nicer-looking way to suppress the unawaited_futures lint, so we can enforce it.

Once this is in and published I'll send a PR updating the lint description to point to this new way of suppressing the lint

dart-archive/linter@master...davidmorgan:document-unawaited-futures

[bwilkerson]

This marks an interesting departure. Prior to this the meta package only defined top-level constants and classes that were intended to be used as annotations (or "metadata", hence the package name). If this is accepted, then we are not only extending the content of the package to include top-level functions, but, more importantly, we are creating a precedent of using no-op functions to annotate expressions. I think we want to think carefully about creating such a precedent.

@leafpetersen @lrhn @eernstg Given that the language does not allow annotations to be associated with expressions (or statements, or anything other than declarations), what are your thoughts on using functions to annotate expressions?

If we decide to add this function, we might also consider putting it and any similar future functions in a separate library.

[davidmorgan]

Thanks. For context, see discussion on dart-lints@. The other option considered was using a comment:

/* unawaited */ doSomethingAsync();

But there seemed to be slight preference for a function. package:meta seems the logical place, since it's effectively an annotation. (Note however that we don't have to do anything special with it; you can use any function like this from any package and it will silence the unawaited_futures lint).

[bwilkerson]

Wow, that's a long thread.

I don't like the idea of a second comment-format for ignoring hints. Such "enhancements" tend to multiply to the point where they become unusable.

Given that there are other functions already being proposed, I'd really like to see them in a separate library, perhaps one dedicated to support (functions or annotations) related to dart:async.

[davidmorgan]

Yes, it's a hot topic :)

The other suggested methods were unsafeCast, which would signal to compilers that they're allowed to omit a type check, and allowDynamicCall, which turns off the --strict-dynamic-calls check. Ignoring the details of these proposals, it's not clear to me where we'd put them, or things of the same type, if not in package:meta. So I think the proposal is explicitly to expand the scope of package:meta to include functions acting as annotations. @matanlurey

(I hadn't actually worked out that the name meta refers to annotations, I'd always understood it in a more general sense.)

[bwilkerson]

The other suggested methods were unsafeCast, which would signal to compilers that they're allowed to omit a type check, and allowDynamicCall, which turns off the --strict-dynamic-calls check.

Oh, well, that's likely to be a whole other ball of wax. There has been strong opposition in the past to putting anything that controls the behaviors of the various compilers anywhere other than in the dart: libraries. I'll be surprised if that sentiment has changed.

Of course, unsafeCast might just work like unawaited in the sense that it's use effective accomplishes the goal without any need to change the compilers.

[matanlurey]

@bwilkerson:

Such "enhancements" tend to multiply to the point where they become unusable.

Luckily the lint is already unusable today (internally), so this makes it usable! If the trade-off is that we need to add a no-op function into a package whose goal is entirely to allow different types of static analysis, so be it?

@bwilkerson:

Putting it and any similar future functions in a separate library.

@davidmorgan:

So I think the proposal is explicitly to expand the scope of package:meta to include functions acting as annotations

I don't know anywhere there is a rule written that package:meta is constrained to just annotations. It's pretty ... meta to try to set strict guidelines on what is meta, no? I don't believe this hurts users not interested in these functions.

@bwilkerson:

Oh, well, that's likely to be a whole other ball of wax. There has been strong opposition in the past to putting anything that controls the behaviors of the various compilers ... I'll be surprised if that sentiment has changed.

There is already such configuration in package:meta:
https://github.com/dart-lang/sdk/blob/master/pkg/meta/lib/dart2js.dart

But as you mentioned, neither unsafeCast nor allowDynamicCalls actually change compiler behavior. They are both no-op functions that allow more strict types of static analysis to take place (ban implicit casts, ban dynamic calls, respectively).

[bwilkerson]

Such "enhancements" tend to multiply to the point where they become unusable.

Luckily the lint is already unusable today (internally), so this makes it usable! If the trade-off is that we need to add a no-op function into a package whose goal is entirely to allow different types of static analysis, so be it?

I'll note that by "enhancements" I was referring to additional comment structures, not functions. I don't want analyzer to have to try to interpret lots of different comment structures, nor have to worry about possible conflicts with comments that were not intended to suppress errors but happen to do so.

Putting it and any similar future functions in a separate library.

I don't know anywhere there is a rule written that package:meta is constrained to just annotations.

So, again, I never claimed that there's any rule. I was merely noting that this is a change from what we have accepted into the package before. I didn't even claim that the change was bad, only that I thought it was worth thinking about before accepting it.

Also, note that I said "library", not "package". We already have multiple libraries in the package, and I think it's worth considering whether we want this function to be in a separate library.

(And while it's not a rule, I will point out that the README states:

This package defines annotations that can be used by the tools that are shipped with the Dart SDK.

so we might also want to expand the README if we're adding this.)

[matanlurey]

@bwilkerson:

I don't want analyzer to have to try to interpret lots of different comment structures, nor have to worry about possible conflicts with comments that were not intended to suppress errors but happen to do so.

I thought the point of the linter was to do extra analysis you didn't want on (by default). It seems reasonable that if we opt into unawaited_futures, we'd do this analysis. If we need to fork the rule (unawaited_futures_google), I'd be open to that as well (/cc @davidmorgan) but it would be unfortunate.

So we might also want to expand the README if we're adding this.)

I'd be very happy to help expand the README.

[bwilkerson]

We're obviously not communicating well :-) Apologies for my part in it. I'll try again to be clear.

I don't want analyzer to have to try to interpret lots of different comment structures, nor have to worry about possible conflicts with comments that were not intended to suppress errors but happen to do so.

I thought the point of the linter was to do extra analysis you didn't want on (by default). It seems reasonable that if we opt into unawaited_futures, we'd do this analysis. If we need to fork the rule (unawaited_futures_google), I'd be open to that as well (/cc @davidmorgan) but it would be unfortunate.

Yes. The point of the linter is to do extra analysis that should be optional. Yes, if a user opts into a lint rule then the lint should run. But that's not what I'm talking about.

I was assuming a familiarity with the implementation that might not exist. We have architected the linter in order to attempt to make it as easy as possible to write lint rules. One aspect of that was a decision to not require lint authors to be concerned with whether their lint is being ignored in some location. The whole "ignore" comment mechanism is handled as a post-processing step. Lint authors generate all lints and the linter framework weeds out the ones that shouldn't be reported.

I don't want to ask lint authors to add code to check whether there is a special comment that disables their lint. But I also don't want to try to teach the linter framework about a large number of possible comment contents that should disable specific lints. There are several negatives to either approach, and I don't think either would be a positive change to the lint ecosystem.

There is already such configuration in package:meta:
https://github.com/dart-lang/sdk/blob/master/pkg/meta/lib/dart2js.dart

I missed commenting on this the first time, but I think it's important to clear up a potential misunderstanding.

That library is not used by dart2js.

The meta package was created by the analyzer team in order to define annotations (or functions, or whatever) that are used by the analyzer to provide additional hints or lints. The annotations in that library are specifically designed to provide additional analysis for users writing code that is expected to be compiled by dart2js. But dart2js does not use those annotations.

As long as none of the backends (other than analyzer) need to have special knowledge of the functions that are being proposed, then there won't likely be any push back.

But that is one of my concerns (though previously not explicitly expressed): the analyzer doesn't need to know about these functions either. The function unawaited could be defined anywhere and still have the effect of disabling the lint. The unsafeCast is the same way.

I would like to preserve the use of the meta package for things that are used by the analyzer. I understand the convenience argument, and I'm not necessarily saying "no", but I really don't want this package to be a catch all for things not needed by the analyzer. The unsafeCast function seems like a good example. Analyzer doesn't produce the error being avoided, so the meta package doesn't seem like the right place for it.

[matanlurey]

@bwilkerson:

We're obviously not communicating well :-) Apologies for my part in it. I'll try again to be clear.

No worries. We've tried to be very transparent and have lots of discussions in the open internally in dart-lints@ - it's impossible to make every internal discussion public - so please let us know if there is a better way to have internal-specific discussions with your team. I think dart-lints@ works well?

I don't want to ask lint authors to add code to check whether there is a special comment that disables their lint. But I also don't want to try to teach the linter framework about a large number of possible comment contents that should disable specific lints.

I totally understand this requirement. I have two ideas/points:

1. We already ask lint authors to add code to check for certain code to disable specific lints, that's called writing the lint 😏 - there are always cases that lints ignore and cases that lints lint, and sometimes we even split into multiple lints because it's not clear what cases should or should not be covered by a specific lint.

2. I would be open to, but regret, forking the linter project, and having one that complies more with internal Google requirements, but as a result, is harder to contribute to. We're trying to avoid that, though.

But dart2js does not use those annotations.

I mean, I guess we are defining use differently. Dart2JS does check for the existence of those annotations to compile differently. But maybe the wording I am looking for is utilizing and not using?

As long as none of the backends (other than analyzer) need to have special knowledge of the functions that are being proposed, then there won't likely be any push back

I talked at length about this with @dgrove @leafpetersen and @vsmenon already before suggesting these changes, and I did not get any pushback. If there is anyone else you feel I should try and reach out to/convince I'm happy to spend the time.

... [but] the function unawaited could be defined anywhere and still have the effect of disabling the lint. The unsafeCast is the same way

I guess I misunderstand, the idea was to statically check that unawaited is defined in package:meta/meta.dart (or wherever it goes), not just an arbitrary function called unawaited.

The unsafeCast function seems like a good example. Analyzer doesn't produce the error being avoided, so the meta package doesn't seem like the right place for it.

We can track the requirements for specific functions (like unsafeCast) elsewhere, but the idea here is just that is an allowed implicit cast. That is, you could use --no-implicit-casts, and still do an implicit cast if its done through a unsafeCast.

(Whether Dart2JS decides to emit different code in an unsafe/non-spec mode for that function is an entirely different discussion unrelated to this request or the analyzer)

[bwilkerson]

I think dart-lints@ works well?

Unfortunately, I'm not subscribed to that group. I don't have time to engage in all of the conversations there. I do care about the linter, but most of the conversations appear to revolve around whether and how to use existing lints. When the conversation changes to one about how to change the lints, linter, or meta package, then perhaps the analyzer team could be looped in.

We already ask lint authors to add code to check for certain code to disable specific lints ...

Sure, but I'm talking about a different kind of checking. At least in my mind they are different in significant ways.

But dart2js does not use those annotations.

I mean, I guess we are defining use differently.

No, I was just mistaken. Your comment prompted me to look again at the library, and I'm saddened to see that it isn't what I thought it was. The annotations do appear to be applicable only to dart2js and to have nothing to do with analyzer. Sigh.

... [but] the function unawaited could be defined anywhere and still have the effect of disabling the lint. The unsafeCast is the same way ...

I guess I misunderstand, the idea was to statically check that unawaited is defined in package:meta/meta.dart (or wherever it goes), not just an arbitrary function called unawaited.

As David already pointed out:

(Note however that we don't have to do anything special with it; you can use any function like this from any package and it will silence the unawaited_futures lint).

Analyzer won't care where the function is defined or what it's name is. The lint looks for expression statements within an async method where the type of the expression is Future. The proposed function has a return type of void, and that's all that's needed to disable the lint. Any function from any library that does not return Future could perform the same function.

I believe that unsafeCast is the same way. I think that anyone could write a generic function that encapsulates the cast and it would stop the CFE from reporting an error at the site of the invocation of that function. The CFE won't need to care what the function is named or where it was defined.

[matanlurey]

@bwilkerson:

When the conversation changes to one about how to change the lints, linter, or meta package, then perhaps the analyzer team could be looped in.

OK. We are hoping to keep the current processes pretty lightweight. If you have any feedback on how we could loop in your team more appropriately - neither too much nor too little I guess is the request - then @davidmorgan can definitely keep those ideas in mind! 👍

[davidmorgan]

Thanks Brian, Matan. I'll try to loop in people from the analyzer team on lint discussions when we start talking about changes to lints, that makes perfect sense.

Brian, thanks for clearing up "library" vs "package", I also got confused on that point. I currently think package:meta is the right package for unawaited, although I'm open to other suggestions. I'm not too worried about which library it ends up in, but I'm not sure I see a strong case for a new library, either.

It is true that the analyzer does not need to know about unawaited, but there is a dependency of sorts: the linter docs and the lint message itself will need to refer to it, making it the recommended way to suppress the lint. Here's a preview of what that could look like:

dart-archive/linter@master...davidmorgan:document-unawaited-futures

So in my mind even though it's a no-op, and could be defined anywhere and everywhere, it is part of the linter API.

Brian, do you have any suggestions for making progress on the PR, please? I agree that it needs careful consideration--it looks like you pinged various people who might weigh in but they do not seem to have done so yet :)--if the next step is to get feedback from them then I'll ping them.

That said, I'm completely open to take a step back and considering other options. My impression was that the discussion had reached the bikeshedding stage and it was time to send a PR, but that may have been premature.

I've updated the PR: updated README.md, added 'import dart:async', and changed the Future<Object> arg to a Future<void>.

Thanks!

[bwilkerson]

That said, I'm completely open to take a step back and considering other options.

Thanks! I really appreciate that.

I can understand why this method could look like it's strongly related to the linter, but the more I think about it the more convinced I am that we don't want functions like unawaited or unsafeCast to be in the meta package. I really think they belong in a separate (as in new) package. (I'm not sure what to call the new package, but that's because I'm not sure how to characterize what will go in it. Right now we're effectively talking about functions that get around limitations in the type system, but that seems like too narrow a definition for a package :-)

As for documentation for the lint, I'd probably say something like:

Fixing Violations

There are two common ways to fix a violation of this lint.

The first, and usually best, is to add an await before the flagged expression. This will change the semantics of the code by causing the code following the flagged expression to wait until after the future has been completed, but that's typically the semantics you want (and what this lint was designed to catch).

The second is to wrap the expression in the invocation of a function that does nothing other than accept a Future and return void. The function unawaited, defined in package:newPackage/async.dart is an example of such a function. This should only be done if you really want the computation of the Future's value to occur at an unknown time relative to the code following the flagged expression.

Of course, you can also, as always, add an 'ignore' comment to suppress the violation.

That makes it clear that there's nothing special about unawaited other than its signature. (And the lack of anything special about the function is one of the reasons I think it should not be in the meta package.)

[davidmorgan]

Thanks Brian. That's certainly an option. My main worry is that if we set the barrier to adoption too high, people will just use // ignore comments. Learning about a new package just to avoid // ignore might be asking too much. Adding a dependency on a package is already quite a lot to ask, I think, and because of that I wasn't really a big fan of using package:meta in the first place. I'm looking for the least worst option at this point :)

In the SDK it would be much more accessible/usable, but I don't want to go down that route because I think that if/when it does make it into the SDK it should be as a keyword, not a function. If we add a function to the SDK it'll be a lot more work to clean up later than if we add it to a package. I could be persuaded on this, but I don't think there is much enthusiasm for adding this sort of thing to the SDK anyway.

One option we didn't yet consider is to just define unawaited inline where it's needed. This is the same line of code count as adding the import, and avoids adding a dependency :/ ...

If that sounds ridiculous as a means to avoid writing '// ignore: unawaited_futures', maybe it is. But there are a few good reasons we don't want people to do that:

• The 'ignore' format is brittle; it has to be on the exact line before or as a suffix on the exact line where the warning would occur; for multi-line statements this can be awkward; and it can be broken by whitespace changes.
• There is no indication as to which lints are okay to ignore. For almost everything we're going to end up enforcing in google3, we don't expect any false positives. So we don't want to train people to use '// ignore'.

Maybe another way to approach this would be to ask: can we create a better story for all the lints that we know have false positives?

Any ideas greatly appreciated ;)

[bwilkerson]

My main worry is that if we set the barrier to adoption too high, people will just use // ignore comments. ... I'm looking for the least worst option at this point :)

That's a valid concern, but even putting it in meta won't guarantee that people will add a dependency on meta rather than adding a comment.

But from my perspective, adding it to meta is also a significantly bad option. :-)

... I don't think there is much enthusiasm for adding this sort of thing to the SDK anyway.

I agree, it seems unlikely that we could add it to the SDK.

One option we didn't yet consider is to just define unawaited inline where it's needed.

I believe this was mentioned in the other thread, but you could write

var _ = futureValuedFunction();

It has the advantage that it will often not add any lines to the code and should be fairly easy to spot in a code review (maybe even easier than unawaited(futureValuedFunction())).

The 'ignore' format is brittle ...

Yes, it is a bit. We discussed the format for a long time and eventually settled on this as a "least bad" option. But I'm happy to re-open the discussion if you have a better alternative.

There is no indication as to which lints are okay to ignore.

True. I suppose we could entertain the notion of an extended syntax for lints that allows users to specify whether a lint is ignorable, and then produce a hint if there is an ignore comment for an un-ignorable lint.

Maybe another way to approach this would be to ask: can we create a better story for all the lints that we know have false positives?

I think that's a great question to ask. I don't know that I have any answers to offer, but maybe others do.

[davidmorgan]

Thanks. Let me throw it back to the (internal) gallery...

[bwilkerson]

Perhaps add some or all of the analyzer team to the mailing list?

[natebosch]

I believe this was mentioned in the other thread, but you could write

var _ = futureValuedFunction();

It has the advantage that it will often not add any lines to the code and should be fairly easy to spot in a code review (maybe even easier than unawaited(futureValuedFunction())).

This doesn't scale well when there are multiple calls that aren't awaited.

var _ = returnsFutureString();
_ = returnsFutureInt();

Static error, can't assign Future<int> to Future<String>

Future _ = returnsFutureString();
_ = returnsFutureInt();

Noisier and adds ceremony when refactoring code - remove the call to returnsFutureString() and need to touch unrelated lines to move the variable declaration.

var _1 = returnsFutureString();
var _2 = returnsFutureInt();

Pretty ugly...

[bwilkerson]

I agree it's ugly. I did realize after writing that that if we were going to use this trick (which I don't think we are) that we should use void rather than var.

[a14n]

My preference would be to put such functions (used to silent linter warnings) directly in the linter package. That means that devs would have to add a linter dev_dependency in their pubspec.yaml (but if/when linter package becomes an analyzer plugin - #57754 - it would be the same thing).

[bwilkerson]

These functions would need to be used in production code, which would require a normal dependency. Depending on the linter package would pull in too many other dependencies. Putting these functions in a new package (that has no dependencies on anything else) would be much better.

[a14n]

My preference would be to put such functions (used to silent linter warnings) directly in the linter package.

I should have say: in the linter repository . So there will be 2 packages : the current linter and a new linter_helper (or something like that).

My point was that code used only to silence lints should be really close to the code defining the lints.

[bwilkerson]

I'm not sure why. Most of the functions being discussed do not need to be known to the linter. Instead, they work by changing the static type information in such a way that the lint simply won't fire. Hence, there's no reason for them to be in the linter's repo.

[matanlurey]

Indeed, some of the contents of the package (i.e. around dynamic calls) are unrelated to the linter.

[davidmorgan]

Closing this PR in favour of creating a new package. Thanks everyone!

[eernstg]

(Just coming back from a nice vacation ;-) I can see that this issue was just closed, but here's my response to a question that was raised here:

@leafpetersen @lrhn @eernstg Given that the language does not
allow annotations to be associated with expressions (or statements,
or anything other than declarations), what are your thoughts on using
functions to annotate expressions?

In the concrete examples that I've gleaned from this long thread, it seems that the need for declaring an expression to be 'unawaited' may be caused by questionable design choices elsewhere. Here's an example that was given to show what we might do when we don't want to await a future:

/* unawaited */ doSomethingAsync();

In the case where doSomethingAsync was designed to be a fire-and-forget action, it should have had return type void in the first place.

A fire-and-forget action should have some semantic encapsulation property (as in "this invocation will do things asynchronously, but nobody else depends on those things being completed", e.g., because it's writing to a finite logging buffer and it might take time until the logging process makes room for more data—but nobody else in this process depends on that). Conversely, if doSomethingAsync does work that any caller whatsoever may need to await the completion of then it is a delicate and error-prone choice for a particular call site to ignore (and "unawait") the returned future. This means that it should be a property of the callee whether the action is fire-and-forget, not a property of the call site.

So, ideally, there aren't any of these call sites because fire-and-forget async functions will have return type void, and expressions with static type Future<T> for some T should be awaited. Every time.

Not so ideally there will be exceptions, but if we can make them rare then there is no need to work really hard to make them concise or convenient—in which case an // ignore: unawaited_futures comment seems OK to me.

For the general question about using functions to annotate expressions: I suspect that it wouldn't be very good for the overall readability of the code (and I don't know whether we can rely 100% on compilers to inline those functions, so they may also cause some overhead at run time).

So if we can't avoid having them, it would at least be nice if we could use some naming convention to make them stand out as "not a real function call, just an annotation", say lintIgnore_UnsafeCast(e).

But my primary recommendation would be to use // ignore comments also for expressions (maybe by declaring fresh local variables in order to be able to associate a specific expression with such a comment), and then strive to keep the rest of the ecosystem in a good shape such that this kind of exception isn't needed very often.

[davidmorgan]

Thanks Eric. The topic of whether APIs can know if callers will always want to 'await' was discussed on the internal thread. Logging came up as an example of something it should be possible to await, but that most callers will not want to await.

The main argument against using // ignore comments is that they work for all errors, warnings, hints and lints. Most of these should never be ignored. So we want something different to make it very clear what's on the allowed path (in the coding style sense of allowed) and what isn't.

And since we want something new/different, we have a choice between making the comments more powerful and adding functions. Functions are also nicer syntactically, and that leads to the current proposal :)

[eernstg]

The main argument against using // ignore comments is that they work for all errors..

Oh, then I misunderstood. I assumed that basically all occurrences of // ignore in this thread really meant // ignore: unawaited_futures.

[davidmorgan]

Well, specifically for this case, we do mean // ignore: unawaited_futures. But the problem in telling people they should use that (i.e. creating a workflow where it's expected) is that they may then also think it's okay to write e.g. // ignore: unused_imports, and we'd really prefer they didn't.

[a14n]

they may then also think it's okay to write e.g. // ignore: unused_imports, and we'd really prefer they didn't.

Now you need a new lint only_ignore_lints :)

[eernstg]

// ignore: ignore. ;-)

[davidmorgan]

:D

There are a bunch of lints we don't want people to ignore, either. All the ones we've enforced so far, for example:

https://github.com/dart-lang/linter/blob/master/example/google.yaml