New lint rule: collection_methods_unrelated_type #3692
Conversation
|
We need to wait on an analyzer release with https://dart-review.googlesource.com/c/sdk/+/259506 in order to test. |
There was a problem hiding this comment.
Before I review this further, I'd like to understand what criteria you used to decide that a single monolithic lint would be better that individual lints. I think there are more good reasons for having smaller lints, but would like to understand your reasoning.
| ExpectedArgumentKind.assignableToCollectionTypeArgument, | ||
| ), | ||
| // TODO(srawlins): Queue? It's not in mock SDK or [TypeProvider]. | ||
| // Argument to `Queue<E>.remove` should be assignable to `E`. |
There was a problem hiding this comment.
We only have accessors in TypeProvide for the types from the SDK that are referenced in the spec (because those are the ones that analyzer needs to use to validate the code. You'll need to add it to the mock sdk and then use something like typeProvider.objectElement.library.getClass('Queue') to get the element.
|
@bwilkerson A monolithic lint rule just because in the discussion at #1307, folks voiced that they couldn't imagine wanting to enable a subset of the rules. With the advent of You noted in that discussion that it would be simpler for a user to bulk-ignore, for example, all I have no strong opinion. |
But harder for a user to bulk-ignore a subset of the cases. I'm not sure it matters, especially if we can deprecate the existing individual rules. But I was curious. Thanks. |
This is needed in order to test dart-lang/linter#3692 Bug: dart-lang/linter#3692 Change-Id: I8be22ca319647fba0e6ff9a462571bd972e383c5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/260501 Commit-Queue: Samuel Rawlins <srawlins@google.com> Reviewed-by: Phil Quitslund <pquitslund@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
|
This PR is now ready for review. I resolved all of the comments left on it while it was in draft. |
| var argumentType = argument.staticType; | ||
| if (argumentType == null) return; | ||
|
|
||
| switch (methodDefinition.expectedArgumentKind) { |
There was a problem hiding this comment.
Consider adding a method to MethodDefinition that takes the argumentType and collectionType and returns true if the lint should be reported. I think that would simplify the code, and might even eliminate the need for the ExpectedArgumentKind.
There was a problem hiding this comment.
It's possible... I think it would actually add complexity. There are a few particulars that we'd have to push into MethodDefinition like:
- it would also need the TypeProvider, only for the Set.containsAll, Set.removeAll, and Set.retainAll cases,
- it would still need to be this big switch based on ExpectedArgumentKind; I don't think we could remove it.
Description
Fixes #1307
This reports when an "unrelated" argument is given for the following methods:
Iterable.contains,List.remove,Map.containsKey,Map.containsValue,Map.remove,Map.[],Queue.remove,Set.containsAll,Set.difference,Set.intersection,Set.lookup,Set.remove,Set.removeAll,Set.retainAll.