Skip to content

Commit

Permalink
linter: Fix prefer_const_constructors for some generic cases
Browse files Browse the repository at this point in the history 
Fixes https://github.com/dart-lang/linter/issues/4531

This involves moving `approximateContextType`, but it is not changed.

Change-Id: I477377a0ae233a7043f1d2e9a9d511cb435514bc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362602
Commit-Queue: Samuel Rawlins <srawlins@google.com>
Reviewed-by: Phil Quitslund <pquitslund@google.com>
  • Loading branch information
@srawlins
srawlins authored and Commit Queue committed Apr 16, 2024
1 parent e07cd16 commit f15bcc9
Show file tree
Hide file tree
Showing 4 changed files with 186 additions and 140 deletions.
124 changes: 124 additions & 0 deletions pkg/linter/lib/src/extensions.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import 'package:analyzer/dart/element/type.dart';
import 'package:analyzer/file_system/physical_file_system.dart';
import 'package:analyzer/src/dart/ast/ast.dart'; // ignore: implementation_imports
import 'package:analyzer/src/dart/element/member.dart'; // ignore: implementation_imports
// ignore: implementation_imports
import 'package:analyzer/src/dart/element/type.dart' show InvalidTypeImpl;
import 'package:collection/collection.dart';

import 'analyzer.dart';
Expand Down Expand Up @@ -319,6 +321,76 @@ extension ElementExtension on Element {
}

extension ExpressionExtension on Expression? {
/// A very, very, very rough approximation of the context type of this node.
///
/// This approximation will never be accurate for some expressions.
DartType? get approximateContextType {
var self = this;
if (self == null) return null;
var ancestor = self.parent;
var ancestorChild = self;
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;
}
}

switch (ancestor) {
// TODO(srawlins): Handle [AwaitExpression], [BinaryExpression],
// [CascadeExpression], [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 self.staticParameterElement?.type ?? InvalidTypeImpl.instance;
case AssignmentExpression():
// Allow `x = LinkedHashMap()`.
return ancestor.staticType;
case ConditionalExpression():
return ancestor.staticType;
case ConstructorFieldInitializer():
var fieldElement = ancestor.fieldName.staticElement;
return (fieldElement is VariableElement) ? fieldElement.type : null;
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 = function.approximateContextType;
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 ancestor.thisOrAncestorOfType<FunctionBody>().expectedReturnType;
case VariableDeclaration(parent: VariableDeclarationList(:var type)):
// Allow `LinkedHashSet<int> s = node` and
// `LinkedHashMap<int> s = node`.
return type?.type;
case YieldStatement():
return ancestor.thisOrAncestorOfType<FunctionBody>().expectedReturnType;
}

return null;
}

bool get isNullLiteral => this?.unParenthesized is NullLiteral;
}

Expand All @@ -327,6 +399,58 @@ extension FieldDeclarationExtension on FieldDeclaration {
!isStatic && parent is ExtensionTypeDeclaration;
}

extension FunctionBodyExtension on FunctionBody? {
/// Attempts to calculate the expected return type of the function represented
/// by this node, accounting for an approximation of the function's context
/// type, in the case of a function literal.
DartType? get expectedReturnType {
var self = this;
if (self == null) return null;
var parent = self.parent;
if (parent is FunctionExpression) {
var grandparent = parent.parent;
if (grandparent is FunctionDeclaration) {
var returnType = grandparent.declaredElement?.returnType;
return self._expectedReturnableOrYieldableType(returnType);
}
var functionType = parent.approximateContextType;
if (functionType is! FunctionType) return null;
var returnType = functionType.returnType;
return self._expectedReturnableOrYieldableType(returnType);
}
if (parent is MethodDeclaration) {
var returnType = parent.declaredElement?.returnType;
return self._expectedReturnableOrYieldableType(returnType);
}
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) {
var self = this;
if (self == null) return null;
if (returnType is! InterfaceType) return null;
if (self.isAsynchronous) {
if (!self.isGenerator && returnType.isDartAsyncFuture) {
return returnType.typeArguments.firstOrNull;
}
if (self.isGenerator && returnType.isDartAsyncStream) {
return returnType.typeArguments.firstOrNull;
}
} else {
if (self.isGenerator && returnType.isDartCoreIterable) {
return returnType.typeArguments.firstOrNull;
}
}
return returnType;
}
}

extension InhertanceManager3Extension on InheritanceManager3 {
/// Returns the class member that is overridden by [member], if there is one,
/// as defined by [getInherited].
Expand Down
131 changes: 2 additions & 129 deletions pkg/linter/lib/src/rules/prefer_collection_literals.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@ 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 @@ -98,7 +96,7 @@ class _Visitor extends SimpleAstVisitor<void> {

// Maps.
if (node.isHashMap) {
var approximateContextType = _approximateContextType(node);
var approximateContextType = node.approximateContextType;
if (approximateContextType is InvalidType) return;
if (approximateContextType.isTypeHashMap) return;
}
Expand All @@ -111,7 +109,7 @@ class _Visitor extends SimpleAstVisitor<void> {

// Sets.
if (node.isHashSet) {
var approximateContextType = _approximateContextType(node);
var approximateContextType = node.approximateContextType;
if (approximateContextType is InvalidType) return;
if (approximateContextType.isTypeHashSet) return;
}
Expand Down Expand Up @@ -143,131 +141,6 @@ class _Visitor extends SimpleAstVisitor<void> {
rule.reportLint(node);
}
}

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

switch (ancestor) {
// TODO(srawlins): Handle [AwaitExpression], [BinaryExpression],
// [CascadeExpression], [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 ConditionalExpression():
return ancestor.staticType;
case ConstructorFieldInitializer():
var fieldElement = ancestor.fieldName.staticElement;
return (fieldElement is VariableElement) ? fieldElement.type : null;
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>(),
);
}

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 {
Expand Down
41 changes: 30 additions & 11 deletions pkg/linter/lib/src/rules/prefer_const_constructors.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
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 '../analyzer.dart';
import '../extensions.dart';

const _desc = r'Prefer const with constant constructors.';

Expand Down Expand Up @@ -84,25 +86,42 @@ class _Visitor extends SimpleAstVisitor<void> {

@override
void visitInstanceCreationExpression(InstanceCreationExpression node) {
if (node.isConst) return;
if (node.constructorName.type.isDeferred) return;

var element = node.constructorName.staticElement;
if (element == null) return;
if (!element.isConst) return;

if (element.isConst && !node.isConst) {
// Handled by analyzer hint.
if (element.hasLiteral) return;
// Handled by an analyzer warning.
if (element.hasLiteral) return;

var enclosingElement = element.enclosingElement;
if (enclosingElement is ClassElement &&
enclosingElement.isDartCoreObject) {
// Skip lint for `new Object()`, because it can be used for Id creation.
return;
}
var enclosingElement = element.enclosingElement;
if (enclosingElement is ClassElement && enclosingElement.isDartCoreObject) {
// Skip lint for `new Object()`, because it can be used for ID creation.
return;
}

if (context.canBeConst(node)) {
rule.reportLint(node);
if (enclosingElement.typeParameters.isNotEmpty &&
node.constructorName.type.typeArguments == null) {
var approximateContextType = node.approximateContextType;
var contextTypeAsInstanceOfEnclosing =
approximateContextType?.asInstanceOf(enclosingElement);
if (contextTypeAsInstanceOfEnclosing != null) {
if (contextTypeAsInstanceOfEnclosing.typeArguments
.any((e) => e is TypeParameterType)) {
// The context type has type parameters, which may be substituted via
// upward inference from the static type of `node`. Changing `node`
// from non-const to const will affect inference to change its own
// type arguments to `Never`, which affects that upward inference.
// See https://github.com/dart-lang/linter/issues/4531.
return;
}
}
}

if (context.canBeConst(node)) {
rule.reportLint(node);
}
}
}
Loading

0 comments on commit f15bcc9

Please sign in to comment.