Fix variable initialization checks / revise flow logic (#2578)

BREAKING CHANGE: Initialization of global variables sometimes wasn't guaranteed before, allowing unsafe behavior if initialization indeed wasn't performed before access. To mitigate, variables from now on require either an initializer, a primitive type with a trivial default value (typically `0`), a nullable type (if a reference, defaulting to `null`) or otherwise annotation with definitive assignment (i.e. `let someObject!: ...`, then inserting a runtime check upon access).

Author: dcodeIO

src/compiler.ts

@@ -49,6 +49,8 @@
   "Only variables, functions and enums become WebAssembly module exports.": 235,
   "Literal '{0}' does not fit into 'i64' or 'u64' types.": 236,
   "Index signature accessors in type '{0}' differ in types.": 237,
+  "Initializer, definitive assignment or nullable type expected.": 238,
+  "Definitive assignment has no effect on local variables.": 239,
 
   "Importing the table disables some indirect call optimizations.": 901,
   "Exporting the table disables some indirect call optimizations.": 902,
@@ -106,6 +108,7 @@
   "Declaration expected.": 1146,
   "'const' declarations must be initialized.": 1155,
   "Unterminated regular expression literal.": 1161,
+  "Declarations with initializers cannot also have definite assignment assertions.": 1263,
   "Interface declaration cannot have 'implements' clause.": 1176,
   "Binary digit expected.": 1177,
   "Octal digit expected.": 1178,
@@ -165,6 +168,7 @@
   "Variable '{0}' used before its declaration.": 2448,
   "Cannot redeclare block-scoped variable '{0}'" : 2451,
   "The type argument for type parameter '{0}' cannot be inferred from the usage. Consider specifying the type arguments explicitly.": 2453,
+  "Variable '{0}' is used before being assigned.": 2454,
   "Type alias '{0}' circularly references itself.": 2456,
   "Type '{0}' has no property '{1}'.": 2460,
   "The '{0}' operator cannot be applied to type '{1}'.": 2469,

src/diagnosticMessages.json

@@ -246,6 +246,10 @@ export class Flow {
   thisFieldFlags: Map<Property,FieldFlags> | null = null;
   /** The label we break to when encountering a return statement, when inlining. */
   inlineReturnLabel: string | null = null;
+  /** Alternative flows if a compound expression is true-ish. */
+  trueFlows: Map<ExpressionRef,Flow> | null = null;
+  /** Alternative flows if a compound expression is false-ish. */
+  falseFlows: Map<ExpressionRef,Flow> | null = null;
 
   /** Tests if this is an inline flow. */
   get isInline(): bool {
@@ -308,21 +312,31 @@ export class Flow {
   }
 
   /** Forks this flow to a child flow. */
-  fork(resetBreakContext: bool = false): Flow {
+  fork(
+    /** Whether a new break context is established, e.g. by a block. */
+    newBreakContext: bool = false,
+    /** Whether a new continue context is established, e.g. by a loop. */
+    newContinueContext: bool = newBreakContext
+  ): Flow {
     let branch = new Flow(this.targetFunction, this.inlineFunction);
     branch.parent = this;
+    branch.flags = this.flags;
     branch.outer = this.outer;
-    if (resetBreakContext) {
-      branch.flags = this.flags & ~(
+    if (newBreakContext) {
+      branch.flags &= ~(
         FlowFlags.Breaks |
-        FlowFlags.ConditionallyBreaks |
+        FlowFlags.ConditionallyBreaks
+      );
+    } else {
+      branch.breakLabel = this.breakLabel;
+    }
+    if (newContinueContext) {
+      branch.flags &= ~(
         FlowFlags.Continues |
         FlowFlags.ConditionallyContinues
       );
     } else {
-      branch.flags = this.flags;
       branch.continueLabel = this.continueLabel;
-      branch.breakLabel = this.breakLabel;
     }
     branch.localFlags = this.localFlags.slice();
     if (this.sourceFunction.is(CommonFlags.Constructor)) {
@@ -335,6 +349,52 @@ export class Flow {
     return branch;
   }
 
+  /** Forks this flow to a child flow where `condExpr` is true-ish. */
+  forkThen(
+    /** Condition that turned out to be true. */
+    condExpr: ExpressionRef,
+    /** Whether a new break context is established, e.g. by a block. */
+    newBreakContext: bool = false,
+    /** Whether a new continue context is established, e.g. by a loop. */
+    newContinueContext: bool = newBreakContext
+  ): Flow {
+    let flow = this.fork(newBreakContext, newContinueContext);
+    let trueFlows = this.trueFlows;
+    if (trueFlows && trueFlows.has(condExpr)) {
+      flow.inherit(changetype<Flow>(trueFlows.get(condExpr)));
+    }
+    flow.inheritNonnullIfTrue(condExpr);
+    return flow;
+  }
+
+  /** Remembers the alternative flow if `condExpr` turns out `true`. */
+  noteThen(condExpr: ExpressionRef, trueFlow: Flow): void {
+    let trueFlows = this.trueFlows;
+    if (!trueFlows) this.trueFlows = trueFlows = new Map();
+    trueFlows.set(condExpr, trueFlow);
+  }
+
+  /** Forks this flow to a child flow where `condExpr` is false-ish. */
+  forkElse(
+    /** Condition that turned out to be false. */
+    condExpr: ExpressionRef
+  ): Flow {
+    let flow = this.fork();
+    let falseFlows = this.falseFlows;
+    if (falseFlows && falseFlows.has(condExpr)) {
+      flow.inherit(changetype<Flow>(falseFlows.get(condExpr)));
+    }
+    flow.inheritNonnullIfFalse(condExpr);
+    return flow;
+  }
+
+  /** Remembers the alternative flow if `condExpr` turns out `false`. */
+  noteElse(condExpr: ExpressionRef, falseFlow: Flow): void {
+    let falseFlows = this.falseFlows;
+    if (!falseFlows) this.falseFlows = falseFlows = new Map();
+    falseFlows.set(condExpr, falseFlow);
+  }
+
   /** Gets a free temporary local of the specified type. */
   getTempLocal(type: Type): Local {
     let local = this.targetFunction.addLocal(type);
@@ -523,36 +583,27 @@ export class Flow {
     }
   }
 
-  /** Pushes a new break label to the stack, for example when entering a loop that one can `break` from. */
-  pushBreakLabel(): string {
+  /** Pushes a new control flow label, for example when entering a loop that one can `break` from. */
+  pushControlFlowLabel(): i32 {
     let targetFunction = this.targetFunction;
     let id = targetFunction.nextBreakId++;
     let stack = targetFunction.breakStack;
     if (!stack) targetFunction.breakStack = [ id ];
     else stack.push(id);
-    let label = id.toString();
-    targetFunction.breakLabel = label;
-    return label;
+    return id;
   }
 
-  /** Pops the most recent break label from the stack. */
-  popBreakLabel(): void {
+  /** Pops the most recent control flow label and validates that it matches. */
+  popControlFlowLabel(expectedLabel: i32): void {
     let targetFunction = this.targetFunction;
-    let stack = assert(targetFunction.breakStack);
-    let length = assert(stack.length);
-    stack.pop();
-    if (length > 1) {
-      targetFunction.breakLabel = stack[length - 2].toString();
-    } else {
-      targetFunction.breakLabel = null;
-      targetFunction.breakStack = null;
-    }
+    let stack = assert(targetFunction.breakStack); // should exist
+    assert(stack.length); // should not be empty
+    assert(stack.pop() == expectedLabel); // should match
   }
 
   /** Inherits flags of another flow into this one, i.e. a finished inner block. */
   inherit(other: Flow): void {
     assert(other.targetFunction == this.targetFunction);
-    assert(other.parent == this); // currently the case, but might change
     let otherFlags = other.flags;
 
     // respective inner flags are irrelevant if contexts differ
@@ -571,18 +622,10 @@ export class Flow {
     this.thisFieldFlags = other.thisFieldFlags;
   }
 
-  /** Inherits flags of a conditional branch joining again with this one, i.e. then without else. */
-  inheritBranch(other: Flow, conditionKind: ConditionKind = ConditionKind.Unknown): void {
-    assert(other.targetFunction == this.targetFunction);
-    switch (conditionKind) {
-      case ConditionKind.True: this.inherit(other); // always executes
-      case ConditionKind.False: return;             // never executes
-    }
 
-    // Note that flags in `this` flow have already happened. For instance,
-    // a return cannot be undone no matter what'd happen in subsequent branches,
-    // but an allocation, which doesn't terminate, can become conditional. Not
-    // all flags have a corresponding conditional flag that's tracked.
+  /** Merges only the side effects of a branch, i.e. when not taken. */
+  mergeSideEffects(other: Flow): void {
+    assert(other.targetFunction == this.targetFunction);
 
     let thisFlags = this.flags;
     let otherFlags = other.flags;
@@ -653,8 +696,13 @@ export class Flow {
     }
 
     this.flags = newFlags | (thisFlags & (FlowFlags.UncheckedContext | FlowFlags.CtorParamContext));
+  }
 
-    // local flags
+  /** Merges a branch joining again with this flow, i.e. then without else. */
+  mergeBranch(other: Flow): void {
+    this.mergeSideEffects(other);
+
+    // Local flags matter if the branch does not terminate
     let thisLocalFlags = this.localFlags;
     let numThisLocalFlags = thisLocalFlags.length;
     let otherLocalFlags = other.localFlags;
@@ -675,12 +723,12 @@ export class Flow {
     // only be set if it has been observed prior to entering the branch.
   }
 
-  /** Inherits mutual flags of two alternate branches becoming this one, i.e. then with else. */
-  inheritMutual(left: Flow, right: Flow): void {
+  /** Inherits two alternate branches to become this flow, i.e. then with else. */
+  inheritAlternatives(left: Flow, right: Flow): void {
     assert(left.targetFunction == right.targetFunction);
     assert(left.targetFunction == this.targetFunction);
-    // This differs from the previous method in that no flags are guaranteed
-    // to happen unless it is the case in both flows.
+    // Differs from `mergeBranch` in that the alternatives are intersected to
+    // then become this branch.
 
     let leftFlags = left.flags;
     let rightFlags = right.flags;
@@ -881,7 +929,7 @@ export class Flow {
   }
 
   /** Updates local states to reflect that this branch is only taken when `expr` is true-ish. */
-  inheritNonnullIfTrue(
+  private inheritNonnullIfTrue(
     /** Expression being true. */
     expr: ExpressionRef,
     /** If specified, only set the flag if also nonnull in this flow. */
@@ -995,7 +1043,7 @@ export class Flow {
   }
 
   /** Updates local states to reflect that this branch is only taken when `expr` is false-ish. */
-  inheritNonnullIfFalse(
+  private inheritNonnullIfFalse(
     /** Expression being false. */
     expr: ExpressionRef,
     /** If specified, only set the flag if also nonnull in this flow. */

src/flow.ts

@@ -3617,32 +3617,6 @@ export class BinaryModule {
   ) {}
 }
 
-/** Tests if an expression needs an explicit 'unreachable' when it is the terminating statement. */
-export function needsExplicitUnreachable(expr: ExpressionRef): bool {
-  // not applicable if pushing a value to the stack
-  if (binaryen._BinaryenExpressionGetType(expr) != TypeRef.Unreachable) {
-    return false;
-  }
-
-  switch (binaryen._BinaryenExpressionGetId(expr)) {
-    case ExpressionId.Unreachable:
-    case ExpressionId.Return: return false;
-    case ExpressionId.Break: {
-      return binaryen._BinaryenBreakGetCondition(expr) != 0;
-    }
-    case ExpressionId.Block: {
-      if (!binaryen._BinaryenBlockGetName(expr)) { // can't break out of it
-        let numChildren = binaryen._BinaryenBlockGetNumChildren(expr); // last child needs unreachable
-        return (
-          numChildren > 0 &&
-          needsExplicitUnreachable(binaryen._BinaryenBlockGetChildAt(expr, numChildren - 1))
-        );
-      }
-    }
-  }
-  return true;
-}
-
 // TypeBuilder
 
 const DEBUG_TYPEBUILDER = false;

src/module.ts

@@ -985,6 +985,12 @@ export class Parser extends DiagnosticEmitter {
       }
       initializer = this.parseExpression(tn, Precedence.Comma + 1);
       if (!initializer) return null;
+      if (flags & CommonFlags.DefinitelyAssigned) {
+        this.error(
+          DiagnosticCode.Declarations_with_initializers_cannot_also_have_definite_assignment_assertions,
+          initializer.range
+        );
+      }
     } else if (!isFor) {
       if (flags & CommonFlags.Const) {
         if (!(flags & CommonFlags.Ambient)) {
@@ -1001,7 +1007,7 @@ export class Parser extends DiagnosticEmitter {
       }
     }
     let range = Range.join(identifier.range, tn.range());
-    if (initializer && (flags & CommonFlags.DefinitelyAssigned) != 0) {
+    if ((flags & CommonFlags.DefinitelyAssigned) != 0 && (flags & CommonFlags.Ambient) != 0) {
       this.error(
         DiagnosticCode.A_definite_assignment_assertion_is_not_permitted_in_this_context,
         range
@@ -2383,12 +2389,15 @@ export class Parser extends DiagnosticEmitter {
         }
         initializer = this.parseExpression(tn);
         if (!initializer) return null;
+        if (flags & CommonFlags.DefinitelyAssigned) {
+          this.error(
+            DiagnosticCode.Declarations_with_initializers_cannot_also_have_definite_assignment_assertions,
+            name.range
+          );
+        }
       }
       let range = tn.range(startPos, tn.pos);
-      if (
-        (flags & CommonFlags.DefinitelyAssigned) != 0 &&
-        (isInterface || initializer || (flags & CommonFlags.Static) != 0)
-      ) {
+      if ((flags & CommonFlags.DefinitelyAssigned) != 0 && (isInterface || (flags & CommonFlags.Ambient) != 0)) {
         this.error(
           DiagnosticCode.A_definite_assignment_assertion_is_not_permitted_in_this_context,
           range

src/parser.ts

@@ -138,7 +138,8 @@ import {
 } from "./resolver";
 
 import {
-  Flow
+  Flow,
+  LocalFlags
 } from "./flow";
 
 import {
@@ -1983,7 +1984,7 @@ export class Program extends DiagnosticEmitter {
           }
         }
         classReference = classReference.base;
-      } while(classReference);
+      } while (classReference);
     } else {
       let signatureReference = type.getSignature();
       if (signatureReference) {
@@ -3767,7 +3768,8 @@ export class Function extends TypedElement {
     this.original = this;
     let program = prototype.program;
     this.type = signature.type;
-    this.flow = Flow.createDefault(this);
+    let flow = Flow.createDefault(this);
+    this.flow = flow;
     if (!prototype.is(CommonFlags.Ambient)) {
       let localIndex = 0;
       let thisType = signature.thisType;
@@ -3782,6 +3784,7 @@ export class Function extends TypedElement {
         if (!scopedLocals) this.flow.scopedLocals = scopedLocals = new Map();
         scopedLocals.set(CommonNames.this_, local);
         this.localsByIndex[local.index] = local;
+        flow.setLocalFlag(local.index, LocalFlags.Initialized);
       }
       let parameterTypes = signature.parameterTypes;
       for (let i = 0, k = parameterTypes.length; i < k; ++i) {
@@ -3797,6 +3800,7 @@ export class Function extends TypedElement {
         if (!scopedLocals) this.flow.scopedLocals = scopedLocals = new Map();
         scopedLocals.set(parameterName, local);
         this.localsByIndex[local.index] = local;
+        flow.setLocalFlag(local.index, LocalFlags.Initialized);
       }
     }
     registerConcreteElement(program, this);
@@ -3872,15 +3876,13 @@ export class Function extends TypedElement {
   // used by flows to keep track of break labels
   nextBreakId: i32 = 0;
   breakStack: i32[] | null = null;
-  breakLabel: string | null = null;
 
   /** Finalizes the function once compiled, releasing no longer needed resources. */
   finalize(module: Module, ref: FunctionRef): void {
     this.ref = ref;
     let breakStack = this.breakStack;
-    assert(!breakStack || !breakStack.length); // internal error
-    this.breakStack = breakStack = null;
-    this.breakLabel = null;
+    assert(!breakStack || !breakStack.length); // should be empty
+    this.breakStack = null;
     this.addDebugInfo(module, ref);
   }
 

src/program.ts

@@ -49,7 +49,7 @@ import { E_ALLOCATION_TOO_LARGE, E_ALREADY_PINNED, E_NOT_PINNED } from "../util/
 // @ts-ignore: decorator
 @lazy let pinSpace = initLazy(changetype<Object>(memory.data(offsetof<Object>())));
 // @ts-ignore: decorator
-@lazy let iter: Object; // null
+@lazy let iter: Object = changetype<Object>(0); // unsafe initializion below
 
 function initLazy(space: Object): Object {
   space.nextWithColor = changetype<usize>(space);

std/assembly/rt/itcms.ts

@@ -137,7 +137,7 @@ import { E_ALLOCATION_TOO_LARGE } from "../util/error";
 @inline const ROOT_SIZE: usize = HL_END + sizeof<usize>();
 
 // @ts-ignore: decorator
-@lazy export let ROOT: Root;
+@lazy export let ROOT: Root = changetype<Root>(0); // unsafe initializion below
 
 /** Gets the second level map of the specified first level. */
 // @ts-ignore: decorator