Warn on call to Map.remove with unrelated type

[a14n]

final map = <String,String>{};
map.remove(1); // LINT
map.remove(""); // OK

[srawlins]

Seems like this can just be folded into list_remove_unrelated_type? Can't imagine why someone would want to use one and not the other.

[a14n]

But the name of the lint is completely unrelated to map.

[bwilkerson]

More importantly, extending a rule to produce lints where it never did before is, in some sense, a breaking change for anyone using the lint.

[srawlins]

I don't think that the name should prevent us from re-using it. We can create an alias for list_remove_unrelated_type called remove_unrelated_type (or better yet, rename the rule and create an alias for the old name).

Is there a reason to not make a breaking change? Just bump the appropriate version number.

[a14n]

Is there a reason to not make a breaking change? Just bump the appropriate version number.

Unfortunately linter package is not like any other package. It's bundled in the sdk with the analyzer. So a simple new release of the sdk may come with a new version of linter. Actually the version number of linter is only used to link it into the sdk.

Note that it would be a normal package if it'd become an anlyzer plugin.

[bwilkerson]

Note that it would be a normal package ...

Yes, but that comes with other costs, and it isn't clear to me that that's the best path forward for our users.

[srawlins]

Since @pq has been working on deprecation recently, what say we deprecate list_remove_unrelated_type and rename to collection_remove_unrelated_type?

[bwilkerson]

Sounds reasonable. And if we do that, then I'm going to want us to have a way to implement a quick-fix to change the name for users, which our current deprecation support doesn't include.

[munificent]

Other operations we might want to either roll into the existing lints or define new lints for:

Map.containsKey(Object? key)
Map.containsValue(Object? value)
Map.operator[Object? value]
Set.contains(Object? value)
Set.lookup(Object? object)
Set.remove(Object? value)

[lrhn]

The complete list (as I see it) would be:

Iterable.contains(Object?)  // inherited by List/Set/Queue too
List.remove(Object?)
Set.remove(Object?)
Set.lookup(Object?)
Set.containsAll(Iterable<Object?>)
Map.remove(Object?)
Map.containsKey(Object?)
Map.containsValue(Object?)
Map.operator [](Object?)
Queue.remove(Object?)

I've omitted the following:

Set.removeAll(Iterable<Object?>)
Set.retainAll(Iterable<Object?>)
Set.intersection(Set<Object?>)
Set.difference(Set<Object?>)

The intersection and difference makes sense for arbitrary set types, mathematically and type-wise, it's really a symmetric operation, and it's only lack of intersection types that make us choose the type of the first operand.
The removeAll and retainAllare destructive versions of the same operations, so I'd keep allowing any element type there too.
These differ from containsAll where having any member that is not a potential element of the set will guarantee a false result. There might still be situations where that can be useful, but I think it's, on average, better to check the iterable type to avoid mistakes.

(I have also changed Characters.contains from package:characters to have type bool contains(covariant String ...) just to get better warnings.)

[srawlins]

Complete with dart-lang/linter#3692