[XL] Change in dart/sdk[main]: AST. Implement DirectAssignment and UnqualifiedNameAssignmentTarget.
Paul Berry (Gerrit)
Paul Berry added 4 comments![]( "Open in Gerrit")
/// A non-value source occurrence that denotes an assignment destination.Can you clarify what is meant by "non-value source occurrence"? I'm confused by:
- The meaning of "source occurrence"; is this just another way of saying "AST node"?
- What it means for a source occurrence to "not be a value". Maybe what you mean is that in contrast to an AST node whose runtime behavior includes generating an output value, the runtime behavior of an `AssignmentTarget` includes receiving an input value and storing it at a location (or propagating it to a setter function)?
/// A direct assignment using `=`.Can you add a few words here to contrast this with kinds of assignments that are not direct assignments?
/// A [ReachableNamedTargetResolution] describes the operations that can beCan you clarify the difference between `Reachable` and `Unreachable` with some examples? I'm not able to figure out what the difference is.
The AI suggestions seem to say that we use `Unreachable` for invocations whose target type is `Never`, and null-aware invocations whose target type is `Null`. Is that correct? If so, I'm concerned that the `Reachable` vs `Unreachable` nomenclature is going to be confusing, because reachability is a flow analysis concept that's related but not the same. E.g., in the code below, flow analysis considers both `isEven`s to be unreachable, but I believe that the notion you're defining here considers only the second `isEven` to be unreachable:
```
f(int i, Null n) {
if (n != null) {
print(i.isEven);
}
print(n?.isEven);
}
```
Assuming my analysis is correct, I would prefer if we could come up with different terminology than reachable vs. unreachable, to avoid confusion with the flow analysis notion of reachability.
/// The operations resolved for a named assignment target whose access path canOnce we resolve what to do about `NamedTargetResolution` (see my review comment above), I would appreciate if a little more detail could be added to this doc comment, so that a reader doesn't have to refer to the `NamedTargetResolution` doc comment to understand how this differs from `UnreachableNamedTargetResolution`.
Related details
- Johnni Winther
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 4 comments![]( "Open in Gerrit")
/// A non-value source occurrence that denotes an assignment destination.Can you clarify what is meant by "non-value source occurrence"? I'm confused by:
- The meaning of "source occurrence"; is this just another way of saying "AST node"?
- What it means for a source occurrence to "not be a value". Maybe what you mean is that in contrast to an AST node whose runtime behavior includes generating an output value, the runtime behavior of an `AssignmentTarget` includes receiving an input value and storing it at a location (or propagating it to a setter function)?
Done
Can you add a few words here to contrast this with kinds of assignments that are not direct assignments?
Done
/// A [ReachableNamedTargetResolution] describes the operations that can beCan you clarify the difference between `Reachable` and `Unreachable` with some examples? I'm not able to figure out what the difference is.
The AI suggestions seem to say that we use `Unreachable` for invocations whose target type is `Never`, and null-aware invocations whose target type is `Null`. Is that correct? If so, I'm concerned that the `Reachable` vs `Unreachable` nomenclature is going to be confusing, because reachability is a flow analysis concept that's related but not the same. E.g., in the code below, flow analysis considers both `isEven`s to be unreachable, but I believe that the notion you're defining here considers only the second `isEven` to be unreachable:
```
f(int i, Null n) {
if (n != null) {
print(i.isEven);
}
print(n?.isEven);
}
```Assuming my analysis is correct, I would prefer if we could come up with different terminology than reachable vs. unreachable, to avoid confusion with the flow analysis notion of reachability.
I think you are right, it was overcomplicated. I decided to fold everything into single `NamedTargetResolution`, where both `read` and `write` could be `null`.
/// The operations resolved for a named assignment target whose access path canOnce we resolve what to do about `NamedTargetResolution` (see my review comment above), I would appreciate if a little more detail could be added to this doc comment, so that a reader doesn't have to refer to the `NamedTargetResolution` doc comment to understand how this differs from `UnreachableNamedTargetResolution`.
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
Related details
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
Note, I thought some more about it, and will inline `NamedTargetResolution` into assignment targets, as `NamedReadResolution? get readResolution` and `NamedWriteResolution? get writeResolution`. I will ping the CL again, when it is ready.
Related details
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
The refactoring was done.
The CL is ready for review.
Related details
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Berry (Gerrit)
Paul Berry voted and added 1 comment![]( "Open in Gerrit")
Votes added by Paul Berry
| Code-Review | +1 |
1 comment
- Johnni Winther
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson added 2 comments![]( "Open in Gerrit")
The server changes lgtm.
Statement statement = statements.first;I'm not seeing anything to explain why this needs an explicit type, but I'll assume that it's necessary.
Related details
- Johnni Winther
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
Statement statement = statements.first;I'm not seeing anything to explain why this needs an explicit type, but I'll assume that it's necessary.
This is not jump back to API types, so that the type checks below promote.
Related details
- Brian Wilkerson
- Johnni Winther
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted Code-Review+1![]( "Open in Gerrit")
- Johnni Winther
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Johnni Winther
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
dart-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
dart-...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
AST. Implement DirectAssignment and UnqualifiedNameAssignmentTarget.
- M pkg/analysis_server/lib/src/services/correction/dart/convert_to_switch_expression.dart
- M pkg/analyzer/api.txt
- M pkg/analyzer/lib/analysis_rule/rule_visitor_registry.g.dart
- M pkg/analyzer/lib/dart/ast/ast.dart
- M pkg/analyzer/lib/dart/ast/visitor.g.dart
- M pkg/analyzer/lib/src/dart/analysis/driver.dart
- M pkg/analyzer/lib/src/dart/analysis/index.dart
- M pkg/analyzer/lib/src/dart/analysis/referenced_names.dart
- M pkg/analyzer/lib/src/dart/analysis/search.dart
- M pkg/analyzer/lib/src/dart/ast/ast.dart
- M pkg/analyzer/lib/src/dart/ast/ast.g.dart
- M pkg/analyzer/lib/src/dart/ast/element_locator.dart
- M pkg/analyzer/lib/src/dart/ast/to_source_visitor.dart
- M pkg/analyzer/lib/src/dart/constant/potentially_constant.dart
- M pkg/analyzer/lib/src/dart/micro/utils.dart
- M pkg/analyzer/lib/src/dart/resolver/assignment_expression_resolver.dart
- M pkg/analyzer/lib/src/dart/resolver/flow_analysis_visitor.dart
- M pkg/analyzer/lib/src/dart/resolver/property_element_resolver.dart
- M pkg/analyzer/lib/src/dart/resolver/resolution_visitor.dart
- M pkg/analyzer/lib/src/dart/resolver/this_lookup.dart
- M pkg/analyzer/lib/src/diagnostic/diagnostic_factory.dart
- M pkg/analyzer/lib/src/error/assignment_verifier.dart
- M pkg/analyzer/lib/src/error/best_practices_verifier.dart
- M pkg/analyzer/lib/src/error/const_argument_verifier.dart
- M pkg/analyzer/lib/src/error/dead_code_verifier.dart
- M pkg/analyzer/lib/src/error/element_usage_detector.dart
- M pkg/analyzer/lib/src/error/unused_local_elements_verifier.dart
- M pkg/analyzer/lib/src/error/use_result_verifier.dart
- M pkg/analyzer/lib/src/fasta/ast_builder.dart
- M pkg/analyzer/lib/src/generated/error_verifier.dart
- M pkg/analyzer/lib/src/generated/resolver.dart
- M pkg/analyzer/lib/src/generated/variable_type_provider.dart
- M pkg/analyzer/lib/src/hint/sdk_constraint_verifier.dart
- M pkg/analyzer/lib/src/lint/linter_visitor.g.dart
- M pkg/analyzer/lib/src/summary2/ast_binary_reader.dart
- M pkg/analyzer/lib/src/summary2/ast_binary_tag.dart
- M pkg/analyzer/lib/src/summary2/ast_binary_writer.dart
- M pkg/analyzer/lib/src/summary2/informative_data.dart
- M pkg/analyzer/lib/src/test_utilities/find_node.dart
- M pkg/analyzer/lib/src/util/ast_data_extractor.dart
- M pkg/analyzer/lib/src/wolf/ir/ast_to_ir.dart
- M pkg/analyzer/test/generated/collection_literal_parser_test.dart
- M pkg/analyzer/test/generated/complex_parser_test.dart
- M pkg/analyzer/test/generated/expression_parser_test.dart
- M pkg/analyzer/test/generated/nnbd_parser_test.dart
- M pkg/analyzer/test/generated/patterns_parser_test.dart
- M pkg/analyzer/test/generated/recovery_parser_test.dart
- M pkg/analyzer/test/generated/strong_mode_test.dart
- M pkg/analyzer/test/generated/top_level_parser_test.dart
- M pkg/analyzer/test/generated/utilities_test.dart
- M pkg/analyzer/test/src/dart/analysis/index_test.dart
- M pkg/analyzer/test/src/dart/analysis/search_test.dart
- M pkg/analyzer/test/src/dart/ast/element_locator_test.dart
- M pkg/analyzer/test/src/dart/ast/to_source_visitor_test.dart
- M pkg/analyzer/test/src/dart/resolution/assignment_test.dart
- M pkg/analyzer/test/src/dart/resolution/extension_method_test.dart
- M pkg/analyzer/test/src/dart/resolution/function_reference_test.dart
- M pkg/analyzer/test/src/dart/resolution/resolution.dart
- M pkg/analyzer/test/src/diagnostics/deprecated_member_use_test.dart
- M pkg/analyzer/test/src/diagnostics/invalid_use_of_internal_member_test.dart
- M pkg/analyzer/test/src/diagnostics/null_safety_read_write_test.dart
- M pkg/analyzer/test/src/diagnostics/sdk_version_since_test.dart
- M pkg/analyzer/test/src/diagnostics/unused_element_test.dart
- M pkg/analyzer/test/src/diagnostics/use_of_nullable_value_test.dart
- M pkg/analyzer/test/src/fasta/recovery/missing_code_test.dart
- M pkg/analyzer/test/src/summary/elements/const_test.dart
- M pkg/analyzer/test/src/summary/resolved_ast_printer.dart
- M pkg/analyzer/test/src/wolf/ir/ast_to_ir_test.dart
Code-Review: +1 by Paul Berry, +1 by Brian Wilkerson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |