Restrict "prefer_initializing_formals" to avoid false positives.
When an explicit initializer or assignment is turned into an initializing formal, it's possible for that to change the semantics if there are other references to the same parameter.
For example:
```dart
// Before:
class C {
int? x;
C(int? x) {
print(this.x);
this.x = x;
}
}
// After:
class C {
int? x;
C(this.x) {
print(this.x);
}
}
```
This prints "null" before and the argument value after.
The fix might even lead to invalid code:
```dart
// Before:
class C {
int? x;
int? y;
C(int? x) : y = (x = 2) {
this.x = x;
}
}
// After:
class C {
int? x;
int? y;
C(this.x) : y = (x = 2);
}
```
This becomes a compile error because `x` is final when it refers to an initializing formal in the initializer list.
It's also not enough to look for secondary writes inside the constructor:
```dart
// Before:
class C {
int? x;
Function()? closure;
C(int? x) {
closure = () {
print(x);
};
this.x = x;
}
}
// After:
class C {
int? x;
Function()? closure;
C(this.x) {
closure = () {
print(x);
};
}
}
// Given:
main() {
var c = C(1);
c.x = 2;
c.closure!();
}
```
This prints "1" before and "2" after.
I think the safest thing is to be conservative and not show the lint if there are any other references to the parameter anywhere in the constructor, even reads.
Fix #58607.
Change-Id: Ib976cd5bfc4bf44439ecd4175090dbabfe1cde16
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/482985
Commit-Queue: Paul Berry <paulberry@google.com>
Commit-Queue: Bob Nystrom <rnystrom@google.com>
Reviewed-by: Paul Berry <paulberry@google.com>
Reviewed-by: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Auto-Submit: Bob Nystrom <rnystrom@google.com>
This commit is contained in:
committed by
Commit Queue
parent
dc2d8a06b9
commit
3ea7130e4e
@@ -85,8 +85,6 @@ class AnalysisContextCollectionImpl implements AnalysisContextCollection {
|
||||
}
|
||||
scheduler.start();
|
||||
}
|
||||
// TODO(scheglov): https://github.com/dart-lang/linter/issues/3134
|
||||
// ignore: prefer_initializing_formals
|
||||
this.scheduler = scheduler;
|
||||
|
||||
_throwIfAnyNotAbsoluteNormalizedPath(includedPaths);
|
||||
|
||||
@@ -120,8 +120,6 @@ class _ConstructorChecker {
|
||||
}
|
||||
|
||||
_nodesToLintByField.forEach((field, nodes) {
|
||||
if (nodes.length > 1) return;
|
||||
|
||||
for (var lintNode in nodes) {
|
||||
_rule.reportAtNode(lintNode, arguments: [field.name!]);
|
||||
}
|
||||
@@ -168,10 +166,36 @@ class _ConstructorChecker {
|
||||
return;
|
||||
}
|
||||
|
||||
// There can't be any other references to the parameter. If there are, it's
|
||||
// possible removing the initializer/assignment and moving it up to be an
|
||||
// initializing formal could be a semantic change.
|
||||
var visitor = _ReferenceCounter(parameter);
|
||||
// Visit the initializers and body directly so that we ignore references in
|
||||
// the doc comment.
|
||||
_constructor.initializers.accept(visitor);
|
||||
_constructor.body.accept(visitor);
|
||||
if (visitor.count > 1) return;
|
||||
|
||||
_nodesToLintByField.putIfAbsent(field, () => []).add(node);
|
||||
}
|
||||
}
|
||||
|
||||
/// Counts references in the visited AST to a given parameter.
|
||||
class _ReferenceCounter extends RecursiveAstVisitor<void> {
|
||||
final FormalParameterElement parameterElement;
|
||||
|
||||
int count = 0;
|
||||
|
||||
_ReferenceCounter(this.parameterElement);
|
||||
|
||||
@override
|
||||
void visitSimpleIdentifier(SimpleIdentifier node) {
|
||||
if (node.element == parameterElement) {
|
||||
count++;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
class _Visitor extends SimpleAstVisitor<void> {
|
||||
final AnalysisRule _rule;
|
||||
final RuleContext _context;
|
||||
|
||||
@@ -55,7 +55,7 @@ class A {
|
||||
}
|
||||
class C extends A {
|
||||
int? c, d;
|
||||
C(int c, int d) : super(c, d) {
|
||||
C(int c, int d) : super(1, 2) {
|
||||
this.c = c;
|
||||
this.d = d;
|
||||
}
|
||||
@@ -78,6 +78,61 @@ class C {
|
||||
''');
|
||||
}
|
||||
|
||||
test_assignedInBody_multipleReference_body() async {
|
||||
await assertNoDiagnostics(r'''
|
||||
class C {
|
||||
num x = 0;
|
||||
C(num x) {
|
||||
print(x);
|
||||
this.x = x;
|
||||
}
|
||||
}
|
||||
''');
|
||||
}
|
||||
|
||||
test_assignedInBody_multipleReference_closure() async {
|
||||
await assertNoDiagnostics(r'''
|
||||
class C {
|
||||
int? x;
|
||||
Function()? closure;
|
||||
C(int? x) {
|
||||
closure = () {
|
||||
print(x);
|
||||
};
|
||||
this.x = x;
|
||||
}
|
||||
}
|
||||
''');
|
||||
}
|
||||
|
||||
test_assignedInBody_multipleReference_docComment() async {
|
||||
await assertDiagnostics(
|
||||
r'''
|
||||
class C {
|
||||
num x = 0;
|
||||
|
||||
/// References to [x] in this doc comment like [x] and [x] are ignored.
|
||||
C(num x) {
|
||||
this.x = x;
|
||||
}
|
||||
}
|
||||
''',
|
||||
[lint(115, 10)],
|
||||
);
|
||||
}
|
||||
|
||||
test_assignedInBody_multipleReference_initializer() async {
|
||||
await assertNoDiagnostics(r'''
|
||||
class C {
|
||||
num x = 0;
|
||||
num y = 0;
|
||||
C(num x) : y = x {
|
||||
this.x = x;
|
||||
}
|
||||
}
|
||||
''');
|
||||
}
|
||||
|
||||
test_assignedInBody_namedParameters() async {
|
||||
await assertDiagnostics(
|
||||
r'''
|
||||
@@ -198,7 +253,7 @@ class C extends A {
|
||||
C(int c, int d)
|
||||
: this.c = c,
|
||||
this.d = d,
|
||||
super(c, d);
|
||||
super(1, 2);
|
||||
}
|
||||
''',
|
||||
[lint(103, 10), lint(123, 10)],
|
||||
|
||||
Reference in New Issue
Block a user