[M] Change in dart/sdk[main]: [cfe] Make VariableBase.context independent from VariableBase.parent
Chloe Stefantsova (Gerrit)
Chloe Stefantsova voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Johnni Winther
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnni Winther (Gerrit)
Johnni Winther voted Code-Review+1![]( "Open in Gerrit")
- Chloe Stefantsova
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
abstract VariableContext context;DBC
How is this going to work wrt kernel binary serializartion? At least for the VM, we need scopes, contexts and variables serialized before they can be used (to avoid multiple passes over kernel binary). So, during deserialization do you plan to create variable objects without parents and then corresponding VariableDeclaration statements would grab their ownership? How do we guarantee consistency if sub-tree containing VariableDeclaration is removed?
Related details
- Chloe Stefantsova
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
[cfe] Make VariableBase.context independent from VariableBase.parent
Previously the getter VariableBase.context was redirecting to
VariableBase.parent, effectively requiring a change of the ownership
over the variable. This CL makes VariableBase.context a field of
VariableBase, allowing for the previously existing ownership structure
of the AST tree nodes.
The change allows to avoid workarounds for the existing
ownership-restoring logic, such as the assignments to the `parent`
pointer made in the constructors of the AST nodes and their
`transformChildren` and `transformOrRemoveChildren` methods.
Part of https://github.com/dart-lang/sdk/issues/61572
- M pkg/front_end/lib/src/builder/formal_parameter_builder.dart
- M pkg/front_end/lib/src/kernel/body_builder.dart
- M pkg/front_end/lib/src/kernel/internal_ast.dart
- M pkg/front_end/lib/src/type_inference/inference_visitor.dart
- M pkg/kernel/lib/src/ast/statements.dart
- M pkg/kernel/lib/src/ast/types.dart
- M pkg/kernel/lib/src/ast/variables.dart
- M pkg/kernel/lib/src/equivalence.dart
- M pkg/kernel/lib/verifier.dart
Code-Review: +1 by Johnni Winther
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova added 1 comment![]( "Open in Gerrit")
abstract VariableContext context;DBC
How is this going to work wrt kernel binary serializartion? At least for the VM, we need scopes, contexts and variables serialized before they can be used (to avoid multiple passes over kernel binary). So, during deserialization do you plan to create variable objects without parents and then corresponding VariableDeclaration statements would grab their ownership? How do we guarantee consistency if sub-tree containing VariableDeclaration is removed?
Yes, we plan to serialize scopes, contexts, and variables. The parent pointers of the variables will point to the same nodes they point to now, while their context pointers will point to the contexts. As for the consistency in the sub-trees removal case, I don't see how having two pointers (one to the `TreeNode` parent, and the other one to the `VariableContext`) is a worse situation than what we currently have, with just one pointer. But maybe I'm missing something?
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexander Markov (Gerrit)
Alexander Markov added 1 comment![]( "Open in Gerrit")
abstract VariableContext context;Chloe StefantsovaDBC
How is this going to work wrt kernel binary serializartion? At least for the VM, we need scopes, contexts and variables serialized before they can be used (to avoid multiple passes over kernel binary). So, during deserialization do you plan to create variable objects without parents and then corresponding VariableDeclaration statements would grab their ownership? How do we guarantee consistency if sub-tree containing VariableDeclaration is removed?
Yes, we plan to serialize scopes, contexts, and variables. The parent pointers of the variables will point to the same nodes they point to now, while their context pointers will point to the contexts. As for the consistency in the sub-trees removal case, I don't see how having two pointers (one to the `TreeNode` parent, and the other one to the `VariableContext`) is a worse situation than what we currently have, with just one pointer. But maybe I'm missing something?
Please correct me if I'm wrong, but my understanding is that `parent` pointer also reflects "ownership" of the child nodes. With Scope->Context->Variable ownership it would be correct to remove sub-trees using that variable via non-owning pointers (similarly to how it is currently correct to remove VariableGet/VariableSet which references VariableDeclaration). However, if we keep parent pointers intact removing sub-tree with VariableDeclaration but keeping Variable in Context would mean there are dangling references to removed nodes (Context->Variable/VariableDeclaration->it's parent).
Related details
- Chloe Stefantsova
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova added 1 comment![]( "Open in Gerrit")
abstract VariableContext context;Chloe StefantsovaDBC
How is this going to work wrt kernel binary serializartion? At least for the VM, we need scopes, contexts and variables serialized before they can be used (to avoid multiple passes over kernel binary). So, during deserialization do you plan to create variable objects without parents and then corresponding VariableDeclaration statements would grab their ownership? How do we guarantee consistency if sub-tree containing VariableDeclaration is removed?
Alexander MarkovYes, we plan to serialize scopes, contexts, and variables. The parent pointers of the variables will point to the same nodes they point to now, while their context pointers will point to the contexts. As for the consistency in the sub-trees removal case, I don't see how having two pointers (one to the `TreeNode` parent, and the other one to the `VariableContext`) is a worse situation than what we currently have, with just one pointer. But maybe I'm missing something?
Please correct me if I'm wrong, but my understanding is that `parent` pointer also reflects "ownership" of the child nodes. With Scope->Context->Variable ownership it would be correct to remove sub-trees using that variable via non-owning pointers (similarly to how it is currently correct to remove VariableGet/VariableSet which references VariableDeclaration). However, if we keep parent pointers intact removing sub-tree with VariableDeclaration but keeping Variable in Context would mean there are dangling references to removed nodes (Context->Variable/VariableDeclaration->it's parent).
The dangling reference problem would exist in the case where the `parent` pointer points to the `VariableContext`, too. Considering the following Dart program:
```
foo(bool c) {
if (c) {
int x = 0;
return () => [c, x];
}
return null;
}
```
Before the work on scopes and contexts began we, roughly, had the following:
```
FunctionNode (with PositionalParameter('c', parent=FunctionNode))
Block #1 (parent=FunctionNode)
IfStatement (parent=Block #1)
Block #2 (parent=IfStatement)
VariableDeclaration('x') (parent=Block #2)
ReturnStatement #1 (parent=Block #2)
ReturnStatement #2 (parent=Block #1)
```
If we remove the `IfStatement` node, the remainder of the AST won't have any references to the `VariableDeclaration`.
```
FunctionNode (with PositionalParameter('c', parent=FunctionNode))
Block #1 (parent=FunctionNode)
ReturnStatement #2 (parent=Block #1)
```
Prior to this CL, with the experiment flag enabled, the same program would look, roughly, as follows:
```
FunctionNode (with reference to positional parameter 'c', Context(PositionalParameter('c', parent=Context), VariableDeclaration('x', parent=Context)))
Block #1 (parent=FunctionNode)
IfStatement (parent=Block #1)
Block #2 (parent=IfStatement)
VariableInitialization (with refrence to 'x', parent=Block #2)
ReturnStatement #1 (parent=Block #2)
ReturnStatement #2 (parent=Block #1)
```
When we remove the `IfStatement`, the context still holds `VariableDeclaration('x')` it owns:
```
FunctionNode (with reference to positional parameter 'c', Context(PositionalParameter('c', parent=Context), VariableDeclaration('x', parent=Context)))
Block #1 (parent=FunctionNode)
ReturnStatement #2 (parent=Block #1)
```
With the changes in this CL the situation with dangling references won't change too much. Here's, roughly, how the compiled program would look like:
```
FunctionNode (with PositionalParameter('c', parent=FunctionNode), Context(reference to 'c', reference to 'x'))
Block #1 (parent=FunctionNode)
IfStatement (parent=Block #1)
Block #2 (parent=IfStatement)
VariableInitialization (with VariableDeclaration('x', parent=VariableInitialization), parent=Block #2)
ReturnStatement #1 (parent=Block #2)
ReturnStatement #2 (parent=Block #1)
```
When the `IfStatement` is removed, the dangling reference to `x` is still there in the `Context`. The only difference is that the reference is non-owning.
```
FunctionNode (with PositionalParameter('c', parent=FunctionNode), Context(reference to 'c', reference to 'x'))
Block #1 (parent=FunctionNode)
ReturnStatement #2 (parent=Block #1)
```
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |