[S] Change in dart/sdk[main]: [analysis_server] Include declaring parameters in member declarations...
Danny Tuppeny (Gerrit)
Danny Tuppeny added 4 comments![]( "Open in Gerrit")
Future<void> test_class_primaryConstructor_fields() async {
addTestFile('''
class A(final int foo);
class B.named(final int foo);
class X(int foo); // Not declaring
''');I'm assuming that members declared in primary constructors should show up in the member declarations list, but I don't know if that assumption is correct.
I'm also unsure if this functionality is actually used - VS Code doesn't use it, and if I'm understanding https://github.com/dart-lang/sdk/issues/60043#issuecomment-3883948834 correctly, IntelliJ might not either?
// TODO(dantup): Is parameter the right thing here? We get it because
// we use .nonSynthetic on the field (because otherwise we crash trying to
// get the nameOffset) and that gives us a parameter in the constructor.
assertHasDeclaration(ElementKind.PARAMETER, 'A');
assertHasDeclaration(ElementKind.PARAMETER, 'B.named');I'm unsure about this. They are parameters of the constructor in the code, but they're showing up in the results because they are fields/getters.
// Is declaring parameter.
.where((parameter) => parameter.finalOrVarKeyword != null)Is this the correct way to detect these parameters? I thought there might be something more explicit, but I couldn't spot anything.
(again, I'm not certain about putting these parameter names in `classMemberNames`, but without it the search would never look in these files)
element.fields.where((e) => e.isOriginDeclaration).forEach(addElement);
element.fields
.where((e) => e.isOriginDeclaringFormalParameter)
.map((element) => element.nonSynthetic)
.forEach(addElement);Is there a cleaner way to get the declaring parameters than this?
Related details
- Brian Wilkerson
- Konstantin Shcheglov
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
Future<void> test_class_primaryConstructor_fields() async {
addTestFile('''
class A(final int foo);
class B.named(final int foo);
class X(int foo); // Not declaring
''');I'm assuming that members declared in primary constructors should show up in the member declarations list, but I don't know if that assumption is correct.
I'm also unsure if this functionality is actually used - VS Code doesn't use it, and if I'm understanding https://github.com/dart-lang/sdk/issues/60043#issuecomment-3883948834 correctly, IntelliJ might not either?
Yes, the fields declared by a declaring parameter should show up.
// TODO(dantup): Is parameter the right thing here? We get it because
// we use .nonSynthetic on the field (because otherwise we crash trying to
// get the nameOffset) and that gives us a parameter in the constructor.
assertHasDeclaration(ElementKind.PARAMETER, 'A');
assertHasDeclaration(ElementKind.PARAMETER, 'B.named');I'm unsure about this. They are parameters of the constructor in the code, but they're showing up in the results because they are fields/getters.
We should be reporting them the same way we report fields. It's reasonable to use `nonSynthetic` to get the offset, but not to get the element kind.
Related details
- Danny Tuppeny
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 2 comments![]( "Open in Gerrit")
Future<void> test_class_primaryConstructor_fields() async {
addTestFile('''
class A(final int foo);
class B.named(final int foo);
class X(int foo); // Not declaring
''');Brian WilkersonI'm assuming that members declared in primary constructors should show up in the member declarations list, but I don't know if that assumption is correct.
I'm also unsure if this functionality is actually used - VS Code doesn't use it, and if I'm understanding https://github.com/dart-lang/sdk/issues/60043#issuecomment-3883948834 correctly, IntelliJ might not either?
Yes, the fields declared by a declaring parameter should show up.
Acknowledged
// TODO(dantup): Is parameter the right thing here? We get it because
// we use .nonSynthetic on the field (because otherwise we crash trying to
// get the nameOffset) and that gives us a parameter in the constructor.
assertHasDeclaration(ElementKind.PARAMETER, 'A');
assertHasDeclaration(ElementKind.PARAMETER, 'B.named');Brian WilkersonI'm unsure about this. They are parameters of the constructor in the code, but they're showing up in the results because they are fields/getters.
We should be reporting them the same way we report fields. It's reasonable to use `nonSynthetic` to get the offset, but not to get the element kind.
sgtm, fixed! This also changed the container from `B.named` here to `B` since it's a field in the class, not a parameter in the constructor. This feels correct to me.
Related details
- Brian Wilkerson
- 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 2 comments![]( "Open in Gerrit")
// Is declaring parameter.
.where((parameter) => parameter.finalOrVarKeyword != null)Is this the correct way to detect these parameters? I thought there might be something more explicit, but I couldn't spot anything.
(again, I'm not certain about putting these parameter names in `classMemberNames`, but without it the search would never look in these files)
Nope, this is the way, the same as `FragmentBuilder` does it.
I decided to take over this.
https://dart-review.googlesource.com/c/sdk/+/490942
element.fields.where((e) => e.isOriginDeclaration).forEach(addElement);
element.fields
.where((e) => e.isOriginDeclaringFormalParameter)
.forEach(addElement);Probably better to have single `element.fields` with ORed filter.
Related details
- Brian Wilkerson
- Danny Tuppeny
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 3 comments![]( "Open in Gerrit")
This won't be able to land until Konstantin's change at https://dart-review.googlesource.com/c/sdk/+/490942 has, but it should should still pass the bots etc. as it's based on top of that CL.
// Is declaring parameter.
.where((parameter) => parameter.finalOrVarKeyword != null)Konstantin ShcheglovIs this the correct way to detect these parameters? I thought there might be something more explicit, but I couldn't spot anything.
(again, I'm not certain about putting these parameter names in `classMemberNames`, but without it the search would never look in these files)
Nope, this is the way, the same as `FragmentBuilder` does it.
I decided to take over this.
https://dart-review.googlesource.com/c/sdk/+/490942
Great, thanks! I've removed these parts of my change and based it on your CL.
element.fields.where((e) => e.isOriginDeclaration).forEach(addElement);
element.fields
.where((e) => e.isOriginDeclaringFormalParameter)
.forEach(addElement);Probably better to have single `element.fields` with ORed filter.
Thanks, fixed - originally I had `.nonSynthetic` on this branch but forgot to recombine them when I removed that.
Related details
- Brian Wilkerson
- Konstantin Shcheglov
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![]( "Open in Gerrit")
- Danny Tuppeny
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
Note: We should ensure this doesn't get +2'd before Konstantin's related change is ready. I noticed that it has been approved (and is therefore "submitable") but has an outstanding comment. Because this change is based on top of it, I suspect trying to commit this will also commit that.
(In hindsight, I probably should've waited for that change and not rebased this on top of it 🙃)
Related details
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danny Tuppeny (Gerrit)
Danny Tuppeny added 1 comment![]( "Open in Gerrit")
Note: We should ensure this doesn't get +2'd before Konstantin's related change is ready. I noticed that it has been approved (and is therefore "submitable") but has an outstanding comment. Because this change is based on top of it, I suspect trying to commit this will also commit that.
(In hindsight, I probably should've waited for that change and not rebased this on top of it 🙃)
That change landed, so this is fine now (although @sche...@google.com it requires your review for the rebase and addressing your comment).
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 Code-Review+1![]( "Open in Gerrit")
- Danny Tuppeny
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 Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Danny Tuppeny
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
[analysis_server] Include declaring parameters in member declarations search
- M pkg/analysis_server/lib/src/services/search/search_engine_internal.dart
- M pkg/analysis_server/test/search/member_declarations_test.dart
- M pkg/analyzer/lib/src/dart/analysis/search.dart
Code-Review: +1 by Konstantin Shcheglov, +1 by Brian Wilkerson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |