That makes hashCode inconsistent with equals. For lists and map arguments, you'd basically only have consistency with const expressions. As an example,

final c1 = new MethodCall('foo', {'bar':'baz'});
final c2 = new MethodCall('foo', {'bar':'baz'});
assert(c1 == c2);
assert(c1.hashCode() != c2.hashCode());

I think we should change hashValues to work with any type instead. Mostly because I'm sure I've made the same mistake in a number of other places.

This was already broken before, since hashValues intentionally doesn't do anything with lists and doesn't do anything deep with maps.

hashValues intentionally asserts with Iterables because you almost never want to compare the actual object of a list. It definitely shouldn't automatically be deep because that's very expensive and expensive things should look expensive.

The real solution here is to not use "dynamic". Every time we use "dynamic" we end up with bugs because it basically turns off the analyzer. Do we have to make the arguments dynamic?

Also do we need MethodCall to be comparable in this deep way?

Thanks for your time, Ian.

This was already broken before [...]

Yes, you're right of course. My original code expected hashValues to go deep.

hashValues intentionally asserts with Iterables because you almost never want to
compare the actual object of a list. It definitely shouldn't automatically be deep
because that's very expensive and expensive things should look expensive.

Expecting equality and hash code to be deep is a remnant from my Java days where the Collections framework does it that way. I repeatedly find myself missing that behavior of Dart lists and maps.

The real solution here is to not use "dynamic". Every time we use "dynamic" we
end up with bugs because it basically turns off the analyzer. Do we have to make
the arguments dynamic?

The argument of a MethodCall could have been made List<dynamic> or Map<String, dynamic>, bringing overhead in code and at runtime to situations where we just want to pass a bool/int/String value. And I guess it would only move the problem with dynamic to the list entries or map values? In the end, the argument is dynamic for the same reason hashValues is parameterized with Object: We don't have a type that captures precisely the valid method call arguments (trees of nested lists and maps with bool/int/String leaves), nor a type that captures the non-Iterables accepted by hashValues.

Also do we need MethodCall to be comparable in this deep way?

I originally gave MethodCall a custom equals (and therefore hashCode) to facilitate writing unit tests. I don't have a production code use case, but one could conceivably have a Map<MethodCall, dynamic> cache in a situation where calculating the result is expensive. Whether we want to support that use case is an open question. If not, we could (remove the custom equality and hash and fix up the unit tests, or) accept a hash code with more collisions:

int get hashCode => method.hashCode;

If we do want to support MethodCalls as map keys, we could instead do

int get hashCode => hashValues(method, _deepHash(argument));

I'd vote for adding deep equality and hash code functions to dart:ui, possibly naming them treeEquals and treeHashCode or similar to reflect that they do recursive descent through lists and maps, and that they would fail on cyclic values. Then they look expensive, and we can use them for things like MethodCall arguments where the tree can reasonably be expected to be small.

I think I'd rather just remove these ==/hashCode methods and change the unit tests to manually verify the arguments are what they expect. It should have pretty minimal impact on the tests (and if it's a lot we can always write a custom matcher), and would avoid anyone relying on comparing method calls.

Your call :-) @cbracken Do you want to change this PR to do as Ian suggests? Otherwise, I'll be happy to do it in another PR and then I think we should close this one.

constructor first

Done.

Constructor before fields (I guess?)

Prettier indentation with trailing comma here?

This was intentional to make the parens symmetric with the opening line.