[XL] Change in dart/sdk[main]: Support the feature 'anonymous methods' in the analyzer.
Erik Ernst (Gerrit)
Erik Ernst added 11 comments![]( "Open in Gerrit")
Erik ErnstIt'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?
I'll do that!
Hello Paul,
I responded to several of your comments. A few of them have not been resolved. I need to stop using a synthetic function expression, but I don't know how to do that and still get the same resolution; so that's still a TODO.
assert(afterParameters.isA(TokenType.FUNCTION));Erik ErnstI'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.
Done
assert(afterParameters.isA(TokenType.FUNCTION));Erik ErnstThanks 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.
Certainly, trying to recover now. I put a TODO there because I need to discover the correct way to handle the situation where we don't just expect one token, but one of several possible ones.
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`?
Paul BerryI marked the optional parameter as experimental. Was that what you intended?
Erik ErnstYes, thanks!
Done
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;`.
Paul BerryI 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!
Erik ErnstIMHO, 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.
I definitely need to change the approach such that there is no synthetic function expression. This means that I need to restore the same resolution using a bottom-up approach rather than just using the existing analysis of a function expression, and I just haven't found a way to do that, yet.
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_.
Paul BerryI 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`?
Erik ErnstThat'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`.
Tried it again, they do show up. Now testing for `isStatic`.
.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.
Paul BerryWe 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).
Erik ErnstI'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.
I think the current implementation of lexical lookup could be modified such that it behaves more precisely like the description in the specification (in particular, it should _always_ return `(getter: null, setter: null)` when it has found an instance member with the requested basename). With that, there's no need to change the lookup for a lookup that starts somewhere inside an anonymous method, and we'll simply get the correct behavior because it _is_ the same for those lookups and for other lookups. I haven't tried it, yet.
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
Johnni WintherI 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`?
Erik ErnstOne 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();
}
...
}
At this time I think there's no need to do this after all. We should be able to make an adjustment to the lexical lookup such that it _always_ returns the empty result (`(getter: null, setter: null)`) in the case where an instance member with the right basename is found in an instance scope, which is also what the specification says.
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.
Paul BerrySo 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?
Erik ErnstAh, sorry. I misunderstood. Yes, I think you are right.
Acknowledged
analyzeExpression(Erik ErnstWe 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.
I'll make sure there is no synthetic function expression, I just need to discover how to get the same resolution.
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 1 comment![]( "Open in Gerrit")
bool _beginsAnonymousMethod(Token token) =>To recover better from erroneous code like `e1.(p) e2`, I think we should change this method so that if `token` is `(`, it looks ahead to the matching `)`, and if it's *not* followed by `{` or `=>`, this method should return `false`.
That should ensure that when `e1.(p) e2` is parsed, the parser will generate a synthetic identifier after the `.`, which will allow code completion to work properly after the `.`.
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")
Resolving a couple of review threads about the parser that have been addressed in CL464704.
token = parseFormalParameters(dot, MemberKind.Local);Erik ErnstWhy might want a now member kind `MemberKind.AnonymousMethod` for this.
Erik ErnstI'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.
Added the new MemberKind in CL https://dart-review.googlesource.com/c/sdk/+/464704.
assert(afterParameters.isA(TokenType.FUNCTION));Erik ErnstThanks 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.
Certainly, trying to recover now. I put a TODO there because I need to discover the correct way to handle the situation where we don't just expect one token, but one of several possible ones.
https://dart-review.googlesource.com/c/sdk/+/464704 changes this to test whether there's a `FUNCTION` next, and otherwise report a recoverable error.
To recover better from erroneous code like `e1.(p) e2`, I think we should change this method so that if `token` is `(`, it looks ahead to the matching `)`, and if it's *not* followed by `{` or `=>`, this method should return `false`.
That should ensure that when `e1.(p) e2` is parsed, the parser will generate a synthetic identifier after the `.`, which will allow code completion to work properly after the `.`.
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |