[L] Change in dart/sdk[main]: [analyzer] Adds warning for missng await in return in try block
Felipe Morschel (Gerrit)
Felipe Morschel has uploaded the change for review![]( "Open in Gerrit")
Commit message
[analyzer] Adds warning for missng await in return in try block
Change information
- M pkg/analysis_server/lib/src/lsp/handlers/commands/fix_all_in_workspace.dart
- M pkg/analysis_server/lib/src/services/correction/dart/add_await.dart
- M pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml
- M pkg/analysis_server/lib/src/services/correction/fix_internal.dart
- M pkg/analysis_server/lib/src/utilities/profiling.dart
- M pkg/analysis_server/test/src/services/correction/fix/add_await_test.dart
- M pkg/analyzer/lib/src/diagnostic/diagnostic.g.dart
- M pkg/analyzer/lib/src/diagnostic/diagnostic_code_values.g.dart
- A pkg/analyzer/lib/src/error/async_return_visitor.dart
- M pkg/analyzer/lib/src/generated/error_verifier.dart
- M pkg/analyzer/messages.yaml
- M pkg/analyzer/test/src/diagnostics/test_all.dart
- A pkg/analyzer/test/src/diagnostics/unawaited_return_in_try_block_test.dart
- M pkg/dds/lib/src/dap/protocol_converter.dart
- M pkg/front_end/lib/src/base/crash.dart
- M pkg/test_runner/lib/src/browser_controller.dart
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel added 1 comment![]( "Open in Gerrit")
Once I get your approval, I (or you) will add the missing reviewers for the landing changes here.
Can I get a pre-review before landing https://github.com/flutter/flutter/pull/183466, please? Thanks a lot!
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
And just to note around here. This unblocks https://dart-review.googlesource.com/c/sdk/+/477660.
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 9 comments![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
9 comments
final TypeSystem typeSystem;Each of these fields can be private.
if (returnType is! TypeImpl) return;Why is this check here? `TypeSystem.futureValueType` accepts a DartType.
if (typeSystem is TypeSystemImpl &&Why do we need TypeSystemImpl? Both `isAssignableTo` and `futureValueType` are available on TypeSystem.
bool get withinTryBlock {This name is a little strange; it sounds like something that returns a ReturnStatement within a try block.
How about 'isWithinTryBlock`.
reportAtToken: _reportUnawaitedReturnInTryBlock,Why do we wrap up this instance method and send it to `AsyncReturnVisitor`? I think it would be much simpler to just send the `DiagnosticReporter`.
`async` function and within a `try` block without using `await`.Maybe more readable by flipping these two clauses:
.. is returned within a `try` block, in an `async` function, without using `await`.
I'm also not sure "using await" is the best verbiage, but maybe it's fine. Otherwise we might say something like "without awaiting the returned Future."
class UnawaitedReturnInTryBlockTest extends PubPackageResolutionTest {Great test coverage. I have only one more I can think of:
How about an `async*` generator? Return statements are not legal in `async*`, but I guess we should not compound the confusion, and we should not report this warning in this case.
Future<void> test_insideClosure() async {I think we need one more "within closure" test for a case I'm thinking of:
```
void foo() {
try {
var x = () async => return Future.value(null);
} catch (_) {}
}
```
In this case, the return statement is illegal, but I want to make sure we don't also report the lint. So if we encounter an ExpressionFunctionBody (I think), then we also want to stop searching up the syntax tree for a try-statement.
Future<void> test_sync() async {What does "sync" mean here?
Related details
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 9 comments![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
9 comments
Each of these fields can be private.
Thanks!
Why is this check here? `TypeSystem.futureValueType` accepts a DartType.
Recent change I opened because of the previous CL: https://github.com/dart-lang/sdk/issues/62583.
Thanks for noting!
Why do we need TypeSystemImpl? Both `isAssignableTo` and `futureValueType` are available on TypeSystem.
We needed them because of the previous comment: `TypeSystem.futureValueType` wasn't a thing yet.
This name is a little strange; it sounds like something that returns a ReturnStatement within a try block.
How about 'isWithinTryBlock`.
Yes, thanks!
reportAtToken: _reportUnawaitedReturnInTryBlock,Why do we wrap up this instance method and send it to `AsyncReturnVisitor`? I think it would be much simpler to just send the `DiagnosticReporter`.
Because we either do this or we also send the exact diagnostic and handle it differently here and over at the lint CL https://dart-review.googlesource.com/c/sdk/+/477660. I think this is easier to reason about for both cases, but if you disagree, I can change it.
`async` function and within a `try` block without using `await`.Maybe more readable by flipping these two clauses:
.. is returned within a `try` block, in an `async` function, without using `await`.
I'm also not sure "using await" is the best verbiage, but maybe it's fine. Otherwise we might say something like "without awaiting the returned Future."
Thanks!
class UnawaitedReturnInTryBlockTest extends PubPackageResolutionTest {Great test coverage. I have only one more I can think of:
How about an `async*` generator? Return statements are not legal in `async*`, but I guess we should not compound the confusion, and we should not report this warning in this case.
Thanks!
I think we need one more "within closure" test for a case I'm thinking of:
```
void foo() {
try {
var x = () async => return Future.value(null);
} catch (_) {}
}
```In this case, the return statement is illegal, but I want to make sure we don't also report the lint. So if we encounter an ExpressionFunctionBody (I think), then we also want to stop searching up the syntax tree for a try-statement.
Great case!
What does "sync" mean here?
I'll rename it. Since the return value of the `async` is not a `Future`, the internal completer handles it like a `sync` complete.
The test is better if named as `nonFuture`.
Related details
- Konstantin Shcheglov
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted and added 1 comment![]( "Open in Gerrit")
Votes added by Samuel Rawlins
| Code-Review | +1 |
1 comment
reportAtToken: _reportUnawaitedReturnInTryBlock,Felipe MorschelWhy do we wrap up this instance method and send it to `AsyncReturnVisitor`? I think it would be much simpler to just send the `DiagnosticReporter`.
Because we either do this or we also send the exact diagnostic and handle it differently here and over at the lint CL https://dart-review.googlesource.com/c/sdk/+/477660. I think this is easier to reason about for both cases, but if you disagree, I can change it.
Ah of course, because of the lint rule. That makes sense.
Related details
- Felipe Morschel
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Felipe Morschel (Gerrit)
Felipe Morschel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Felipe Morschel
| Auto-Submit | +1 |
1 comment
@sche...@google.com, could you please take a look at this?
I'd like to land this as soon as the Flutter PR (https://github.com/flutter/flutter/pull/183466) lands, so we can stop delaying this warning to avoid the bug explained in the issue.
Thanks a lot!
Related details
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |