linter: Refactor prefer_collection_literals to use context type more There is a basic premise in this rule which we cannot satisfy exactly: we need to disallow `LinkedHashSet()` unless the context type requires the developer to use `LinkedHashSet`. But the context type is long gone when the lint rule is run. This CL adds some logic to try to attempt figuring out the context type in the cases where users have filed bugs, but it will never be super accurate. Fixes https://github.com/dart-lang/linter/issues/4736 Fixes https://github.com/dart-lang/linter/issues/3057 Fixes https://github.com/dart-lang/linter/issues/1649 Fixes https://github.com/dart-lang/linter/issues/2795 Change-Id: I3e6c6de81084dca2825488c89830ab3e7ea63186 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/323680 Reviewed-by: Phil Quitslund <pquitslund@google.com> Reviewed-by: Konstantin Shcheglov <scheglov@google.com> Commit-Queue: Samuel Rawlins <srawlins@google.com>
diff --git a/pkg/analysis_server/test/src/services/correction/fix/convert_to_map_literal_test.dart b/pkg/analysis_server/test/src/services/correction/fix/convert_to_map_literal_test.dart index 3b1ea9f..f29794a 100644 --- a/pkg/analysis_server/test/src/services/correction/fix/convert_to_map_literal_test.dart +++ b/pkg/analysis_server/test/src/services/correction/fix/convert_to_map_literal_test.dart
@@ -87,26 +87,4 @@ var m = <String, int>{}; '''); } - - Future<void> test_typedef() async { - await resolveTestCode(''' -typedef M = Map<String, int>; -var m = M(); -'''); - await assertHasFix(''' -typedef M = Map<String, int>; -var m = <String, int>{}; -'''); - } - - Future<void> test_typedef_declaredType() async { - await resolveTestCode(''' -typedef M = Map<String, int>; -Map<String, int> m = M(); -'''); - await assertHasFix(''' -typedef M = Map<String, int>; -Map<String, int> m = {}; -'''); - } }
diff --git a/pkg/linter/lib/src/rules/prefer_collection_literals.dart b/pkg/linter/lib/src/rules/prefer_collection_literals.dart index 8533ea3..e9ce1b1 100644 --- a/pkg/linter/lib/src/rules/prefer_collection_literals.dart +++ b/pkg/linter/lib/src/rules/prefer_collection_literals.dart
@@ -4,7 +4,11 @@ import 'package:analyzer/dart/ast/ast.dart'; import 'package:analyzer/dart/ast/visitor.dart'; +import 'package:analyzer/dart/element/element.dart'; import 'package:analyzer/dart/element/type.dart'; +import 'package:analyzer/dart/element/type_provider.dart'; +// ignore: implementation_imports +import 'package:analyzer/src/dart/element/type.dart' show InvalidTypeImpl; import '../analyzer.dart'; import '../extensions.dart'; @@ -32,8 +36,8 @@ **EXCEPTIONS:** -There are cases with `LinkedHashSet` or `LinkedHashMap` where a literal constructor -will trigger a type error so those will be excluded from the lint. +When a `LinkedHashSet` or `LinkedHashMap` is expected, a collection literal is +not preferred (or allowed). ```dart void main() { @@ -72,7 +76,7 @@ @override void registerNodeProcessors( NodeLintRegistry registry, LinterContext context) { - var visitor = _Visitor(this); + var visitor = _Visitor(this, context.typeProvider); registry.addInstanceCreationExpression(this, visitor); registry.addMethodInvocation(this, visitor); } @@ -80,17 +84,25 @@ class _Visitor extends SimpleAstVisitor<void> { final LintRule rule; - _Visitor(this.rule); + final TypeProvider typeProvider; + _Visitor(this.rule, this.typeProvider); @override void visitInstanceCreationExpression(InstanceCreationExpression node) { var constructorName = node.constructorName.name?.name; + if (node.constructorName.type.element is TypeAliasElement) { + // Allow the use of typedef constructors. + return; + } + // Maps. - if (_isMap(node) || _isHashMap(node)) { - if (_shouldSkipLinkedHashLint(node, _isTypeHashMap)) { - return; - } + if (node.isHashMap) { + var approximateContextType = _approximateContextType(node); + if (approximateContextType is InvalidType) return; + if (approximateContextType.isTypeHashMap) return; + } + if (node.isMap || node.isHashMap) { if (constructorName == null && node.argumentList.arguments.isEmpty) { rule.reportLint(node); } @@ -98,14 +110,15 @@ } // Sets. - if (_isSet(node) || _isHashSet(node)) { - if (_shouldSkipLinkedHashLint(node, _isTypeHashSet)) { - return; - } - + if (node.isHashSet) { + var approximateContextType = _approximateContextType(node); + if (approximateContextType is InvalidType) return; + if (approximateContextType.isTypeHashSet) return; + } + if (node.isSet || node.isHashSet) { var args = node.argumentList.arguments; if (constructorName == null) { - // Skip: LinkedHashSet(equals: (a, b) => false, hashCode: (o) => 13) + // Allow `LinkedHashSet(equals: (a, b) => false, hashCode: (o) => 13)`. if (args.isEmpty) { rule.reportLint(node); } @@ -122,7 +135,7 @@ @override void visitMethodInvocation(MethodInvocation node) { - // ['foo', 'bar', 'baz'].toSet(); + // Something like `['foo', 'bar', 'baz'].toSet()`. if (node.methodName.name != 'toSet') { return; } @@ -131,64 +144,130 @@ } } - bool _isHashMap(Expression expression) => - _isTypeHashMap(expression.staticType); - - bool _isHashSet(Expression expression) => - _isTypeHashSet(expression.staticType); - - bool _isMap(Expression expression) => - expression.staticType?.isDartCoreMap ?? false; - - bool _isSet(Expression expression) => - expression.staticType?.isDartCoreSet ?? false; - - bool _isTypeHashMap(DartType? type) => - type.isSameAs('LinkedHashMap', 'dart.collection'); - - bool _isTypeHashSet(DartType? type) => - type.isSameAs('LinkedHashSet', 'dart.collection'); - - bool _shouldSkipLinkedHashLint( - InstanceCreationExpression node, bool Function(DartType node) typeCheck) { - if (_isHashMap(node) || _isHashSet(node)) { - // Skip: LinkedHashSet<int> s = ...; or LinkedHashMap<int> s = ...; - var parent = node.parent; - if (parent is VariableDeclaration) { - var parent2 = parent.parent; - if (parent2 is VariableDeclarationList) { - var assignmentType = parent2.type?.type; - if (assignmentType != null && typeCheck(assignmentType)) { - return true; - } - } - } - // Skip: function(LinkedHashSet()); when function(LinkedHashSet mySet) or - // function(LinkedHashMap()); when function(LinkedHashMap myMap) - if (parent is ArgumentList) { - var paramType = node.staticParameterElement?.type; - if (paramType == null || typeCheck(paramType)) { - return true; - } - } - - // Skip: void f({required LinkedHashSet<Foo> s}) - if (parent is NamedExpression) { - var paramType = parent.staticParameterElement?.type; - if (paramType != null && typeCheck(paramType)) { - return true; - } - } - - // Skip: <int, LinkedHashSet>{}.putIfAbsent(3, () => LinkedHashSet()); - // or <int, LinkedHashMap>{}.putIfAbsent(3, () => LinkedHashMap()); - if (parent is ExpressionFunctionBody) { - var expressionType = parent.expression.staticType; - if (expressionType != null && typeCheck(expressionType)) { - return true; - } + /// A very, very rough approximation of the context type of [node]. + /// + /// This approximation will never be accurate for some expressions. + DartType? _approximateContextType(Expression node) { + var ancestor = node.parent; + var ancestorChild = node; + while (ancestor != null) { + if (ancestor is ParenthesizedExpression) { + ancestorChild = ancestor; + ancestor = ancestor.parent; + } else if (ancestor is CascadeExpression && + ancestorChild == ancestor.target) { + ancestorChild = ancestor; + ancestor = ancestor.parent; + } else { + break; } } - return false; + + switch (ancestor) { + // TODO(srawlins): Handle [AwaitExpression], [BinaryExpression], + // [CascadeExpression], [ConditionalExpression], [SwitchExpressionCase], + // likely others. Or move everything here to an analysis phase which + // has the actual context type. + case ArgumentList(): + // Allow `function(LinkedHashSet())` for `function(LinkedHashSet mySet)` + // and `function(LinkedHashMap())` for `function(LinkedHashMap myMap)`. + return node.staticParameterElement?.type ?? InvalidTypeImpl.instance; + case AssignmentExpression(): + // Allow `x = LinkedHashMap()`. + return ancestor.staticType; + case ExpressionFunctionBody(parent: var function) + when function is FunctionExpression: + // Allow `<int, LinkedHashSet>{}.putIfAbsent(3, () => LinkedHashSet())` + // and `<int, LinkedHashMap>{}.putIfAbsent(3, () => LinkedHashMap())`. + var functionType = _approximateContextType(function); + return functionType is FunctionType ? functionType.returnType : null; + case NamedExpression(): + // Allow `void f({required LinkedHashSet<Foo> s})`. + return ancestor.staticParameterElement?.type ?? + InvalidTypeImpl.instance; + case ReturnStatement(): + return _expectedReturnType( + ancestor.thisOrAncestorOfType<FunctionBody>(), + ); + case VariableDeclaration(parent: VariableDeclarationList(:var type)): + // Allow `LinkedHashSet<int> s = node` and + // `LinkedHashMap<int> s = node`. + return type?.type; + case YieldStatement(): + return _expectedReturnType( + ancestor.thisOrAncestorOfType<FunctionBody>(), + ); + } + + return null; } + + /// Extracts the expected type for return statements or yield statements. + /// + /// For example, for an asynchronous [body] in a function with a declared + /// [returnType] of `Future<int>`, this returns `int`. (Note: it would be more + /// accurate to use `FutureOr<int>` and an assignability check, but `int` is + /// an approximation that works for now; this should probably be revisited.) + DartType? _expectedReturnableOrYieldableType( + DartType? returnType, + FunctionBody body, + ) { + if (returnType == null || returnType is InvalidType) return null; + if (body.isAsynchronous) { + if (!body.isGenerator && returnType.isDartAsyncFuture) { + var typeArgs = (returnType as InterfaceType).typeArguments; + return typeArgs.isEmpty ? null : typeArgs.first; + } + if (body.isGenerator && returnType.isDartAsyncStream) { + var typeArgs = (returnType as InterfaceType).typeArguments; + return typeArgs.isEmpty ? null : typeArgs.first; + } + } else { + if (body.isGenerator && returnType.isDartCoreIterable) { + var typeArgs = (returnType as InterfaceType).typeArguments; + return typeArgs.isEmpty ? null : typeArgs.first; + } + } + return returnType; + } + + /// Attempts to calculate the expected return type of the function represented + /// by [body], accounting for an approximation of the function's context type, + /// in the case of a function literal. + DartType? _expectedReturnType(FunctionBody? body) { + if (body == null) return null; + var parent = body.parent; + if (parent is FunctionExpression) { + var grandparent = parent.parent; + if (grandparent is FunctionDeclaration) { + var returnType = grandparent.declaredElement?.returnType; + return _expectedReturnableOrYieldableType(returnType, body); + } + var functionType = _approximateContextType(parent); + if (functionType is! FunctionType) return null; + var returnType = functionType.returnType; + return _expectedReturnableOrYieldableType(returnType, body); + } + if (parent is MethodDeclaration) { + var returnType = parent.declaredElement?.returnType; + return _expectedReturnableOrYieldableType(returnType, body); + } + return null; + } +} + +extension on Expression { + bool get isHashMap => staticType.isTypeHashMap; + + bool get isHashSet => staticType.isTypeHashSet; + + bool get isMap => staticType?.isDartCoreMap ?? false; + + bool get isSet => staticType?.isDartCoreSet ?? false; +} + +extension on DartType? { + bool get isTypeHashMap => isSameAs('LinkedHashMap', 'dart.collection'); + + bool get isTypeHashSet => isSameAs('LinkedHashSet', 'dart.collection'); }
diff --git a/pkg/linter/test/rules/prefer_collection_literals_test.dart b/pkg/linter/test/rules/prefer_collection_literals_test.dart index 80fb78d..40da7be 100644 --- a/pkg/linter/test/rules/prefer_collection_literals_test.dart +++ b/pkg/linter/test/rules/prefer_collection_literals_test.dart
@@ -17,7 +17,36 @@ @override String get lintRule => 'prefer_collection_literals'; - @failingTest + test_assignment() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +void f(LinkedHashSet<int> s) { + s = LinkedHashSet(); +} +'''); + } + + test_assignment_withCascade() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +void f(LinkedHashSet<int> s) { + s = LinkedHashSet()..addAll([1, 2, 3]); +} +'''); + } + + test_assignment_withParentheses() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +void f(LinkedHashSet<int> s) { + s = ((LinkedHashSet())); +} +'''); + } + test_closure_returns_linkedHashSet() async { await assertDiagnostics(r''' import 'dart:collection'; @@ -93,6 +122,80 @@ ]); } + test_returnStatement_async() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +Future<LinkedHashSet<int>> f() async { + return LinkedHashSet(); +} +'''); + } + + test_returnStatement_asyncStar() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +Stream<LinkedHashSet<int>> f() async* { + yield LinkedHashSet(); +} +'''); + } + + test_returnStatement_functionDeclaration() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +LinkedHashSet<int> f() { + return LinkedHashSet(); +} +'''); + } + + test_returnStatement_functionExpression() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +void f() { + g(() { + return LinkedHashSet(); + }); +} + +void g(LinkedHashSet<int> Function()) {} +'''); + } + + test_returnStatement_methodDeclaration() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +class C { + LinkedHashSet<int> f() { + return LinkedHashSet(); + } +} +'''); + } + + test_returnStatement_syncStar() async { + await assertNoDiagnostics(r''' +import 'dart:collection'; + +Iterable<LinkedHashSet<int>> f() sync* { + yield LinkedHashSet(); +} +'''); + } + + test_typedefConstruction() async { + await assertNoDiagnostics(r''' +typedef MyMap = Map<int, int>; + +var x = MyMap(); +'''); + } + test_undefinedFunction() async { await assertDiagnostics(r''' import 'dart:collection';
diff --git a/pkg/linter/test_data/rules/prefer_collection_literals.dart b/pkg/linter/test_data/rules/prefer_collection_literals.dart index ba92825..ec01589f 100644 --- a/pkg/linter/test_data/rules/prefer_collection_literals.dart +++ b/pkg/linter/test_data/rules/prefer_collection_literals.dart
@@ -58,14 +58,16 @@ printIndentedHashSet(0, LinkedHashSet<int>()); // OK Set<int> ss8 = LinkedHashSet.from([1, 2, 3]); // LINT - LinkedHashSet<int> ss9 = LinkedHashSet.from([1, 2, 3]); // OK + LinkedHashSet<int> ss9 = LinkedHashSet.from([1, 2, 3]); // OK Iterable iter = Iterable.empty(); // OK var sss = Set.from(iter); // OK - LinkedHashSet<String> sss1 = <int, LinkedHashSet<String>>{}.putIfAbsent(3, () => LinkedHashSet<String>()); // OK + LinkedHashSet<String> sss1 = <int, LinkedHashSet<String>>{} + .putIfAbsent(3, () => LinkedHashSet<String>()); // OK - var lhs = LinkedHashSet(equals: (a, b) => false, hashCode: (o) => 13)..addAll({}); // OK + var lhs = LinkedHashSet(equals: (a, b) => false, hashCode: (o) => 13) + ..addAll({}); // OK LinkedHashMap hashMap = LinkedHashMap(); // OK Object hashMap2 = LinkedHashMap(); // LINT @@ -76,7 +78,8 @@ printMap(LinkedHashMap<int, int>()); // LINT printHashMap(LinkedHashMap<int, int>()); // OK - LinkedHashMap<String, String> lhm = <int, LinkedHashMap<String,String>>{}.putIfAbsent(3, () => LinkedHashMap<String, String>()); // OK + LinkedHashMap<String, String> lhm = <int, LinkedHashMap<String, String>>{} + .putIfAbsent(3, () => LinkedHashMap<String, String>()); // OK } void printObject(Object o) => print('$o');
diff --git a/pkg/linter/tool/machine/rules.json b/pkg/linter/tool/machine/rules.json index 8581e7f..4933826 100644 --- a/pkg/linter/tool/machine/rules.json +++ b/pkg/linter/tool/machine/rules.json
@@ -1661,7 +1661,7 @@ "flutter" ], "fixStatus": "hasFix", - "details": "**DO** use collection literals when possible.\n\n**BAD:**\n```dart\nvar addresses = Map<String, String>();\nvar uniqueNames = Set<String>();\nvar ids = LinkedHashSet<int>();\nvar coordinates = LinkedHashMap<int, int>();\n```\n\n**GOOD:**\n```dart\nvar addresses = <String, String>{};\nvar uniqueNames = <String>{};\nvar ids = <int>{};\nvar coordinates = <int, int>{};\n```\n\n**EXCEPTIONS:**\n\nThere are cases with `LinkedHashSet` or `LinkedHashMap` where a literal constructor\nwill trigger a type error so those will be excluded from the lint.\n\n```dart\nvoid main() {\n LinkedHashSet<int> linkedHashSet = LinkedHashSet.from([1, 2, 3]); // OK\n LinkedHashMap linkedHashMap = LinkedHashMap(); // OK\n \n printSet(LinkedHashSet<int>()); // LINT\n printHashSet(LinkedHashSet<int>()); // OK\n\n printMap(LinkedHashMap<int, int>()); // LINT\n printHashMap(LinkedHashMap<int, int>()); // OK\n}\n\nvoid printSet(Set<int> ids) => print('$ids!');\nvoid printHashSet(LinkedHashSet<int> ids) => printSet(ids);\nvoid printMap(Map map) => print('$map!');\nvoid printHashMap(LinkedHashMap map) => printMap(map);\n```\n", + "details": "**DO** use collection literals when possible.\n\n**BAD:**\n```dart\nvar addresses = Map<String, String>();\nvar uniqueNames = Set<String>();\nvar ids = LinkedHashSet<int>();\nvar coordinates = LinkedHashMap<int, int>();\n```\n\n**GOOD:**\n```dart\nvar addresses = <String, String>{};\nvar uniqueNames = <String>{};\nvar ids = <int>{};\nvar coordinates = <int, int>{};\n```\n\n**EXCEPTIONS:**\n\nWhen a `LinkedHashSet` or `LinkedHashMap` is expected, a collection literal is\nnot preferred (or allowed).\n\n```dart\nvoid main() {\n LinkedHashSet<int> linkedHashSet = LinkedHashSet.from([1, 2, 3]); // OK\n LinkedHashMap linkedHashMap = LinkedHashMap(); // OK\n \n printSet(LinkedHashSet<int>()); // LINT\n printHashSet(LinkedHashSet<int>()); // OK\n\n printMap(LinkedHashMap<int, int>()); // LINT\n printHashMap(LinkedHashMap<int, int>()); // OK\n}\n\nvoid printSet(Set<int> ids) => print('$ids!');\nvoid printHashSet(LinkedHashSet<int> ids) => printSet(ids);\nvoid printMap(Map map) => print('$map!');\nvoid printHashMap(LinkedHashMap map) => printMap(map);\n```\n", "sinceDartSdk": "2.0.0" }, {