Ensure that hashCode and runtimeType work on null.

Ensure that hashCode and runtimeType work on null.

BUG=5510
Committed: https://code.google.com/p/dart/source/detail?r=12948

[kasperl, 2012-09-27 08:09:38]

LGTM.

https://codereview.chromium.org/10979050/diff/1/tests/corelib/null.dart
File tests/corelib/null.dart (right):

https://codereview.chromium.org/10979050/diff/1/tests/corelib/null.dart#newco...
tests/corelib/null.dart:24: // var nsm = null.noSuchMethod;
File a bug for this?

https://codereview.chromium.org/10979050/diff/1/tests/corelib/null.dart#newco...
tests/corelib/null.dart:27: Expect.equals(hc(), null.hashCode());
What's the expected? I guess null.hashCode() and null.toString() are the
expected results so they so should go before the actual value (parameter order).

[ngeoffray, 2012-09-27 08:36:54]

https://codereview.chromium.org/10979050/diff/1/lib/compiler/implementation/l...
File lib/compiler/implementation/lib/interceptors.dart (right):

https://codereview.chromium.org/10979050/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/interceptors.dart:622: if (receiver is !String)
return UNINTERCEPTED(receiver.hashCode());
Don't forget JS array too.

https://codereview.chromium.org/10979050/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/interceptors.dart:671: // TODO(lrn): These
getters should be generated added automatically for all
remove 'added'

https://codereview.chromium.org/10979050/diff/1/lib/compiler/implementation/l...
lib/compiler/implementation/lib/interceptors.dart:672: // methods.
methods -> interceptors

https://codereview.chromium.org/10979050/diff/1/tests/corelib/corelib.status
File tests/corelib/corelib.status (right):

https://codereview.chromium.org/10979050/diff/1/tests/corelib/corelib.status#...
tests/corelib/corelib.status:51: null_test: Fail # Bug 5513
Why is that still a fail? I believe the changes you made in the interceptors
file covers the test.

[Lasse Reichstein Nielsen, 2012-09-27 09:32:35]

Turns out making null hashable changes the behavior of HashMap, which relied on
null.hashCode() to throw to prevent null keys. Adding explicit checks, and
updated co19 expectations for a bad test.

Please take another look.

https://codereview.chromium.org/10979050/diff/1/tests/corelib/null.dart
File tests/corelib/null.dart (right):

https://codereview.chromium.org/10979050/diff/1/tests/corelib/null.dart#newco...
tests/corelib/null.dart:24: // var nsm = null.noSuchMethod;
Done. And the code should have been uncommented.

https://codereview.chromium.org/10979050/diff/1/tests/corelib/null.dart#newco...
tests/corelib/null.dart:27: Expect.equals(hc(), null.hashCode());
Good point, yes they were intended as expectations,.

[ahe, 2012-09-27 09:38:51]

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart
File lib/core/map.dart (right):

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart#newcode7
lib/core/map.dart:7: * Null values are supported, but null keys are not.
Why have this limitation?

[Lasse Reichstein Nielsen, 2012-09-27 09:55:37]

http://codereview.chromium.org/10979050/diff/1/lib/compiler/implementation/li...
File lib/compiler/implementation/lib/interceptors.dart (right):

http://codereview.chromium.org/10979050/diff/1/lib/compiler/implementation/li...
lib/compiler/implementation/lib/interceptors.dart:622: if (receiver is !String)
return UNINTERCEPTED(receiver.hashCode());
And bool. Yey.

http://codereview.chromium.org/10979050/diff/1/lib/compiler/implementation/li...
lib/compiler/implementation/lib/interceptors.dart:671: // TODO(lrn): These
getters should be generated added automatically for all
On 2012/09/27 08:36:54, ngeoffray wrote:
> remove 'added'

Done.

http://codereview.chromium.org/10979050/diff/1/lib/compiler/implementation/li...
lib/compiler/implementation/lib/interceptors.dart:672: // methods.
"intercepted methods"

http://codereview.chromium.org/10979050/diff/1/tests/corelib/corelib.status
File tests/corelib/corelib.status (right):

http://codereview.chromium.org/10979050/diff/1/tests/corelib/corelib.status#n...
tests/corelib/corelib.status:51: null_test: Fail # Bug 5513
We don't closurize noSuchMethod yet (and I didn't mean to comment it out). We do
pass the rest of the test.

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart
File lib/core/map.dart (right):

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart#newcode7
lib/core/map.dart:7: * Null values are supported, but null keys are not.
It was always there.
The code as written assumed null wouldn't be Hashable, so it used null as an
internal marker.
The implementation should probably be changed, but that's outside the scope of
this CL.

[ahe, 2012-09-27 10:17:21]

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart
File lib/core/map.dart (right):

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart#newcode7
lib/core/map.dart:7: * Null values are supported, but null keys are not.
On 2012/09/27 09:55:37, Lasse Reichstein Nielsen wrote:
> It was always there.
> The code as written assumed null wouldn't be Hashable, so it used null as an
> internal marker.
> The implementation should probably be changed, but that's outside the scope of
> this CL.

You're changing the specification in this CL. I think that's outside the scope
of a dart2js change.

If the implementation is wrong, how about filing a bug?

[Lasse Reichstein Nielsen, 2012-09-27 10:35:53]

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart
File lib/core/map.dart (right):

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart#newcode7
lib/core/map.dart:7: * Null values are supported, but null keys are not.
I don't think I'm changing the specification. Saying "Null values are supported"
but not mentioning keys at least suggests that null keys are not. 
The implementation never supported null keys.

I'm changing something else, but keeping the behavior of Map exactly as before,
and making the implicit contract explicit.
If there was a bug, it was in the documentation being vague, which this change
fixes.

Now, for a later change of the Library, we *may* want to allow null keys. I'm
not actually sure I like that, but it'll be consistent with other containers
that allow null (like List). When that happens, the Map implementation will need
to change. And if we don't want null keys, I want to throw an ArgumentError
instead of a NullPointerException, so the implementation will have to change
anyway. That will be a change to HashMap.

[ahe, 2012-09-27 10:45:33]

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart
File lib/core/map.dart (right):

http://codereview.chromium.org/10979050/diff/6001/lib/core/map.dart#newcode7
lib/core/map.dart:7: * Null values are supported, but null keys are not.
On 2012/09/27 10:35:53, Lasse Reichstein Nielsen wrote:
> I don't think I'm changing the specification. Saying "Null values are
supported"
> but not mentioning keys at least suggests that null keys are not. 
> The implementation never supported null keys.
> 
> I'm changing something else, but keeping the behavior of Map exactly as
before,
> and making the implicit contract explicit.
> If there was a bug, it was in the documentation being vague, which this change
> fixes.
> 
> Now, for a later change of the Library, we *may* want to allow null keys. I'm
> not actually sure I like that, but it'll be consistent with other containers
> that allow null (like List). When that happens, the Map implementation will
need
> to change. And if we don't want null keys, I want to throw an ArgumentError
> instead of a NullPointerException, so the implementation will have to change
> anyway. That will be a change to HashMap.

This all sounds like something that needs to be recorded in a bug report, not a
review comment.

[Lasse Reichstein Nielsen, 2012-09-27 10:51:45]

I'll make a bug report on Maps not accepting null as key.

[ahe, 2012-09-27 11:00:24]

http://codereview.chromium.org/10979050/diff/7010/tests/corelib/corelib.status
File tests/corelib/corelib.status (right):

http://codereview.chromium.org/10979050/diff/7010/tests/corelib/corelib.statu...
tests/corelib/corelib.status:51: null_test: Fail # Bug 5513
If dart2js is not passing this test, we need a separate test to ensure the
changes you have made to dart2js are tested.

[ngeoffray, 2012-09-27 11:00:29]

LGTM

http://codereview.chromium.org/10979050/diff/7010/tests/corelib/null_test.dart
File tests/corelib/null_test.dart (right):

http://codereview.chromium.org/10979050/diff/7010/tests/corelib/null_test.dar...
tests/corelib/null_test.dart:36: var nsm = null.noSuchMethod;
Maybe you can remove these two lines, so that dart2js passes this test, and add
another test?

[Lasse Reichstein Nielsen, 2012-09-27 11:25:13]

http://codereview.chromium.org/10979050/diff/7010/tests/corelib/null_test.dart
File tests/corelib/null_test.dart (right):

http://codereview.chromium.org/10979050/diff/7010/tests/corelib/null_test.dar...
tests/corelib/null_test.dart:36: var nsm = null.noSuchMethod;
Done.

[ahe, 2012-09-27 11:30:54]

LGTM

http://codereview.chromium.org/10979050/diff/4005/lib/compiler/implementation...
File lib/compiler/implementation/lib/interceptors.dart (right):

http://codereview.chromium.org/10979050/diff/4005/lib/compiler/implementation...
lib/compiler/implementation/lib/interceptors.dart:622: if (receiver is bool)
return receiver ? 0x40377024 : 0xc18c0076;
How and why did you pick these numbers?
I think both numbers are larger than 0x1fffffff which may cause performance
problems.

http://codereview.chromium.org/10979050/diff/4005/runtime/lib/object_patch.dart
File runtime/lib/object_patch.dart (right):

http://codereview.chromium.org/10979050/diff/4005/runtime/lib/object_patch.da...
runtime/lib/object_patch.dart:29: /* patch */ Type get runtimeType => null;
Perhaps you should throw an exception instead.