Report incomplete expression-statements as errors.

Previously, a dangling type or function reference would be eliminated
silently with optimizations on, or would assert when optimizations were
off.

Change-Id: Ib2e273b6f069724e8872c9cb97351b647b875a62
Bug: skia:12472
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/469525
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

gn/sksl.gni

@@ -131,6 +131,7 @@ skia_sksl_sources = [
   "$_src/sksl/ir/SkSLDiscardStatement.h",
   "$_src/sksl/ir/SkSLDoStatement.cpp",
   "$_src/sksl/ir/SkSLDoStatement.h",
+  "$_src/sksl/ir/SkSLExpression.cpp",
   "$_src/sksl/ir/SkSLExpression.h",
   "$_src/sksl/ir/SkSLExpressionStatement.cpp",
   "$_src/sksl/ir/SkSLExpressionStatement.h",

resources/sksl/runtime_errors/IllegalShaderUse.rts

@@ -1,4 +1,4 @@
-// Expect >= 7 errors (currently 9, due to double-reporting)
+// Expect >= 8 errors (currently 12, due to double-reporting)
 
 // Correct declaration (used in some test functions)
 uniform shader s1;
@@ -16,3 +16,4 @@ half4  parameter(shader s) { return s.eval(xy); }
 shader returned()          { return s1; }
 half4  constructed()       { return shader(s1).eval(xy); }
 half4  expression(bool b)  { return (b ? s1 : s2).eval(xy); }
+half4  dangling_eval()     { s1.eval; }

src/sksl/ir/SkSLExpression.cpp

@@ -0,0 +1,34 @@
+/*
+ * Copyright 2021 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "include/sksl/SkSLErrorReporter.h"
+#include "src/sksl/SkSLContext.h"
+#include "src/sksl/ir/SkSLExpression.h"
+
+namespace SkSL {
+
+bool Expression::isIncomplete(const Context& context) const {
+    switch (this->kind()) {
+        case Kind::kFunctionReference:
+        case Kind::kExternalFunctionReference:
+            context.fErrors->error(fLine, "expected '(' to begin function call");
+            return true;
+
+        case Kind::kMethodReference:
+            context.fErrors->error(fLine, "expected '(' to begin method call");
+            return true;
+
+        case Kind::kTypeReference:
+            context.fErrors->error(fLine, "expected '(' to begin constructor invocation");
+            return true;
+
+        default:
+            return false;
+    }
+}
+
+}  // namespace SkSL

src/sksl/ir/SkSLExpression.h

@@ -132,6 +132,13 @@ public:
         return false;
     }
 
+    /**
+     * Returns true if this expression is incomplete. Specifically, dangling function/method-call
+     * references that were never invoked, or type references that were never constructed, are
+     * considered incomplete expressions and should result in an error.
+     */
+    bool isIncomplete(const Context& context) const;
+
     /**
      * Compares this constant expression against another constant expression. Returns kUnknown if
      * we aren't able to deduce a result (an expression isn't actually constant, the types are

src/sksl/ir/SkSLExpressionStatement.cpp

@@ -14,6 +14,10 @@ namespace SkSL {
 
 std::unique_ptr<Statement> ExpressionStatement::Make(const Context& context,
                                                      std::unique_ptr<Expression> expr) {
+    if (expr->isIncomplete(context)) {
+        return nullptr;
+    }
+
     if (context.fConfig->fSettings.fOptimize) {
         // Expression-statements without any side effect can be replaced with a Nop.
         if (!expr->hasSideEffects()) {

src/sksl/ir/SkSLForStatement.cpp

@@ -99,14 +99,10 @@ std::unique_ptr<Statement> ForStatement::Convert(const Context& context, int lin
         }
     }
 
-    if (next) {
-        // The type of the next-expression doesn't matter, but it needs to be a complete expression.
-        // Report an error on intermediate expressions like FunctionReference or TypeReference.
-        const Type& nextType = next->type();
-        next = nextType.coerceExpression(std::move(next), context);
-        if (!next) {
-            return nullptr;
-        }
+    // The type of the next-expression doesn't matter, but it needs to be a complete expression.
+    // Report an error on intermediate expressions like FunctionReference or TypeReference.
+    if (next && next->isIncomplete(context)) {
+        return nullptr;
     }
 
     std::unique_ptr<LoopUnrollInfo> unrollInfo;

src/sksl/ir/SkSLType.cpp

@@ -14,12 +14,9 @@
 #include "src/sksl/ir/SkSLConstructorArrayCast.h"
 #include "src/sksl/ir/SkSLConstructorCompoundCast.h"
 #include "src/sksl/ir/SkSLConstructorScalarCast.h"
-#include "src/sksl/ir/SkSLExternalFunctionReference.h"
-#include "src/sksl/ir/SkSLFunctionReference.h"
 #include "src/sksl/ir/SkSLProgram.h"
 #include "src/sksl/ir/SkSLSymbolTable.h"
 #include "src/sksl/ir/SkSLType.h"
-#include "src/sksl/ir/SkSLTypeReference.h"
 
 namespace SkSL {
 
@@ -748,22 +745,14 @@ const Type* Type::clone(SymbolTable* symbolTable) const {
 
 std::unique_ptr<Expression> Type::coerceExpression(std::unique_ptr<Expression> expr,
                                                    const Context& context) const {
-    if (!expr) {
-        return nullptr;
-    }
-    const int line = expr->fLine;
-    if (expr->is<FunctionReference>() || expr->is<ExternalFunctionReference>()) {
-        context.fErrors->error(line, "expected '(' to begin function call");
-        return nullptr;
-    }
-    if (expr->is<TypeReference>()) {
-        context.fErrors->error(line, "expected '(' to begin constructor invocation");
+    if (!expr || expr->isIncomplete(context)) {
         return nullptr;
     }
     if (expr->type() == *this) {
         return expr;
     }
 
+    const int line = expr->fLine;
     const Program::Settings& settings = context.fConfig->fSettings;
     if (!expr->coercionCost(*this).isPossible(settings.fAllowNarrowingConversions)) {
         context.fErrors->error(line, "expected '" + this->displayName() + "', but found '" +

tests/sksl/errors/IncompleteExpression.glsl

@@ -1,5 +1,5 @@
+### Compilation failed:
 
-out vec4 sk_FragColor;
-vec4 main() {
-    return vec4(0.0);
-}
+error: 2: expected '(' to begin constructor invocation
+error: 3: expected a type, but found 'return'
+2 errors

tests/sksl/errors/IncompleteFunctionCall.glsl

@@ -1,5 +1,5 @@
+### Compilation failed:
 
-out vec4 sk_FragColor;
-vec4 main() {
-    return vec4(0.0);
-}
+error: 2: expected '(' to begin function call
+error: 3: expected a type, but found 'return'
+2 errors

tests/sksl/runtime_errors/IllegalShaderUse.skvm

@@ -10,4 +10,6 @@ error: 15: unknown identifier 's'
 error: 16: functions may not return opaque type 'shader'
 error: 17: cannot construct 'shader'
 error: 18: ternary expression of opaque type 'shader' not allowed
-10 errors
+error: 19: expected '(' to begin method call
+error: 19: expected a type, but found '}'
+12 errors

tests/sksl/runtime_errors/Ossfuzz36655.skvm

@@ -1,8 +1,12 @@
 ### Compilation failed:
 
-error: 20: static if has non-static test
-error: 24: static if has non-static test
-error: 31: static if has non-static test
-error: 38: static if has non-static test
-error: 44: static if has non-static test
-5 errors
+error: 2: expected '(' to begin constructor invocation
+error: 2: expected a declaration, but found ';'
+error: 2: expected a declaration, but found ';'
+error: 2: expected a declaration, but found ';'
+error: 2: expected a declaration, but found ';'
+error: 2: expected a declaration, but found ';'
+error: 2: expected a declaration, but found ';'
+error: 2: expected a declaration, but found ';'
+error: 3: expected a type, but found '}'
+9 errors