Error on uninitialized non-nullables

[email protected], [email protected]

Review URL: https://chromereviews.googleplex.com/157727013

Author: vsmenon

pkg/dev_compiler/lib/src/checker/checker.dart

@@ -468,6 +468,19 @@ class CodeChecker extends RecursiveAstVisitor {
         var initializer = variable.initializer;
         if (initializer != null) {
           variable.initializer = checkAssignment(initializer, dartType);
+        } else if (_rules.maybePrimitiveType(dartType)) {
+          var element = variable.element;
+          if (element is FieldElement && !element.isStatic) {
+            // Initialized - possibly implicitly - during construction.
+            // Handle this via a runtime check during code generation.
+
+            // TODO(vsm): Detect statically whether this can fail and
+            // report a static error (must fail) or warning (can fail).
+          } else {
+            var staticInfo =
+                new InvalidVariableDeclaration(_rules, variable, dartType);
+            _recordMessage(staticInfo);
+          }
         }
       }
     }

pkg/dev_compiler/lib/src/checker/rules.dart

@@ -28,6 +28,7 @@ abstract class TypeRules {
   bool isNumType(DartType t) => t == provider.intType.superclass;
   bool isStringType(DartType t) => t == provider.stringType;
   bool isPrimitiveType(DartType t) => false;
+  bool maybePrimitiveType(DartType t) => false;
 
   StaticInfo checkAssignment(Expression expr, DartType t);
 
@@ -84,10 +85,12 @@ class RestrictedRules extends TypeRules {
   final CheckerReporter _reporter;
   final bool covariantGenerics;
   final bool relaxedCasts;
+  final List<DartType> _primitives;
 
   RestrictedRules(TypeProvider provider, this._reporter,
       {this.covariantGenerics: true, this.relaxedCasts: true})
-      : super(provider);
+      : _primitives = [provider.intType, provider.doubleType],
+        super(provider) {}
 
   DartType getStaticType(Expression expr) {
     var type = expr.staticType;
@@ -96,7 +99,24 @@ class RestrictedRules extends TypeRules {
     return provider.dynamicType;
   }
 
-  bool isPrimitiveType(DartType t) => isIntType(t) || isDoubleType(t);
+  bool isPrimitiveType(DartType t) => _primitives.contains(t);
+
+  bool maybePrimitiveType(DartType t) {
+    // Return true iff t *may* be a primitive type.
+    // If t is a generic type parameter, return true if it may be
+    // instantiated as a primitive.
+    if (isPrimitiveType(t)) {
+      return true;
+    } else if (t is TypeParameterType) {
+      var bound = t.element.bound;
+      if (bound == null) {
+        return true;
+      }
+      return _primitives.any((DartType p) => isSubTypeOf(p, bound));
+    } else {
+      return false;
+    }
+  }
 
   // TODO(leafp): Revisit this.
   bool isGroundType(DartType t) {
@@ -230,7 +250,10 @@ class RestrictedRules extends TypeRules {
 
     // Null can be assigned to anything non-primitive.
     // FIXME: Can this be anything besides null?
-    if (t1.isBottom) return !isPrimitiveType(t2);
+    if (t1.isBottom) {
+      // Return false iff t2 *may* be a primitive type.
+      return !maybePrimitiveType(t2);
+    }
     if (t2.isBottom) return false;
 
     if (t2.isDynamic) return true;

pkg/dev_compiler/lib/src/codegen/js_codegen.dart

@@ -473,11 +473,11 @@ $name.prototype[Symbol.iterator] = function() {
       NodeList<ConstructorInitializer> initializers]) {
 
     // Run field initializers if they can have side-effects.
-    var unsetFields = new Map<String, Expression>();
+    var unsetFields = new Map<String, VariableDeclaration>();
     for (var declaration in fields) {
       for (var field in declaration.fields.variables) {
         if (_isFieldInitConstant(field)) {
-          unsetFields[field.name.name] = field.initializer;
+          unsetFields[field.name.name] = field;
         } else {
           _visitNode(field, suffix: ';\n');
         }
@@ -510,12 +510,20 @@ $name.prototype[Symbol.iterator] = function() {
     }
 
     // Initialize all remaining fields
-    unsetFields.forEach((name, expression) {
+    unsetFields.forEach((name, field) {
       out.write('this.$name = ');
+      var expression = field.initializer;
       if (expression != null) {
         expression.accept(this);
       } else {
-        out.write('null');
+        var type = rules.elementType(field.element);
+        if (rules.maybePrimitiveType(type)) {
+          out.write('dart.as(null, ');
+          _writeTypeName(type);
+          out.write(')');
+        } else {
+          out.write('null');
+        }
       }
       out.write(';\n');
     });

pkg/dev_compiler/lib/src/info.dart

@@ -354,8 +354,16 @@ class StaticTypeError extends StaticError {
 
   String get message =>
       'Type check failed: $node ($baseType) is not of type $expectedType';
+}
 
-  Level get level => Level.SEVERE;
+class InvalidVariableDeclaration extends StaticError {
+  final DartType expectedType;
+
+  InvalidVariableDeclaration(
+      TypeRules rules, VariableDeclaration declaration, this.expectedType)
+      : super(declaration);
+
+  String get message => 'Type check failed: null is not of type $expectedType';
 }
 
 class InvalidRuntimeCheckError extends StaticError {

pkg/dev_compiler/test/checker/checker_test.dart

@@ -32,11 +32,23 @@ main() {
   test('Primitives', () {
     testChecker({
       '/main.dart': '''
+        int /*severe:InvalidVariableDeclaration*/a;
+        double /*severe:InvalidVariableDeclaration*/b;
+        num c;
+
+        class A {
+          int a;
+          double b;
+          num c;
+
+          static int /*severe:InvalidVariableDeclaration*/x;
+          static double /*severe:InvalidVariableDeclaration*/y;
+          static num z;
+        }
+
         void main() {
-          // TODO(vsm): This declaration should be an error.
-          int x;
-          // TODO(vsm): This declaration should be an error.
-          double y;
+          int /*severe:InvalidVariableDeclaration*/x;
+          double /*severe:InvalidVariableDeclaration*/y;
           num z;
           bool b;
 
@@ -66,6 +78,45 @@ main() {
     });
   });
 
+  test('Primitives and generics', () {
+    testChecker({
+      '/main.dart': '''
+        class A<T> {
+          // TODO(vsm): This needs a static info indicating a runtime
+          // check at construction.
+          T x;
+
+          // TODO(vsm): Should this be a different type of DownCast?
+          T foo() => /*warning:DownCastLiteral*/null;
+
+          void bar() {
+            int /*severe:InvalidVariableDeclaration*/x;
+            num y;
+            // TODO(vsm): This should be a runtime check:
+            // Transformed to: T z = cast(null, T)
+            T /*severe:InvalidVariableDeclaration*/z;
+          }
+        }
+
+        class B<T extends List> {
+          T x;
+
+          // T cannot be primitive.
+          T foo() => null;
+        }
+
+        class C<T extends num> {
+          // TODO(vsm): This needs a static info indicating a runtime
+          // check at construction.
+          T x;
+
+          // TODO(vsm): Should this be a different type of DownCast?
+          T foo() => /*warning:DownCastLiteral*/null;
+        }
+      '''
+    });
+  });
+
   test('Unbound variable', () {
     testChecker({
       '/main.dart': '''
@@ -96,8 +147,8 @@ main() {
       void main() {
          dynamic y;
          Object o;
-         int i;
-         double d;
+         int i = 0;
+         double d = 0.0;
          num n;
          A a;
          B b;
@@ -122,8 +173,8 @@ main() {
       void main() {
          dynamic y;
          Object o;
-         int i;
-         double d;
+         int i = 0;
+         double d = 0.0;
          num n;
          A a;
          B b;
@@ -148,8 +199,8 @@ main() {
       void main() {
          dynamic y;
          Object o;
-         int i;
-         double d;
+         int i = 0;
+         double d = 0.0;
          num n;
          A a;
          B b;
@@ -176,8 +227,8 @@ main() {
       void main() {
          dynamic y;
          Object o;
-         int i;
-         double d;
+         int i = 0;
+         double d = 0.0;
          num n;
          A a;
          B b;

pkg/dev_compiler/test/checker/inferred_type_test.dart

@@ -86,7 +86,7 @@ main() {
   test('infer type on var from field', () {
     testChecker({
       '/main.dart': '''
-      int x;
+      int x = 0;
 
       test1() {
         var a = x;
@@ -100,7 +100,7 @@ main() {
         c = 4;
       }
 
-      int y; // field def after use
+      int y = 0; // field def after use
       final z = 42; // should infer `int`
     '''
     });

pkg/dev_compiler/test/codegen/constructors.dart

@@ -47,7 +47,7 @@ class I {
 }
 
 class J {
-  int nonInitialized;
+  num nonInitialized;
   bool initialized;
 
   J() : initialized = true;

pkg/dev_compiler/test/codegen/expect/_internal/_internal.js

@@ -92,7 +92,7 @@ var _internal;
       }
       singleWhere(test) {
         let length = this.length;
-        let match = null;
+        let match = dart.as(null, E);
         let matchFound = false;
         for (let i = 0; i < length; i++) {
           let element = this.elementAt(i);
@@ -282,7 +282,7 @@ var _internal;
         this._iterable = iterable;
         this._length = iterable.length;
         this._index = 0;
-        this._current = null;
+        this._current = dart.as(null, E);
       }
       get current() { return this._current; }
       moveNext() {
@@ -291,7 +291,7 @@ var _internal;
           throw new core.ConcurrentModificationError(this._iterable);
         }
         if (this._index >= length) {
-          this._current = null;
+          this._current = dart.as(null, E);
           return false;
         }
         this._current = this._iterable.elementAt(this._index);
@@ -344,15 +344,15 @@ var _internal;
       constructor(_iterator, _f) {
         this._iterator = _iterator;
         this._f = _f;
-        this._current = null;
+        this._current = dart.as(null, T);
         super();
       }
       moveNext() {
         if (this._iterator.moveNext()) {
           this._current = this._f(this._iterator.current);
           return true;
         }
-        this._current = null;
+        this._current = dart.as(null, T);
         return false;
       }
       get current() { return this._current; }
@@ -428,15 +428,15 @@ var _internal;
         this._iterator = _iterator;
         this._f = _f;
         this._currentExpansion = dart.as(new EmptyIterator(), core.Iterator$(T));
-        this._current = null;
+        this._current = dart.as(null, T);
       }
       _nextExpansion() {
       }
       get current() { return this._current; }
       moveNext() {
         if (this._currentExpansion === null) return false;
         while (!this._currentExpansion.moveNext()) {
-          this._current = null;
+          this._current = dart.as(null, T);
           if (this._iterator.moveNext()) {
             this._currentExpansion = null;
             this._currentExpansion = dart.as(this._f(this._iterator.current).iterator, core.Iterator$(T));
@@ -509,7 +509,7 @@ var _internal;
         return false;
       }
       get current() {
-        if (this._remaining < 0) return null;
+        if (this._remaining < 0) return dart.as(null, E);
         return this._iterator.current;
       }
     }
@@ -549,7 +549,7 @@ var _internal;
         return true;
       }
       get current() {
-        if (this._isFinished) return null;
+        if (this._isFinished) return dart.as(null, E);
         return this._iterator.current;
       }
     }
@@ -739,7 +739,7 @@ var _internal;
       constructor() {
       }
       moveNext() { return false; }
-      get current() { return null; }
+      get current() { return dart.as(null, E); }
     }
     return EmptyIterator;
   });