[XL] Change in dart/sdk[main]: Take steps to support anonymous block bodies
Erik Ernst (Gerrit)
Erik Ernst added 1 comment![]( "Open in Gerrit")
The tests `$SDK/tests/language/anonymous_methods/block/*_test.dart` are now accepted by the analyzer.
Related details
- 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 added 3 comments![]( "Open in Gerrit")
final TypeImpl? _returnType;It took me a while to figure out what this is for.
I *think* what it's for is to allow `ErrorVerifier.visitAnonymousMethodInvocation` to override the standard behavior of the `returnType` getter (which is to look up the return type in the element), because I guess anonymous method invocations don't have an associated element?
But if they don't have elements, why not? Our usual rule is that something needs to be an element if you can refer to it by name, OR if it's needed as the parent element to something you *can* refer to by name. If an anonymous method has a parameter, then that parameter must have a parameter element, and that parameter element should have a parent which is the anonymous method element, right?
Assuming that's true (that anonymous methods *do* have elements), then you shouldn't have to modify `EnclosingExecutableContext` at all; you should just be able to pass in the correct element when you call the constructor.
var returnType =This seems wrong to me for a couple of reasons:
1. Converting a context type to a type is not safe, because a context type can have holes in it (e.g., the context might be `List<_>`).
2. The return type of the anonymous method should have been determined precisely (using type inference) by the resolver, so using the context type is not necessarily incorrect; the resolver might have chosen a different type based on the types appearing in `return` statements in the body.
flowAnalysis.flow?.handleBreak(enclosingBody);Please add a comment:
```
// TODO(paulberry): add a `FlowAnalysis.handleReturn` method so that the resolver
// doesn't have to pretend that this is a break statement in order to get correct
// behavior.
```
I will address this in a follow-up CL.
Related details
- Erik Ernst
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ernst (Gerrit)
Erik Ernst added 4 comments![]( "Open in Gerrit")
PTAL - I made the requested changes as far as possible (for me, at this time ;-), and I need some more help in order to know that I'm going in the right direction.
final TypeImpl? _returnType;It took me a while to figure out what this is for.
I *think* what it's for is to allow `ErrorVerifier.visitAnonymousMethodInvocation` to override the standard behavior of the `returnType` getter (which is to look up the return type in the element), because I guess anonymous method invocations don't have an associated element?
But if they don't have elements, why not? Our usual rule is that something needs to be an element if you can refer to it by name, OR if it's needed as the parent element to something you *can* refer to by name. If an anonymous method has a parameter, then that parameter must have a parameter element, and that parameter element should have a parent which is the anonymous method element, right?
Assuming that's true (that anonymous methods *do* have elements), then you shouldn't have to modify `EnclosingExecutableContext` at all; you should just be able to pass in the correct element when you call the constructor.
In an earlier iteration of this work I did create a new kind of element to represent the anonymous method. However, this caused several other requirements to arise: About 45 element visitors were suddenly broken, and there was a need to introduce a fragment to deliver from `declaredFragment`, and various other stuff that I don't remember in full detail.
I think we might have discussed this, but in any case I tried to get by without a new element class, thinking that it would be sufficient for the anonymous method to have a kind of element which is minimal by reusing an existing `...Element` class. In the end it should presumably just be a node in the tree such that there is no unexpected null value in a `parent`, and hopefully the formal parameter element doesn't really need any services from it.
However, I went back to this strategy and introduced a few private classes in error_verifier.dart starting from line 204. It just takes a couple of steps (and each step makes it stop with a new member which should have been there).
Could you give me a hint about a systematic method to discover how to build this connected graph of classes and objects that are needed in order to introduce a new kind of element?
var returnType =This seems wrong to me for a couple of reasons:
1. Converting a context type to a type is not safe, because a context type can have holes in it (e.g., the context might be `List<_>`).
2. The return type of the anonymous method should have been determined precisely (using type inference) by the resolver, so using the context type is not necessarily incorrect; the resolver might have chosen a different type based on the types appearing in `return` statements in the body.
Right, the correct return type is actually available from the enclosing `AnonymousMethodInvocationImpl.staticType`. I'll close this thread because the previous comment carries the required information about what is not yet done in the appropriate manner.
flowAnalysis.flow?.handleBreak(enclosingBody);Please add a comment:
```
// TODO(paulberry): add a `FlowAnalysis.handleReturn` method so that the resolver
// doesn't have to pretend that this is a break statement in order to get correct
// behavior.
```I will address this in a follow-up CL.
- 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 added 1 comment![]( "Open in Gerrit")
Adding Konstantin for context. We are currently discussing what we want the element model to look like.
Related details
- Erik Ernst
- 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")
final TypeImpl? _returnType;Erik ErnstIt took me a while to figure out what this is for.
I *think* what it's for is to allow `ErrorVerifier.visitAnonymousMethodInvocation` to override the standard behavior of the `returnType` getter (which is to look up the return type in the element), because I guess anonymous method invocations don't have an associated element?
But if they don't have elements, why not? Our usual rule is that something needs to be an element if you can refer to it by name, OR if it's needed as the parent element to something you *can* refer to by name. If an anonymous method has a parameter, then that parameter must have a parameter element, and that parameter element should have a parent which is the anonymous method element, right?
Assuming that's true (that anonymous methods *do* have elements), then you shouldn't have to modify `EnclosingExecutableContext` at all; you should just be able to pass in the correct element when you call the constructor.
In an earlier iteration of this work I did create a new kind of element to represent the anonymous method. However, this caused several other requirements to arise: About 45 element visitors were suddenly broken, and there was a need to introduce a fragment to deliver from `declaredFragment`, and various other stuff that I don't remember in full detail.
I think we might have discussed this, but in any case I tried to get by without a new element class, thinking that it would be sufficient for the anonymous method to have a kind of element which is minimal by reusing an existing `...Element` class. In the end it should presumably just be a node in the tree such that there is no unexpected null value in a `parent`, and hopefully the formal parameter element doesn't really need any services from it.
However, I went back to this strategy and introduced a few private classes in error_verifier.dart starting from line 204. It just takes a couple of steps (and each step makes it stop with a new member which should have been there).
Could you give me a hint about a systematic method to discover how to build this connected graph of classes and objects that are needed in order to introduce a new kind of element?
See how we build fragments and elements in `ElementBindingVisitor`, specifically `visitFunctionExpression`. Note that we create `LocalFunctionFragmentImpl` and then use it to wrap as enclosingFragment typeParameters and formalParameters.
```
var holder = ElementHolder(fragment);
_withElementHolder(holder, () {
super.visitFunctionExpression(node);
fragment.typeParameters = holder.typeParameters;
fragment.formalParameters = holder.formalParameters;
});
```
I think there is a lot of similarities with anonymous methods.
Related details
- Erik Ernst
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")
final TypeImpl? _returnType;Erik ErnstIt took me a while to figure out what this is for.
I *think* what it's for is to allow `ErrorVerifier.visitAnonymousMethodInvocation` to override the standard behavior of the `returnType` getter (which is to look up the return type in the element), because I guess anonymous method invocations don't have an associated element?
But if they don't have elements, why not? Our usual rule is that something needs to be an element if you can refer to it by name, OR if it's needed as the parent element to something you *can* refer to by name. If an anonymous method has a parameter, then that parameter must have a parameter element, and that parameter element should have a parent which is the anonymous method element, right?
Assuming that's true (that anonymous methods *do* have elements), then you shouldn't have to modify `EnclosingExecutableContext` at all; you should just be able to pass in the correct element when you call the constructor.
Konstantin ShcheglovIn an earlier iteration of this work I did create a new kind of element to represent the anonymous method. However, this caused several other requirements to arise: About 45 element visitors were suddenly broken, and there was a need to introduce a fragment to deliver from `declaredFragment`, and various other stuff that I don't remember in full detail.
I think we might have discussed this, but in any case I tried to get by without a new element class, thinking that it would be sufficient for the anonymous method to have a kind of element which is minimal by reusing an existing `...Element` class. In the end it should presumably just be a node in the tree such that there is no unexpected null value in a `parent`, and hopefully the formal parameter element doesn't really need any services from it.
However, I went back to this strategy and introduced a few private classes in error_verifier.dart starting from line 204. It just takes a couple of steps (and each step makes it stop with a new member which should have been there).
Could you give me a hint about a systematic method to discover how to build this connected graph of classes and objects that are needed in order to introduce a new kind of element?
See how we build fragments and elements in `ElementBindingVisitor`, specifically `visitFunctionExpression`. Note that we create `LocalFunctionFragmentImpl` and then use it to wrap as enclosingFragment typeParameters and formalParameters.
```
var holder = ElementHolder(fragment);
_withElementHolder(holder, () {
super.visitFunctionExpression(node);
fragment.typeParameters = holder.typeParameters;
fragment.formalParameters = holder.formalParameters;
});
```I think there is a lot of similarities with anonymous methods.
You will need to add `LocalFunctionFragmentImpl declaredFragment` to `AnonymousMethodInvocation`. Which sounds unusual, as now invocation has its own declaration. I see the comment describes it in terms of `AnonymousMethod`, which would be more natural place for `declaredFragment` IMHO. But maybe it is just unusual to me.
FYI, we consider splitting `LocalFunctionElement` into `LocalExecutableElement` superclass with leaf subclasses `LocalFunctionElement` proper, `FunctionExpressionElement`, and `AnonymousMethodElement`. This will align elements with syntax, which sometimes is useful. Nothing for you to do here, for now we will continue using `LocalFunctionFragmentImpl` (and `LocalFunctionElementImpl` that it produces).
Related details
- Erik Ernst
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ernst (Gerrit)
Erik Ernst added 2 comments![]( "Open in Gerrit")
No visible progress, just replying to very helpful advice! ;-D
final TypeImpl? _returnType;Erik ErnstIt took me a while to figure out what this is for.
I *think* what it's for is to allow `ErrorVerifier.visitAnonymousMethodInvocation` to override the standard behavior of the `returnType` getter (which is to look up the return type in the element), because I guess anonymous method invocations don't have an associated element?
But if they don't have elements, why not? Our usual rule is that something needs to be an element if you can refer to it by name, OR if it's needed as the parent element to something you *can* refer to by name. If an anonymous method has a parameter, then that parameter must have a parameter element, and that parameter element should have a parent which is the anonymous method element, right?
Assuming that's true (that anonymous methods *do* have elements), then you shouldn't have to modify `EnclosingExecutableContext` at all; you should just be able to pass in the correct element when you call the constructor.
Konstantin ShcheglovIn an earlier iteration of this work I did create a new kind of element to represent the anonymous method. However, this caused several other requirements to arise: About 45 element visitors were suddenly broken, and there was a need to introduce a fragment to deliver from `declaredFragment`, and various other stuff that I don't remember in full detail.
I think we might have discussed this, but in any case I tried to get by without a new element class, thinking that it would be sufficient for the anonymous method to have a kind of element which is minimal by reusing an existing `...Element` class. In the end it should presumably just be a node in the tree such that there is no unexpected null value in a `parent`, and hopefully the formal parameter element doesn't really need any services from it.
However, I went back to this strategy and introduced a few private classes in error_verifier.dart starting from line 204. It just takes a couple of steps (and each step makes it stop with a new member which should have been there).
Could you give me a hint about a systematic method to discover how to build this connected graph of classes and objects that are needed in order to introduce a new kind of element?
Konstantin ShcheglovSee how we build fragments and elements in `ElementBindingVisitor`, specifically `visitFunctionExpression`. Note that we create `LocalFunctionFragmentImpl` and then use it to wrap as enclosingFragment typeParameters and formalParameters.
```
var holder = ElementHolder(fragment);
_withElementHolder(holder, () {
super.visitFunctionExpression(node);
fragment.typeParameters = holder.typeParameters;
fragment.formalParameters = holder.formalParameters;
});
```I think there is a lot of similarities with anonymous methods.
You will need to add `LocalFunctionFragmentImpl declaredFragment` to `AnonymousMethodInvocation`. Which sounds unusual, as now invocation has its own declaration. I see the comment describes it in terms of `AnonymousMethod`, which would be more natural place for `declaredFragment` IMHO. But maybe it is just unusual to me.
FYI, we consider splitting `LocalFunctionElement` into `LocalExecutableElement` superclass with leaf subclasses `LocalFunctionElement` proper, `FunctionExpressionElement`, and `AnonymousMethodElement`. This will align elements with syntax, which sometimes is useful. Nothing for you to do here, for now we will continue using `LocalFunctionFragmentImpl` (and `LocalFunctionElementImpl` that it produces).
Thanks, Konstantin!
I'm actually thinking that the naming is conceptually helpful because it refers to a particular constraint: The new AST nodes model "anonymous method invocations", and nothing models the "anonymous method" as a separate thing. This might help ensuring that the "anonymous method" is never considered to be like a function expression --- and that's what we want because it does allow us to get a better flow analysis and a better context type for returned expressions than we could hope for if we just treat it as a function literal which is invoked on the receiver.
Related details
- Konstantin Shcheglov
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ernst (Gerrit)
Erik Ernst added 2 comments![]( "Open in Gerrit")
I made the changes suggested in review comments, and all tests succeed, but surely it would be great to have more experienced eyes on the correctness of these changes. PTAL!
final TypeImpl? _returnType;Erik ErnstIt took me a while to figure out what this is for.
I *think* what it's for is to allow `ErrorVerifier.visitAnonymousMethodInvocation` to override the standard behavior of the `returnType` getter (which is to look up the return type in the element), because I guess anonymous method invocations don't have an associated element?
But if they don't have elements, why not? Our usual rule is that something needs to be an element if you can refer to it by name, OR if it's needed as the parent element to something you *can* refer to by name. If an anonymous method has a parameter, then that parameter must have a parameter element, and that parameter element should have a parent which is the anonymous method element, right?
Assuming that's true (that anonymous methods *do* have elements), then you shouldn't have to modify `EnclosingExecutableContext` at all; you should just be able to pass in the correct element when you call the constructor.
Konstantin ShcheglovIn an earlier iteration of this work I did create a new kind of element to represent the anonymous method. However, this caused several other requirements to arise: About 45 element visitors were suddenly broken, and there was a need to introduce a fragment to deliver from `declaredFragment`, and various other stuff that I don't remember in full detail.
I think we might have discussed this, but in any case I tried to get by without a new element class, thinking that it would be sufficient for the anonymous method to have a kind of element which is minimal by reusing an existing `...Element` class. In the end it should presumably just be a node in the tree such that there is no unexpected null value in a `parent`, and hopefully the formal parameter element doesn't really need any services from it.
However, I went back to this strategy and introduced a few private classes in error_verifier.dart starting from line 204. It just takes a couple of steps (and each step makes it stop with a new member which should have been there).
Could you give me a hint about a systematic method to discover how to build this connected graph of classes and objects that are needed in order to introduce a new kind of element?
Konstantin ShcheglovSee how we build fragments and elements in `ElementBindingVisitor`, specifically `visitFunctionExpression`. Note that we create `LocalFunctionFragmentImpl` and then use it to wrap as enclosingFragment typeParameters and formalParameters.
```
var holder = ElementHolder(fragment);
_withElementHolder(holder, () {
super.visitFunctionExpression(node);
fragment.typeParameters = holder.typeParameters;
fragment.formalParameters = holder.formalParameters;
});
```I think there is a lot of similarities with anonymous methods.
Erik ErnstYou will need to add `LocalFunctionFragmentImpl declaredFragment` to `AnonymousMethodInvocation`. Which sounds unusual, as now invocation has its own declaration. I see the comment describes it in terms of `AnonymousMethod`, which would be more natural place for `declaredFragment` IMHO. But maybe it is just unusual to me.
FYI, we consider splitting `LocalFunctionElement` into `LocalExecutableElement` superclass with leaf subclasses `LocalFunctionElement` proper, `FunctionExpressionElement`, and `AnonymousMethodElement`. This will align elements with syntax, which sometimes is useful. Nothing for you to do here, for now we will continue using `LocalFunctionFragmentImpl` (and `LocalFunctionElementImpl` that it produces).
Thanks, Konstantin!
I'm actually thinking that the naming is conceptually helpful because it refers to a particular constraint: The new AST nodes model "anonymous method invocations", and nothing models the "anonymous method" as a separate thing. This might help ensuring that the "anonymous method" is never considered to be like a function expression --- and that's what we want because it does allow us to get a better flow analysis and a better context type for returned expressions than we could hope for if we just treat it as a function literal which is invoked on the receiver.
I used the advice to update the `ElementBindingVisitor` and other things, and it is now (again) passing all tests. So I'll close this thread.
Related details
- Konstantin Shcheglov
- 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
lgtm, but please wait for Konstantin's review before landing this, since he's picked up subtleties that I've missed.
Related details
- Erik Ernst
- Konstantin Shcheglov
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 added 1 comment![]( "Open in Gerrit")
BTW, the failure you're seeing on the `g3-cbuild-try` bot is likely due to the fact that your CL is based on the state of the repo from Feb 23. All the bots try to merge CLs up to main before testing them, but AIUI, `g3-cbuild-try` uses a less sophisticated merging algorithm than Gerrit does, so it sometimes hits merge conflicts even when Gerrit thinks there are none.
You should be able to get a green trybot run by rebasing your CL onto the latest main.
Related details
- Erik Ernst
- 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 Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Erik Ernst
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ernst (Gerrit)
Erik Ernst added 1 comment![]( "Open in Gerrit")
Hello Sam, this CL contains a small change to api.txt, could you take a look at that?
Related details
- 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 Code-Review+1![]( "Open in Gerrit")
- Erik Ernst
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Erik Ernst (Gerrit)
Erik Ernst 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. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
Take steps to support anonymous block bodies
This CL adds support for anonymous block bodies (as in `e.{...}`) by
generalizing the flow analysis to handle begin/end of anonymous block
bodies and treating them similarly to labeled statements (and treating
`return` statements using `handleBreak`). It generalizes `handleBreak`
and the internal make `_StatementToContext` to handle `Node` keys rather
than just `Statement` keys, such that an anonymous block body can be the
context. It adds a `bodyContext` instance variable to
`AnonymousBodyImpl` to be used during flow analysis of anonymous block
bodies. `BodyInferenceContext` gets a new factory constructor in order
to allow an anonymous block body to be the context. Finally,
`ErrorVerifier` is generalized to handle the case where a return
statement is returning from an anonymous block body.
- M pkg/_fe_analyzer_shared/lib/src/flow_analysis/flow_analysis.dart
- M pkg/analyzer/api.txt
- M pkg/analyzer/lib/src/dart/ast/ast.dart
- M pkg/analyzer/lib/src/dart/resolver/body_inference_context.dart
- M pkg/analyzer/lib/src/dart/resolver/element_binding_visitor.dart
- M pkg/analyzer/lib/src/generated/error_verifier.dart
- M pkg/analyzer/lib/src/generated/resolver.dart
- A tests/language/anonymous_methods/block/cascaded_test.dart
- A tests/language/anonymous_methods/block/context_test.dart
- A tests/language/anonymous_methods/block/null_aware_cascaded_test.dart
- A tests/language/anonymous_methods/block/null_aware_plain_test.dart
- A tests/language/anonymous_methods/block/plain_test.dart
- A tests/language/anonymous_methods/block/promotion_test.dart
- A tests/language/anonymous_methods/block/this_not_enclosing_test.dart
- M tests/language/anonymous_methods/expression/promotion_test.dart
Code-Review: +1 by Paul Berry, +1 by Konstantin Shcheglov, +1 by Samuel Rawlins
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |