warning on missing await in non-async function.

[bsutton]

I have a problem with the following code:

void recreateDir(String path)  {
  deleteDir(path);
  createDir(path);
}

Future<void> deleteDir(String path) async {};

Future<void> createDir(String path) async {};

Dart will happily let me call async functions from a non-async function without awaiting them.

In the above example it is problematic to call createDir before deleteDir which may occur if they are unawaited.

If the method createDir is async and I use the lint 'unawaited_futures' I get an appropriate warning.
However if I forget to add the async keyword to the createDir signature then I have a serious problem and no warning.

We need a lint to warn of the above issue.
The above example is a mistake that I make regularly in my own code and its rather hard to debug.

[eernstg]

unawaited_futures targets expressions in async function bodies only, and it would probably be inconveniently aggressive to make it target other function bodies.

However, the actual problem of not awaiting certain futures might very well arise in a non-async function body because of a modification of some other part of a software system: Something changes from non-async to async, say, a top-level function void foo() ... changes to Future<void> foo() ..., and now we're suddenly discarding a future at the statement foo(); in a function bar, even though nothing changed in the library that declares bar. In that situation, the right fix could be to make bar's body async as well, and do await foo();.

So if we don't want to make unawaited_futures target non-async bodies as well, we might need a lint that flags situations where a future is discarded in a non-async body?

[bsutton]

Whilst a library changing a return type is an interesting edge case, the core case and the one that trips my over continuously is simply calling an async function from an non-async method. I managed to do this on a regular basis and would imagine that it affects other users equally. Is there actually a reason why the linter does't report futures being discarded in a non-async body? What is the use case? Why does Dart allow async methods to be called from an non-async method at all? There seems to be a use case where user ishappy to run and forget an async method but this seems to be handled via the 'unawaited' method. Surely the same logic applies to this situation? As it stands this 'oversight' is a significant source of bugs which are extremely difficult to find. S. Brett Sutton Noojee Contact Solutions 03 8320 8100

…

On Wed, 15 Sept 2021 at 18:52, Erik Ernst ***@***.***> wrote: unawaited_futures targets expressions in async function bodies only, and it would probably be inconveniently aggressive to make it target other function bodies. However, the actual problem of not awaiting certain futures might very well arise in a non-async function body because of a modification of some other part of a software system: Something changes from non-async to async, say, a top-level function void foo() ... changes to Future<void> foo() ..., and now we're suddenly discarding a future at the statement foo(); in a function bar, even though nothing changed in the library that declares bar. In that situation, the right fix could be to make bar's body async as well, and do await foo();. So if we don't want to make unawaited_futures target non-async bodies as well, we might need a lint that flags situations where a future is *discarded* in a non-async body? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#58512>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OBGYSYPIXZLVPUTB7LUCBNFNANCNFSM5EBQJZMA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[eernstg]

I agree that, in my example, the invocation of foo(); as a statement could very will be written when the return type of foo is already Future<void>. In other words, we care about the situation, not the fact that it could also arise because of a change in some other library (but the latter makes software maintenance particularly difficult).

There's nothing wrong per se with normal non-async code that calls a function that returns a future: That future could be stored (say, in a set), and some other piece of code could retrieve it and await it later. That's the reason why I'm focusing on non-async code that silently discards a future.

[bsutton]

That future could be stored (say, in a set),

So how are these two methods fundamentally different? /// void testSync() { _callasync1(); _alist.add(_callasync2()); } /// Future<void> testAsync() async { _callasync1(); _alist.add(_callasync2()); } Future<int> _callasync1() async => Future.value(1); Future<int> _callasync2() async => Future.value(1); var _alist = <Future<int>>[]; The testAsync() method generates a warning on the call to _callasync1 whilst the testSync method does't generate a warning. In both cases the lack of an await signals that there may be a coding error. What I don't understand is what is the design purpose behind allowing a sync method to call async methods. Even with my lack of understanding of the design intent it would appear that linting both to require unawaited doesn't appear to be harmful and will stop common and difficult to find bugs. Right now, to find the bug in testSync you actually have to inspect each line that contains a method call and check the doco for each method. This is tedious. The addition of a lint to catch this seems to meet the criteria for adding lints. I also don't understand your statement:

That's the reason why I'm focusing on non-async code that silently

*discards* a future. Is that not the core issue I've raised? I feel like I'm missing some subtle difference in interpretation. S. Brett Sutton Noojee Contact Solutions 03 8320 8100

…

On Wed, 15 Sept 2021 at 20:23, Erik Ernst ***@***.***> wrote: I agree that, in my example, the invocation of foo(); as a statement could very will be written when the return type of foo is already Future<void>. In other words, we care about the situation, not the fact that it could also arise because of a change in some other library (but the latter makes software maintenance particularly difficult). There's nothing wrong per se with normal non-async code that calls a function that returns a future: That future could be stored (say, in a set), and some other piece of code could retrieve it and await it later. That's the reason why I'm focusing on non-async code that silently *discards* a future. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#58512>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OAVMNUVX3STHEFLKC3UCBXYTANCNFSM5EBQJZMA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[eernstg]

The testAsync() method generates a warning on the call to _callasync1
whilst the testSync method does't generate a warning.

No, unawaited_futures would lint testAsync() because the invocation _callasync1() does not await the returned future, and the new lint I suggested we'd need (call it discarded_futures) would lint testSync() because the expression statement _callasync1(); discards the returned future.

None of the invocations of _alist.add get a lint, because none of them imply that the future returned from callasync... will be ignored. As I mentioned:

There's nothing wrong per se with normal non-async code that
calls a function that returns a future: That future could be stored ...

and that could of course also happen in an async method.

@bsutton wrote:

what is the design purpose behind allowing a sync method to call async methods

A sync method can call an async method, obtain one or more futures, and handle each of them just like any other object that needs further processing. In particular, the sync method doesn't have to perform any await actions itself, because it could be part of the application logic that the futures are stored somehow, and some async method will discover this and await them.

That's the reason why I'm focusing on non-async code that silently
discards a future.

Is that not the core issue I've raised?

You've raised the issue that it is a problem that unawaited_futures skips over testSync entirely, and I agree that this is a problem. That's the reason why I'm suggesting a new lint, discarded_futures, to flag it as problematic.

With that new lint, you could enable both, and you would then get the diagnostic messages that you want. All is good.

At the same time, everybody else would get to control the situation when they decide whether or not to enable discarded_futures: If they enable it, and it turns out to flag thousands of locations where they do discard a future in sync code, and they do know what they are doing, then they can just ignore the new lint. More likely, they actually do catch some bugs if they enable the new lint, and they might then do it just a few libraries at a time, such that they can fix it at their own pace.

But if we just abruptly change unawaited_futures such that it flags testSync, then (1) the developer can't actually do await inside testSync, and (2) the problem is not that there is a missing await, the problem is that we might never be able to perform the await if we discard the future. It's still possible that the developers will need to change testSync to be an async method (and return a future itself), but that may be a much, much bigger effort, because it could propagate to any number of other declarations.

[bsutton]

OK, so I think we are on the same page. I agree that a new lint is required and discarded_futures sounds like a reasonable name. I assume that the new lint will recommend adding an 'unawaited' wrapper to the call? If so then that would be great. S. Brett Sutton Noojee Contact Solutions 03 8320 8100

…

On Thu, 16 Sept 2021 at 21:59, Erik Ernst ***@***.***> wrote: The testAsync() method generates a warning on the call to _callasync1 whilst the testSync method does't generate a warning. No, unawaited_futures would lint testAsync() because the invocation _callasync1() *does not await* the returned future, and the new lint I suggested we'd need (call it discarded_futures) would lint testSync() because the expression statement _callasync1() *discards* the returned future. None of the invocations of _alist.add get a lint, because none of them imply that the future returned from callasync... will be ignored. This is the case that I mentioned: There's nothing wrong per se with normal non-async code that calls a function that returns a future: That future could be stored ... @bsutton <https://github.com/bsutton> wrote: what is the design purpose behind allowing a sync method to call async methods A sync method can call an async method, obtain one or more futures, and handle each of them just like any other object that needs further processing. In particular, the sync method doesn't have to perform any await actions itself, because it could be part of the application logic that the futures are stored somehow, and some async method will discover this and await them. That's the reason why I'm focusing on non-async code that silently *discards* a future. Is that not the core issue I've raised? You've raised the issue that it is a problem that unawaited_futures skips over testSync entirely, and I agree that this is a problem. That's the reason why I'm suggesting a new lint, discarded_futures, to flag it as problematic. With that new lint, you could enable both, and you would then get the diagnostic messages that you want. All is good. At the same time, everybody else would get to control the situation when they decide whether or not to enable discarded_futures: If they enable it, and it turns out to flag thousands of locations where they do discard a future in sync code, and they do know what they are doing, then they can just ignore the new lint. More likely, they actually do catch some bugs if they enable the new lint, and they might then do it just a few libraries at a time, such that they can fix it at their own pace. But if we just abruptly change unawaited_futures such that it flags testSync, then (1) the developer can't actually do await inside testSync, and (2) the problem is not that there is a missing await, the problem is that *we might never be able to perform the await if we discard the future*. It's still possible that the developers will need to change testSync to be an async method (and return a future itself), but that may be a much, much bigger effort, because it could propagate to any number of other declarations. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58512>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG32OEI3GYQ6ZB5KQR3QZDUCHLZJANCNFSM5EBQJZMA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[srawlins]

I believe this has been implemented as discarded_futures.

[bsutton]

closing as discared_futures resolves my problem.

Thanks.