Skip to content
This repository has been archived by the owner on Nov 20, 2024. It is now read-only.

avoid_shadowing #872

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions example/all.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ linter:
- avoid_returning_null_for_void
- avoid_returning_this
- avoid_setters_without_getters
- avoid_shadowing
- avoid_single_cascade_in_expression_statements
- avoid_slow_async_io
- avoid_types_as_parameter_names
Expand Down
2 changes: 2 additions & 0 deletions lib/src/rules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import 'package:linter/src/rules/avoid_returning_null_for_void.dart';
import 'package:linter/src/rules/avoid_returning_this.dart';
import 'package:linter/src/rules/avoid_setters_without_getters.dart';
import 'package:linter/src/rules/avoid_single_cascade_in_expression_statements.dart';
import 'package:linter/src/rules/avoid_shadowing.dart';
import 'package:linter/src/rules/avoid_slow_async_io.dart';
import 'package:linter/src/rules/avoid_types_as_parameter_names.dart';
import 'package:linter/src/rules/avoid_types_on_closure_parameters.dart';
Expand Down Expand Up @@ -166,6 +167,7 @@ void registerLintRules() {
..register(new AvoidReturningThis())
..register(new AvoidSettersWithoutGetters())
..register(new AvoidSingleCascadeInExpressionStatements())
..register(new AvoidShadowing())
..register(new AvoidSlowAsyncIo())
..register(new AvoidTypesAsParameterNames())
..register(new AvoidUnusedConstructorParameters())
Expand Down
204 changes: 204 additions & 0 deletions lib/src/rules/avoid_shadowing.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

library linter.src.rules.avoid_shadowing;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the library directive.


import 'package:analyzer/dart/ast/ast.dart';
import 'package:analyzer/dart/ast/visitor.dart';
import 'package:analyzer/dart/element/element.dart';
import 'package:linter/src/analyzer.dart';

const desc = r'Do not use a name already visible.';

const details = r'''

**DO** Do not use a name already visible as getter or variable.

**BAD:**
```
var k = null;

class A {
var a;
}

class B extends A {
get b => null;

m() {
var a; // LINT
var b; // LINT
var k; // LINT
}
}
```

**GOOD:**
```
var k = null;

class A {
var a;
}

class B extends A {
get b => null;

m() {
var c; // OK
var d; // OK
var e; // OK
}
}
```
''';

class AvoidShadowing extends LintRule implements NodeLintRule {
AvoidShadowing()
: super(
name: 'avoid_shadowing',
description: desc,
details: details,
group: Group.errors);

@override
void registerNodeProcessors(NodeLintRegistry registry) {
final visitor = new _Visitor(this);
registry.addVariableDeclarationStatement(this, visitor);
}
}

class _Visitor extends SimpleAstVisitor {
final LintRule rule;

_Visitor(this.rule);

@override
visitVariableDeclarationStatement(VariableDeclarationStatement node) {
final variables = node.variables.variables.toList();
final library = variables.first.declaredElement.library;

// exclude pattern : var name = this.name;
variables.removeWhere((variable) {
final initializer = variable.initializer;
return initializer is PropertyAccess &&
initializer.propertyName.name == variable.name.name &&
(initializer.target is ThisExpression ||
initializer.target is SuperExpression);
});

// exclude pattern : same name as current getter name
var currentGetter = _getCurrentGetter(node);
if (currentGetter != null) {
variables.removeWhere(
(variable) => currentGetter.name.name == variable.name.name);
}

bool skipInstanceMembers = false;
AstNode current = node;
while (current != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem very efficient, but perhaps in practice it doesn't matter. Have we done any timing measures for this rule?

One easy improvement would be to return before line 98 if variables is empty.

if (current is ClassDeclaration) {
_checkClass(current, variables, onlyStatics: skipInstanceMembers);
} else if (current is ConstructorDeclaration) {
_checkParameters(current.parameters, variables);
if (current.factoryKeyword != null) {
skipInstanceMembers = true;
}
} else if (current is MethodDeclaration) {
_checkParameters(current.parameters, variables);
if (current.isStatic) {
skipInstanceMembers = true;
}
} else if (current is FunctionExpression) {
_checkParameters(current.parameters, variables);
} else if (current is FunctionDeclaration) {
_checkParameters(current.functionExpression.parameters, variables);
} else if (current is FunctionDeclarationStatement) {
_checkParameters(
current.functionDeclaration.functionExpression.parameters,
variables);
}

if (current.parent is Block) {
_checkParentBlock(current, variables);
}

current = current.parent;
}
_checkLibrary(library, variables);
}

void _checkClass(
ClassDeclaration clazz,
List<VariableDeclaration> variables, {
bool onlyStatics,
}) {
for (final variable in variables) {
final name = variable.name.name;
final getter = clazz.declaredElement
.lookUpGetter(name, clazz.declaredElement.library);
if (getter != null && (!onlyStatics || getter.isStatic))
rule.reportLint(variable);
}
}

void _checkLibrary(
LibraryElement library, List<VariableDeclaration> variables) {
final topLevelVariableNames = library.units
.expand((u) => u.topLevelVariables)
.map((e) => e.name)
.toList();
for (final variable in variables) {
final name = variable.name.name;
if (topLevelVariableNames.contains(name)) {
rule.reportLint(variable);
}
}
}

void _checkParameters(
FormalParameterList parameters, List<VariableDeclaration> variables) {
if (parameters == null) return;

final parameterNames =
parameters.parameterElements.map((e) => e.name).toList();

for (final variable in variables) {
final name = variable.name.name;
if (parameterNames.contains(name)) {
rule.reportLint(variable);
}
}
}

void _checkParentBlock(AstNode node, List<VariableDeclaration> variables) {
final block = node.parent as Block;
final names = <String>[];
for (final statement in block.statements.takeWhile((n) => n != node)) {
if (statement is VariableDeclarationStatement) {
names.addAll(statement.variables.variables.map((e) => e.name.name));
}
}
for (final variable in variables) {
final name = variable.name.name;
if (names.contains(name)) {
rule.reportLint(variable);
}
}
}

MethodDeclaration _getCurrentGetter(VariableDeclarationStatement node) {
AstNode current = node.parent;
while (current != null) {
if (current is Block || current is FunctionBody) {
current = current.parent;
} else if (current is MethodDeclaration && current.isGetter) {
return current;
} else {
break;
}
}
return null;
}
}
108 changes: 108 additions & 0 deletions test/rules/avoid_shadowing.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
// Copyright (c) 2018, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.

// test w/ `pub run test -N avoid_shadowing`

var top = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any tests for shadowing parameter names.

Also, I suspect that the rule as written will produce multiple lints for the same location if the name being shadowed is defined in multiple locations. Consider a test such as:

class C {
  int x;
  m(int x) {
    int x; // LINT
  }
}

(Can we verify that only one lint is produced in this case?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

It looks like several report on the same token for the same rule do not result in lint duplications.


m() {
f() {
var top; // LINT
var a; // OK
}

var a; // OK
var b; // OK
var top; // LINT

g() {
var a; // LINT
var c; // OK
}
}

class A {
var a;
get b => null;

ma() {
var a; // LINT
var b; // LINT
var top; // LINT
}

static sm() {
var a; // OK
var b; // OK
var c; // OK
}
}

class B extends A {
mb() {
var a; // LINT
var b; // LINT
var c; // OK
}
}

class C extends Object with A {
mc() {
var a; // LINT
var b; // LINT
var c; // OK
}
}

// special case for factory and static
class D {
var a;
get b => null;
static get c => null;

static sm() {
var a; // OK
var b; // OK
var c; // LINT
}

factory D() {
var a; // OK
var b; // OK
var c; // LINT
}
}

// 2 shadowing
class E {
int x;
m(int x) {
int x; // LINT
}
}

// exclude pattern : var x = this.x;
class F {
int x;
m() {
var x = this.x; // OK
}
}

// exclude pattern : var x = super.x;
class F2 extends F {
m() {
var x = super.x; // OK
}
}

// exclude pattern : same name as current getter name
class G {
var i;
get g {
var i = ''; // LINT
var g = ''; // OK
return g;
}
}