Make a failing instanceof, and a condition with no value, a real error

tslang/lib/TypeScript/MLIRGenImpl.h

@@ -4782,8 +4782,40 @@ class MLIRGenImpl
                                                NodeArray<TypeNode> typeArguments, NodeArray<Expression> arguments,
                                                const GenContext &genContext);
 
-    mlir::Value mlirGenInstanceOfOpaque(mlir::Location location, mlir::Value thisPtrValue, mlir::Value classRefVal, const GenContext &genContext)
+    // A statement's condition that is there has to produce a value. "Succeeded with no value" is
+    // what a failing expression can hand back (a null mlir::Value becomes a success), and taking
+    // it for success built no test at all: an `if` generated nothing, a `for` became endless,
+    // a `do` dereferenced the null value (segfault) and a `while` failed with "empty block".
+    mlir::LogicalResult conditionHasValue(mlir::Location location, const ValueOrLogicalResult &result)
     {
+        if (mlir::failed(result.result))
+        {
+            return mlir::failure();
+        }
+
+        if (!result.value)
+        {
+            // postponed like every message: shown only if the module then fails to compile
+            emitError(location, "the condition has no value");
+            return mlir::failure();
+        }
+
+        return mlir::success();
+    }
+
+    // A failure here has to be a failure, not a null value: ValueOrLogicalResult takes a null
+    // mlir::Value as success with no value, and an `if` whose condition has no value quietly
+    // generates nothing - `if (a instanceof C) return a;` in the generated `___unbox<C>` lost its
+    // test, so `<C>anyValue` compiled into a cast that always throws, with no error shown.
+    ValueOrLogicalResult mlirGenInstanceOfOpaque(mlir::Location location, mlir::Value thisPtrValue, mlir::Value classRefVal, const GenContext &genContext)
+    {
+        auto classType = dyn_cast<mlir_ts::ClassType>(classRefVal.getType());
+        if (!classType)
+        {
+            emitError(location, "instanceof: ") << to_print(classRefVal.getType()) << " is not a class";
+            return mlir::failure();
+        }
+
         // get VTable we can use VTableOffset
         auto vtablePtr = builder.create<mlir_ts::VTableOffsetRefOp>(location, getOpaqueType(),
                                                                     thisPtrValue, 0 /*VTABLE index*/);
@@ -4792,56 +4824,53 @@ class MLIRGenImpl
         auto instanceOfPtr = builder.create<mlir_ts::VTableOffsetRefOp>(
             location, getOpaqueType(), vtablePtr, 0 /*InstanceOf index*/);
 
-        if (auto classType = dyn_cast<mlir_ts::ClassType>(classRefVal.getType()))
-        {
-            auto classInfo = getClassInfoByFullName(classType.getName().getValue());
+        auto classInfo = getClassInfoByFullName(classType.getName().getValue());
 
-            auto resultRtti = mlirGenPropertyAccessExpression(location, classRefVal, RTTI_NAME, genContext);
-            if (!resultRtti)
-            {
-                return mlir::Value();
-            }
+        auto resultRtti = mlirGenPropertyAccessExpression(location, classRefVal, RTTI_NAME, genContext);
+        if (resultRtti.failed_or_no_value())
+        {
+            return mlir::failure();
+        }
 
-            // A class imported from a shared library (-shared) keeps its rtti behind a pointer
-            // resolved at load time, and the property access already reads through it - so the
-            // value is a string here too. This used to insist on a ref for such a class and fail,
-            // which dropped the call: `x instanceof C` did not compile and `<C>anyValue` always
-            // threw "Can't cast from any type".
-            auto rttiOfClassValue = V(resultRtti);
-            if (classInfo->isDynamicImport)
+        // A class imported from a shared library (-shared) keeps its rtti behind a pointer
+        // resolved at load time, and the property access already reads through it - so the
+        // value is a string here too. This used to insist on a ref for such a class and fail,
+        // which dropped the call: `x instanceof C` did not compile and `<C>anyValue` always
+        // threw "Can't cast from any type".
+        auto rttiOfClassValue = V(resultRtti);
+        if (classInfo->isDynamicImport)
+        {
+            if (auto valueRefType = dyn_cast<mlir_ts::RefType>(rttiOfClassValue.getType()))
             {
-                if (auto valueRefType = dyn_cast<mlir_ts::RefType>(rttiOfClassValue.getType()))
-                {
-                    rttiOfClassValue = builder.create<mlir_ts::LoadOp>(location, valueRefType.getElementType(), rttiOfClassValue);
-                }
+                rttiOfClassValue = builder.create<mlir_ts::LoadOp>(location, valueRefType.getElementType(), rttiOfClassValue);
             }
+        }
 
-            if (!isa<mlir_ts::StringType>(rttiOfClassValue.getType()))
-            {
-                emitError(location, "unsupported RTTI value type ") << to_print(rttiOfClassValue.getType());
-                return mlir::Value();
-            }
+        if (!isa<mlir_ts::StringType>(rttiOfClassValue.getType()))
+        {
+            emitError(location, "unsupported RTTI value type ") << to_print(rttiOfClassValue.getType());
+            return mlir::failure();
+        }
 
-            auto instanceOfFuncType = mlir_ts::FunctionType::get(
-                builder.getContext(), SmallVector<mlir::Type>{getOpaqueType(), getStringType()},
-                SmallVector<mlir::Type>{getBooleanType()});
+        auto instanceOfFuncType = mlir_ts::FunctionType::get(
+            builder.getContext(), SmallVector<mlir::Type>{getOpaqueType(), getStringType()},
+            SmallVector<mlir::Type>{getBooleanType()});
 
-            // TODO: check result
-            auto result = cast(location, instanceOfFuncType, instanceOfPtr, genContext);
-            EXIT_IF_FAILED_OR_NO_VALUE(result)
-            auto funcPtr = V(result);
+        auto result = cast(location, instanceOfFuncType, instanceOfPtr, genContext);
+        if (result.failed_or_no_value())
+        {
+            return mlir::failure();
+        }
 
-            // call methos, we need to send, this, and rtti info
-            auto callResult = builder.create<mlir_ts::CallIndirectOp>(
-                MLIRHelper::getCallSiteLocation(funcPtr, location),
-                funcPtr, mlir::ValueRange{thisPtrValue, rttiOfClassValue});
+        auto funcPtr = V(result);
 
-            return callResult.getResult(0);
-        }
+        // call methos, we need to send, this, and rtti info
+        auto callResult = builder.create<mlir_ts::CallIndirectOp>(
+            MLIRHelper::getCallSiteLocation(funcPtr, location),
+            funcPtr, mlir::ValueRange{thisPtrValue, rttiOfClassValue});
 
-        // error
-        return mlir::Value();
-    }    
+        return ValueOrLogicalResult(callResult.getResult(0));
+    }
 
     ValueOrLogicalResult mlirGenInstanceOfLogic(BinaryExpression binaryExpressionAST, const GenContext &genContext)
     {

tslang/lib/TypeScript/MLIRGenStatements.cpp

@@ -497,7 +497,7 @@ namespace mlirgen
 
         // condition
         auto result = mlirGen(ifStatementAST->expression, genContext);
-        EXIT_IF_FAILED_OR_NO_VALUE(result)
+        EXIT_IF_FAILED(conditionHasValue(location, result))
         auto condValue = V(result);
 
         // special case: in case of LiteralValue do not process If value is False
@@ -589,7 +589,7 @@ namespace mlirgen
 
         builder.setInsertionPointToStart(&doWhileOp.getCond().front());
         auto result = mlirGen(doStatementAST->expression, loopGenContext);
-        EXIT_IF_FAILED(result)
+        EXIT_IF_FAILED(conditionHasValue(location, result))
         auto conditionValue = V(result);
 
         if (conditionValue.getType() != getBooleanType())
@@ -629,7 +629,7 @@ namespace mlirgen
         // condition
         builder.setInsertionPointToStart(&whileOp.getCond().front());
         auto result = mlirGen(whileStatementAST->expression, loopGenContext);
-        EXIT_IF_FAILED_OR_NO_VALUE(result)
+        EXIT_IF_FAILED(conditionHasValue(location, result))
         auto conditionValue = V(result);
 
         // a condition known to be false: the body never runs and, as in mlirGen(IfStatement), is not
@@ -725,6 +725,12 @@ namespace mlirgen
         builder.setInsertionPointToStart(&forOp.getCond().front());
         auto result = mlirGen(forStatementAST->condition, loopGenContext);
         EXIT_IF_FAILED(result)
+        // no value means no condition (`for (;;)`) only when there is none to generate
+        if (forStatementAST->condition)
+        {
+            EXIT_IF_FAILED(conditionHasValue(location, result))
+        }
+
         auto conditionValue = V(result);
         if (conditionValue)
         {

tslang/test/tester/CMakeLists.txt

@@ -2226,6 +2226,21 @@ foreach(call_arity_case ${call_arity_cases})
                                     FAIL_REGULAR_EXPRESSION "Stack dump|Assertion failed")
 endforeach()
 
+# A statement's condition that produces no value is an error, not a statement generated
+# without its test (an `if` that vanished, an endless `for`, a segfault in `do`, an "empty
+# block" error from `while`). The void call
+# is the ordinary way to get one; the case that mattered was a failing `instanceof` in the
+# generated `___unbox<C>`, which made `<C>anyValue` always throw - see conditionHasValue.
+foreach(condition_value_name if while do for)
+    add_test(NAME test-compile-condition-value-${condition_value_name}
+             COMMAND $<TARGET_FILE:tslang> --emit=obj --no-default-lib -mm=none
+                     "${PROJECT_SOURCE_DIR}/test/tester/condition-value/${condition_value_name}.ts"
+                     -o "${CMAKE_CURRENT_BINARY_DIR}/condition-value-${condition_value_name}.obj")
+    set_tests_properties(test-compile-condition-value-${condition_value_name}
+                         PROPERTIES PASS_REGULAR_EXPRESSION "the condition has no value"
+                                    FAIL_REGULAR_EXPRESSION "Stack dump|Assertion failed")
+endforeach()
+
 # The calls the check must still accept: an omitted optional, default-valued or void
 # parameter, and varargs with and without extra arguments.
 tslang_add_test(NAME test-compile-call-arity-positive COMMAND test-runner "${PROJECT_SOURCE_DIR}/test/tester/call-arity/positive.ts")

tslang/test/tester/condition-value/do.ts

@@ -0,0 +1,10 @@
+// A do condition that produces no value is an error ("the condition has no value"), not a
+// statement generated without its test - see conditionHasValue in MLIRGenImpl.h.
+
+function nothing(): void {}
+
+function main() {
+    do {
+        print("body");
+    } while (nothing());
+}

tslang/test/tester/condition-value/for.ts

@@ -0,0 +1,10 @@
+// A for condition that produces no value is an error ("the condition has no value"), not a
+// statement generated without its test - see conditionHasValue in MLIRGenImpl.h.
+
+function nothing(): void {}
+
+function main() {
+    for (let i = 0; nothing(); i++) {
+        print("body");
+    }
+}

tslang/test/tester/condition-value/if.ts

@@ -0,0 +1,10 @@
+// A if condition that produces no value is an error ("the condition has no value"), not a
+// statement generated without its test - see conditionHasValue in MLIRGenImpl.h.
+
+function nothing(): void {}
+
+function main() {
+    if (nothing()) {
+        print("then");
+    }
+}

tslang/test/tester/condition-value/while.ts

@@ -0,0 +1,10 @@
+// A while condition that produces no value is an error ("the condition has no value"), not a
+// statement generated without its test - see conditionHasValue in MLIRGenImpl.h.
+
+function nothing(): void {}
+
+function main() {
+    while (nothing()) {
+        print("body");
+    }
+}