[L] Change in dart/sdk[main]: CQ. Start using InstanceScope in ResolutionVisitor, a little.
Johnni Winther (Gerrit)
Johnni Winther added 1 comment![]( "Open in Gerrit")
I'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
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 added 2 comments![]( "Open in Gerrit")
/// This getter is not in the instance scope, but it shadows whateverI don't understand this comment (or the one on line 320). Can you please add a doc comment to this class with a brief code sample, and an explanation of what `InstanceScopeLookupResultImpl` results from analyzing it?
var prefixElement = nameScope.lookup(prefixName).getter;
// Might be shadowed by an instance member.
// Look again to report `prefixShadowedByLocalDeclaration`.
if (prefixElement == null) {
var result = nameScope.lookup(prefixName);
if (result is InstanceScopeLookupResultImpl) {
prefixElement = result.instanceGetter ?? result.instanceSetter;
}
}
It's inefficient to call `nameScope.lookup` twice.
```suggestion
var lookupResult = nameScope.lookup(prefixName);
var prefixElement = lookupResult.getter;
// Might be shadowed by an instance member.
// Look again to report `prefixShadowedByLocalDeclaration`.
if (prefixElement == null) {
if (lookupResult is InstanceScopeLookupResultImpl) {
prefixElement =
lookupResult.instanceGetter ?? result.instanceSetter;
}
}
```
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 3 comments![]( "Open in Gerrit")
I'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
See "17.37 Lexical Lookup": Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this.
/// This getter is not in the instance scope, but it shadows whateverI don't understand this comment (or the one on line 320). Can you please add a doc comment to this class with a brief code sample, and an explanation of what `InstanceScopeLookupResultImpl` results from analyzing it?
The scope :-P of using this class was reduced, so I decided to remove it altogether.
var prefixElement = nameScope.lookup(prefixName).getter;
// Might be shadowed by an instance member.
// Look again to report `prefixShadowedByLocalDeclaration`.
if (prefixElement == null) {
var result = nameScope.lookup(prefixName);
if (result is InstanceScopeLookupResultImpl) {
prefixElement = result.instanceGetter ?? result.instanceSetter;
}
}
It's inefficient to call `nameScope.lookup` twice.
```suggestion
var lookupResult = nameScope.lookup(prefixName);
var prefixElement = lookupResult.getter;// Might be shadowed by an instance member.
// Look again to report `prefixShadowedByLocalDeclaration`.
if (prefixElement == null) {
if (lookupResult is InstanceScopeLookupResultImpl) {
prefixElement =
lookupResult.instanceGetter ?? result.instanceSetter;
}
}```
True, this is more efficient. It does not matter though, this branch is taken only if there is an error to be reported. I changed it to different re-lookup code.
Related details
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 3 comments![]( "Open in Gerrit")
/// This getter is not in the instance scope, but it shadows whateverKonstantin ShcheglovI don't understand this comment (or the one on line 320). Can you please add a doc comment to this class with a brief code sample, and an explanation of what `InstanceScopeLookupResultImpl` results from analyzing it?
The scope :-P of using this class was reduced, so I decided to remove it altogether.
Done
var prefixElement = nameScope.lookup(prefixName).getter;
// Might be shadowed by an instance member.
// Look again to report `prefixShadowedByLocalDeclaration`.
if (prefixElement == null) {
var result = nameScope.lookup(prefixName);
if (result is InstanceScopeLookupResultImpl) {
prefixElement = result.instanceGetter ?? result.instanceSetter;
}
}
Konstantin ShcheglovIt's inefficient to call `nameScope.lookup` twice.
```suggestion
var lookupResult = nameScope.lookup(prefixName);
var prefixElement = lookupResult.getter;// Might be shadowed by an instance member.
// Look again to report `prefixShadowedByLocalDeclaration`.
if (prefixElement == null) {
if (lookupResult is InstanceScopeLookupResultImpl) {
prefixElement =
lookupResult.instanceGetter ?? result.instanceSetter;
}
}```
True, this is more efficient. It does not matter though, this branch is taken only if there is an error to be reported. I changed it to different re-lookup code.
Done
Related details
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnni Winther (Gerrit)
Johnni Winther added 1 comment![]( "Open in Gerrit")
Konstantin ShcheglovI'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
See "17.37 Lexical Lookup": Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this.
In the CFE the body scope contains the members declared (not inherited) in the enclosing declaration. This would match the case in spec
Case <D exists> ...
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 assuming Johnni's concerns are addressed.
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")
Konstantin ShcheglovI'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
Johnni WintherSee "17.37 Lexical Lookup": Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this.
In the CFE the body scope contains the members declared (not inherited) in the enclosing declaration. This would match the case in spec
Case <D exists> ...
I'm not sure what you mean by this. The specification say that the lookup yields nothing. My understanding is that the instance members are in the scope, and they do shadow whatever could otherwise be accessible via scope lookup in the enclosing (library, or library fragment) scope. But the specification is written in this way to force going through `this.` for instance members.
"Case ⟨D exists⟩" is only the first step in the lexical lookup.
There is also "In the second and last step" that has this quote: "Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this."
The change to `InstanceScope` was done by Erik in https://dart-review.googlesource.com/c/sdk/+/459740/23/pkg/analyzer/lib/src/dart/element/scope.dart
Related details
- Erik Ernst
- Johnni Winther
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Johnni Winther (Gerrit)
Johnni Winther added 1 comment![]( "Open in Gerrit")
Konstantin ShcheglovI'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
Johnni WintherSee "17.37 Lexical Lookup": Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this.
Konstantin ShcheglovIn the CFE the body scope contains the members declared (not inherited) in the enclosing declaration. This would match the case in spec
Case <D exists> ...
I'm not sure what you mean by this. The specification say that the lookup yields nothing. My understanding is that the instance members are in the scope, and they do shadow whatever could otherwise be accessible via scope lookup in the enclosing (library, or library fragment) scope. But the specification is written in this way to force going through `this.` for instance members.
"Case ⟨D exists⟩" is only the first step in the lexical lookup.
There is also "In the second and last step" that has this quote: "Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this."
The change to `InstanceScope` was done by Erik in https://dart-review.googlesource.com/c/sdk/+/459740/23/pkg/analyzer/lib/src/dart/element/scope.dart
I'm asking, not because it is blocker for this CL, but because it is a difference between the scopes in the CFE and analyzer, which could indicate that one of us is wrong. I'll talk to Erik as well.
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. |
Johnni Winther (Gerrit)
Johnni Winther voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Erik Ernst (Gerrit)
Erik Ernst added 2 comments![]( "Open in Gerrit")
Konstantin ShcheglovI'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
Johnni WintherSee "17.37 Lexical Lookup": Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this.
Konstantin ShcheglovIn the CFE the body scope contains the members declared (not inherited) in the enclosing declaration. This would match the case in spec
Case <D exists> ...
I'm not sure what you mean by this. The specification say that the lookup yields nothing. My understanding is that the instance members are in the scope, and they do shadow whatever could otherwise be accessible via scope lookup in the enclosing (library, or library fragment) scope. But the specification is written in this way to force going through `this.` for instance members.
"Case ⟨D exists⟩" is only the first step in the lexical lookup.
There is also "In the second and last step" that has this quote: "Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this."
The change to `InstanceScope` was done by Erik in https://dart-review.googlesource.com/c/sdk/+/459740/23/pkg/analyzer/lib/src/dart/element/scope.dart
In the specification, a lexical lookup that finds an instance member with the given basename yields the result "nothing" (and the search stops there, it does not continue to search the library scope). This means that the resolution of an identifier expression `id` is treated uniformly when it denotes an instance member (it makes no difference whether it's declared in the enclosing syntax or inherited), because the expression is then treated as `this.id`. Similarly for an assignable expression which is an identifier. When the instance member is inherited or doesn't exist at all, the result is also "nothing". This means that the subsequent processing based on `this.id` is able to resolve `id` to an instance member or to an extension member access on `this`. However, when we're not looking up an identifier to evaluate or assign, the transformation from `id` to `this.id` does not take place; for example, this covers the case where we're looking up a type. In this case the error message should reflect the fact that either nothing was found ("unknown identifier" or the like), or an instance member was found, and we definitely can't use it ("`foo` is not a type"). So we should probably have a slightly richer result from the lexical lookup: In the case where the result is "nothing", we should be able to see whether the search ended because we found an instance member, or it was because we didn't find anything at all with that basename.
- Konstantin Shcheglov
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")
Konstantin ShcheglovI'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
Johnni WintherSee "17.37 Lexical Lookup": Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this.
Konstantin ShcheglovIn the CFE the body scope contains the members declared (not inherited) in the enclosing declaration. This would match the case in spec
Case <D exists> ...
Erik ErnstI'm not sure what you mean by this. The specification say that the lookup yields nothing. My understanding is that the instance members are in the scope, and they do shadow whatever could otherwise be accessible via scope lookup in the enclosing (library, or library fragment) scope. But the specification is written in this way to force going through `this.` for instance members.
"Case ⟨D exists⟩" is only the first step in the lexical lookup.
There is also "In the second and last step" that has this quote: "Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this."
The change to `InstanceScope` was done by Erik in https://dart-review.googlesource.com/c/sdk/+/459740/23/pkg/analyzer/lib/src/dart/element/scope.dart
In the specification, a lexical lookup that finds an instance member with the given basename yields the result "nothing" (and the search stops there, it does not continue to search the library scope). This means that the resolution of an identifier expression `id` is treated uniformly when it denotes an instance member (it makes no difference whether it's declared in the enclosing syntax or inherited), because the expression is then treated as `this.id`. Similarly for an assignable expression which is an identifier. When the instance member is inherited or doesn't exist at all, the result is also "nothing". This means that the subsequent processing based on `this.id` is able to resolve `id` to an instance member or to an extension member access on `this`. However, when we're not looking up an identifier to evaluate or assign, the transformation from `id` to `this.id` does not take place; for example, this covers the case where we're looking up a type. In this case the error message should reflect the fact that either nothing was found ("unknown identifier" or the like), or an instance member was found, and we definitely can't use it ("`foo` is not a type"). So we should probably have a slightly richer result from the lexical lookup: In the case where the result is "nothing", we should be able to see whether the search ended because we found an instance member, or it was because we didn't find anything at all with that basename.
The danger of the result "nothing" is that since scopes are nested, a local lookup resulting in "nothing", normally means look in the parent scope. This is why I prefer the uniform handling that it *is* found, but rejected when it cannot be transformed into `this.id` either because we are looking for a type or `this` is not in scope, or some other reason (there are probably more).
Note that this interpretation is also used in the definition of documentation comment use of `[id]`, where an (instance) member declared in the same class-like declaration can be accessed through simple name, even though there is no notion here for transforming to `this.id`.
Related details
- Konstantin Shcheglov
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")
Konstantin ShcheglovI'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
Johnni WintherSee "17.37 Lexical Lookup": Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this.
Konstantin ShcheglovIn the CFE the body scope contains the members declared (not inherited) in the enclosing declaration. This would match the case in spec
Case <D exists> ...
Erik ErnstI'm not sure what you mean by this. The specification say that the lookup yields nothing. My understanding is that the instance members are in the scope, and they do shadow whatever could otherwise be accessible via scope lookup in the enclosing (library, or library fragment) scope. But the specification is written in this way to force going through `this.` for instance members.
"Case ⟨D exists⟩" is only the first step in the lexical lookup.
There is also "In the second and last step" that has this quote: "Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this."
The change to `InstanceScope` was done by Erik in https://dart-review.googlesource.com/c/sdk/+/459740/23/pkg/analyzer/lib/src/dart/element/scope.dart
Johnni WintherIn the specification, a lexical lookup that finds an instance member with the given basename yields the result "nothing" (and the search stops there, it does not continue to search the library scope). This means that the resolution of an identifier expression `id` is treated uniformly when it denotes an instance member (it makes no difference whether it's declared in the enclosing syntax or inherited), because the expression is then treated as `this.id`. Similarly for an assignable expression which is an identifier. When the instance member is inherited or doesn't exist at all, the result is also "nothing". This means that the subsequent processing based on `this.id` is able to resolve `id` to an instance member or to an extension member access on `this`. However, when we're not looking up an identifier to evaluate or assign, the transformation from `id` to `this.id` does not take place; for example, this covers the case where we're looking up a type. In this case the error message should reflect the fact that either nothing was found ("unknown identifier" or the like), or an instance member was found, and we definitely can't use it ("`foo` is not a type"). So we should probably have a slightly richer result from the lexical lookup: In the case where the result is "nothing", we should be able to see whether the search ended because we found an instance member, or it was because we didn't find anything at all with that basename.
The danger of the result "nothing" is that since scopes are nested, a local lookup resulting in "nothing", normally means look in the parent scope. This is why I prefer the uniform handling that it *is* found, but rejected when it cannot be transformed into `this.id` either because we are looking for a type or `this` is not in scope, or some other reason (there are probably more).
Note that this interpretation is also used in the definition of documentation comment use of `[id]`, where an (instance) member declared in the same class-like declaration can be accessed through simple name, even though there is no notion here for transforming to `this.id`.
Any approach that has the same behavior is of course fine as an implementation strategy.
However, with features like anonymous methods where the type of `this` may differ from the statically known type of an enclosing class/mixin/... declaration, it's important that we don't commit to the choice of an instance member of the enclosing type-introducing declaration. For example, the original `this` could have an `int get foo` and the current `this` in an anonymous method could have a `void foo()`, and this means that we need to consider `this.id` to find the right member to use during the subsequent analysis. In this case it would be directly misleading to receive that `int get foo` reference as the result of the lookup.
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 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
CQ. Start using InstanceScope in ResolutionVisitor, a little.
The previous implementation was using LocalScope that does not do
filtering out instance members as required by the specification.
I also added a few tests, some of which demonstrate what was worked,
what was broken, and what is still broken.
Work toward fixing https://github.com/dart-lang/sdk/issues/62622
- M pkg/analyzer/lib/src/dart/resolver/ast_rewrite.dart
- M pkg/analyzer/lib/src/dart/resolver/named_type_resolver.dart
- M pkg/analyzer/lib/src/dart/resolver/resolution_visitor.dart
- M pkg/analyzer/test/src/dart/resolution/function_reference_test.dart
- M pkg/analyzer/test/src/diagnostics/pattern_assignment_not_local_variable_test.dart
- M pkg/analyzer/test/src/diagnostics/prefix_shadowed_by_local_declaration_test.dart
- M tests/language/class_modifiers/base/base_class_syntax_error_test.dart
- M tests/language/sealed_class/sealed_class_syntax_error_test.dart
Code-Review: +1 by Johnni Winther, +1 by Paul Berry
| 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")
Konstantin ShcheglovI'm a bit surprised that instance member are not normal returned from scope lookups within the body scope. In the CFE they are always part of the body scope and will there naturally resolve (shadow) without additional action.
Johnni WintherSee "17.37 Lexical Lookup": Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this.
Konstantin ShcheglovIn the CFE the body scope contains the members declared (not inherited) in the enclosing declaration. This would match the case in spec
Case <D exists> ...
Erik ErnstI'm not sure what you mean by this. The specification say that the lookup yields nothing. My understanding is that the instance members are in the scope, and they do shadow whatever could otherwise be accessible via scope lookup in the enclosing (library, or library fragment) scope. But the specification is written in this way to force going through `this.` for instance members.
"Case ⟨D exists⟩" is only the first step in the lexical lookup.
There is also "In the second and last step" that has this quote: "Consider the case where D is an instance member declaration in a class or mixin A. The lexical lookup then yields nothing. In this case it is guaranteed that ℓ has access to this."
The change to `InstanceScope` was done by Erik in https://dart-review.googlesource.com/c/sdk/+/459740/23/pkg/analyzer/lib/src/dart/element/scope.dart
Johnni WintherIn the specification, a lexical lookup that finds an instance member with the given basename yields the result "nothing" (and the search stops there, it does not continue to search the library scope). This means that the resolution of an identifier expression `id` is treated uniformly when it denotes an instance member (it makes no difference whether it's declared in the enclosing syntax or inherited), because the expression is then treated as `this.id`. Similarly for an assignable expression which is an identifier. When the instance member is inherited or doesn't exist at all, the result is also "nothing". This means that the subsequent processing based on `this.id` is able to resolve `id` to an instance member or to an extension member access on `this`. However, when we're not looking up an identifier to evaluate or assign, the transformation from `id` to `this.id` does not take place; for example, this covers the case where we're looking up a type. In this case the error message should reflect the fact that either nothing was found ("unknown identifier" or the like), or an instance member was found, and we definitely can't use it ("`foo` is not a type"). So we should probably have a slightly richer result from the lexical lookup: In the case where the result is "nothing", we should be able to see whether the search ended because we found an instance member, or it was because we didn't find anything at all with that basename.
Erik ErnstThe danger of the result "nothing" is that since scopes are nested, a local lookup resulting in "nothing", normally means look in the parent scope. This is why I prefer the uniform handling that it *is* found, but rejected when it cannot be transformed into `this.id` either because we are looking for a type or `this` is not in scope, or some other reason (there are probably more).
Note that this interpretation is also used in the definition of documentation comment use of `[id]`, where an (instance) member declared in the same class-like declaration can be accessed through simple name, even though there is no notion here for transforming to `this.id`.
Any approach that has the same behavior is of course fine as an implementation strategy.
However, with features like anonymous methods where the type of `this` may differ from the statically known type of an enclosing class/mixin/... declaration, it's important that we don't commit to the choice of an instance member of the enclosing type-introducing declaration. For example, the original `this` could have an `int get foo` and the current `this` in an anonymous method could have a `void foo()`, and this means that we need to consider `this.id` to find the right member to use during the subsequent analysis. In this case it would be directly misleading to receive that `int get foo` reference as the result of the lookup.
(Just closing the thread, I don't think any further actions are needed in response to this discussion).
(Just closing a thread that I had commented on.)
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |