Add Null class to dart:core.

Add Null class to dart:core.

Make dart2js accept this class as the type of `null`.

Dart2js still fails on tests on the form: x is Null
VM does not recognize Null as the type of `null`.

BUG= http://dartbug.com/12382
R=fschneider@google.com, karlklose@google.com, sgjesse@google.com

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

[Lasse Reichstein Nielsen, 2013-08-13 08:25:57]

https://codereview.chromium.org/23012003/diff/1/sdk/lib/core/null.dart
File sdk/lib/core/null.dart (right):

https://codereview.chromium.org/23012003/diff/1/sdk/lib/core/null.dart#newcode20
sdk/lib/core/null.dart:20: factory Null() {
Should this be a private constructor, e.g., "Null._".
It is only here to prevent the default constructor from being added, not because
anyone needs to call it.

[ahe, 2013-08-13 09:17:55]

I'd argue the time is past half-implemented features, so before this goes in,
I'd like to make sure that it works as expected:

* Just like int, bool, double, and String, you cannot implement or subclass Null
(I think dart2js already checks this).

* 'null is Null' works. This may require some work in dart2js. Karl and Nicolas
should be able to help with that (but see below).

* This should also work:
  class Bar<S, T> { check(o) => return new Foo<T>.isT(o); }
  class Foo<T> { isT(o) => o is T; }
  new Foo<Null>.isT(null)
  new Bar<dynamic, Null>.isT(null)

* And reflectClass(Null), and reflect(null).type (for dart2js, see
Primitives.getConstructorOrInterceptor).

I think this makes 'null is Null' work, but not type as argument:

diff --git a/dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart
b/dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart
index 3e072dc..c237ece 100644
--- a/dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart
+++ b/dart/sdk/lib/_internal/compiler/implementation/ssa/codegen.dart
@@ -2347,7 +2347,13 @@ abstract class SsaCodeGenerator implements HVisitor,
HBlockInformationVisitor {
       ClassElement objectClass = compiler.objectClass;
       Element element = type.element;
 
-      if (identical(element, objectClass) || type.treatAsDynamic) {
+      if (element == compiler.nullClass) {
+        if (negative) {
+          checkNonNull(input);
+        } else {
+          checkNull(input);
+        }
+      } else if (identical(element, objectClass) || type.treatAsDynamic) {
         // The constant folder also does this optimization, but we make
         // it safe by assuming it may have not run.
         push(newLiteralBool(!negative), node);
diff --git a/dart/sdk/lib/_internal/compiler/implementation/ssa/optimize.dart
b/dart/sdk/lib/_internal/compiler/implementation/ssa/optimize.dart
index 2b01077..9460e6b 100644
--- a/dart/sdk/lib/_internal/compiler/implementation/ssa/optimize.dart
+++ b/dart/sdk/lib/_internal/compiler/implementation/ssa/optimize.dart
@@ -583,7 +583,8 @@ class SsaConstantFolder extends HBaseVisitor implements
OptimizationPhase {
     // raw type.
     } else if (!RuntimeTypes.hasTypeArguments(type)) {
       TypeMask expressionMask = expressionType.computeMask(compiler);
-      TypeMask typeMask = new TypeMask.nonNullSubtype(type);
+      TypeMask typeMask = (element == compiler.nullClass)
+          ? new TypeMask.subtype(type) : new TypeMask.nonNullSubtype(type);
       if (expressionMask.union(typeMask, compiler) == typeMask) {
         return graph.addConstantBool(true, compiler);
       } else if (expressionMask.intersection(typeMask, compiler).isEmpty) {

[Søren Gjesse, 2013-08-13 14:53:52]

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart
File sdk/lib/core/null.dart (right):

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart#new...
sdk/lib/core/null.dart:12: * Extending or implementing `Null` is not allowed,
and the
Comment cut short.

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart#new...
sdk/lib/core/null.dart:15: /**
Will this dartdoc ever end up in the documentation as _ is private?

https://codereview.chromium.org/23012003/diff/8001/tests/language/null_test.dart
File tests/language/null_test.dart (right):

https://codereview.chromium.org/23012003/diff/8001/tests/language/null_test.d...
tests/language/null_test.dart:19: class NotCalled {
Maybe name EqualsNotCalled

https://codereview.chromium.org/23012003/diff/8001/tests/language/null_test.d...
tests/language/null_test.dart:37: 
Please comment why this is here.

[rmacnak, 2013-08-14 01:23:13]

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart
File sdk/lib/core/null.dart (right):

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart#new...
sdk/lib/core/null.dart:12: * Extending or implementing `Null` is not allowed,
and the
On 2013/08/13 14:53:52, Søren Gjesse wrote:
> Comment cut short.

"It is a compile-time error for a class to attempt to extend or implement Null."

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart#new...
sdk/lib/core/null.dart:21: factory Null._() {
Please mimic the factory in bool.

[Lasse Reichstein Nielsen, 2013-08-14 08:18:35]

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart
File sdk/lib/core/null.dart (right):

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart#new...
sdk/lib/core/null.dart:12: * Extending or implementing `Null` is not allowed,
and the
On 2013/08/14 01:23:13, Ryan Macnak wrote:
> On 2013/08/13 14:53:52, Søren Gjesse wrote:
> > Comment cut short.
> 
> "It is a compile-time error for a class to attempt to extend or implement
Null."

Done.

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart#new...
sdk/lib/core/null.dart:15: /**
Probably, but not displayed by default.
I'll copy the bool constructor and omit the comment.

https://codereview.chromium.org/23012003/diff/8001/sdk/lib/core/null.dart#new...
sdk/lib/core/null.dart:21: factory Null._() {
On 2013/08/14 01:23:13, Ryan Macnak wrote:
> Please mimic the factory in bool.

Done.

https://codereview.chromium.org/23012003/diff/8001/tests/language/null_test.dart
File tests/language/null_test.dart (right):

https://codereview.chromium.org/23012003/diff/8001/tests/language/null_test.d...
tests/language/null_test.dart:19: class NotCalled {
On 2013/08/13 14:53:52, Søren Gjesse wrote:
> Maybe name EqualsNotCalled

Done.

https://codereview.chromium.org/23012003/diff/8001/tests/language/null_test.d...
tests/language/null_test.dart:37: 
On 2013/08/13 14:53:52, Søren Gjesse wrote:
> Please comment why this is here.

Done.

[Lasse Reichstein Nielsen, 2013-08-14 08:20:19]

Filed bug 12445 that was detected by an added test.

That and "null is T" where T is Null, are the only two bugs left.

[Lasse Reichstein Nielsen, 2013-08-14 09:07:11]

Only remaining bug is 12445 which is unrelated to Null (it applies to String and
bool as well).

[Søren Gjesse, 2013-08-14 10:15:12]

lgtm

[Lasse Reichstein Nielsen, 2013-08-14 11:40:21]

VM people, please review changes.

I'll remove the comments from the test and language.status before committing (or
remove the status line if the closure equality change gets in first, which is
quite possible)

[karlklose, 2013-08-14 11:55:36]

dart2js changes LGTM.

[Florian Schneider, 2013-08-14 12:22:12]

VM changes LGTM.

Please make sure all tests pass: Closure equality should work as expected as of
r26114.

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

https://codereview.chromium.org/23012003/diff/31001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1106: // The class 'Null' is not register in the class
dictionary because it is not
Not from your code: This comment about the  Null class is not meaningful.

[rmacnak, 2013-08-14 16:25:30]

Debug VM crashes on 'x as Null'. From flow_graph_type_propagator.cc remove lines
570-571:

//We should never test for an instance of null.
ASSERT(!type.IsNullType());

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

https://codereview.chromium.org/23012003/diff/31001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1106: // The class 'Null' is not register in the class
dictionary because it is not
On 2013/08/14 12:22:12, Florian Schneider wrote:
> Not from your code: This comment about the  Null class is not meaningful.

This will be cleaned up in https://chromiumcodereview.appspot.com/22819005/

https://codereview.chromium.org/23012003/diff/40001/runtime/vm/flow_graph_bui...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/23012003/diff/40001/runtime/vm/flow_graph_bui...
runtime/vm/flow_graph_builder.cc:1178: if (cls.IsNullClass()) {
This doesn't need to be a special case. The else clause below will handle it.

[Lasse Reichstein Nielsen, 2013-08-15 09:12:39]

Committed patchset #12 manually as r26175 (presubmit successful).

[ahe, 2013-08-15 11:16:46]

https://codereview.chromium.org/23012003/diff/45001/sdk/lib/_internal/lib/for...
File sdk/lib/_internal/lib/foreign_helper.dart (right):

https://codereview.chromium.org/23012003/diff/45001/sdk/lib/_internal/lib/for...
sdk/lib/_internal/lib/foreign_helper.dart:181: String JS_NULL_CLASS_NAME() {}
I would have preferred if you had extended JS_GET_NAME instead.

https://codereview.chromium.org/23012003/diff/45001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/23012003/diff/45001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_rti.dart:387: || getConstructorName(type) ==
JS_NULL_CLASS_NAME();
I have a strong suspicion that it would make sense to cache
getConstructorName(type).

https://codereview.chromium.org/23012003/diff/45001/tests/language/language.s...
File tests/language/language.status (right):

https://codereview.chromium.org/23012003/diff/45001/tests/language/language.s...
tests/language/language.status:307: null_test/none: Fail  # Issue 12482
This really means that this test isn't passing on dart2js.

So there is no test coverage of this change in dart2js. I consider that a big
problem, but I'm not going to artificially increase the priority of reflecting
on null. So I'd really appreciate if you could move the mirrors part to a
different test.

https://codereview.chromium.org/23012003/diff/45001/tests/language/null_test....
File tests/language/null_test.dart (right):

https://codereview.chromium.org/23012003/diff/45001/tests/language/null_test....
tests/language/null_test.dart:8: import "dart:mirrors";
It's problematic that you import mirrors here.

First, this is a language test, and mirrors are library that shouldn't be
required to pass language tests.

Second, it disables many optimizations in dart2js.

Could you move the mirror parts to tests/lib/mirrors.

https://codereview.chromium.org/23012003/diff/45001/tests/language/null_test....
tests/language/null_test.dart:51: new BadInherit();  // Make sure class is
referenced.
Extra space after ;

https://codereview.chromium.org/23012003/diff/45001/tests/language/null_test....
tests/language/null_test.dart:190: Expect.equals("null",
Function.apply(toString, []));
Function.apply also turns of a lot of optimization in dart2js, so moving this
part to a separate file would be great.

[Lasse Reichstein Nielsen, 2013-08-15 12:24:44]

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

https://codereview.chromium.org/23012003/diff/31001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:1106: // The class 'Null' is not register in the class
dictionary because it is not
Leaving it untouched.

https://codereview.chromium.org/23012003/diff/40001/runtime/vm/flow_graph_bui...
File runtime/vm/flow_graph_builder.cc (right):

https://codereview.chromium.org/23012003/diff/40001/runtime/vm/flow_graph_bui...
runtime/vm/flow_graph_builder.cc:1178: if (cls.IsNullClass()) {
Done.

[Lasse Reichstein Nielsen, 2013-08-15 13:06:10]

https://codereview.chromium.org/23012003/diff/45001/sdk/lib/_internal/lib/for...
File sdk/lib/_internal/lib/foreign_helper.dart (right):

https://codereview.chromium.org/23012003/diff/45001/sdk/lib/_internal/lib/for...
sdk/lib/_internal/lib/foreign_helper.dart:181: String JS_NULL_CLASS_NAME() {}
Why be different from, e.g., Object and Function?

https://codereview.chromium.org/23012003/diff/45001/sdk/lib/_internal/lib/js_...
File sdk/lib/_internal/lib/js_rti.dart (right):

https://codereview.chromium.org/23012003/diff/45001/sdk/lib/_internal/lib/js_...
sdk/lib/_internal/lib/js_rti.dart:387: || getConstructorName(type) ==
JS_NULL_CLASS_NAME();
On 2013/08/15 11:16:46, ahe wrote:
> I have a strong suspicion that it would make sense to cache
> getConstructorName(type).

Done.

https://codereview.chromium.org/23012003/diff/45001/tests/language/language.s...
File tests/language/language.status (right):

https://codereview.chromium.org/23012003/diff/45001/tests/language/language.s...
tests/language/language.status:307: null_test/none: Fail  # Issue 12482
The mirror part has been moved.
Most of it was already in lib/mirrors/null_test.

https://codereview.chromium.org/23012003/diff/45001/tests/language/null_test....
File tests/language/null_test.dart (right):

https://codereview.chromium.org/23012003/diff/45001/tests/language/null_test....
tests/language/null_test.dart:8: import "dart:mirrors";
Moved, and import removed.

https://codereview.chromium.org/23012003/diff/45001/tests/language/null_test....
tests/language/null_test.dart:51: new BadInherit();  // Make sure class is
referenced.
Do you want an extra space, or do you think that there is an extra space?

Two spaces before a comment should be correct.
Looking at tests/language, two spaces occurs 697 times, one space 281 times, and
three spaces 33 times. Two wins!

https://codereview.chromium.org/23012003/diff/45001/tests/language/null_test....
tests/language/null_test.dart:190: Expect.equals("null",
Function.apply(toString, []));
I think I'll just remove it. We are not testing Function.apply here, and it
should work the same as calling.

[Ivan Posva, 2013-08-16 23:13:47]

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

https://codereview.chromium.org/23012003/diff/45001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:420: cls.set_is_prefinalized();
Please change this back to set_is_finalized().