diff --git a/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml b/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml index 3af2ade5b522..e562c1df243d 100644 --- a/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml +++ b/pkg/analysis_server/lib/src/services/correction/error_fix_status.yaml @@ -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: |- @@ -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 \ No newline at end of file + The same fix as CompileTimeErrorCode.URI_DOES_NOT_EXIST diff --git a/pkg/linter/example/all.yaml b/pkg/linter/example/all.yaml index 8d54383b21fe..7b668895cd3d 100644 --- a/pkg/linter/example/all.yaml +++ b/pkg/linter/example/all.yaml @@ -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 diff --git a/pkg/linter/lib/src/rules.dart b/pkg/linter/lib/src/rules.dart index c0fd1864bb9e..0e73c6fe6832 100644 --- a/pkg/linter/lib/src/rules.dart +++ b/pkg/linter/lib/src/rules.dart @@ -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'; @@ -358,6 +359,7 @@ void registerLintRules() { ..register(NullCheckOnNullableTypeParameter()) ..register(NullClosures()) ..register(OmitLocalVariableTypes()) + ..register(OmitObviousLocalVariableTypes()) ..register(OneMemberAbstracts()) ..register(OnlyThrowErrors()) ..register(OverriddenFields()) diff --git a/pkg/linter/lib/src/rules/always_specify_types.dart b/pkg/linter/lib/src/rules/always_specify_types.dart index 6fe1a4f7e378..fdfe0cd49618 100644 --- a/pkg/linter/lib/src/rules/always_specify_types.dart +++ b/pkg/linter/lib/src/rules/always_specify_types.dart @@ -73,8 +73,11 @@ class AlwaysSpecifyTypes extends LintRule { categories: {Category.style}); @override - List get incompatibleRules => - const ['avoid_types_on_closure_parameters', 'omit_local_variable_types']; + List get incompatibleRules => const [ + 'avoid_types_on_closure_parameters', + 'omit_local_variable_types', + 'omit_obvious_local_variable_types', + ]; @override LintCode get lintCode => code; diff --git a/pkg/linter/lib/src/rules/omit_obvious_local_variable_types.dart b/pkg/linter/lib/src/rules/omit_obvious_local_variable_types.dart new file mode 100644 index 000000000000..c9f52f4d3f7d --- /dev/null +++ b/pkg/linter/lib/src/rules/omit_obvious_local_variable_types.dart @@ -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> possibleDesserts(Set pantry) { + List> desserts = >[]; + for (List recipe in cookbook) { + if (pantry.containsAll(recipe)) { + desserts.add(recipe); + } + } + + return desserts; +} +``` + +**GOOD:** +```dart +List> possibleDesserts(Set pantry) { + var desserts = >[]; + for (List 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 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 { + 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 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; + } +} diff --git a/pkg/linter/test/rules/all.dart b/pkg/linter/test/rules/all.dart index c6865b589af1..49a923f65e20 100644 --- a/pkg/linter/test/rules/all.dart +++ b/pkg/linter/test/rules/all.dart @@ -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; @@ -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(); diff --git a/pkg/linter/test/rules/omit_obvious_local_variable_types_test.dart b/pkg/linter/test/rules/omit_obvious_local_variable_types_test.dart new file mode 100644 index 000000000000..ce3a1f263690 --- /dev/null +++ b/pkg/linter/test/rules/omit_obvious_local_variable_types_test.dart @@ -0,0 +1,299 @@ +// 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:test_reflective_loader/test_reflective_loader.dart'; + +import '../rule_test_support.dart'; + +main() { + defineReflectiveSuite(() { + defineReflectiveTests(OmitObviousLocalVariableTypesTest); + }); +} + +@reflectiveTest +class OmitObviousLocalVariableTypesTest extends LintRuleTest { + @override + String get lintRule => 'omit_obvious_local_variable_types'; + + test_cascade() async { + await assertDiagnostics(r''' +f() { + A a = A()..x..x..x; +} + +class A { + final x = 0; +} +''', [ + lint(8, 1), + ]); + } + + test_forEach_inferredList() async { + await assertDiagnostics(r''' +f() { + for (int i in [1, 2, 3]) { } +} +''', [ + lint(13, 3), + ]); + } + + test_forEach_listWithNonObviousElement() async { + await assertDiagnostics(r''' +f() { + var j = "Hello".length; + for (int i in [j, 1, j + 1]) { } +} +''', [ + lint(39, 3), + ]); + } + + test_forEach_noDeclaredType() async { + await assertNoDiagnostics(r''' +f() { + for (var i in [1, 2, 3]) { } +} +'''); + } + + test_forEach_nonObviousIterable() async { + await assertNoDiagnostics(r''' +f() { + var list = [1, 2, 3]; + for (int i in list) { } +} +'''); + } + + test_forEach_typedList() async { + await assertDiagnostics(r''' +f() { + for (int i in [1, 2, 3]) { } +} +''', [ + lint(13, 3), + ]); + } + + test_genericInvocation_paramIsType() async { + await assertNoDiagnostics(r''' +T bar(T d) => d; + +String f() { + String h = bar(''); + return h; +} +'''); + } + + test_genericInvocation_typeNeededForInference() async { + await assertNoDiagnostics(r''' +T bar(dynamic d) => d; + +String f() { + String h = bar(''); + return h; +} +'''); + } + + test_genericInvocation_typeParamProvided() async { + await assertNoDiagnostics(r''' +T bar(dynamic d) => d; + +String f() { + String h = bar(''); + return h; +} +'''); + } + + test_instanceCreation_generic() async { + await assertDiagnostics(r''' +f() { + A a = A(); +} + +class A {} +''', [ + lint(8, 6), + ]); + } + + test_instanceCreation_generic_ok() async { + await assertNoDiagnostics(r''' +f() { + A a = A(); +} + +class A {} +'''); + } + + test_instanceCreation_nonGeneric() async { + await assertDiagnostics(r''' +f() { + A a = A(); +} + +class A {} +''', [ + lint(8, 1), + ]); + } + + test_literal_bool() async { + await assertDiagnostics(r''' +f() { + bool b = true; +} +''', [ + lint(8, 4), + ]); + } + + test_literal_double() async { + await assertDiagnostics(r''' +f() { + double d = 1.5; +} +''', [ + lint(8, 6), + ]); + } + + // The type is not obvious. + test_literal_doubleTypedInt() async { + await assertNoDiagnostics(r''' +f() { + double d = 1; +} +'''); + } + + test_literal_int() async { + await assertDiagnostics(r''' +f() { + int i = 1; +} +''', [ + lint(8, 3), + ]); + } + + // `Null` is not obvious, the inferred type is `dynamic`. + test_literal_null() async { + await assertNoDiagnostics(r''' +f() { + Null nil = null; +} +'''); + } + + test_literal_string() async { + await assertDiagnostics(r''' +f() { + String s = "A string"; +} +''', [ + lint(8, 6), + ]); + } + + test_literal_symbol() async { + await assertDiagnostics(r''' +f() { + Symbol s = #print; +} +''', [ + lint(8, 6), + ]); + } + + test_local_multiple() async { + await assertDiagnostics(r''' +f() { + String a = 'a', b = 'b'; +} +''', [ + lint(8, 6), + ]); + } + + test_local_multiple_ok() async { + await assertNoDiagnostics(r''' +f() { + var a = 'a', b = 'b'; +} +'''); + } + + /// Types are considered an important part of the pattern so we + /// intentionally do not lint on declared variable patterns. + test_pattern_list_destructured() async { + await assertNoDiagnostics(r''' +f() { + var [int a] = [1]; +} +'''); + } + + test_pattern_map_destructured() async { + await assertNoDiagnostics(r''' +f() { + var {'a': int a} = {'a': 1}; +} +'''); + } + + test_pattern_object_destructured() async { + await assertNoDiagnostics(r''' +class A { + int a; + A(this.a); +} +f() { + final A(a: int _b) = A(1); +} +'''); + } + + test_pattern_record_destructured() async { + await assertNoDiagnostics(r''' +f(Object o) { + switch (o) { + case (int x, String s): + } +} +'''); + } + + test_switch_pattern_object() async { + await assertNoDiagnostics(r''' +class A { + int a; + A(this.a); +} + +f() { + switch (A(1)) { + case A(a: >0 && int b): + } +} +'''); + } + + test_switch_pattern_record() async { + await assertNoDiagnostics(r''' +f() { + switch ((1, 2)) { + case (int a, final int b): + } +} +'''); + } +} diff --git a/pkg/linter/tool/since/sdk.yaml b/pkg/linter/tool/since/sdk.yaml index 0f6c196abd01..0407ce010942 100644 --- a/pkg/linter/tool/since/sdk.yaml +++ b/pkg/linter/tool/since/sdk.yaml @@ -114,6 +114,7 @@ noop_primitive_operations: 2.14.0 null_check_on_nullable_type_parameter: 2.12.0 null_closures: 2.0.0 omit_local_variable_types: 2.0.0 +omit_obvious_local_variable_types: 3.5.0-wip one_member_abstracts: 2.0.0 only_throw_errors: 2.0.0 overridden_fields: 2.0.0