[M] Change in dart/sdk[main]: [analyzer] Implement static extension getters
Chloe Stefantsova (Gerrit)
New activity on the change![]( "Open in Gerrit")
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. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 4 comments![]( "Open in Gerrit")
required LibraryElement targetLibrary,It looks that we don't use it.
if (extension.extension.extendedType.element == declaration) extension,This is not exactly what the specification uses. I see following: "let M be the set containing each accessible extension whose on-declaration is D, and whose static members include one with the name m, or which declares a constructor named C.m."
And there is definition of "on-declaration" somewhere above, with some exceptions. I think we should have `InterfaceElementImpl get onDeclaration` in `ExtensionElementImpl`.
.havingMemberWithBaseName(name, isStatic: true)This is not exactly what the specification uses. I see following: "In the case where D does not declare any static members whose basename is the basename of m, and D does not declare any constructors named C.m2 where m2 is the basename of m, let M be the set containing each accessible extension whose on-declaration is D, and whose static members include one with the name m, or which declares a constructor named C.m."
Note, it says "with the name m", not `m2` or "basename of m".
I think we implementation would be most clear, if we followed the specification as close as possible. Naming the same `M` is of course not necessary, but semantically it should be what the specification says. Otherwise later it would be very hard to match the code and the specification.
We can freely add new classes as we need to express this new semantics, we don't have to fit into existing classes, if it does not fit :-)
void _lookupExtensionForDeclaration(InterfaceElement declaration) {It looks that we use it only once, and don't do any early returns, so can inline it.
Related details
- Chloe Stefantsova
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova added 5 comments![]( "Open in Gerrit")
Konstantin, thank you for the review and the suggestions. I've updated the CL accordingly. PTAL.
It looks that we don't use it.
Thanks for spotting this! Removed.
if (extension.extension.extendedType.element == declaration) extension,This is not exactly what the specification uses. I see following: "let M be the set containing each accessible extension whose on-declaration is D, and whose static members include one with the name m, or which declares a constructor named C.m."
And there is definition of "on-declaration" somewhere above, with some exceptions. I think we should have `InterfaceElementImpl get onDeclaration` in `ExtensionElementImpl`.
Good idea. I've added the getter. I also added more tests to make sure the specified exceptions are covered.
.havingMemberWithBaseName(name, isStatic: true)This is not exactly what the specification uses. I see following: "In the case where D does not declare any static members whose basename is the basename of m, and D does not declare any constructors named C.m2 where m2 is the basename of m, let M be the set containing each accessible extension whose on-declaration is D, and whose static members include one with the name m, or which declares a constructor named C.m."
Note, it says "with the name m", not `m2` or "basename of m".
I think we implementation would be most clear, if we followed the specification as close as possible. Naming the same `M` is of course not necessary, but semantically it should be what the specification says. Otherwise later it would be very hard to match the code and the specification.
We can freely add new classes as we need to express this new semantics, we don't have to fit into existing classes, if it does not fit :-)
It makes sense. I've updated the code to reflect the difference between the name and the basename of a member.
void _lookupExtensionForDeclaration(InterfaceElement declaration) {It looks that we use it only once, and don't do any early returns, so can inline it.
- 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 11 comments![]( "Open in Gerrit")
/// In case the extension declaration doesn't have an on-time, `null` isWhat is this?
@trackedIncludedInIdPlease read documentation for this annotation.
Here, `onDeclaration` is `trackedIndirectly`, because its value depends only on `extendedType`, which is tracked.
switch (extendedType.element.kind) {Lets use type matching, instead of kind matching, i.e. match `case ClassElementImpl()`- this is IMHO more readable.
if (extendedType.isDartAsyncFutureOr) {I cannot find a test for this. I wonder how to test it better... Lets add a few element model tests. See `ExtensionElementTest`, add tests line `test_onDeclaration_class`, `test_onDeclaration_mixin`, etc. And one of them covering also ``FutureOr. You will need to update `_Element2Writer` to write this new property. This might affect expectations for a few tests, to update them automatically, see `NodeTextExpectationsCollector.updatingIsEnabled`
// * A type of the form T? or FutureOr<T>, for any type T.I think we did not check for this.
bool isStatic = false,We use it only for static extensions, so we can simplify this to inlining `true` in the body.
_NotInstantiatedExtensionWithMember(I think re-using this class is confusing.
First, the specification says "whose static members include one with the name m". So, we are not looking for both getter and setter, but for one explicitly with this name.
And correspondingly, using `_NotInstantiatedExtensionWithMember` that has two slots is confusing. Add a new one.
And this is still not what the specification says: it also includes constructors. Although maybe this is incomplete implementation :-)
static int get a => 0;Could you use the name `foo` instead of `a`? I usually prefer it for a single name, with a benefit (in addition being customary) that it is a bit longer and stands out.
enum En { element; }Instead of inventing new name, I often just name these `A`, regardless if it is class, mixing, enum, etc.
test_static_getter_success_onTypedef_noTypeArguments() async {It is not on typedef, because typedefs get erased during building types. It is on class, but `viaTypedef` (search for it in the analyzer tests).
Related details
- Chloe Stefantsova
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova added 14 comments![]( "Open in Gerrit")
Thank you for another round of suggestions, Konstantin. They are really helpful. I've updated the CL accordingly. PTAL.
/// In case the extension declaration doesn't have an on-time, `null` isChloe StefantsovaWhat is this?
I meant to say "on-declaration". Fixed.
Please read documentation for this annotation.
Here, `onDeclaration` is `trackedIndirectly`, because its value depends only on `extendedType`, which is tracked.
Done
Lets use type matching, instead of kind matching, i.e. match `case ClassElementImpl()`- this is IMHO more readable.
Done
I cannot find a test for this. I wonder how to test it better... Lets add a few element model tests. See `ExtensionElementTest`, add tests line `test_onDeclaration_class`, `test_onDeclaration_mixin`, etc. And one of them covering also ``FutureOr. You will need to update `_Element2Writer` to write this new property. This might affect expectations for a few tests, to update them automatically, see `NodeTextExpectationsCollector.updatingIsEnabled`
In case you meant a `FutureOr` test in `pkg/analyzer/test/src/dart/resolution/extension_method_test.dart`, it's called `test_static_getter_failure_onFutureOr`.
As for the tests exposing the `onDeclaration` property in the element text, I've added those as you suggested.
// * A type of the form T? or FutureOr<T>, for any type T.I think we did not check for this.
Good catch. I've added the check.
We use it only for static extensions, so we can simplify this to inlining `true` in the body.
Agreed. I've inlined the parameter and renamed the method into `havingStaticMemberWithName`.
I think re-using this class is confusing.
First, the specification says "whose static members include one with the name m". So, we are not looking for both getter and setter, but for one explicitly with this name.
And correspondingly, using `_NotInstantiatedExtensionWithMember` that has two slots is confusing. Add a new one.
And this is still not what the specification says: it also includes constructors. Although maybe this is incomplete implementation :-)
I agree. To address the issue I've created a specialized class `_ExtensionWithMemberWithName` for this specific use case. It has only one slot. Additionally, I created some more classes for representing results with exactly one slot: `SimpleNameResolutionResult`, `ExtensionNameResolutionResult`, `SingleExtensionNameResolutionResult`, and `ExtensionNameResolutionError`. They parallel the classes `SimpleResolutionResult`, `ExtensionResolutionResult`, `SingleExtensionResolutionResult`, and `ExtensionResolutionError`.
As for the constructors, they are intentionally excluded from the scope of this CL and from the scope of some of my future CLs due to the specification still being in flux w.r.t. constructors. For now I'm only focusing on getters, setters, and methods. Furthermore, this CL focuses exclusively on getters, and the cases of setters and methods are deliberately omitted as well.
if (extension.extension.extendedType.element == declaration) extension,Chloe StefantsovaThis is not exactly what the specification uses. I see following: "let M be the set containing each accessible extension whose on-declaration is D, and whose static members include one with the name m, or which declares a constructor named C.m."
And there is definition of "on-declaration" somewhere above, with some exceptions. I think we should have `InterfaceElementImpl get onDeclaration` in `ExtensionElementImpl`.
Good idea. I've added the getter. I also added more tests to make sure the specified exceptions are covered.
Marked as resolved.
.havingMemberWithBaseName(name, isStatic: true)Chloe StefantsovaThis is not exactly what the specification uses. I see following: "In the case where D does not declare any static members whose basename is the basename of m, and D does not declare any constructors named C.m2 where m2 is the basename of m, let M be the set containing each accessible extension whose on-declaration is D, and whose static members include one with the name m, or which declares a constructor named C.m."
Note, it says "with the name m", not `m2` or "basename of m".
I think we implementation would be most clear, if we followed the specification as close as possible. Naming the same `M` is of course not necessary, but semantically it should be what the specification says. Otherwise later it would be very hard to match the code and the specification.
We can freely add new classes as we need to express this new semantics, we don't have to fit into existing classes, if it does not fit :-)
It makes sense. I've updated the code to reflect the difference between the name and the basename of a member.
Marked as resolved.
Could you use the name `foo` instead of `a`? I usually prefer it for a single name, with a benefit (in addition being customary) that it is a bit longer and stands out.
Done
Instead of inventing new name, I often just name these `A`, regardless if it is class, mixing, enum, etc.
Done
test_static_getter_success_onTypedef_noTypeArguments() async {It is not on typedef, because typedefs get erased during building types. It is on class, but `viaTypedef` (search for it in the analyzer tests).
- 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 9 comments![]( "Open in Gerrit")
when extendedType.nullabilitySuffix != NullabilitySuffix.question &&This is... not ideal, to repeat this code again and again.
We could pre-filter at the top of the method.
final ExecutableElement? memberWithName;Maybe just `member`.
_ExtensionWithMemberWithName(this.extension, {required this.memberWithName});Maybe both named, or both unnamed.
I usually prefer named, extra characters don't bother me :-)
List<_ExtensionWithMemberWithName> applicableToDeclaration({I wonder how useful it is to have this as a separate method.
We will need something to support completion, so maybe there?
But it all operated private classes, so I'm not sure.
ExtensionNameResolutionResult findExtensionForDeclaration(Saying "Name" here is not very useful, it is alway based on some name.
A more useful name would be IMHO `StaticExtensionResolutionResult`, and correspondingly `findStaticExtension`.
memberWithName: extension.memberWithName as InternalExecutableElement,I think it would be better to use a more specific type at the source code instead of casting here. We might avoid the cast at all.
enum ExtensionNameResolutionError implements ExtensionNameResolutionResult {Maybe even simplify it to a sealed hierarchy of 3 classes: none, ambiguous, single. We did not have sealed classes when we were working on extensions originally. Now we can do better! :-D
class SimpleNameResolutionResult {If we don't have other uses than for static extensions, I'd prefer that we used the most specific names that reflects this only use for now. This is all non-API, so we will able to rename it in the future if needed.
final InternalExecutableElement? memberWithName;I would reduce this name here and everywhere to just `member`.
The fact that it has this name is kind of implied.
Related details
- Chloe Stefantsova
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova added 10 comments![]( "Open in Gerrit")
Thank you so much for another round of suggestions, Konstantin. I've uploaded the patchsets applying them. PTAL.
when extendedType.nullabilitySuffix != NullabilitySuffix.question &&This is... not ideal, to repeat this code again and again.
We could pre-filter at the top of the method.
Done
final ExecutableElement? memberWithName;Chloe StefantsovaMaybe just `member`.
Done
_ExtensionWithMemberWithName(this.extension, {required this.memberWithName});Maybe both named, or both unnamed.
I usually prefer named, extra characters don't bother me :-)
Done
List<_ExtensionWithMemberWithName> applicableToDeclaration({I wonder how useful it is to have this as a separate method.
We will need something to support completion, so maybe there?
But it all operated private classes, so I'm not sure.
Inlined.
ExtensionNameResolutionResult findExtensionForDeclaration(Saying "Name" here is not very useful, it is alway based on some name.
A more useful name would be IMHO `StaticExtensionResolutionResult`, and correspondingly `findStaticExtension`.
Done
memberWithName: extension.memberWithName as InternalExecutableElement,I think it would be better to use a more specific type at the source code instead of casting here. We might avoid the cast at all.
To address that, I used a more specific type at the source. But it lead to three more casts in other places. Specifically in `pkg/analyzer/lib/src/dart/resolver/applicable_extensions.dart` in method `havingStaticMemberWithName`: `getter as GetterElementImpl`, `setter as SetterElementImpl`, and `method as MethodElementImpl`. You can see it in the latest patchset. I'll leave it up to you to decide which casts you'd prefer.
enum ExtensionNameResolutionError implements ExtensionNameResolutionResult {Maybe even simplify it to a sealed hierarchy of 3 classes: none, ambiguous, single. We did not have sealed classes when we were working on extensions originally. Now we can do better! :-D
Done
If we don't have other uses than for static extensions, I'd prefer that we used the most specific names that reflects this only use for now. This is all non-API, so we will able to rename it in the future if needed.
Renamed to `SimpleStaticExtensionResolutionResult`.
final InternalExecutableElement? memberWithName;I would reduce this name here and everywhere to just `member`.
The fact that it has this name is kind of implied.
- 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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Konstantin Shcheglov
| Code-Review | +1 |
1 comment
class SingleExtensionNameResolutionResultWould be nice to fit "static" word into here as well.
Related details
- Chloe Stefantsova
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chloe Stefantsova (Gerrit)
Chloe Stefantsova voted and added 2 comments![]( "Open in Gerrit")
Votes added by Chloe Stefantsova
| Commit-Queue | +2 |
2 comments
Thank you for the review, Konstantin!
Would be nice to fit "static" word into here as well.
- Paul Berry
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: pkg/analyzer/lib/src/dart/resolver/extension_member_resolver.dart
Insertions: 15, Deletions: 15.
The diff is too large to show. Please review the diff.
```
Change information
[analyzer] Implement static extension getters
This CL adds support for static extension getters in the CFE. The
feature is observable under the `static-extensions` experimental
feature flag. Only the positive support is added; the diagnostics will
be added in follow-up CLs.
Part of https://github.com/dart-lang/sdk/issues/61487
- M pkg/analyzer/lib/src/dart/element/element.dart
- M pkg/analyzer/lib/src/dart/resolver/applicable_extensions.dart
- M pkg/analyzer/lib/src/dart/resolver/extension_member_resolver.dart
- M pkg/analyzer/lib/src/dart/resolver/property_element_resolver.dart
- M pkg/analyzer/lib/src/dart/resolver/resolution_result.dart
- M pkg/analyzer/lib/src/dart/resolver/type_property_resolver.dart
- M pkg/analyzer/test/src/dart/resolution/extension_method_test.dart
- M pkg/analyzer/test/src/summary/element_text.dart
- M pkg/analyzer/test/src/summary/elements/const_test.dart
- M pkg/analyzer/test/src/summary/elements/default_value_test.dart
- M pkg/analyzer/test/src/summary/elements/duplicate_declaration_test.dart
- M pkg/analyzer/test/src/summary/elements/extension_test.dart
- M pkg/analyzer/test/src/summary/elements/metadata_test.dart
- M pkg/analyzer/test/src/summary/elements/offsets_test.dart
- M pkg/analyzer/test/src/summary/elements/record_type_test.dart
- M pkg/analyzer/test/src/summary/elements/since_sdk_version_test.dart
- M pkg/analyzer/test/src/summary/elements/top_level_variable_test.dart
- M pkg/analyzer/test/src/summary/elements/type_inference_test.dart
Code-Review: +1 by Konstantin Shcheglov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |