Fix false positives in prefer_collection_literals #2340
Conversation
Towards #1649 Allow using `LinkedHashSet` and `LinkedHashMap` for named arguments, assignment to a variable, and when used in a binary expression where a static type is pushed down to the arguments. Refactor `_shouldSkipLinkedHashLint`. Extract a method to determine the type that is enforced statically for the expression rather than duplicate the subsequent check for each type of parent node.
| return parent.arguments.first.staticParameterElement?.type; | ||
| } | ||
| if (parent is NamedExpression) { | ||
| return parent.staticType; |
There was a problem hiding this comment.
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.
| return parent.staticType; | ||
| } | ||
| if (parent is ExpressionFunctionBody) { | ||
| return parent.expression.staticType; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| return parent.leftHandSide.staticType; | ||
| } | ||
| if (parent is BinaryExpression) { | ||
| return parent.staticType; |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| /// Returns the static type which is pushed into an expression by it's parent. | ||
| DartType /*?*/ _enforcedType(AstNode parent) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
pq
added
the
linter-set-internal
|
@natebosch are you still interested in landing this? It looks like there are some unresolved comments. No worries if we should just close it for now and pick it up later. |
|
I can't find anything worth landing here other than bringing more attention to the false negative we discovered. https://github.com/dart-lang/linter/pull/3705/files It's been a while since I looked at this, but IIRC I found some more cases that this code doesn't cover and this version likely isn't enough of an improvement to land on it's own. It's probably more worthwhile to focus on exposing the context type calculation that analyzer does to the linter as discussed in #2340 (comment) |
Towards dart-lang/sdk#57993
Allow using
LinkedHashSetandLinkedHashMapfor named arguments,assignment to a variable, and when used in a binary expression where a
static type is pushed down to the arguments.
Refactor
_shouldSkipLinkedHashLint. Extract a method to determine thetype that is enforced statically for the expression rather than
duplicate the subsequent check for each type of parent node.