Make Null a public class of dart:core.

Cleanup making Null a public class of dart:core in the VM. Remove special cases for Null in lookup and compile-time type tests.

BUG=http://dartbug.com/12364
R=asiva@google.com, regis@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26205

[rmacnak, 2013-08-13 20:21:12]

Complication: I cannot implement this in the VM without adding Null to the
version of dart:core in sdk/lib, but adding this breaks dart2js. Removing Null
from js_helper (included in this CL) fixes most of this breakage, but it still
leaves about half of the mirror tests failing on dart2js.

[rmacnak, 2013-08-13 20:29:15]

On 2013/08/13 20:21:12, Ryan Macnak wrote:
> Complication: I cannot implement this in the VM without adding Null to the
> version of dart:core in sdk/lib, but adding this breaks dart2js. Removing Null
> from js_helper (included in this CL) fixes most of this breakage, but it still
> leaves about half of the mirror tests failing on dart2js.

Nevermind, this can be done without touching sdk/lib. Made a mistake with patch
versus normal class.

[ahe, 2013-08-13 20:41:58]

Please coordinate with lrn, see: https://codereview.chromium.org/23012003/

In particular, see my suggestions for testing in the other CL. We need to make
sure 'is' and 'as' tests work as expected, and we need to make sure that you
can/can't subclass/implement Null as required by the specification.

[rmacnak, 2013-08-13 21:18:13]

Amended to a VM-only implementation (doesn't touch sdk/lib).

[Ivan Posva, 2013-08-13 21:26:43]

https://chromiumcodereview.appspot.com/22819005/diff/27001/tests/language/nul...
File tests/language/null_is_test.dart (right):

https://chromiumcodereview.appspot.com/22819005/diff/27001/tests/language/nul...
tests/language/null_is_test.dart:17: test(null);
How about testing that

Expect.isFalse(1 is Null);
Expect.isFalse("1" is Null);

and similar?

[rmacnak, 2013-08-13 22:11:20]

Fix type cast for Null. Prohibit extending or implementing Null. Now passes
tests for the above found in the dart2js counterpart to this CL

https://chromiumcodereview.appspot.com/23012003/ patch set #2

[siva, 2013-08-14 18:53:31]

lgtm

https://chromiumcodereview.appspot.com/22819005/diff/32001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/22819005/diff/32001/runtime/vm/object....
runtime/vm/object.cc:10468: (type_class() ==
Type::Handle(Type::NullType()).type_class());
why not
return HasResolvedTypeClass() && (type_class() ==
Isolate:;Current()_>object_store()->null_class());

You could also change the pattern in
IsBoolType

[rmacnak, 2013-08-14 22:31:34]

https://chromiumcodereview.appspot.com/22819005/diff/32001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/22819005/diff/32001/runtime/vm/object....
runtime/vm/object.cc:10468: (type_class() ==
Type::Handle(Type::NullType()).type_class());
On 2013/08/14 18:53:31, siva wrote:
> why not
> return HasResolvedTypeClass() && (type_class() ==
> Isolate:;Current()_>object_store()->null_class());
> 
> You could also change the pattern in
> IsBoolType

Done.

[regis, 2013-08-15 17:27:26]

LGTM

https://codereview.chromium.org/22819005/diff/54001/runtime/vm/flow_graph_bui...
File runtime/vm/flow_graph_builder.cc (left):

https://codereview.chromium.org/22819005/diff/54001/runtime/vm/flow_graph_bui...
runtime/vm/flow_graph_builder.cc:1226: 
Any particular reason to remove these 2 blank lines? Or is it just a not quite
reverted change?

https://codereview.chromium.org/22819005/diff/54001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/22819005/diff/54001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:420: isolate->object_store()->set_null_class(cls);
Shouldn't this be done in Object::InitFromSnapshot, exactly like for
object_class?

https://codereview.chromium.org/22819005/diff/54001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:860: pending_classes.Add(cls, Heap::kOld);
Shouldn't this paragraph be moved to after the comment "Allocate and initialize
the pre-allocated classes in the core library." below, since Null is now found
in the core lib? Not required, though.

[rmacnak, 2013-08-15 17:56:03]

https://chromiumcodereview.appspot.com/22819005/diff/54001/runtime/vm/flow_gr...
File runtime/vm/flow_graph_builder.cc (left):

https://chromiumcodereview.appspot.com/22819005/diff/54001/runtime/vm/flow_gr...
runtime/vm/flow_graph_builder.cc:1226: 
On 2013/08/15 17:27:26, regis wrote:
> Any particular reason to remove these 2 blank lines? Or is it just a not quite
> reverted change?

No reason, just an artifact of merging with dart2js folks making the same
change.

https://chromiumcodereview.appspot.com/22819005/diff/54001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/22819005/diff/54001/runtime/vm/object....
runtime/vm/object.cc:420: isolate->object_store()->set_null_class(cls);
On 2013/08/15 17:27:26, regis wrote:
> Shouldn't this be done in Object::InitFromSnapshot, exactly like for
> object_class?

object_class is also created in both Init(Isolate*) and InitFromSnapshot.

https://chromiumcodereview.appspot.com/22819005/diff/54001/runtime/vm/object....
runtime/vm/object.cc:860: pending_classes.Add(cls, Heap::kOld);
On 2013/08/15 17:27:26, regis wrote:
> Shouldn't this paragraph be moved to after the comment "Allocate and
initialize
> the pre-allocated classes in the core library." below, since Null is now found
> in the core lib? Not required, though.

Moved null and bool down.

[rmacnak, 2013-08-15 18:34:32]

Committed patchset #6 manually as r26205 (presubmit successful).

[Lasse Reichstein Nielsen, 2013-08-16 08:04:13]

Drive-by-and-too-late comments.

https://chromiumcodereview.appspot.com/22819005/diff/57001/runtime/lib/null_p...
File runtime/lib/null_patch.dart (right):

https://chromiumcodereview.appspot.com/22819005/diff/57001/runtime/lib/null_p...
runtime/lib/null_patch.dart:14: int get hashCode {
Add /* patch */ in front?

https://chromiumcodereview.appspot.com/22819005/diff/57001/runtime/lib/null_p...
runtime/lib/null_patch.dart:15: return 2011;  // The year Dart was announced and
a prime.
I'd prefer either zero or some real random smi.
"Arbitrary" hash codes like this tend to cluster in the low end of the range.

I can see this was just retained from earlier, but do consider updating it. We
could add it to the Null class directly, if dart2js sees no problem with it.

https://chromiumcodereview.appspot.com/22819005/diff/57001/runtime/lib/null_p...
runtime/lib/null_patch.dart:19: return 'null';
Both the factory constructor and the toString methods are already in the Null
class. No need to override them.

https://chromiumcodereview.appspot.com/22819005/diff/57001/runtime/vm/code_ge...
File runtime/vm/code_generator.cc (right):

https://chromiumcodereview.appspot.com/22819005/diff/57001/runtime/vm/code_ge...
runtime/vm/code_generator.cc:1214: const Class& receiver_class =
Class::Handle(receiver.clazz());
I like how this change makes code *simpler*! :)

https://chromiumcodereview.appspot.com/22819005/diff/57001/tests/language/nul...
File tests/language/null_is_test.dart (right):

https://chromiumcodereview.appspot.com/22819005/diff/57001/tests/language/nul...
tests/language/null_is_test.dart:33: // Test where the argument is not a
compile-time constant.
If this function gets inlined, the n will be a compile-time constant.
Consider doing something to confuse the value beyond the capability of static
analysis.