Revert "linter: Refactor prefer_collection_literals to use context type more"
This reverts commit cd8a3370e7d854ded526329178eb8c046917ddf7.
Reason for revert: lint starts barking at the wrong tree: b/298917960
Original change's description:
> 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>
Change-Id: I980053dd51ffd4447721e0ad7436b07ca704b554
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324021
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Alexander Thomas <athom@google.com>
Commit-Queue: Ilya Yanok <yanok@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 f29794a..3b1ea9f 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,4 +87,26 @@
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 e9ce1b1..8533ea3 100644
--- a/pkg/linter/lib/src/rules/prefer_collection_literals.dart
+++ b/pkg/linter/lib/src/rules/prefer_collection_literals.dart
@@ -4,11 +4,7 @@
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';
@@ -36,8 +32,8 @@
**EXCEPTIONS:**
-When a `LinkedHashSet` or `LinkedHashMap` is expected, a collection literal is
-not preferred (or allowed).
+There are cases with `LinkedHashSet` or `LinkedHashMap` where a literal constructor
+will trigger a type error so those will be excluded from the lint.
```dart
void main() {
@@ -76,7 +72,7 @@
@override
void registerNodeProcessors(
NodeLintRegistry registry, LinterContext context) {
- var visitor = _Visitor(this, context.typeProvider);
+ var visitor = _Visitor(this);
registry.addInstanceCreationExpression(this, visitor);
registry.addMethodInvocation(this, visitor);
}
@@ -84,25 +80,17 @@
class _Visitor extends SimpleAstVisitor<void> {
final LintRule rule;
- final TypeProvider typeProvider;
- _Visitor(this.rule, this.typeProvider);
+ _Visitor(this.rule);
@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 (node.isHashMap) {
- var approximateContextType = _approximateContextType(node);
- if (approximateContextType is InvalidType) return;
- if (approximateContextType.isTypeHashMap) return;
- }
- if (node.isMap || node.isHashMap) {
+ if (_isMap(node) || _isHashMap(node)) {
+ if (_shouldSkipLinkedHashLint(node, _isTypeHashMap)) {
+ return;
+ }
if (constructorName == null && node.argumentList.arguments.isEmpty) {
rule.reportLint(node);
}
@@ -110,15 +98,14 @@
}
// Sets.
- if (node.isHashSet) {
- var approximateContextType = _approximateContextType(node);
- if (approximateContextType is InvalidType) return;
- if (approximateContextType.isTypeHashSet) return;
- }
- if (node.isSet || node.isHashSet) {
+ if (_isSet(node) || _isHashSet(node)) {
+ if (_shouldSkipLinkedHashLint(node, _isTypeHashSet)) {
+ return;
+ }
+
var args = node.argumentList.arguments;
if (constructorName == null) {
- // Allow `LinkedHashSet(equals: (a, b) => false, hashCode: (o) => 13)`.
+ // Skip: LinkedHashSet(equals: (a, b) => false, hashCode: (o) => 13)
if (args.isEmpty) {
rule.reportLint(node);
}
@@ -135,7 +122,7 @@
@override
void visitMethodInvocation(MethodInvocation node) {
- // Something like `['foo', 'bar', 'baz'].toSet()`.
+ // ['foo', 'bar', 'baz'].toSet();
if (node.methodName.name != 'toSet') {
return;
}
@@ -144,130 +131,64 @@
}
}
- /// 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;
+ 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;
+ }
}
}
-
- 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;
+ return false;
}
-
- /// 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 40da7be..80fb78d 100644
--- a/pkg/linter/test/rules/prefer_collection_literals_test.dart
+++ b/pkg/linter/test/rules/prefer_collection_literals_test.dart
@@ -17,36 +17,7 @@
@override
String get lintRule => 'prefer_collection_literals';
- 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()));
-}
-''');
- }
-
+ @failingTest
test_closure_returns_linkedHashSet() async {
await assertDiagnostics(r'''
import 'dart:collection';
@@ -122,80 +93,6 @@
]);
}
- 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 ec01589f..ba92825 100644
--- a/pkg/linter/test_data/rules/prefer_collection_literals.dart
+++ b/pkg/linter/test_data/rules/prefer_collection_literals.dart
@@ -58,16 +58,14 @@
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
@@ -78,8 +76,7 @@
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 4933826..8581e7f 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\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",
+ "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",
"sinceDartSdk": "2.0.0"
},
{
