Skip to content

Commit

Permalink
Revert "Revert "linter: Refactor prefer_collection_literals to use co…
Browse files Browse the repository at this point in the history 
…ntext type more""

This reverts commit cbdae14.

In addition, Fix prefer_collection_literals for methods with expression bodies

Original 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: I958ba69a56866c18523ce6cbf62645ef8e028f6b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/324260
Commit-Queue: Samuel Rawlins <[email protected]>
Reviewed-by: Konstantin Shcheglov <[email protected]>
  • Loading branch information
@srawlins
srawlins authored and Commit Queue committed Sep 5, 2023
1 parent 53fb57e commit 4bddf8a
Show file tree
Hide file tree
Showing 5 changed files with 294 additions and 93 deletions.
22 changes: 0 additions & 22 deletions pkg/analysis_server/test/src/services/correction/fix/convert_to_map_literal_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,28 +85,6 @@ var m = Map<String, int>();
''');
await assertHasFix('''
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 = {};
''');
}
}
219 changes: 154 additions & 65 deletions pkg/linter/lib/src/rules/prefer_collection_literals.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -32,8 +36,8 @@ var coordinates = <int, int>{};
**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() {
Expand Down Expand Up @@ -72,40 +76,49 @@ class PreferCollectionLiterals extends LintRule {
@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);
}
}

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);
}
return;
}

// 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);
}
Expand All @@ -122,7 +135,7 @@ class _Visitor extends SimpleAstVisitor<void> {

@override
void visitMethodInvocation(MethodInvocation node) {
// ['foo', 'bar', 'baz'].toSet();
// Something like `['foo', 'bar', 'baz'].toSet()`.
if (node.methodName.name != 'toSet') {
return;
}
Expand All @@ -131,64 +144,140 @@ class _Visitor extends SimpleAstVisitor<void> {
}
}

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;
/// 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 _isTypeHashMap(DartType? type) =>
type.isSameAs('LinkedHashMap', 'dart.collection');
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 functionParent = function.parent;
if (functionParent is FunctionDeclaration) {
return functionParent.returnType?.type;
}
var functionType = _approximateContextType(function);
return functionType is FunctionType ? functionType.returnType : null;
case ExpressionFunctionBody(parent: var function)
when function is FunctionDeclaration:
return function.returnType?.type;
case ExpressionFunctionBody(parent: var function)
when function is MethodDeclaration:
return function.returnType?.type;
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>(),
);
}

bool _isTypeHashSet(DartType? type) =>
type.isSameAs('LinkedHashSet', 'dart.collection');
return null;
}

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;
}
}
/// 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;
}
// 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;
}
if (body.isGenerator && returnType.isDartAsyncStream) {
var typeArgs = (returnType as InterfaceType).typeArguments;
return typeArgs.isEmpty ? null : typeArgs.first;
}

// Skip: void f({required LinkedHashSet<Foo> s})
if (parent is NamedExpression) {
var paramType = parent.staticParameterElement?.type;
if (paramType != null && typeCheck(paramType)) {
return true;
}
} else {
if (body.isGenerator && returnType.isDartCoreIterable) {
var typeArgs = (returnType as InterfaceType).typeArguments;
return typeArgs.isEmpty ? null : typeArgs.first;
}
}
return returnType;
}

// 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;
}
/// 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 false;
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');
}
Loading

0 comments on commit 4bddf8a

Please sign in to comment.