Migrate convertSwitch to SwitchStatement::Make.

This splits switch() construction into two stages.

- One version of Make takes an array of case-values and case-statement
lists, and is responsible for reporting errors if the case-values are
not unique or are improperly typed. This is what the IR generator or DSL
will start with on its first encounter with the switch statement.

- The other version of Make takes an array of already-processed
SwitchCases and can assume the invariant that they're all correctly-
typed with unique values. This is what we will have when a statement
is inlined or otherwise cloned. (We still assert this invariant, for
correctness' sake, but in release mode we assume it.)

This CL doesn't perform any optimizations at Make time yet; it does work
equivalent to how `switch` works in the IR generator today. It does
improve duplicate case-label checking slightly; duplicate case labels
are now reported, and duplicate `default:` labels are detected.
(Multiple `default` labels won't pass the parser, but they can be
constructed in DSL.)

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

gn/sksl.gni

@@ -128,6 +128,7 @@ skia_sksl_sources = [
   "$_src/sksl/ir/SkSLStatement.h",
   "$_src/sksl/ir/SkSLStructDefinition.h",
   "$_src/sksl/ir/SkSLSwitchCase.h",
+  "$_src/sksl/ir/SkSLSwitchStatement.cpp",
   "$_src/sksl/ir/SkSLSwitchStatement.h",
   "$_src/sksl/ir/SkSLSwizzle.cpp",
   "$_src/sksl/ir/SkSLSwizzle.h",

src/sksl/SkSLIRGenerator.cpp

@@ -771,56 +771,6 @@ std::unique_ptr<Statement> IRGenerator::convertDo(const ASTNode& d) {
     return this->convertDo(std::move(statement), std::move(test));
 }
 
-std::unique_ptr<Statement> IRGenerator::convertSwitch(
-                                                int offset,
-                                                bool isStatic,
-                                                std::unique_ptr<Expression> value,
-                                                ExpressionArray caseValues,
-                                                SkTArray<StatementArray> caseStatements,
-                                                std::shared_ptr<SymbolTable> symbolTable) {
-    SkASSERT(caseValues.size() == caseStatements.size());
-    if (this->strictES2Mode()) {
-        this->errorReporter().error(offset, "switch statements are not supported");
-        return nullptr;
-    }
-
-    if (!value->type().isEnum()) {
-        value = this->coerce(std::move(value), *fContext.fTypes.fInt);
-        if (!value) {
-            return nullptr;
-        }
-    }
-    SkTHashSet<SKSL_INT> intValues;
-    std::vector<std::unique_ptr<SwitchCase>> cases;
-    for (size_t i = 0; i < caseValues.size(); ++i) {
-        int caseOffset;
-        std::unique_ptr<Expression> caseValue;
-        if (caseValues[i]) {
-            caseOffset = caseValues[i]->fOffset;
-            caseValue = this->coerce(std::move(caseValues[i]), value->type());
-            if (!caseValue) {
-                return nullptr;
-            }
-            SKSL_INT v = 0;
-            if (!ConstantFolder::GetConstantInt(*caseValue, &v)) {
-                this->errorReporter().error(caseValue->fOffset,
-                                            "case value must be a constant integer");
-                return nullptr;
-            }
-            if (intValues.contains(v)) {
-                this->errorReporter().error(caseValue->fOffset, "duplicate case value");
-            }
-            intValues.add(v);
-        } else {
-            caseOffset = offset;
-        }
-        cases.push_back(std::make_unique<SwitchCase>(caseOffset, std::move(caseValue),
-                                                     std::move(caseStatements[i])));
-    }
-    return std::make_unique<SwitchStatement>(offset, isStatic, std::move(value),
-                                             std::move(cases), symbolTable);
-}
-
 std::unique_ptr<Statement> IRGenerator::convertSwitch(const ASTNode& s) {
     SkASSERT(s.fKind == ASTNode::Kind::kSwitch);
 
@@ -854,8 +804,8 @@ std::unique_ptr<Statement> IRGenerator::convertSwitch(const ASTNode& s) {
         }
         caseStatements.push_back(std::move(statements));
     }
-    return this->convertSwitch(s.fOffset, s.getBool(), std::move(value), std::move(caseValues),
-                               std::move(caseStatements), fSymbolTable);
+    return SwitchStatement::Make(fContext, s.fOffset, s.getBool(), std::move(value),
+                                 std::move(caseValues), std::move(caseStatements), fSymbolTable);
 }
 
 std::unique_ptr<Statement> IRGenerator::convertExpressionStatement(const ASTNode& s) {

src/sksl/SkSLIRGenerator.h

@@ -202,12 +202,6 @@ private:
     std::unique_ptr<Statement> convertDo(std::unique_ptr<Statement> stmt,
                                          std::unique_ptr<Expression> test);
     std::unique_ptr<Statement> convertDo(const ASTNode& d);
-    std::unique_ptr<Statement> convertSwitch(int offset,
-                                             bool isStatic,
-                                             std::unique_ptr<Expression> value,
-                                             ExpressionArray caseValues,
-                                             SkTArray<StatementArray> caseStatements,
-                                             std::shared_ptr<SymbolTable> symbolTable);
     std::unique_ptr<Statement> convertSwitch(const ASTNode& s);
     std::unique_ptr<Expression> convertBinaryExpression(const ASTNode& expression);
     std::unique_ptr<Extension> convertExtension(int offset, StringFragment name);

src/sksl/SkSLInliner.cpp

@@ -530,9 +530,8 @@ std::unique_ptr<Statement> Inliner::inlineStatement(int offset,
                 cases.push_back(std::make_unique<SwitchCase>(offset, expr(sc->value()),
                                                              stmts(sc->statements())));
             }
-            return std::make_unique<SwitchStatement>(offset, ss.isStatic(), expr(ss.value()),
-                                                     std::move(cases),
-                                                     SymbolTable::WrapIfBuiltin(ss.symbols()));
+            return SwitchStatement::Make(*fContext, offset, ss.isStatic(), expr(ss.value()),
+                                        std::move(cases), SymbolTable::WrapIfBuiltin(ss.symbols()));
         }
         case Statement::Kind::kVarDeclaration: {
             const VarDeclaration& decl = statement.as<VarDeclaration>();

src/sksl/dsl/priv/DSLWriter.cpp

@@ -15,6 +15,7 @@
 #include "src/sksl/SkSLIRGenerator.h"
 #include "src/sksl/dsl/DSLCore.h"
 #include "src/sksl/ir/SkSLConstructor.h"
+#include "src/sksl/ir/SkSLSwitchStatement.h"
 
 #if !SKSL_USE_THREAD_LOCAL
 #include <pthread.h>
@@ -135,9 +136,9 @@ DSLExpression DSLWriter::ConvertPrefix(Operator op, std::unique_ptr<Expression>
 DSLStatement DSLWriter::ConvertSwitch(std::unique_ptr<Expression> value,
                                       ExpressionArray caseValues,
                                       SkTArray<SkSL::StatementArray> caseStatements) {
-    return IRGenerator().convertSwitch(/*offset=*/-1, /*isStatic=*/false, std::move(value),
-                                       std::move(caseValues), std::move(caseStatements),
-                                       IRGenerator().fSymbolTable);
+    return SwitchStatement::Make(Context(), /*offset=*/-1, /*isStatic=*/false, std::move(value),
+                                 std::move(caseValues), std::move(caseStatements),
+                                 IRGenerator().fSymbolTable);
 }
 
 

src/sksl/ir/SkSLSwitchStatement.cpp

@@ -0,0 +1,165 @@
+/*
+ * Copyright 2021 Google LLC
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "src/sksl/ir/SkSLSwitchStatement.h"
+
+#include <forward_list>
+
+#include "include/private/SkTHash.h"
+#include "src/sksl/SkSLConstantFolder.h"
+#include "src/sksl/SkSLContext.h"
+#include "src/sksl/SkSLProgramSettings.h"
+#include "src/sksl/ir/SkSLSymbolTable.h"
+#include "src/sksl/ir/SkSLType.h"
+
+namespace SkSL {
+
+std::unique_ptr<Statement> SwitchStatement::clone() const {
+    std::vector<std::unique_ptr<SwitchCase>> cloned;
+    for (const std::unique_ptr<SwitchCase>& sc : this->cases()) {
+        cloned.emplace_back(&sc->clone().release()->as<SwitchCase>());
+    }
+    return std::make_unique<SwitchStatement>(fOffset,
+                                             this->isStatic(),
+                                             this->value()->clone(),
+                                             std::move(cloned),
+                                             SymbolTable::WrapIfBuiltin(this->symbols()));
+}
+
+String SwitchStatement::description() const {
+    String result;
+    if (this->isStatic()) {
+        result += "@";
+    }
+    result += String::printf("switch (%s) {\n", this->value()->description().c_str());
+    for (const auto& c : this->cases()) {
+        result += c->description();
+    }
+    result += "}";
+    return result;
+}
+
+static std::forward_list<const SwitchCase*> find_duplicate_case_values(
+        const std::vector<std::unique_ptr<SwitchCase>>& cases) {
+    std::forward_list<const SwitchCase*> duplicateCases;
+    SkTHashSet<SKSL_INT> intValues;
+    bool foundDefault = false;
+
+    for (const std::unique_ptr<SwitchCase>& sc : cases) {
+        const std::unique_ptr<Expression>& valueExpr = sc->value();
+
+        // A null case-value indicates the `default` switch-case.
+        if (!valueExpr) {
+            if (foundDefault) {
+                duplicateCases.push_front(sc.get());
+                continue;
+            }
+            foundDefault = true;
+            continue;
+        }
+
+        // GetConstantInt already succeeded when the SwitchCase was first assembled, so it should
+        // succeed this time too.
+        SKSL_INT intValue = 0;
+        SkAssertResult(ConstantFolder::GetConstantInt(*valueExpr, &intValue));
+        if (intValues.contains(intValue)) {
+            duplicateCases.push_front(sc.get());
+            continue;
+        }
+        intValues.add(intValue);
+    }
+
+    return duplicateCases;
+}
+
+std::unique_ptr<Statement> SwitchStatement::Make(const Context& context,
+                                                 int offset,
+                                                 bool isStatic,
+                                                 std::unique_ptr<Expression> value,
+                                                 ExpressionArray caseValues,
+                                                 SkTArray<StatementArray> caseStatements,
+                                                 std::shared_ptr<SymbolTable> symbolTable) {
+    SkASSERT(caseValues.size() == caseStatements.size());
+    if (context.fConfig->strictES2Mode()) {
+        context.fErrors.error(offset, "switch statements are not supported");
+        return nullptr;
+    }
+
+    if (!value->type().isEnum()) {
+        value = context.fTypes.fInt->coerceExpression(std::move(value), context);
+        if (!value) {
+            return nullptr;
+        }
+    }
+
+    std::vector<std::unique_ptr<SwitchCase>> cases;
+    for (int i = 0; i < caseValues.count(); ++i) {
+        int caseOffset;
+        std::unique_ptr<Expression> caseValue;
+        if (caseValues[i]) {
+            caseOffset = caseValues[i]->fOffset;
+
+            // Case values must be the same type as the switch value--`int` or a particular enum.
+            caseValue = value->type().coerceExpression(std::move(caseValues[i]), context);
+            if (!caseValue) {
+                return nullptr;
+            }
+            // Case values must be a literal integer or a `const int` variable reference.
+            SKSL_INT intValue;
+            if (!ConstantFolder::GetConstantInt(*caseValue, &intValue)) {
+                context.fErrors.error(caseValue->fOffset, "case value must be a constant integer");
+                return nullptr;
+            }
+        } else {
+            // The null case-expression corresponds to `default:`.
+            caseOffset = offset;
+        }
+        cases.push_back(std::make_unique<SwitchCase>(caseOffset, std::move(caseValue),
+                                                     std::move(caseStatements[i])));
+    }
+
+    // Detect duplicate `case` labels and report an error.
+    // (Using forward_list here to optimize for the common case of no results.)
+    std::forward_list<const SwitchCase*> duplicateCases = find_duplicate_case_values(cases);
+    if (!duplicateCases.empty()) {
+        duplicateCases.reverse();
+        for (const SwitchCase* sc : duplicateCases) {
+            if (sc->value() != nullptr) {
+                context.fErrors.error(sc->fOffset,
+                                      "duplicate case value '" + sc->value()->description() + "'");
+            } else {
+                context.fErrors.error(sc->fOffset, "duplicate default case");
+            }
+        }
+        return nullptr;
+    }
+
+    return SwitchStatement::Make(context, offset, isStatic, std::move(value), std::move(cases),
+                                 std::move(symbolTable));
+}
+
+std::unique_ptr<Statement> SwitchStatement::Make(const Context& context,
+                                                 int offset,
+                                                 bool isStatic,
+                                                 std::unique_ptr<Expression> value,
+                                                 std::vector<std::unique_ptr<SwitchCase>> cases,
+                                                 std::shared_ptr<SymbolTable> symbolTable) {
+    // Confirm that every switch-case has been coerced to the proper type.
+    SkASSERT(std::all_of(cases.begin(), cases.end(), [&](const std::unique_ptr<SwitchCase>& sc) {
+        return !sc->value() ||  // `default` case has a null value
+               value->type() == sc->value()->type();
+    }));
+
+    // Confirm that every switch-case value is unique.
+    SkASSERT(find_duplicate_case_values(cases).empty());
+
+    // TODO(skia:11340): Optimize static switches.
+    return std::make_unique<SwitchStatement>(offset, isStatic, std::move(value), std::move(cases),
+                                             std::move(symbolTable));
+}
+
+}  // namespace SkSL

src/sksl/ir/SkSLSwitchStatement.h

@@ -11,8 +11,12 @@
 #include "src/sksl/ir/SkSLStatement.h"
 #include "src/sksl/ir/SkSLSwitchCase.h"
 
+#include <memory>
+#include <vector>
+
 namespace SkSL {
 
+class Expression;
 class SymbolTable;
 
 /**
@@ -22,6 +26,7 @@ class SwitchStatement final : public Statement {
 public:
     static constexpr Kind kStatementKind = Kind::kSwitch;
 
+    // Use SwitchStatement::Make to check invariants when creating switch statements.
     SwitchStatement(int offset, bool isStatic, std::unique_ptr<Expression> value,
                     std::vector<std::unique_ptr<SwitchCase>> cases,
                     const std::shared_ptr<SymbolTable> symbols)
@@ -31,6 +36,25 @@ public:
         , fCases(std::move(cases))
         , fSymbols(std::move(symbols)) {}
 
+    // Create a `switch` statement with an array of case-values and case-statements.
+    // Coerces case values to the proper type and reports an error if cases are duplicated.
+    static std::unique_ptr<Statement> Make(const Context& context,
+                                           int offset,
+                                           bool isStatic,
+                                           std::unique_ptr<Expression> value,
+                                           ExpressionArray caseValues,
+                                           SkTArray<StatementArray> caseStatements,
+                                           std::shared_ptr<SymbolTable> symbolTable);
+
+    // Create a `switch` statement with an array of SwitchCases.
+    // The array of SwitchCases must already contain non-overlapping, correctly-typed case values.
+    static std::unique_ptr<Statement> Make(const Context& context,
+                                           int offset,
+                                           bool isStatic,
+                                           std::unique_ptr<Expression> value,
+                                           std::vector<std::unique_ptr<SwitchCase>> cases,
+                                           std::shared_ptr<SymbolTable> symbolTable);
+
     std::unique_ptr<Expression>& value() {
         return fValue;
     }
@@ -55,31 +79,9 @@ public:
         return fSymbols;
     }
 
-    std::unique_ptr<Statement> clone() const override {
-        std::vector<std::unique_ptr<SwitchCase>> cloned;
-        for (const std::unique_ptr<SwitchCase>& sc : this->cases()) {
-            cloned.emplace_back(&sc->clone().release()->as<SwitchCase>());
-        }
-        return std::unique_ptr<Statement>(new SwitchStatement(
-                                                      fOffset,
-                                                      this->isStatic(),
-                                                      this->value()->clone(),
-                                                      std::move(cloned),
-                                                      SymbolTable::WrapIfBuiltin(this->symbols())));
-    }
+    std::unique_ptr<Statement> clone() const override;
 
-    String description() const override {
-        String result;
-        if (this->isStatic()) {
-            result += "@";
-        }
-        result += String::printf("switch (%s) {\n", this->value()->description().c_str());
-        for (const auto& c : this->cases()) {
-            result += c->description();
-        }
-        result += "}";
-        return result;
-    }
+    String description() const override;
 
 private:
     bool fIsStatic;

tests/SkSLDSLTest.cpp

@@ -1119,11 +1119,17 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLSwitch, r, ctxInfo) {
     Statement x = Switch(5,
         Case(0, a = 0, Break()),
         Case(1, a = 1, Continue()),
+        Case(2, a = 2  /*Fallthrough*/),
         Default(Discard())
     );
-    EXPECT_EQUAL(x,
-        "switch (5) { case 0: (a = 0.0); break; case 1: (a = 1.0); continue; default: discard; }");
-
+    EXPECT_EQUAL(x, R"(
+        switch (5) {
+            case 0: (a = 0.0); break;
+            case 1: (a = 1.0); continue;
+            case 2: (a = 2.0);
+            default: discard;
+        }
+    )");
     Statement y = Switch(b);
     EXPECT_EQUAL(y, "switch (b) {}");
 
@@ -1131,10 +1137,15 @@ DEF_GPUTEST_FOR_MOCK_CONTEXT(DSLSwitch, r, ctxInfo) {
     EXPECT_EQUAL(z, "switch (b) { default: case 0: case 1: }");
 
     {
-        ExpectError error(r, "error: duplicate case value\n");
+        ExpectError error(r, "error: duplicate case value '0'\n");
         Switch(0, Case(0), Case(0)).release();
     }
 
+    {
+        ExpectError error(r, "error: duplicate default case\n");
+        Switch(0, Default(a = 0), Default(a = 1)).release();
+    }
+
     {
         ExpectError error(r, "error: case value must be a constant integer\n");
         Var b(kInt);

tests/sksl/errors/SwitchDuplicateCase.glsl

@@ -1,4 +1,4 @@
 ### Compilation failed:
 
-error: 5: duplicate case value
+error: 5: duplicate case value '0'
 1 error