Improve DSL error reporting with invalid array sizes

This eliminates an extra "expected int literal" error when the
array size is an invalid expression.

Change-Id: Iaf5d15316df3ec5200d51d73c14d7e428ce17be9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/443236
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>

include/sksl/DSLExpression.h

@@ -124,13 +124,18 @@ public:
 
     /**
      * Returns true if this object contains an expression. DSLExpressions which were created with
-     * the empty constructor or which have already been release()ed are not valid. DSLExpressions
+     * the empty constructor or which have already been release()ed do not have a value.
-     * created with errors are still considered valid (but contain a poison value).
+     * DSLExpressions created with errors are still considered to have a value (but contain poison).
      */
-    bool valid() const {
+    bool hasValue() const {
         return fExpression != nullptr;
     }
 
+    /**
+     * Returns true if this object contains an expression which is not poison.
+     */
+    bool isValid() const;
+
     void swap(DSLExpression& other);
 
     /**
@@ -141,9 +146,9 @@ public:
 
 private:
     /**
-     * Calls release if this expression is valid, otherwise returns null.
+     * Calls release if this expression has a value, otherwise returns null.
      */
-    std::unique_ptr<SkSL::Expression> releaseIfValid();
+    std::unique_ptr<SkSL::Expression> releaseIfPossible();
 
     /**
      * Invalidates this object and returns the SkSL expression it represents coerced to the

include/sksl/DSLStatement.h

@@ -48,10 +48,10 @@ public:
 
     DSLStatement& operator=(DSLStatement&& other) = default;
 
-    bool valid() { return fStatement != nullptr; }
+    bool hasValue() { return fStatement != nullptr; }
 
     std::unique_ptr<SkSL::Statement> release() {
-        SkASSERT(this->valid());
+        SkASSERT(this->hasValue());
         return std::move(fStatement);
     }
 
@@ -60,7 +60,7 @@ private:
 
     DSLStatement(std::unique_ptr<SkSL::Expression> expr);
 
-    std::unique_ptr<SkSL::Statement> releaseIfValid() {
+    std::unique_ptr<SkSL::Statement> releaseIfPossible() {
         return std::move(fStatement);
     }
 
@@ -90,7 +90,7 @@ public:
 
     ~DSLPossibleStatement();
 
-    bool valid() { return fStatement != nullptr; }
+    bool hasValue() { return fStatement != nullptr; }
 
     std::unique_ptr<SkSL::Statement> release() {
         return DSLStatement(std::move(*this)).release();

src/sksl/SkSLDSLParser.cpp

@@ -336,6 +336,10 @@ static skstd::optional<DSLStatement> declaration_statements(SkTArray<DSLVar> var
     return Declare(vars);
 }
 
+static bool is_valid(const skstd::optional<DSLWrapper<DSLExpression>>& expr) {
+    return expr && expr->get().isValid();
+}
+
 SKSL_INT DSLParser::arraySize() {
     Token next = this->peek();
     if (next.fKind == Token::Kind::TK_INT_LITERAL) {
@@ -357,8 +361,10 @@ SKSL_INT DSLParser::arraySize() {
         this->error(next, "array size must be positive");
         return 1;
     } else {
-        this->expression();
+        skstd::optional<DSLWrapper<DSLExpression>> expr = this->expression();
+        if (is_valid(expr)) {
             this->error(next, "expected int literal");
+        }
         return 1;
     }
 }

src/sksl/dsl/DSLCore.cpp

@@ -185,15 +185,15 @@ public:
     static DSLPossibleStatement For(DSLStatement initializer, DSLExpression test,
                                     DSLExpression next, DSLStatement stmt, PositionInfo pos) {
         return ForStatement::Convert(DSLWriter::Context(), /*offset=*/-1,
-                                     initializer.releaseIfValid(), test.releaseIfValid(),
+                                     initializer.releaseIfPossible(), test.releaseIfPossible(),
-                                     next.releaseIfValid(), stmt.release(),
+                                     next.releaseIfPossible(), stmt.release(),
                                      DSLWriter::SymbolTable());
     }
 
     static DSLPossibleStatement If(DSLExpression test, DSLStatement ifTrue, DSLStatement ifFalse,
                                    bool isStatic) {
         return IfStatement::Convert(DSLWriter::Context(), /*offset=*/-1, isStatic, test.release(),
-                                    ifTrue.release(), ifFalse.releaseIfValid());
+                                    ifTrue.release(), ifFalse.releaseIfPossible());
     }
 
     static DSLGlobalVar InterfaceBlock(const DSLModifiers& modifiers, skstd::string_view typeName,
@@ -248,7 +248,7 @@ public:
         // this point we do not know the function's return type. We therefore do not check for
         // errors, or coerce the value to the correct type, until the return statement is actually
         // added to a function. (This is done in FunctionDefinition::Convert.)
-        return SkSL::ReturnStatement::Make(/*offset=*/-1, value.releaseIfValid());
+        return SkSL::ReturnStatement::Make(/*offset=*/-1, value.releaseIfPossible());
     }
 
     static DSLExpression Swizzle(DSLExpression base, SkSL::SwizzleComponent::Type a,
@@ -301,7 +301,7 @@ public:
         SkTArray<StatementArray> statements;
         statements.reserve_back(cases.count());
         for (DSLCase& c : cases) {
-            values.push_back(c.fValue.releaseIfValid());
+            values.push_back(c.fValue.releaseIfPossible());
             statements.push_back(std::move(c.fStatements));
         }
         return DSLWriter::ConvertSwitch(value.release(), std::move(values), std::move(statements),

src/sksl/dsl/DSLExpression.cpp

@@ -35,7 +35,7 @@ DSLExpression::DSLExpression(DSLExpression&& other)
 
 DSLExpression::DSLExpression(std::unique_ptr<SkSL::Expression> expression)
     : fExpression(std::move(expression)) {
-    SkASSERT(this->valid());
+    SkASSERT(this->hasValue());
     DSLWriter::ReportErrors();
 }
 
@@ -106,21 +106,25 @@ DSLExpression::~DSLExpression() {
               "ProgramSettings::fAssertDSLObjectsReleased)");
 }
 
+bool DSLExpression::isValid() const {
+    return this->hasValue() && !fExpression->is<SkSL::Poison>();
+}
+
 void DSLExpression::swap(DSLExpression& other) {
     std::swap(fExpression, other.fExpression);
 }
 
 std::unique_ptr<SkSL::Expression> DSLExpression::release() {
-    SkASSERT(this->valid());
+    SkASSERT(this->hasValue());
     return std::move(fExpression);
 }
 
-std::unique_ptr<SkSL::Expression> DSLExpression::releaseIfValid() {
+std::unique_ptr<SkSL::Expression> DSLExpression::releaseIfPossible() {
     return std::move(fExpression);
 }
 
 DSLType DSLExpression::type() {
-    if (!this->valid()) {
+    if (!this->hasValue()) {
         return kVoid_Type;
     }
     return &fExpression->type();

src/sksl/dsl/DSLFunction.cpp

@@ -39,7 +39,7 @@ void DSLFunction::init(DSLModifiers modifiers, const DSLType& returnType, skstd:
         if (param->fDeclared) {
             DSLWriter::ReportError("parameter has already been used in another function");
         }
-        SkASSERT(!param->fInitialValue.valid());
+        SkASSERT(!param->fInitialValue.hasValue());
         SkASSERT(!param->fDeclaration);
         param->fDeclared = true;
         std::unique_ptr<SkSL::Variable> paramVar = DSLWriter::CreateParameterVar(*param);

src/sksl/dsl/DSLStatement.cpp

@@ -37,12 +37,12 @@ DSLStatement::DSLStatement(DSLExpression expr) {
 
 DSLStatement::DSLStatement(std::unique_ptr<SkSL::Expression> expr)
     : fStatement(SkSL::ExpressionStatement::Make(DSLWriter::Context(), std::move(expr))) {
-    SkASSERT(this->valid());
+    SkASSERT(this->hasValue());
 }
 
 DSLStatement::DSLStatement(std::unique_ptr<SkSL::Statement> stmt)
     : fStatement(std::move(stmt)) {
-    SkASSERT(this->valid());
+    SkASSERT(this->hasValue());
 }
 
 DSLStatement::DSLStatement(DSLPossibleExpression expr, PositionInfo pos)
@@ -50,7 +50,7 @@ DSLStatement::DSLStatement(DSLPossibleExpression expr, PositionInfo pos)
 
 DSLStatement::DSLStatement(DSLPossibleStatement stmt, PositionInfo pos) {
     DSLWriter::ReportErrors(pos);
-    if (stmt.valid()) {
+    if (stmt.hasValue()) {
         fStatement = std::move(stmt.fStatement);
     } else {
         fStatement = SkSL::Nop::Make();

src/sksl/dsl/priv/DSLFPs.cpp

@@ -30,7 +30,7 @@ DSLGlobalVar sk_SampleCoord() {
 }
 
 DSLExpression SampleChild(int index, DSLExpression sampleExpr) {
-    std::unique_ptr<SkSL::Expression> expr = sampleExpr.releaseIfValid();
+    std::unique_ptr<SkSL::Expression> expr = sampleExpr.releaseIfPossible();
     if (expr) {
         SkASSERT(expr->type().isVector());
         SkASSERT(expr->type().componentType().isFloat());

src/sksl/dsl/priv/DSLWriter.cpp

@@ -160,7 +160,7 @@ DSLPossibleExpression DSLWriter::Construct(const SkSL::Type& type, SkSpan<DSLExp
     args.reserve_back(rawArgs.size());
 
     for (DSLExpression& arg : rawArgs) {
-        if (!arg.valid()) {
+        if (!arg.hasValue()) {
             return DSLPossibleExpression(nullptr);
         }
         args.push_back(arg.release());
@@ -268,7 +268,7 @@ const SkSL::Variable* DSLWriter::Var(DSLVarBase& var) {
             // of DSLParser we don't even need DSL variables to show up in the symbol table in the
             // first place.
             var.fDeclaration = DSLWriter::IRGenerator().convertVarDeclaration(
-                    std::move(skslvar), var.fInitialValue.releaseIfValid(),
+                    std::move(skslvar), var.fInitialValue.releaseIfPossible(),
                     /*addToSymbolTable=*/false);
             if (var.fDeclaration) {
                 var.fVar = varPtr;
@@ -294,7 +294,7 @@ std::unique_ptr<SkSL::Statement> DSLWriter::Declaration(DSLVarBase& var) {
     if (!var.fDeclaration) {
         // We should have already reported an error before ending up here, just clean up the
         // initial value so it doesn't assert and return a nop.
-        var.fInitialValue.releaseIfValid();
+        var.fInitialValue.releaseIfPossible();
         return SkSL::Nop::Make();
     }
     return std::move(var.fDeclaration);