Skip to content
This repository has been archived by the owner on Nov 20, 2024. It is now read-only.

Fix false positives in prefer_collection_literals #2340

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
# 0.1.125-dev

* fixed false positives in `prefer_colleciton_literals`.
natebosch marked this conversation as resolved.
Show resolved Hide resolved

# 0.1.124

* fixed false positives in `prefer_constructors_over_static_methods`
Expand Down
60 changes: 30 additions & 30 deletions lib/src/rules/prefer_collection_literals.dart
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ will trigger a type error so those will be excluded from the lint.
void main() {
LinkedHashSet<int> linkedHashSet = LinkedHashSet.from([1, 2, 3]); // OK
LinkedHashMap linkedHashMap = LinkedHashMap(); // OK

printSet(LinkedHashSet<int>()); // LINT
printHashSet(LinkedHashSet<int>()); // OK

Expand Down Expand Up @@ -142,35 +142,35 @@ class _Visitor extends SimpleAstVisitor<void> {

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) {
final paramType = parent.arguments.first.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;
}
}
if (!_isHashMap(node) && !_isHashSet(node)) return false;
var parent = node.parent;
var staticType = _enforcedType(parent);
return staticType != null && !typeCheck(staticType);
}
}

/// Returns the static type which is pushed into an expression by it's parent.
DartType /*?*/ _enforcedType(AstNode parent) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be completely misunderstanding what this method is trying to accomplish, but it looks like it's trying to find the type that's required by the context in which the InstanceCreationExpression appears. If so, then I think what we really need here is the algorithm implemented by the _ContextTypeVisitor in the analysis_server package, which is both more complete and more correct.

If I'm not understanding the purpose of this method then my comments below are probably also wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's exactly the goal. Is there a reasonable way we could expose that functionality here?

Specifically, the problem this was attempting to solve is that in some places if we don't specifically use LinkedHashMap over {} then we'll get some other analysis error. We want to suppress the lint in only those places where honoring the lint would cause a static error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect that we should just add a TODO and tackle that later. It will require moving the _ContextTypeVisitor to the analyzer package and exposing an API through which it can be used. That's a bigger task than I'm asking you to tackle here.

if (parent is VariableDeclaration) {
var parent2 = parent.parent;
if (parent2 is VariableDeclarationList) {
return parent2.type?.type;
}
return false;
}
if (parent is ArgumentList) {
return parent.arguments.first.staticParameterElement?.type;
natebosch marked this conversation as resolved.
Show resolved Hide resolved
}
if (parent is NamedExpression) {
return parent.staticType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right either. I believe that the static type of a NamedExpression is the static type of the expression being named (which is the instance creation expression). I suspect that you need parent.staticParameterElement?.type in order to get the type of the parameter associated with the argument.

}
if (parent is ExpressionFunctionBody) {
return parent.expression.staticType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks wrong. parent.expression will always return the instance creation expression from which we started. I think what you want to do here is recurse to see whether you can find a type that would be inferred as the return type of the closure (assuming that this is the body of a closure).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another existing bug with false negatives.

Is this something you might be able to work on? I can easily add some test cases showing the problem but I'm having a hard time figuring out the right things to look for in the analyzer APIs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can send me the test case(s), I'll be happy to at least look at what it would take to implement them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/dart-lang/linter/pull/3705/files adds a line in the test data library that should have a lint but doesn't due to this bug.

}
if (parent is AssignmentExpression) {
return parent.leftHandSide.staticType;
}
if (parent is BinaryExpression) {
return parent.staticType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also looks wrong. If the instance creation is the right operand then you want the type of the parameter. If it's the left operand then there is no context type.

}
return null;
}
2 changes: 1 addition & 1 deletion lib/src/version.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
// BSD-style license that can be found in the LICENSE file.

/// Package version. Synchronized w/ pubspec.yaml.
const String version = '0.1.124';
const String version = '0.1.125-dev';
2 changes: 1 addition & 1 deletion pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: linter
version: 0.1.124
version: 0.1.125-dev

description: >-
The implementation of the lint rules supported by the analyzer framework.
Expand Down
23 changes: 17 additions & 6 deletions test/rules/prefer_collection_literals.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import 'dart:collection';
void main() {
var listToLint = new List(); //LINT
var mapToLint = new Map(); // LINT
var LinkedHashMapToLint = new LinkedHashMap(); // LINT
var linkedHashMapToLint = new LinkedHashMap(); // LINT

var m1 = Map.unmodifiable({}); //OK
var m2 = Map.fromIterable([]); //OK
Expand Down Expand Up @@ -50,31 +50,42 @@ void main() {

Set<int> ss5 = LinkedHashSet<int>(); // LINT
LinkedHashSet<int> ss6 = LinkedHashSet<int>(); // OK
ss6 = LinkedHashSet(); // OK
ss6 = null ?? LinkedHashSet(); // OK

printSet(Set()); // LINT
printSet(LinkedHashSet<int>()); // LINT
printHashSet(LinkedHashSet<int>()); // OK
printHashSetNamedArgument(ids: LinkedHashSet<int>()); // OK

Set<int> ss7 = LinkedHashSet.from([1, 2, 3]); // LINT
LinkedHashSet<int> ss8 = LinkedHashSet.from([1, 2, 3]); // OK
LinkedHashSet<int> ss8 = 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
hashMap = LinkedHashMap(); // OK
hashMap = null ?? LinkedHashMap(); // OK

printMap(Map()); // LINT
printMap(LinkedHashMap<int, int>()); // LINT
printHashMap(LinkedHashMap<int, int>()); // OK
printHashMapNamedArgument(map: 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 printSet(Set<int> ids) => print('$ids!');
void printHashSet(LinkedHashSet<int> ids) => printSet(ids);
void printHashSetNamedArgument({LinkedHashSet<int> ids}) => printSet(ids);
void printMap(Map map) => print('$map!');
void printHashMap(LinkedHashMap map) => printMap(map);
void printHashMap(LinkedHashMap map) => printMap(map);
void printHashMapNamedArgument({LinkedHashMap<int, int> map}) => printMap(map);