Add unnecessary await in return lint #1981
Conversation
Can you expand on this? We're considering a language change down the line that would make this I don't think this is a good lint to use. |
@kevmoo do you know the story of the warning by any chance? Or maybe you can redirect me to someone? Interesting, thanks Nate for the heads up! I saw flutter using this rule so we also need to coordinate with them in this case. Could you please redirect me to any documents about the language change? I am also interested in it from the DDC perspective (ie changes that need to be done in DDC, if any). As for the rule rationale, I couldn't find any dart-specific explanation for the lint (I wish we had the rationale for all lints!) but my guess that the reasoning is similar to the JS one: https://eslint.org/docs/latest/rules/no-return-await It is probably implementation-dependent, but it makes sense that the caller is kept in stack until the callee finishes if the |
|
@annagrin – I'm not sure, honestly. 🤷 |
I would not expect the JS reasoning to apply in Dart. @sigmundch might know if there are any perf impacts of omitting
|
|
The commit introducing this lint doesn't give any reasoning, it looks very plausible to me that it was an arbitrary style choice. Note that in current implementations the |
| @@ -367,7 +367,7 @@ class Debugger extends Domain { | |||
| // TODO(alanknight): Can these be moved to dart_scope.dart? | |||
| final properties = await visibleProperties(debugger: this, frame: frame); | |||
| final boundVariables = await Future.wait( | |||
| properties.map((property) async => await _boundVariable(property)), | |||
| properties.map((property) async => _boundVariable(property)), | |||
There was a problem hiding this comment.
| properties.map((property) async => _boundVariable(property)), | |
| properties.map(_boundVariable) |
I'd have to measure to be sure, but removing an async along with the await does have a chance at bringing perf improvements. In the cases where it was the only await in the method it could be worth removing along with the async - even if for only style reasons.
This case is nice because it can go further and become a tearoff. I'd make this change independent of the rest of the PR.
There was a problem hiding this comment.
will do, thanks! I wasn't sure if there is any change in semantics:)
| return Future.wait(range | ||
| .map((element) async => await _instanceRefForRemote(element.value))); | ||
| return Future.wait( | ||
| range.map((element) async => _instanceRefForRemote(element.value))); |
There was a problem hiding this comment.
| range.map((element) async => _instanceRefForRemote(element.value))); | |
| range.map((element) => _instanceRefForRemote(element.value))); |
Less interesting case since it doesn't become a tearoff, but if the sole await gets removed it is worth dropping the async too.
There was a problem hiding this comment.
Thanks Nate, I will skip this PR for now and add the suggested change separately.
| @@ -273,7 +273,7 @@ class Locations { | |||
| Future<Set<Location>> _locationsForModule(String module) async { | |||
| final memoizer = _locationMemoizer.putIfAbsent(module, AsyncMemoizer.new); | |||
|
|
|||
| return await memoizer.runOnce(() async { | |||
| return memoizer.runOnce(() async { | |||
There was a problem hiding this comment.
This was the only await, could drop the outer async.
| @@ -588,7 +588,7 @@ ${globalLoadStrategy.loadModuleSnippet}("dart_sdk").developer.invokeExtension( | |||
|
|
|||
| @override | |||
| Future<ScriptList> getScripts(String isolateId) async { | |||
|
Closing this PR due to the unhelpful lint. I made some manual changes for removing unnecessary asyncs and awaits, as suggested by @natebosch, and some more similar ones: #1982 |
Add unnecessary await in return lint: