[L] Change in dart/sdk[main]: [private named parameters] Handle rename refactoring.
Bob Nystrom (Gerrit)
Bob Nystrom voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
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 added 1 comment![]( "Open in Gerrit")
I think the change to the refactoring needs to be conditional based on whether the feature is enabled. Otherwise I think that code for which the feature isn't enabled will be refactored as if it were. Even after the feature ships users can opt out to an earlier language version, so I think this is a long-term requirement.
Related details
- Bob Nystrom
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bob Nystrom (Gerrit)
Bob Nystrom voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bob Nystrom
| Auto-Submit | +1 |
1 comment
I think the change to the refactoring needs to be conditional based on whether the feature is enabled. Otherwise I think that code for which the feature isn't enabled will be refactored as if it were. Even after the feature ships users can opt out to an earlier language version, so I think this is a long-term requirement.
Oh, dear, of course you're right. I completely forgot about that. :-O
I'll get started on 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. |
Bob Nystrom (Gerrit)
Bob Nystrom voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bob Nystrom
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
OK, I think I have it correctly only using private named parameters in a refactor if the surrounding library supports them. PTAL, thanks!
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 Code-Review+1![]( "Open in Gerrit")
- Bob Nystrom
- 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")
? IndexRelationKind.IS_FIELD_REFERENCED_BY_NAMED_ARGUMENTI have not looked through the full CL yet, but the name caught my attention. I think this is wrong, at least here. There is no field reference here:
```
class A {
A({required int a});
}
class B extends A {
B({required super.a}); // ref
}
```
In `B` with `super.a` we reference a named formal parameter `a` from `A`.
Not a field. Moreover, here there are no fields at all.
Related details
- Bob Nystrom
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")
? IndexRelationKind.IS_FIELD_REFERENCED_BY_NAMED_ARGUMENTI have not looked through the full CL yet, but the name caught my attention. I think this is wrong, at least here. There is no field reference here:
```
class A {
A({required int a});
}
class B extends A {
B({required super.a}); // ref
}
```
In `B` with `super.a` we reference a named formal parameter `a` from `A`.
Not a field. Moreover, here there are no fields at all.
Do you have a suggestion for a better name?
Related details
- Bob Nystrom
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")
if (element is FieldFormalParameterElement) {I think this is also wrong, and we should keep it always `IS_REFERENCED_BY`.
```
class A {
A({this.field});
var field;
}
void foo() {
A _ = .new(field: 42);
}
```
Here `field: 42` references a formal parameter, the fact that it is a field formal parameter, does not matter. Or at least I don't understand why it would matter. We always use the `name` of the formal parameter, the fact that it might also have a private name associated with the private field is something that is important only locally for the declaring constructor.
? IndexRelationKind.IS_FIELD_REFERENCED_BY_NAMED_ARGUMENTI have not looked through the full CL yet, but the name caught my attention. I think this is wrong, at least here. There is no field reference here:
```
class A {
A({required int a});
}
class B extends A {
B({required super.a}); // ref
}
```
In `B` with `super.a` we reference a named formal parameter `a` from `A`.
Not a field. Moreover, here there are no fields at all.
I think we should keep always `IS_REFERENCED_BY`.
Related details
- Bob Nystrom
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")
test_isReferencedBy_FieldElement_dotSorthandConstructorInvocation() async {Pre-existing, but the name is wrong.
Looks very similar to another test:
```
test_isReferencedBy_ParameterElement_dotSorthandConstructorInvocation() async {
await _indexTestUnit('''
class A {
A({p});
}
void foo() {
A _ = .new(p: 42);
}
''');
var element = findElement2.parameter('p');
assertElementIndexText(element, r'''
48 5:14 |p| IS_REFERENCED_BY qualified
''');
}
```
We refer not to a field, but a parameter, which in the implementation happens to be storing into a field.
I'd change the name to `test_isReferencedBy_ParameterElement_dotSorthandConstructorInvocation_field`
Related details
- Bob Nystrom
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bob Nystrom (Gerrit)
Bob Nystrom voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bob Nystrom
| Auto-Submit | +1 |
1 comment
? IndexRelationKind.IS_FIELD_REFERENCED_BY_NAMED_ARGUMENTKonstantin ShcheglovI have not looked through the full CL yet, but the name caught my attention. I think this is wrong, at least here. There is no field reference here:
```
class A {
A({required int a});
}
class B extends A {
B({required super.a}); // ref
}
```
In `B` with `super.a` we reference a named formal parameter `a` from `A`.
Not a field. Moreover, here there are no fields at all.
I think we should keep always `IS_REFERENCED_BY`.
Here's the problem I'm trying to solve. Let's say the code looks like:
```dart
class C {
int _x;
C({required this._x});
}
main() {
var c = C(x: 0); // 1
c._x; // 2
}
```The user renames the field `_x` to `_y`. The resulting code should look like:
```dart
class C {
int _y;
C({required this._y});
}
main() {
var c = C(y: 0); // 1
c._y; // 2
}
```On line `// 2`, we simply update the reference to the new field name. But on the line marked `// 1`, we need to use not the field name, but the corresponding public name (`y`). If both of these references use the same `IS_REFERENCED_BY` type, then I don't know how to distinguish these two use sites. At the point that I'm doing the rename refactoring, I don't think I have access to much else about the surrounding context.
The same problem also arises with super parameters. With the analyzer today, let's say you have:
```dart
class A {
final int x;
A({required this.x});
}
class B extends A {
B({required super.x});
}
```If you rename the field `x` to `y`, it renames the super parameter too and gives you:
```dart
class A {
final int y;
A({required this.y});
}
class B extends A {
B({required super.y});
}
```With private named parameters, then, if you start with:
```dart
class A {
final int _x;
A({required this._x});
}
class B extends A {
B({required super.x}); // Note: corresponding public name.
}
```And you rename `_x` to `_y`, then I expect it to give you:
```dart
class A {
final int _y;
A({required this._y});
}
class B extends A {
B({required super.y}); // Note: corresponding public name.
}
```So, again, it needs to use the corresponding public name for some references and not for others.
You're right that `IS_FIELD_REFERENCED_BY_NAMED_ARGUMENT` isn't a good name because the reference might not correspond to a field. How about `IS_REFERENCED_BY_NAMED_ARGUMENT`? Or do you have an idea for a better approach?
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 1 comment![]( "Open in Gerrit")
In
```
class C {
int _x;
C({required this._x});
}
main() {
var c = C(x: 0); // 1
c._x; // 2
}
```
when you rename `_x` to `_y`, you rename the field `_x`. This will rename `// 2` automatically.
But to keep the code correct, you also need to rename the named field formal parameters that referenced `_x`. So, you iterate over the constructors of the `InterfaceElement` that declares `_x` and find these `FieldFormalParameterElement`s, update their names, and corresponding find references to these formal parameters, and so update `// 1`.
Related details
- Bob Nystrom
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bob Nystrom (Gerrit)
Bob Nystrom added 1 comment![]( "Open in Gerrit")
But to keep the code correct, you also need to rename the named field formal parameters that referenced _x. So, you iterate over the constructors of the InterfaceElement that declares _x and find these FieldFormalParameterElements, update their names, and corresponding find references to these formal parameters, and so update // 1.
Yes, that logic is already in there: https://github.com/dart-lang/sdk/blob/main/pkg/analysis_server/lib/src/services/refactoring/legacy/rename_class_member.dart#L521. There's a check to skip private ones, but in this CL, I disable that check when the library supports private named parameters. So I do see the references to the field formal.
The problem is that when I go to update the reference, I need to know whether to update it to the private name or the corresponding public name. Consider:
```dart
class C {
int _x;
C({required this._x}): assert(_x > 0); // 1.
}main() {
C(x: 0); // 2.
}
```Here, there are two references to the field formal. If I rename the field to `_y`, the first field formal reference in the `assert()` needs to change to `_y` and the second in the named argument needs to be `y`. When I go to update the reference I need some way to distinguish these two cases.
Adding a new MatchKind for named arguments seemed like the simplest way to plumb that information through but I'm happy to take another approach if you have a different idea. I don't understand this code very well, so I did the simplest thing I could come up with.
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 1 comment![]( "Open in Gerrit")
```
class C {
int _x;
C({required this._x}): assert(_x > 0); // 1
}
main() {
var c = C(x: 0); // 2
c._x; // 3
}
```
The way I think about this is that:
1. We rename a field `int _x` to `_y`.
2. We find move elements to rename.
3. Here this is named field formal parameter `this._x`. The name that we should use is `_y`.
4. For each element that we rename, we find and update references.
5. For field `int _x` we have single reference at `// 3`, use name `_y`.
6. For formal parameter `this._x` we have two references. It has `privateName` property value which is not `null`, which tells us that any external reference to this formal parameter must use corresponding new public name, here `y`. We do this at `// 2`.
7. The only special case is `// 1`.For `// 1` we have options, where to make a decision. One, is in the indexer in the analyzer, record for a field formal parameter either `IS_REFERENCED_BY`, or`IS_REFERENCED_BY_PRIVATE_NAMED_FIELD_FORMAL_PARAMETER_WHERE_IT_SHOULD_USE_PRIVATE_NAME`. Second, in the server in rename refactoring: check if the element enclosing the match is the constructor that defines it, and again, use private name.
We leaned previously toward indexer providing more precise information. *This* new reference kind I can understand.
Related details
- Bob Nystrom
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bob Nystrom (Gerrit)
Bob Nystrom voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bob Nystrom
| Auto-Submit | +1 |
1 comment
? IndexRelationKind.IS_FIELD_REFERENCED_BY_NAMED_ARGUMENTSecond, in the server in rename refactoring: check if the element enclosing the match is the constructor that defines it, and again, use private name.
I considered something like that. But I couldn't come up with a clean way to find the surrounding context. And, in general, I find that code that looks at parent nodes/elements and surrounding context tends to be brittle as the language evolves. When there is a new language feature that provides a new context where some existing feature could be used, it's really hard to tell where in all of our various tools we need to add a new case to handle that context.
I like the model where the indexer records what it knows about a reference (this reference came from a named argument), and then let the rename refactor decide whether it cares about that property.
I changed the index relation to IS_REFERENCED_BY_NAMED_ARGUMENT since, as you note, the fact that the corresponding formal happens to be a field formal is a different question.
PTAL.
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. |
Bob Nystrom (Gerrit)
Bob Nystrom voted and added 2 comments![]( "Open in Gerrit")
Votes added by Bob Nystrom
| Auto-Submit | +1 |
2 comments
if (element is FieldFormalParameterElement) {I think this is also wrong, and we should keep it always `IS_REFERENCED_BY`.
```
class A {
A({this.field});
var field;
}
void foo() {
A _ = .new(field: 42);
}
```
Here `field: 42` references a formal parameter, the fact that it is a field formal parameter, does not matter. Or at least I don't understand why it would matter. We always use the `name` of the formal parameter, the fact that it might also have a private name associated with the private field is something that is important only locally for the declaring constructor.
Bob Nystrom
Changed this to no longer test if it's a *field* formal. Instead, used IS_REFERENCED_BY_NAMED_ARGUMENT and apply that kind for any identifier that is a reference to a named parameter.
See my other long comment for why we need to distinguish this case from other non-argument references to field parameters.
test_isReferencedBy_FieldElement_dotSorthandConstructorInvocation() async {Pre-existing, but the name is wrong.
Looks very similar to another test:
```
test_isReferencedBy_ParameterElement_dotSorthandConstructorInvocation() async {
await _indexTestUnit('''
class A {
A({p});
}
void foo() {
A _ = .new(p: 42);
}
''');
var element = findElement2.parameter('p');
assertElementIndexText(element, r'''
48 5:14 |p| IS_REFERENCED_BY qualified
''');
}
```
We refer not to a field, but a parameter, which in the implementation happens to be storing into a field.I'd change the name to `test_isReferencedBy_ParameterElement_dotSorthandConstructorInvocation_field`
Done
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 voted Code-Review+1![]( "Open in Gerrit")
- Bob Nystrom
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bob Nystrom (Gerrit)
Bob Nystrom voted Commit-Queue+2![]( "Open in Gerrit")
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bob Nystrom (Gerrit)
Bob Nystrom voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Brian Wilkerson
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
6 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/summary/format.fbs
Insertions: 3, Deletions: 4.
@@ -73,11 +73,10 @@
/// Right: location.
IS_REFERENCED_BY_DOT_SHORTHAND_CONSTRUCTOR_TEAR_OFF,
- /// Left: a field.
- /// Is referenced by a named argument whose corresponding parameter is a
- /// field formal or declaring parameter that refers to that field.
+ /// Left: a parameter.
+ /// Is referenced by a named argument.
/// Right: named argument.
- IS_FIELD_REFERENCED_BY_NAMED_ARGUMENT,
+ IS_REFERENCED_BY_NAMED_ARGUMENT,
/// Left: unresolved member name.
/// Is read at.
```
```
The name of the file: pkg/analyzer/test/src/dart/analysis/index_test.dart
Insertions: 16, Deletions: 16.
@@ -1776,22 +1776,6 @@
''');
}
- test_isReferencedBy_ParameterElement_dotSorthandConstructorInvocation_field() async {
- await _indexTestUnit('''
-class A {
- A({this.field});
- var field;
-}
-void foo() {
- A _ = .new(field: 42);
-}
-''');
- var element = findElement2.fieldFormalParameter('field');
- assertElementIndexText(element, r'''
-70 6:14 |field| IS_REFERENCED_BY_NAMED_ARGUMENT qualified
-''');
- }
-
test_isReferencedBy_FieldElement_enum() async {
await _indexTestUnit('''
enum E {
@@ -2144,6 +2128,22 @@
''');
}
+ test_isReferencedBy_ParameterElement_dotSorthandConstructorInvocation_field() async {
+ await _indexTestUnit('''
+class A {
+ A({this.field});
+ var field;
+}
+void foo() {
+ A _ = .new(field: 42);
+}
+''');
+ var element = findElement2.fieldFormalParameter('field');
+ assertElementIndexText(element, r'''
+70 6:14 |field| IS_REFERENCED_BY_NAMED_ARGUMENT qualified
+''');
+ }
+
test_isReferencedBy_ParameterElement_genericFunctionType() async {
await _indexTestUnit('''
typedef F = void Function({int? p});
```
Change information
[private named parameters] Handle rename refactoring.
Remove the special case handling that would add a public parameter and
a separate initializer in the initializer list since you can now use the
private name as a parameter directly.
Handle updating references at constructor callsites where we need to
rename the argument to the corresponding public name.
- M pkg/analysis_server/lib/src/services/refactoring/legacy/refactoring_internal.dart
- M pkg/analysis_server/lib/src/services/refactoring/legacy/rename.dart
- M pkg/analysis_server/lib/src/services/refactoring/legacy/rename_class_member.dart
- M pkg/analysis_server/lib/src/services/search/search_engine.dart
- M pkg/analysis_server/lib/src/services/search/search_engine_internal.dart
- M pkg/analysis_server/test/protocol_server_test.dart
- M pkg/analysis_server/test/services/refactoring/legacy/rename_class_member_test.dart
- M pkg/analysis_server/test/services/search/search_engine_test.dart
- M pkg/analyzer/lib/src/dart/analysis/index.dart
- M pkg/analyzer/lib/src/dart/analysis/search.dart
- M pkg/analyzer/lib/src/summary/format.fbs
- M pkg/analyzer/lib/src/summary/idl.dart
- M pkg/analyzer/test/src/dart/analysis/index_test.dart
- M pkg/analyzer/test/src/dart/analysis/search_test.dart
Code-Review: +1 by Brian Wilkerson, +1 by Konstantin Shcheglov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |