Skip to content

Commit

Permalink
Add lint omit_obvious_local_variable_types
Browse files Browse the repository at this point in the history
This CL adds an implementation of a lint named
'omit_obvious_local_variable_types' which is a variant of
'omit_local_variable_types' that only flags type annotations
of local variables when the static type of the initializing
expression is obvious, as defined in
https://github.com/dart-lang/linter/issues/3480.

Change-Id: I7e9b5adde5839ad8f9dfa88833da82d7bc1b65de
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/374601
Commit-Queue: Erik Ernst <eernst@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
  • Loading branch information
eernstg authored and Commit Queue committed Jul 16, 2024
1 parent 75617a1 commit 002bcd0
Show file tree
Hide file tree
Showing 8 changed files with 587 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2202,6 +2202,11 @@ LintCode.null_closures:
status: hasFix
LintCode.omit_local_variable_types:
status: hasFix
LintCode.omit_obvious_local_variable_types:
status: needsEvaluation
notes: |-
The fix `ReplaceWithVar` which is used with omit_local_variable_types
should work for this one as well.
LintCode.one_member_abstracts:
status: noFix
notes: |-
Expand Down Expand Up @@ -3755,4 +3760,4 @@ WarningCode.UNUSED_SHOWN_NAME:
WarningCode.URI_DOES_NOT_EXIST_IN_DOC_IMPORT:
status: needsFix
notes: |-
The same fix as CompileTimeErrorCode.URI_DOES_NOT_EXIST
The same fix as CompileTimeErrorCode.URI_DOES_NOT_EXIST
1 change: 1 addition & 0 deletions pkg/linter/example/all.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ linter:
- null_check_on_nullable_type_parameter
- null_closures
- omit_local_variable_types
- omit_obvious_local_variable_types
- one_member_abstracts
- only_throw_errors
- overridden_fields
Expand Down
2 changes: 2 additions & 0 deletions pkg/linter/lib/src/rules.dart
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ import 'rules/noop_primitive_operations.dart';
import 'rules/null_check_on_nullable_type_parameter.dart';
import 'rules/null_closures.dart';
import 'rules/omit_local_variable_types.dart';
import 'rules/omit_obvious_local_variable_types.dart';
import 'rules/one_member_abstracts.dart';
import 'rules/only_throw_errors.dart';
import 'rules/overridden_fields.dart';
Expand Down Expand Up @@ -358,6 +359,7 @@ void registerLintRules() {
..register(NullCheckOnNullableTypeParameter())
..register(NullClosures())
..register(OmitLocalVariableTypes())
..register(OmitObviousLocalVariableTypes())
..register(OneMemberAbstracts())
..register(OnlyThrowErrors())
..register(OverriddenFields())
Expand Down
7 changes: 5 additions & 2 deletions pkg/linter/lib/src/rules/always_specify_types.dart
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,11 @@ class AlwaysSpecifyTypes extends LintRule {
categories: {Category.style});

@override
List<String> get incompatibleRules =>
const ['avoid_types_on_closure_parameters', 'omit_local_variable_types'];
List<String> get incompatibleRules => const [
'avoid_types_on_closure_parameters',
'omit_local_variable_types',
'omit_obvious_local_variable_types',
];

@override
LintCode get lintCode => code;
Expand Down
270 changes: 270 additions & 0 deletions pkg/linter/lib/src/rules/omit_obvious_local_variable_types.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,270 @@
// Copyright (c) 2024, 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.

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

import '../analyzer.dart';
import '../extensions.dart';

const _desc = r'Omit obvious type annotations for local variables.';

const _details = r'''
Don't type annotate initialized local variables when the type is obvious.
Local variables, especially in modern code where functions tend to be small,
have very little scope. Omitting the type focuses the reader's attention on the
more important *name* of the variable and its initialized value. Hence, local
variable type annotations that are obvious should be omitted.
**BAD:**
```dart
List<List<Ingredient>> possibleDesserts(Set<Ingredient> pantry) {
List<List<Ingredient>> desserts = <List<Ingredient>>[];
for (List<Ingredient> recipe in cookbook) {
if (pantry.containsAll(recipe)) {
desserts.add(recipe);
}
}
return desserts;
}
```
**GOOD:**
```dart
List<List<Ingredient>> possibleDesserts(Set<Ingredient> pantry) {
var desserts = <List<Ingredient>>[];
for (List<Ingredient> recipe in cookbook) {
if (pantry.containsAll(recipe)) {
desserts.add(recipe);
}
}
return desserts;
}
```
Sometimes the inferred type is not the type you want the variable to have. For
example, you may intend to assign values of other types later. You may also
wish to write a type annotation explicitly because the type of the initializing
expression is non-obvious and it will be helpful for future readers of the
code to document this type. Or you may wish to commit to a specific type such
that future updates of dependencies (in nearby code, in imports, anywhere)
will not silently change the type of that variable, thus introducing
compile-time errors or run-time bugs in locations where this variable is used.
In those cases, go ahead and annotate the variable with the type you want.
**GOOD:**
```dart
Widget build(BuildContext context) {
Widget result = someGenericFunction(42) ?? Text('You won!');
if (applyPadding) {
result = Padding(padding: EdgeInsets.all(8.0), child: result);
}
return result;
}
```
**This rule is experimental.** It is being evaluated, and it may be changed
or removed. Feedback on its behavior is welcome! The main issue is here:
https://github.com/dart-lang/linter/issues/3480.
''';

class OmitObviousLocalVariableTypes extends LintRule {
static const LintCode code = LintCode('omit_obvious_local_variable_types',
'Unnecessary and obvious type annotation on a local variable.',
correctionMessage: 'Try removing the type annotation.');

OmitObviousLocalVariableTypes()
: super(
name: 'omit_obvious_local_variable_types',
description: _desc,
details: _details,
state: State.experimental(),
categories: {Category.style});

@override
List<String> get incompatibleRules => const ['always_specify_types'];

@override
LintCode get lintCode => code;

@override
void registerNodeProcessors(
NodeLintRegistry registry, LinterContext context) {
var visitor = _Visitor(this);
registry.addForStatement(this, visitor);
registry.addVariableDeclarationStatement(this, visitor);
}
}

class _Visitor extends SimpleAstVisitor<void> {
final LintRule rule;

_Visitor(this.rule);

@override
void visitForStatement(ForStatement node) {
var loopParts = node.forLoopParts;
if (loopParts is ForPartsWithDeclarations) {
_visitVariableDeclarationList(loopParts.variables);
} else if (loopParts is ForEachPartsWithDeclaration) {
var loopVariableType = loopParts.loopVariable.type;
var staticType = loopVariableType?.type;
if (staticType == null || staticType is DynamicType) {
return;
}
var iterable = loopParts.iterable;
if (!iterable.hasObviousType) {
return;
}
var iterableType = iterable.staticType;
if (iterableType.elementTypeOfIterable == staticType) {
rule.reportLint(loopVariableType);
}
}
}

@override
void visitVariableDeclarationStatement(VariableDeclarationStatement node) {
_visitVariableDeclarationList(node.variables);
}

void _visitVariableDeclarationList(VariableDeclarationList node) {
var staticType = node.type?.type;
if (staticType == null ||
staticType is DynamicType ||
staticType.isDartCoreNull) {
return;
}
for (var child in node.variables) {
var initializer = child.initializer;
if (initializer != null && !initializer.hasObviousType) {
return;
}
if (initializer?.staticType != staticType) {
return;
}
}
rule.reportLint(node.type);
}
}

extension on CollectionElement {
DartType? get elementType {
var self = this; // Enable promotion.
switch (self) {
case MapLiteralEntry():
return null;
case ForElement():
// No need to compute the type of a non-obvious element.
return null;
case IfElement():
// We just need a candidate type, ignore `else`.
return self.thenElement.elementType;
case Expression():
return self.staticType;
case SpreadElement():
return self.expression.staticType.elementTypeOfIterable;
}
}

bool get hasObviousType {
var self = this; // Enable promotion.
switch (self) {
case MapLiteralEntry():
return self.key.hasObviousType && self.value.hasObviousType;
case ForElement():
return false;
case IfElement():
return self.thenElement.hasObviousType &&
(self.elseElement?.hasObviousType ?? true);
case Expression():
return self.hasObviousType;
case SpreadElement():
return self.expression.hasObviousType;
}
}
}

extension on DartType? {
DartType? get elementTypeOfIterable {
var self = this; // Enable promotion.
if (self == null) return null;
if (self is InterfaceType) {
var iterableInterfaces =
self.implementedInterfaces.where((type) => type.isDartCoreIterable);
if (iterableInterfaces.length == 1) {
return iterableInterfaces.first.typeArguments.first;
}
}
return null;
}
}

extension on Expression {
bool get hasObviousType {
var self = this; // Enable promotion.
switch (self) {
case TypedLiteral():
if (self.typeArguments != null) {
// A collection literal with explicit type arguments is trivial.
return true;
}
// A collection literal with no explicit type arguments.
var anyElementIsObvious = false;
DartType? theObviousType;
NodeList<CollectionElement> elements = switch (self) {
ListLiteral() => self.elements,
SetOrMapLiteral() => self.elements
};
for (var element in elements) {
if (element.hasObviousType) {
if (anyElementIsObvious) {
continue;
}
anyElementIsObvious = true;
theObviousType = element.elementType;
}
}
if (anyElementIsObvious) {
var theSelfElementType = self.staticType.elementTypeOfIterable;
return theSelfElementType == theObviousType;
}
return false;
case Literal():
// An atomic literal: `Literal` and not `TypedLiteral`.
if (self is IntegerLiteral &&
(self.staticType?.isDartCoreDouble ?? false)) {
return false;
}
return true;
case InstanceCreationExpression():
var createdType = self.constructorName.type;
if (createdType.typeArguments != null) {
// Explicit type arguments provided.
return true;
} else {
DartType? dartType = createdType.type;
if (dartType != null) {
if (dartType is InterfaceType &&
dartType.element.typeParameters.isNotEmpty) {
// A raw type is not trivial.
return false;
}
// A non-generic class or extension type.
return true;
} else {
// An unknown type is not trivial.
return false;
}
}
case CascadeExpression():
return self.target.hasObviousType;
}
return false;
}
}
3 changes: 3 additions & 0 deletions pkg/linter/test/rules/all.dart
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,8 @@ import 'null_check_on_nullable_type_parameter_test.dart'
as null_check_on_nullable_type_parameter;
import 'null_closures_test.dart' as null_closures;
import 'omit_local_variable_types_test.dart' as omit_local_variable_types;
import 'omit_obvious_local_variable_types_test.dart'
as omit_obvious_local_variable_types;
import 'one_member_abstracts_test.dart' as one_member_abstracts;
import 'only_throw_errors_test.dart' as only_throw_errors;
import 'overridden_fields_test.dart' as overridden_fields;
Expand Down Expand Up @@ -393,6 +395,7 @@ void main() {
null_check_on_nullable_type_parameter.main();
null_closures.main();
omit_local_variable_types.main();
omit_obvious_local_variable_types.main();
one_member_abstracts.main();
only_throw_errors.main();
overridden_fields.main();
Expand Down
Loading

0 comments on commit 002bcd0

Please sign in to comment.