[XL] Change in dart/sdk[main]: Support the feature 'anonymous methods' in the analyzer
Erik Ernst (Gerrit)
Erik Ernst added 1 comment![]( "Open in Gerrit")
flutter-analyze-try currently fails with "Running command: ".../dart analyze --fatal-infos" in .../packages/packages/google_maps_flutter/google_maps_flutter_web Analyzing google_maps_flutter_web... error - example/integration_test/overlay_test.dart:44:31 - A value of type 'Size?' can't be assigned to a variable of type 'Size'. Try changing the type of the variable, or casting the right-hand type to 'Size'. - invalid_assignment", and that seems to be a bug in `overlay_test.dart`.
Related details
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 1 comment![]( "Open in Gerrit")
Is there a specification of the feature that I can read?
Related details
- Erik Ernst
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 1 comment![]( "Open in Gerrit")
problemMessage: The formal parameter list of an anonymous method must have exactly one non-optional, positional formal parameter.Consider "non-optional" --> "required" (here and elsewhere). We use the term "required" in other places, so it ought to be clear to users.
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")
Is there a specification of the feature that I can read?
At this time we only have the Github issue where this feature is proposed: https://github.com/dart-lang/language/issues/260. I wanted to explore the typing and inference based on an actual implementation, but a feature specification in the usual style is planned. Also, the language team will surely debate the proposal for a while.
Related details
- Brian Wilkerson
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")
problemMessage: The formal parameter list of an anonymous method must have exactly one non-optional, positional formal parameter.Consider "non-optional" --> "required" (here and elsewhere). We use the term "required" in other places, so it ought to be clear to users.
OK, done!
Related details
- Brian Wilkerson
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
1 comment
The `analysis_server` and `api.txt` changes lgtm.
Related details
- Erik Ernst
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")
FYI, https://dart-review.googlesource.com/c/sdk/+/454660 updates the analyzer code generator to add `@experimental` when generating visit methods. If you rebase atop that CL, that should take care of the failures in the pkg-linux-release-try bot. When you do so, you will have to add `import 'package:meta/meta.dart';` to `pkg/analyzer/lib/dart/ast/visitor.dart` and `pkg/analyzer/lib/src/lint/linter_visitor.dart`.
Related details
- Brian Wilkerson
- Erik Ernst
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")
FYI, https://dart-review.googlesource.com/c/sdk/+/454660 updates the analyzer code generator to add `@experimental` when generating visit methods. If you rebase atop that CL, that should take care of the failures in the pkg-linux-release-try bot. When you do so, you will have to add `import 'package:meta/meta.dart';` to `pkg/analyzer/lib/dart/ast/visitor.dart` and `pkg/analyzer/lib/src/lint/linter_visitor.dart`.
It turns out that Konstantin was already working on a fix for this issue, so I abandoned my change eh favor of his. His change is https://dart-review.googlesource.com/c/sdk/+/454700. It just landed, so you should be able to just rebase and re-run the code generator (`pkg/analyzer/tool/ast/generate.dart`).
Also, I think you will also need to add `import 'package:meta/meta.dart';` to `pkg/analyzer/lib/analysis_rule/rule_visitor_registry.dart`.
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 4 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
4 comments
Neither of these comments are critical, but I'd like to see them addressed before we ship so it might be easier to address them now.
/// [Expression] '.' [Block]"Block" --> "FunctionExpression"?
correctionMessage: Try declaring one positional parameter, and make it required.If we ship this, it might be useful to have correction messages that are a little more specific. I'm thinking of messages like
- Try making the parameter be positional.
- Try making the parameter be required.
- Try adding a single required positional parameter. (when there are no parameters)
- Try removing all but one of the parameters. (when there are multiple)
Type p0: the name of the type of the receiver"p0" --> "receiverType"
"p1" --> "parameterType"
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 5 comments![]( "Open in Gerrit")
Paul BerryFYI, https://dart-review.googlesource.com/c/sdk/+/454660 updates the analyzer code generator to add `@experimental` when generating visit methods. If you rebase atop that CL, that should take care of the failures in the pkg-linux-release-try bot. When you do so, you will have to add `import 'package:meta/meta.dart';` to `pkg/analyzer/lib/dart/ast/visitor.dart` and `pkg/analyzer/lib/src/lint/linter_visitor.dart`.
It turns out that Konstantin was already working on a fix for this issue, so I abandoned my change eh favor of his. His change is https://dart-review.googlesource.com/c/sdk/+/454700. It just landed, so you should be able to just rebase and re-run the code generator (`pkg/analyzer/tool/ast/generate.dart`).
Also, I think you will also need to add `import 'package:meta/meta.dart';` to `pkg/analyzer/lib/analysis_rule/rule_visitor_registry.dart`.
I added a couple of imports (visitor.dart and such need to import the package meta), and it seems to work.
/// [Expression] '.' [Block]Erik Ernst"Block" --> "FunctionExpression"?
Good catch!
We can't just change this to `[FunctionExpression]`. If we do that then we can't use constructs like `receiver.{ code; }`. It's _allowed_, though, and this comment should definitely be updated to match the latest changes.
Done.
correctionMessage: Try declaring one positional parameter, and make it required.If we ship this, it might be useful to have correction messages that are a little more specific. I'm thinking of messages like
- Try making the parameter be positional.
- Try making the parameter be required.
- Try adding a single required positional parameter. (when there are no parameters)
- Try removing all but one of the parameters. (when there are multiple)
Makes sense! Put it into my TODO list.
"p0" --> "receiverType"
"p1" --> "parameterType"
- Brian Wilkerson
- 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 1 comment![]( "Open in Gerrit")
Hello Brian, Johnny, Paul, and Konstantin,
this CL uses `@experimental` and doesn't trigger any diagnostics in that area (or any area). It provides support for the feature 'anonymous methods' (currently only implemented fully in the analyzer, but I intend to implement it in the CFE as well). It's enabled by `--enable-experiment=anonymous-methods`. It would be great if I can land the CL such that it's easier for anyone who is interested in exploring the feature to do that.
The language team hasn't accepted the feature at this time, but we're looking at it, and Leaf supports landing the implementation as a vehicle for exploration.
So how should this be done in order to make sure that it causes the smallest possible disruption for you? I could simply ask for reviews of this CL, or I could split it into smaller pieces and land them sequentially, whatever works better for all of you.
Related details
- Brian Wilkerson
- Johnni Winther
- Konstantin Shcheglov
- Paul Berry
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 and added 2 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
2 comments
The server and linter pieces lgtm.
currentAstNode.parameterName == null) {I don't understand the purpose of this part of the condition. Consider adding a comment explaining it.
Related details
- Erik Ernst
- Johnni Winther
- Konstantin Shcheglov
- 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")
I have a few concerns about this CL.
1. We need reference to the specification in the CL description.
2. We need analyzer tests for parsing, see `ClassDeclarationParserTest`, although here it is not a top-level declaration. But something similar to see AST, parser errors, and recovery.
3. We need analyzer tests for resolution, see `MethodInvocationResolutionTest`.
4. The number of changes required to `Scope` and all the places where we use it makes it too complicated. Without good understanding scope rule for this feature I cannot recommend anything specific, but in general a more palatable implementation would be to do some single action like creating a new `XyzScope` at the point where we enter anonymouse block.
Related details
- Erik Ernst
- Johnni Winther
- Paul Berry
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 added 10 comments![]( "Open in Gerrit")
void endAnonymousMethodInvocation(Token token, Token? functionDefinition) {Rename `token` to `beginToken` and add a `endToken` at the end of the parameter list.
if (isAnonymousMethodsFeatureEnabled &&Add a TODO to remove this from the condition and instead call `reportExperimentNotEnabled` once these feature is planned for release.
if (afterDot.isA(TokenType.OPEN_PAREN)) {Add a listener method
void beginAnonymousMethodInvocation(Token token);
and call it here with `dot` as argument.
token = parseFormalParameters(dot, MemberKind.Local);Why might want a now member kind `MemberKind.AnonymousMethod` for this.
} else if (isAnonymousMethodsFeatureEnabled &&Add a TODO to remove this from the condition and instead call `reportExperimentNotEnabled` once these feature is planned for release.
Token afterDots = token.next!;Call `beginAnymousMethodInvocation(cascadeOperator)` here.
if (isAnonymousMethodsFeatureEnabled &&Add a TODO to remove this from the condition and instead call `reportExperimentNotEnabled` once these feature is planned for release.
if (afterPeriod.isA(TokenType.OPEN_PAREN)) {Call `beginAnonymousMethodInvocation(period)` here.
ScopeLookupResult lookup(String id, {bool skipInstanceMembers = false});Since we cannot mark this as experimental we should add a new method (possibly only to `ScopeImpl`) that handle the new scenario:
@experimental
ScopeLookupResult lookupWithoutInstanceMembers(String id);
extension on AstNode {
bool get _isInAnonymousMethod {
AstNode? currentAstNode = this;
while (currentAstNode != null) {
if (currentAstNode is AnonymousMethodInvocation &&
currentAstNode.parameterName == null) {
return true;
}
currentAstNode = currentAstNode.parent;
}
return false;
}
}We need to store this information in the visitor during traversal to avoid running up the parent relation on each lookup.
This applies to all similar cases below.
Related details
- Erik Ernst
- 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 17 comments![]( "Open in Gerrit")
void endAnonymousMethodInvocation(Token token, Token? functionDefinition) {I realize that the existing code doesn't often live up to this standard, but I find it really helpful when these listener methods can have documentation indicating the sequence of substructures that the listener can expect to find on the stack.
E.g., something like:
```
/// Called after the parser has consumed an anonymous method invocation.
/// Substructures:
/// - The target of the invocation.
/// - formal parameters (possibly implicit--see [handleImplicitFormalParameters]).
/// - The body of the anonymous method (either an expression or a block).
```
Also, I think there should have a named boolean parameter to indicate whether the body is an expression or a block. Without it, the listener is forced to use an `if` test to figure out what kind of body it is.
if (isAnonymousMethodsFeatureEnabled &&There's a lot of code duplication among lines 6859-6878, 7235-7257, and 7279-7302. Can this logic be consolidated into a single helper method?
assert(afterParameters.isA(TokenType.FUNCTION));I'm having trouble convincing myself that this assertion is correct. I think that if we're trying to parse `.(...)`, followed by *any* token other than `{`, then we will get to this line of code. So instead we need to do something like:
```
} else if (afterParameters.isA(TokenType.FUNCTION)) {
functionDefinition = afterParameters;
token = parseExpression(afterParameters);
} else {
// Some kind of recovery logic
}
```
Similar issue at lines 7250 and 7298.
static final anonymous_methods = ExperimentalFeatures.anonymous_methods;No action needed from you right now, but I wonder if we should we mark this (and other fields pertaining to experimental features) as `@experimental`. I'm going to ask this question at the next developer experience / model team sync.
ScopeLookupResult lookup(String id, {bool skipInstanceMembers = false});Since we cannot mark this as experimental we should add a new method (possibly only to `ScopeImpl`) that handle the new scenario:
@experimental
ScopeLookupResult lookupWithoutInstanceMembers(String id);
@johnni...@google.com
Why can't we mark this as experimental? I just tried it out and it seemed to work fine.
Assuming I'm not missing something, I think we should just mark this as experimental.
Also, I think the name `skipInstanceMembers` is confusing, because it suggests that if an instance member is ofund, the lookup will continue looking for a match at top level. How about if we rename this parameter to `noResultForInstanceMembers`?
FunctionExpression get functionExpression;There's no function expression in the grammar here, and I don't think we should synthesize one. Especially because the `FunctionExpression` class has two getters that don't really make sense for anonymous methods: `declaredFragment` and `typeParameters`.
I think this should be replaced with `FormalParameterList? get formalParameters;` and `FunctionBody get body;`.
Token? get parameterName;Each input token should appear in the AST structure exactly once. This token is already inside the formal parameter list, so it should be removed from here.
with DotShorthandMixinI don't think this should be here. Anonymous methods can't be dot short hand invocations.
return ScopeLookupResultImpl(getter: null, setter: null);This seems wrong to me. This causes us to treat getters and setters as no result, whether they are static or not. I think what you want to do is to treat getters, setters, _and_ methods as no result, but only if they are _not static_.
.lookup(node.name, skipInstanceMembers: node._isInAnonymousMethod)I'm getting a little concerned about the number of call sites that need to pass the correct value of `skipInstanceMembers`. I wonder if it would be better to remove this optional parameter, and instead change the logic that implements "implicit `this`" so that when inside an anonymous method, if the identifier is inside an anonymous method, it does a fresh lookup based on the type of `this`.
That would be a more direct implementation of the semantics we've been discussing, and it would avoid having to add this boolean parameter to the analyzer public API.
extension on AstNode {
bool get _isInAnonymousMethod {
AstNode? currentAstNode = this;
while (currentAstNode != null) {
if (currentAstNode is AnonymousMethodInvocation &&
currentAstNode.parameterName == null) {
return true;
}
currentAstNode = currentAstNode.parent;
}
return false;
}
}We need to store this information in the visitor during traversal to avoid running up the parent relation on each lookup.
This applies to all similar cases below.
+1
.lookup(prefixName, skipInstanceMembers: true)We shouldn't pass `skipInstanceMembers: true` here. We should pass `true` when in an anonymous method, and `false` otherwise, as we do at other call sites.
I believe that passing `true` here will reduce the quality of error messages, because for code like `class C { int? x; x? foo; }`, instead of reporting `x isn't a type` (as we do today), we'll report `undefined class x`.
formals = createDefaultFormalParameterList();We should be really careful about creating extra AST structures in non-recovery scenarios. A lot of analysis server features rely on the AST structure directly reflecting the input syntax. Really, we ought to call the analyzer's AST structure a "concrete syntax tree".
In this case, I don't see any harm in leaving `formals` null if the user didn't supply a formal parameter list. This line can be deleted, along with line 985. In which case the local function `createDefaultFormalParameterList` can also be deleted.
functionDefinition: SyntheticStringToken(I'm confused about why we need to create a synthetic token here. Wasn't the `=>` token in the input syntax? If so, we should use it.
if (isDotShorthand(node)) {I think it should be possible to drop this if-statement, and the corresponding one on line 1920. Anonymous methods can never be dot shorthands.
node.parameterName?.stringValue ?? 'this',Change `this` to `value`. This string won't appear anywhere but in error messages if the analyzer needs to describe the expected function type, and I think it will be confusing to users if a parameter called "this" appears in the parameter position of a function type.
analyzeExpression(We shouldn't be analyzing the "function expression" here. This will trigger a bunch of flow analysis logic that makes sense for things that are genuine function expressions (like demoting captured variables), which don't make sense for anonymous methods. We should probably just call `FunctionBodyImpl.resolve` directly.
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 3 comments![]( "Open in Gerrit")
I have a few concerns about this CL.
1. We need reference to the specification in the CL description.
2. We need analyzer tests for parsing, see `ClassDeclarationParserTest`, although here it is not a top-level declaration. But something similar to see AST, parser errors, and recovery.
3. We need analyzer tests for resolution, see `MethodInvocationResolutionTest`.
4. The number of changes required to `Scope` and all the places where we use it makes it too complicated. Without good understanding scope rule for this feature I cannot recommend anything specific, but in general a more palatable implementation would be to do some single action like creating a new `XyzScope` at the point where we enter anonymouse block.
Very good! I wasn't sure about the conventions around where and how to write tests, whether it would have to be in this first CL or could be in subsequent ones, etc, so this information is very useful.
The specification is under construction, I expect to have a usable document this week.
I'll take a look at `ClassDeclarationParserTest` and use the tests I have and/or write new ones, and similarly for `MethodInvocationResolutionTest`.
The changes to the lexical lookup rules may be expressible in a different way. I don't think it is realistic to implement a new kind of scope which would be the current scope of the anonymous method body itself, because the fact that a lexical lookup _starts_ inside an anonymous method should give rise to a different behavior in the body scope of the enclosing class/etc (if any), which could be any number of steps out into enclosing scopes from the place where the search started. The fundamental design choice here is that there is at most one notion of `this` at any given location in code. Currently, in a top-level function, there's no `this`, but if we write an anonymous method then there is one, and it denotes the receiver of the anonymous method invocation (in `e.{... this ...}`, that occurrence of `this` denotes the result of evaluating `e`). This means that if we look up a name `id` from the body of an anonymous method and find (several layers out) an instance member named `id` of the enclosing class, then the search should stop (that is, we don't search the library scope), and the result should be that nothing was found (because we don't have access to the `this` of the class, only the `this` of the anonymous method).
Paul considered the possibility that this would just fall out automatically if we keep the lexical lookup rules unchanged, and I'll experiment with that. However, that seems to be somewhat dangerous because we may then search for an `id` from an anonymous method, find a declaration of `id` in the enclosing class which is an instance member, return a `ScopeLookupResultImpl` with that as the `getter`, and then we need to know that we're _not_ supposed to call that getter using `this.id` (because `this` does not denote the object that has such a getter), but we're supposed to proceed with the analysis of `this.id` taking the new meaning of `this` into account.
So the specification may seem to allow for the lexical lookup to go through this scenario and just remain unchanged, but the actual implementation of `Scope.lookup` does return that getter, and that surely can't be right.
This further motivates the work on the specification, such that we can all know what's the intended behavior. ;-)
I don't understand the purpose of this part of the condition. Consider adding a comment explaining it.
Sure, I added a DartDoc comment on `_isInAnonymousMethod`. The point is that the lexical lookup is different when the receiver is named (as in `e.(self) {...}`) and when it is implicit (as in `e.{...}` where the receiver is `this` and may be induced implicitly on expressions).
Related details
- 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")
Erik ErnstI have a few concerns about this CL.
1. We need reference to the specification in the CL description.
2. We need analyzer tests for parsing, see `ClassDeclarationParserTest`, although here it is not a top-level declaration. But something similar to see AST, parser errors, and recovery.
3. We need analyzer tests for resolution, see `MethodInvocationResolutionTest`.
4. The number of changes required to `Scope` and all the places where we use it makes it too complicated. Without good understanding scope rule for this feature I cannot recommend anything specific, but in general a more palatable implementation would be to do some single action like creating a new `XyzScope` at the point where we enter anonymouse block.
Very good! I wasn't sure about the conventions around where and how to write tests, whether it would have to be in this first CL or could be in subsequent ones, etc, so this information is very useful.
The specification is under construction, I expect to have a usable document this week.
I'll take a look at `ClassDeclarationParserTest` and use the tests I have and/or write new ones, and similarly for `MethodInvocationResolutionTest`.
The changes to the lexical lookup rules may be expressible in a different way. I don't think it is realistic to implement a new kind of scope which would be the current scope of the anonymous method body itself, because the fact that a lexical lookup _starts_ inside an anonymous method should give rise to a different behavior in the body scope of the enclosing class/etc (if any), which could be any number of steps out into enclosing scopes from the place where the search started. The fundamental design choice here is that there is at most one notion of `this` at any given location in code. Currently, in a top-level function, there's no `this`, but if we write an anonymous method then there is one, and it denotes the receiver of the anonymous method invocation (in `e.{... this ...}`, that occurrence of `this` denotes the result of evaluating `e`). This means that if we look up a name `id` from the body of an anonymous method and find (several layers out) an instance member named `id` of the enclosing class, then the search should stop (that is, we don't search the library scope), and the result should be that nothing was found (because we don't have access to the `this` of the class, only the `this` of the anonymous method).
Paul considered the possibility that this would just fall out automatically if we keep the lexical lookup rules unchanged, and I'll experiment with that. However, that seems to be somewhat dangerous because we may then search for an `id` from an anonymous method, find a declaration of `id` in the enclosing class which is an instance member, return a `ScopeLookupResultImpl` with that as the `getter`, and then we need to know that we're _not_ supposed to call that getter using `this.id` (because `this` does not denote the object that has such a getter), but we're supposed to proceed with the analysis of `this.id` taking the new meaning of `this` into account.
So the specification may seem to allow for the lexical lookup to go through this scenario and just remain unchanged, but the actual implementation of `Scope.lookup` does return that getter, and that surely can't be right.
This further motivates the work on the specification, such that we can all know what's the intended behavior. ;-)
If we have `AnonymousMethodScope(FunctionBodyScope(InstanceScope(LibraryScope)))`, does it mean that we should just turn off `InstanceScope`, so that it does not lookup anything itself, and always asks the parent? Maybe this is what we should do: before creating `AnonymousMethodScope` instance, walk its parents chain, and mark `InstanceScope` as disabled, and when discarding `AnonymousMethodScope` re-enable `InstanceScope`.
I read what you wrote above, and suspect that I might have wrong understanding of the scoping rules here, but the idea might still be applicable: do something with scopes in one place, don't force multiple places to pass a flag.
Related details
- Erik Ernst
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")
static final anonymous_methods = ExperimentalFeatures.anonymous_methods;No action needed from you right now, but I wonder if we should we mark this (and other fields pertaining to experimental features) as `@experimental`. I'm going to ask this question at the next developer experience / model team sync.
I discussed this with Brian, Johnni, and Konstantin today, and we decided that this static field should be marked `@experimental`.
Here's our reasoning: for features like `anonymous_methods`, that are being worked on but haven't been officially approved by the language team, we want to make sure that in the future, if we decide not to move forward with the feature, we will be able to remove the feature flag without having to do a major version bump of the analyzer. For features like `declaring_constructors`, that _have_ been officially approved but whose implementation is still in progress, we won't mark the `Feature` field as `@experimental`.
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 24 comments![]( "Open in Gerrit")
Erik ErnstI have a few concerns about this CL.
1. We need reference to the specification in the CL description.
2. We need analyzer tests for parsing, see `ClassDeclarationParserTest`, although here it is not a top-level declaration. But something similar to see AST, parser errors, and recovery.
3. We need analyzer tests for resolution, see `MethodInvocationResolutionTest`.
4. The number of changes required to `Scope` and all the places where we use it makes it too complicated. Without good understanding scope rule for this feature I cannot recommend anything specific, but in general a more palatable implementation would be to do some single action like creating a new `XyzScope` at the point where we enter anonymouse block.
Konstantin ShcheglovVery good! I wasn't sure about the conventions around where and how to write tests, whether it would have to be in this first CL or could be in subsequent ones, etc, so this information is very useful.
The specification is under construction, I expect to have a usable document this week.
I'll take a look at `ClassDeclarationParserTest` and use the tests I have and/or write new ones, and similarly for `MethodInvocationResolutionTest`.
The changes to the lexical lookup rules may be expressible in a different way. I don't think it is realistic to implement a new kind of scope which would be the current scope of the anonymous method body itself, because the fact that a lexical lookup _starts_ inside an anonymous method should give rise to a different behavior in the body scope of the enclosing class/etc (if any), which could be any number of steps out into enclosing scopes from the place where the search started. The fundamental design choice here is that there is at most one notion of `this` at any given location in code. Currently, in a top-level function, there's no `this`, but if we write an anonymous method then there is one, and it denotes the receiver of the anonymous method invocation (in `e.{... this ...}`, that occurrence of `this` denotes the result of evaluating `e`). This means that if we look up a name `id` from the body of an anonymous method and find (several layers out) an instance member named `id` of the enclosing class, then the search should stop (that is, we don't search the library scope), and the result should be that nothing was found (because we don't have access to the `this` of the class, only the `this` of the anonymous method).
Paul considered the possibility that this would just fall out automatically if we keep the lexical lookup rules unchanged, and I'll experiment with that. However, that seems to be somewhat dangerous because we may then search for an `id` from an anonymous method, find a declaration of `id` in the enclosing class which is an instance member, return a `ScopeLookupResultImpl` with that as the `getter`, and then we need to know that we're _not_ supposed to call that getter using `this.id` (because `this` does not denote the object that has such a getter), but we're supposed to proceed with the analysis of `this.id` taking the new meaning of `this` into account.
So the specification may seem to allow for the lexical lookup to go through this scenario and just remain unchanged, but the actual implementation of `Scope.lookup` does return that getter, and that surely can't be right.
This further motivates the work on the specification, such that we can all know what's the intended behavior. ;-)
If we have `AnonymousMethodScope(FunctionBodyScope(InstanceScope(LibraryScope)))`, does it mean that we should just turn off `InstanceScope`, so that it does not lookup anything itself, and always asks the parent? Maybe this is what we should do: before creating `AnonymousMethodScope` instance, walk its parents chain, and mark `InstanceScope` as disabled, and when discarding `AnonymousMethodScope` re-enable `InstanceScope`.
I read what you wrote above, and suspect that I might have wrong understanding of the scoping rules here, but the idea might still be applicable: do something with scopes in one place, don't force multiple places to pass a flag.
The instance scope should not be turned off when a search originates inside an anonymous method scope, because this would allow a reference to a name `id` to resolve to a declaration in the library scope, even in the case where there is an instance member named `id` declared in the current class. For example, with `int x = 1; class A { String x = ''; void foo() { x.length; /*OK*/ true.{ x.isEven; }; }}` the innermost reference to `x` (part of `x.isEven`) should give rise to an "unknown name" error, it should not resolve to the top-level `x`. So we should treat an instance member of the enclosing class with the required basename as evidence that the search is over and nothing was found, we should not proceed to search the library scope. This result should then give rise to an analysis of `this.id`, which may or may not yield a instance member of the actual `this` in the anonymous method (here: of `true`, so there's no `x`) or an extension member (on `bool`).
Paul thought that this might actually be the behavior that we'd obtain without changing anything about lexical lookup. The specification does indeed handle the case where an instance member is found in the lexically enclosing syntax by returning "found nothing" and relying on further work based on `this.id`. However, I can see that `EnclosedScope.lookup` returns the getter (rather than getter:null,setter:null) in this situation, which is the reason why I went down the path where `Scope.lookup` is modified. Here's the example:
class A {
int i13579 = 1;
} class B extends A {
int j13579 = 2;
void foo() => i13579 + j13579;
}The method `EnclosedScope.lookup` had the following extra code:
if (id.endsWith("13579")) {
int level = 0;
Scope? parent = this;
while (parent != null && parent is EnclosedScope) {
++level;
parent = parent.parent;
}
print(
'>>> $runtimeType($level), getter: $getter, setter: $setter',
); // DEBUG
}And it does print out that a getter `j13579` was found, and then it returns a `ScopeLookupResult` holding that getter.
I've addressed a number of comments and taken a step forward in others. There are still more comments that I haven't looked at.
void endAnonymousMethodInvocation(Token token, Token? functionDefinition) {Rename `token` to `beginToken` and add a `endToken` at the end of the parameter list.
Done
void endAnonymousMethodInvocation(Token token, Token? functionDefinition) {I realize that the existing code doesn't often live up to this standard, but I find it really helpful when these listener methods can have documentation indicating the sequence of substructures that the listener can expect to find on the stack.
E.g., something like:
```
/// Called after the parser has consumed an anonymous method invocation.
/// Substructures:
/// - The target of the invocation.
/// - formal parameters (possibly implicit--see [handleImplicitFormalParameters]).
/// - The body of the anonymous method (either an expression or a block).
```Also, I think there should have a named boolean parameter to indicate whether the body is an expression or a block. Without it, the listener is forced to use an `if` test to figure out what kind of body it is.
Done
Add a TODO to remove this from the condition and instead call `reportExperimentNotEnabled` once these feature is planned for release.
Done
There's a lot of code duplication among lines 6859-6878, 7235-7257, and 7279-7302. Can this logic be consolidated into a single helper method?
Of course, good catch! They were more similar than I had noticed. Done.
Add a listener method
void beginAnonymousMethodInvocation(Token token);and call it here with `dot` as argument.
Done, added the invocation of the new listener method before the if statement.
token = parseFormalParameters(dot, MemberKind.Local);Why might want a now member kind `MemberKind.AnonymousMethod` for this.
I'd need a bit of extra info about the implications of doing this (as in "having added `MemberKind.AnonymousMethod`, which other locations will now need to be updated?"). I haven't done anything yet.
Add a TODO to remove this from the condition and instead call `reportExperimentNotEnabled` once these feature is planned for release.
Done, added the TODO in the else part
Token afterDots = token.next!;Erik ErnstCall `beginAnymousMethodInvocation(cascadeOperator)` here.
Done
Add a TODO to remove this from the condition and instead call `reportExperimentNotEnabled` once these feature is planned for release.
Done
if (afterPeriod.isA(TokenType.OPEN_PAREN)) {Erik ErnstCall `beginAnonymousMethodInvocation(period)` here.
Done
static final anonymous_methods = ExperimentalFeatures.anonymous_methods;Paul BerryNo action needed from you right now, but I wonder if we should we mark this (and other fields pertaining to experimental features) as `@experimental`. I'm going to ask this question at the next developer experience / model team sync.
I discussed this with Brian, Johnni, and Konstantin today, and we decided that this static field should be marked `@experimental`.
Here's our reasoning: for features like `anonymous_methods`, that are being worked on but haven't been officially approved by the language team, we want to make sure that in the future, if we decide not to move forward with the feature, we will be able to remove the feature flag without having to do a major version bump of the analyzer. For features like `declaring_constructors`, that _have_ been officially approved but whose implementation is still in progress, we won't mark the `Feature` field as `@experimental`.
Makes sense, done!
ScopeLookupResult lookup(String id, {bool skipInstanceMembers = false});Paul BerrySince we cannot mark this as experimental we should add a new method (possibly only to `ScopeImpl`) that handle the new scenario:
@experimental
ScopeLookupResult lookupWithoutInstanceMembers(String id);
@johnni...@google.com
Why can't we mark this as experimental? I just tried it out and it seemed to work fine.Assuming I'm not missing something, I think we should just mark this as experimental.
Also, I think the name `skipInstanceMembers` is confusing, because it suggests that if an instance member is ofund, the lookup will continue looking for a match at top level. How about if we rename this parameter to `noResultForInstanceMembers`?
I marked the optional parameter as experimental. Was that what you intended?
FunctionExpression get functionExpression;There's no function expression in the grammar here, and I don't think we should synthesize one. Especially because the `FunctionExpression` class has two getters that don't really make sense for anonymous methods: `declaredFragment` and `typeParameters`.
I think this should be replaced with `FormalParameterList? get formalParameters;` and `FunctionBody get body;`.
I do create a function expression in order to be able to perform type inference on it, but it might not need to be in the interface of the public class.
The function expression is helpful in order to be able to perform static analysis on the body in the case where the parameter is declared explicitly (`e.(name) {...}`).
It is also helpful when type inference is performed on the body: An anonymous method invocation like `e.{ CODE }` works very much like `() { CODE } (e)` where the `CODE` in the body is analyzed with access to a `this` whose type is the static type `T` of `e`. However, `() { CODE } (e)` will analyze the function literal with an unknown return type. With the function expression, it can be subject to type inference using a context type of `C Function(T)` where `C` is the context type schema of the anonymous method invocation as a whole.
Similarly for `e.(p) { CODE }`, which works very much like `(T p) { CODE } (e)`. In this case the function expression is literally present in the code, and it makes sense to perform the static analysis on it using the support for static analysis on function expressions (such that `p` is in scope and has type `T`). Again, the context type schema of the expression as a whole occurs as the return type in the context type schema for type inference of the function expression.
That said, I do recognize that there is no guarantee that an anonymous method invocation contains a function expression.
Further advice is obviously very welcome!
Each input token should appear in the AST structure exactly once. This token is already inside the formal parameter list, so it should be removed from here.
Done, it was sufficient to have a bool instance variable keeping track of the existence of said parameter declaration.
I don't think this should be here. Anonymous methods can't be dot short hand invocations.
Right, I thought they might, but it's probably not useful. Done.
return ScopeLookupResultImpl(getter: null, setter: null);This seems wrong to me. This causes us to treat getters and setters as no result, whether they are static or not. I think what you want to do is to treat getters, setters, _and_ methods as no result, but only if they are _not static_.
I added a static member to the class and noted that it did not show up in the `getters` so my conclusion was that static members are not included in this scope. Are static members expected to be found in an `InstanceScope`?
.lookup(node.name, skipInstanceMembers: node._isInAnonymousMethod)I'm getting a little concerned about the number of call sites that need to pass the correct value of `skipInstanceMembers`. I wonder if it would be better to remove this optional parameter, and instead change the logic that implements "implicit `this`" so that when inside an anonymous method, if the identifier is inside an anonymous method, it does a fresh lookup based on the type of `this`.
That would be a more direct implementation of the semantics we've been discussing, and it would avoid having to add this boolean parameter to the analyzer public API.
We can't expect to be inside an anonymous method when the instance member of the enclosing class is encountered (and used to determine that the search is complete, and nothing was found).
extension on AstNode {
bool get _isInAnonymousMethod {
AstNode? currentAstNode = this;
while (currentAstNode != null) {
if (currentAstNode is AnonymousMethodInvocation &&
currentAstNode.parameterName == null) {
return true;
}
currentAstNode = currentAstNode.parent;
}
return false;
}
}Paul BerryWe need to store this information in the visitor during traversal to avoid running up the parent relation on each lookup.
This applies to all similar cases below.
+1
I definitely expected that ;-), but didn't know how to do it in a reasonable way. Right now I haven't done it yet. I thought it would be stored as a bit on each simple identifier, or a bit in each scope, but it sounds like there is a better way to do it using a parameter on `visit` and `accept`?
We shouldn't pass `skipInstanceMembers: true` here. We should pass `true` when in an anonymous method, and `false` otherwise, as we do at other call sites.
I believe that passing `true` here will reduce the quality of error messages, because for code like `class C { int? x; x? foo; }`, instead of reporting `x isn't a type` (as we do today), we'll report `undefined class x`.
Of course, done!
I'm confused about why we need to create a synthetic token here. Wasn't the `=>` token in the input syntax? If so, we should use it.
Done.
if (isDotShorthand(node)) {I think it should be possible to drop this if-statement, and the corresponding one on line 1920. Anonymous methods can never be dot shorthands.
So we can't have `.m.{ CODE }` with context type `C` where `C.m` is a static method? Wouldn't that be a situation where `isDotShorthand(node)` is true?
Change `this` to `value`. This string won't appear anywhere but in error messages if the analyzer needs to describe the expected function type, and I think it will be confusing to users if a parameter called "this" appears in the parameter position of a function type.
- Johnni Winther
- Konstantin Shcheglov
- Paul Berry
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 added 1 comment![]( "Open in Gerrit")
extension on AstNode {
bool get _isInAnonymousMethod {
AstNode? currentAstNode = this;
while (currentAstNode != null) {
if (currentAstNode is AnonymousMethodInvocation &&
currentAstNode.parameterName == null) {
return true;
}
currentAstNode = currentAstNode.parent;
}
return false;
}
}Paul BerryWe need to store this information in the visitor during traversal to avoid running up the parent relation on each lookup.
This applies to all similar cases below.
Erik Ernst+1
I definitely expected that ;-), but didn't know how to do it in a reasonable way. Right now I haven't done it yet. I thought it would be stored as a bit on each simple identifier, or a bit in each scope, but it sounds like there is a better way to do it using a parameter on `visit` and `accept`?
One way of doing this is to record it in a field on the visitor.
This could be provided as a mixin:
mixin InAnynomousMethodMixin {
int _inAnonymousMethodCounter = 0;
void enterAnonymousMethod() {
_inAnonymousMethodCounter++;
}
void exitAnonymousMethod() {
_inAnonymousMethodCounter--;
}
bool get isInAnonymousMethod => _inAnonymousMethodCounter > 0;
}which you then have to mixin and call in visitors
class ResolutionVisitor
extends RecursiveAstVisitor<void>
with InAnonymousMethodMixin
{
...
@override
void visitAnonymousMethod(AnonymousMethod node) {
node.target?.accept(this);
enterAnonymousMethod();
node.functionExpression.accept(this);
exitAnoymousMethod();
}
...
}
Related details
- Erik Ernst
- Konstantin Shcheglov
- 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")
Well, "turn off" could be defined in any way we want it :-)
If the behavior we want is: when we find some `x` in the `InstanceScope` when it is "turned off", then it does not ask its parent (library scope); then we could do
```
class InstanceScope extends EnclosedScope {
bool isTurnedOff = false;
InstanceScope(super.parent, InstanceElement element) {
element.getters.forEach(_addGetter);
element.setters.forEach(_addSetter);
element.methods.forEach(_addGetter);
}@override
ScopeLookupResult lookup(String id) {
if (isTurnedOff) {
if (_getters[id] != null || _setters[id] != null) {
return ScopeLookupResultImpl(getter: null, setter: null);
}
}
- Erik Ernst
- 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 7 comments![]( "Open in Gerrit")
It's taking me a long time to review each iteration of this CL. How would you feel about splitting it up into a series of smaller CLs so that we can review and land the feature incrementally?
assert(afterParameters.isA(TokenType.FUNCTION));Thanks for refactoring this out to its own method! Note that my concern about the assertion (from https://dart-review.googlesource.com/c/sdk/+/449821/comment/81220eff_3578818c/) remains.
ScopeLookupResult lookup(String id, {bool skipInstanceMembers = false});Paul BerrySince we cannot mark this as experimental we should add a new method (possibly only to `ScopeImpl`) that handle the new scenario:
@experimental
ScopeLookupResult lookupWithoutInstanceMembers(String id);
Erik Ernst@johnni...@google.com
Why can't we mark this as experimental? I just tried it out and it seemed to work fine.Assuming I'm not missing something, I think we should just mark this as experimental.
Also, I think the name `skipInstanceMembers` is confusing, because it suggests that if an instance member is ofund, the lookup will continue looking for a match at top level. How about if we rename this parameter to `noResultForInstanceMembers`?
I marked the optional parameter as experimental. Was that what you intended?
Yes, thanks!
FunctionExpression get functionExpression;Erik ErnstThere's no function expression in the grammar here, and I don't think we should synthesize one. Especially because the `FunctionExpression` class has two getters that don't really make sense for anonymous methods: `declaredFragment` and `typeParameters`.
I think this should be replaced with `FormalParameterList? get formalParameters;` and `FunctionBody get body;`.
I do create a function expression in order to be able to perform type inference on it, but it might not need to be in the interface of the public class.
The function expression is helpful in order to be able to perform static analysis on the body in the case where the parameter is declared explicitly (`e.(name) {...}`).
It is also helpful when type inference is performed on the body: An anonymous method invocation like `e.{ CODE }` works very much like `() { CODE } (e)` where the `CODE` in the body is analyzed with access to a `this` whose type is the static type `T` of `e`. However, `() { CODE } (e)` will analyze the function literal with an unknown return type. With the function expression, it can be subject to type inference using a context type of `C Function(T)` where `C` is the context type schema of the anonymous method invocation as a whole.
Similarly for `e.(p) { CODE }`, which works very much like `(T p) { CODE } (e)`. In this case the function expression is literally present in the code, and it makes sense to perform the static analysis on it using the support for static analysis on function expressions (such that `p` is in scope and has type `T`). Again, the context type schema of the expression as a whole occurs as the return type in the context type schema for type inference of the function expression.
That said, I do recognize that there is no guarantee that an anonymous method invocation contains a function expression.
Further advice is obviously very welcome!
IMHO, a very important principle of the design of the analyzer is that unlike a compiler, it does not lower the code it's analyzing to an intermediate representation in order to do analysis. The reason this is important is because it ensures that any feedback provided by the analyzer (e.g., to the analysis server) accurately reflects the syntactic structure of the code the user provided. This ensures that error and warning locations will be correct, aids in code completions and refactorings in the analysis server, and makes lints easier to write.
Because I think this is such an important design principle, I'm going to push back pretty hard against creating a function expression here, especially for the case of `e.{ CODE }`, which really doesn't look syntactically like a function expression at all.
Another reason I think we shouldn't use a function expression here is that the flow analysis we're going to want to do for both `e.{ CODE }` and `e.(p) { CODE }` is pretty substantially different from the flow analysis we would do for function expressions. For example, flow analysis considers any local variable written to by a function expression to be "write captured", and thus it disables all promotions of that local variable inside the function expression and in any flow control paths that follow it. This is necessary for soundness, because in principle after a closure is created, it could in principle execute at any time. Whereas for anonymous methods, since they execute exactly once, at the site where they appear, any local variable writes that occur inside them should be treated as ordinary variable writes, not as write captures.
Can you go into more detail about what goes wrong if you _don't_ create the synthetic function expression? For example, maybe the are some methods in the resolver that make sense to call when analyzing an anonymous method, and those methods expects to be passed a function expression? If we could look at those methods in detail, I'd be glad to make concrete suggestions about how to generalize them to work with anonymous methods.
return ScopeLookupResultImpl(getter: null, setter: null);Erik ErnstThis seems wrong to me. This causes us to treat getters and setters as no result, whether they are static or not. I think what you want to do is to treat getters, setters, _and_ methods as no result, but only if they are _not static_.
I added a static member to the class and noted that it did not show up in the `getters` so my conclusion was that static members are not included in this scope. Are static members expected to be found in an `InstanceScope`?
That's what I would expect, yes. I would be interested in seeing your test case where a static member of the calss didn't show up in `getters`.
.lookup(node.name, skipInstanceMembers: node._isInAnonymousMethod)Erik ErnstI'm getting a little concerned about the number of call sites that need to pass the correct value of `skipInstanceMembers`. I wonder if it would be better to remove this optional parameter, and instead change the logic that implements "implicit `this`" so that when inside an anonymous method, if the identifier is inside an anonymous method, it does a fresh lookup based on the type of `this`.
That would be a more direct implementation of the semantics we've been discussing, and it would avoid having to add this boolean parameter to the analyzer public API.
We can't expect to be inside an anonymous method when the instance member of the enclosing class is encountered (and used to determine that the search is complete, and nothing was found).
I'm sorry, I'm not following you here. What I'm proposing is that when resolving a bare identifier:
- We first call `nameScope.lookup`.
- If the returned element is something other than an instance member (e.g. a static member, a local variable, or a top level declaration), then we proceed as usual.
- If the returned element _is_ an instance member:
- We check if we are in an anonymous method.
- If we are not in an anonymous method, the we proceed as usual.
- If we _are_ in an anonymous method, then we proceed as though `nameScope.lookup` had found nothing.
if (isDotShorthand(node)) {Erik ErnstI think it should be possible to drop this if-statement, and the corresponding one on line 1920. Anonymous methods can never be dot shorthands.
So we can't have `.m.{ CODE }` with context type `C` where `C.m` is a static method? Wouldn't that be a situation where `isDotShorthand(node)` is true?
Ah, sorry. I misunderstood. Yes, I think you are right.
Related details
- Erik Ernst
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |