New lint: avoid_collection_methods_with_unrelated_types

[kevmoo]

This is a more general request related to #57434 – which only deals with remove (list_remove_unrelated_type) and the other existing iterable_contains_unrelated_type

There are more methods on Set and Map that could be generalized here.

• Iterable.contains
• List.remove
• Set.containsAll, difference, intersection, lookup, remove, removeAll, retainAll
• Map.containsKey, containsValue, remove, [] and []=

We'd need to be thoughtful about the Set methods – but at first glance I think linting all of them is reasonable.

CC @srawlins @bwilkerson @a14n

[srawlins]

+💯

[kevmoo]

Here's another friendly ping for this – it'd be super useful!

[kevmoo]

Taking a stab at this...

[kevmoo]

@pq – sorry to get you excited. A bit out of my wheelhouse. Although it did inspire some cleanup.

[srawlins]

I was looking into combining this into one giant list that knew about the Set methods and List methods etc.

And it knew that contains should have an E argument, but containsAll should have an Iterable<E> argument and the code became pretty incomprehensible. I suspect that a lint rule per class might be the solution. So:

• iterable_unrelated_types rule would catch contains (if only this... then just leave the existing lint rule)
• list_unrelated_types rule would catch contains and remove (and then deprecate list_remove_unrelated_types)
• set_unrelated_types rule would catch lookup, remove, difference, intersection, and then also containsAll, removeAll, and retainAll, but that would be new logic.
• map_unrelated_types rule would catch containsKey, containsValue, and remove, but that would also be new logic, to match on the second type argument (for containsValue), whereas everything else has been using the first type argument, exclusively.

[pq]

I suspect that a lint rule per class might be the solution.

Do you imagine that folks would want to enable only a sub-set of the proposed lints? (I'm trying to think why one mega-rule isn't the way to go.)

[kevmoo]

I can't imagine a case where I'd want a subset of these.

…

On Thu, Feb 28, 2019 at 1:11 PM Phil Quitslund ***@***.***> wrote: I suspect that a lint rule *per class* might be the solution. Do you imagine that folks would want to enable only a sub-set of the proposed lints? (I'm trying to think why one mega-rule isn't the way to go.) — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#57844>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABCiliRuaAH17qLD1G4y283MFi_9hPwks5vSEYUgaJpZM4ZOLgP> .

[bwilkerson]

On the surface I'd agree. All of these are cases where we're catching an error at analysis time that otherwise wouldn't be caught until runtime.

I'm not proposing that we do so, but if we took this to the extreme we would have a single lint rule that would subsume this and a lot of other lint rules. After all, why would users not want us to catch such things. We could also reasonably ask why these aren't hints rather than lints so that users don't even need to opt in.

The answer, in my mind, is that these are lints because they can have false positives. Given the following

class A {}
class B {}
bool contains(List<A> list, B b) => list.contains(b);

it seems obvious that A and B are unrelated, and hence this can only be an error. But it isn't if we also define

class C implements A, B {}

and are careful to only call contains with an instance of C.

False positives are why they're lints rather than hints. It's also the reason we might want to split them up into multiple lints. By having a finer level of granularity we allow users to opt out of some checks without opting out of all of them. For example, consider

class C {}
class D {}
bool contains(List<C> list, Map<C, String> map, D d)
    => list.contains(d) || map.containsValue(d);

If we didn't want the lint on contains we could add an ignore comment, but it would also ignore the lint on containsValue. The latter lint isn't a false positive, but we've forced our user to disable it. (Yes, users can play games with formatting to get around this, but if they'd ignored the contains before writing the other code they might not realize the need to do so.) Having them as separate lints makes it easier to control how much is being ignored.

To be clear, I'm not advocating for either answer, just bringing up something that probably ought to be considered when deciding.

[jamesderlin]

I'm mostly commenting to try to help with searching, but I recently found a bug where I accidentally typed someMap[someObject] instead of someMap[someObject.id]. (operator []= does use the Map's key type but operator [] uses Object.)

[eernstg]

Note that the same trade-off has been made in Java: Java.util.HashSet also has an add(E e) and a contains(Object o), with the same rationale: Returning false is not wrong if you ask whether a given list of String contains a given int, but it definitely won't work to add it.

@MichaelRFairhurst just suggested on an internal mailing list that we could use a range (an upper bound type which is used like today's type annotations, and a lower bound which is used to statically reject any actual arguments which are not assignable, in this case: to the collection element type).

That could be a language feature, but if it is restricted to the specific members mentioned here and uses the element type as the lower bound then I believe that it's the same thing as the lint which is proposed here.

[kevmoo]

I'd love to dig back in here – I keep hitting this!

[pq]

It would be interesting to implement this and run it on some representative repos and see if anything interesting shakes out...

@kevmoo: out of curiosity, where are you hitting this most?

[kevmoo]

I hit it when working with *redacted* customer demos. This is a common source of bugs that usually get fixed – but they take a while to find!

…

On Fri, Apr 19, 2019 at 3:24 PM Phil Quitslund ***@***.***> wrote: It would be interesting to implement this and run it on some representative repos and see if anything interesting shakes out... @kevmoo <https://github.com/kevmoo>: out of curiosity, where are you hitting this most? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#57844>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAEFCUTLRAN2WKU5GOHKIDPRJBB3ANCNFSM4GJYXAHQ> .

[fkettelhoit]

Is this still on the agenda? Is there perhaps some improvement coming in the wake of NNBD? I keep hitting this very often since I am frequently switching between IDs and whole records as keys during refactorings, turning maps of the type Map<int, T> into Map<Record, T> and vice versa or going from Set<int> to Set<Record>. More strongly typed versions of Map and Set would of course be the best solution, but even a linter rule would help tremendously to make the whole refactoring feel a bit less untyped.

[fzyzcjy]

Hi another year has passed... I do hope this can be implemented - it is sooooooo useful!

related: https://stackoverflow.com/questions/68251477/dart-flutter-linter-rule-the-type-to-index-a-map-should-be-the-key-type-of-map/68252306#68252306

[bwilkerson]

I believe that this has languished more from a lack of resources than any lack of support. Community contributions are always welcome.

[fzyzcjy]

I want to contribute. But seems that I cannot implememt one and directly use it now :/ have to wait for a release (months!)

[pq]

@fzyzcjy: are you able to use dev SDK builds? If so, our release cycle is much more frequent.

[fzyzcjy]

@pq Unfortunately, no :( I am writing flutter for a app already on app store...

[fzyzcjy]

Related: dart-code-checker/dart-code-metrics#701

[fzyzcjy]

The feature is implemented in this PR: dart-code-checker/dart-code-metrics#705

[srawlins]

WIP at dart-lang/linter#3692 for an implementation in linter.