Skip to content

Commit

Permalink
Revert "linter: Refactor prefer_collection_literals to use context ty…
Browse files Browse the repository at this point in the history 
…pe more"

This reverts commit cd8a337.

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 dart-lang/linter#4736
> Fixes dart-lang/linter#3057
> Fixes dart-lang/linter#1649
> Fixes dart-lang/linter#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>
  • Loading branch information
Ilya Yanok committed Sep 4, 2023
1 parent 44bcd89 commit cbdae14
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 256 deletions.
22 changes: 22 additions & 0 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,6 +85,28 @@ 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 = {};
''');
}
}
209 changes: 65 additions & 144 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,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';
Expand Down Expand Up @@ -36,8 +32,8 @@ var coordinates = <int, int>{};
**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() {
Expand Down Expand Up @@ -76,49 +72,40 @@ class PreferCollectionLiterals extends LintRule {
@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);
}
}

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

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

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

/// 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);

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>(),
);
}
bool _isHashSet(Expression expression) =>
_isTypeHashSet(expression.staticType);

return null;
}
bool _isMap(Expression expression) =>
expression.staticType?.isDartCoreMap ?? 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;
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;
}
}
}
if (body.isGenerator && returnType.isDartAsyncStream) {
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;
}
}
} else {
if (body.isGenerator && returnType.isDartCoreIterable) {
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;
}
}
}
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);
// 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;
}
}
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;
return false;
}
}

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 cbdae14

Please sign in to comment.