Fix assertion when arrays are double-declared.

Multi-dimensional arrays aren't legal in GLSL/SkSL, so this should be
caught and flagged as an error. The parser now verifies that a
variable's type isn't an array-type before accepting a `[` token to
open an array on the variable name.

This CL also refactors the IR generator's `convertArraySize` method to
make sure that various checks are made for all callers. Originally this
restructuring was used to verify array multi-dimensionality, but that
didn't detect errors inside struct declarations (which get no error
checking inside the IR generator) so the IR generator updates no longer
need to check the array dimensions.

Bug: skia:11322
Change-Id: Id33f4bdfb544019ddf995a8196c3c09cfe5a4525
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/369916
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>

gn/sksl_tests.gni

@@ -70,8 +70,21 @@ sksl_error_tests = [
   "/sksl/errors/ArgumentModifiers.sksl",
   "/sksl/errors/ArrayConstructorElementCount.sksl",
   "/sksl/errors/ArrayIndexOutOfRange.sksl",
+  "/sksl/errors/ArrayOfVoid.sksl",
+  "/sksl/errors/ArrayOfVoidInStruct.sksl",
   "/sksl/errors/ArrayReturnTypes.sksl",
+  "/sksl/errors/ArraySplitDimensions.sksl",
+  "/sksl/errors/ArraySplitDimensionsInFuncBody.sksl",
+  "/sksl/errors/ArraySplitDimensionsInFuncDecl.sksl",
+  "/sksl/errors/ArraySplitDimensionsInStruct.sksl",
   "/sksl/errors/ArrayTooManyDimensions.sksl",
+  "/sksl/errors/ArrayTooManyDimensionsInFuncBody.sksl",
+  "/sksl/errors/ArrayTooManyDimensionsInFuncDecl.sksl",
+  "/sksl/errors/ArrayTooManyDimensionsInStruct.sksl",
+  "/sksl/errors/ArrayTypeTooManyDimensions.sksl",
+  "/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.sksl",
+  "/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.sksl",
+  "/sksl/errors/ArrayTypeTooManyDimensionsInStruct.sksl",
   "/sksl/errors/ArrayUnspecifiedDimensions.sksl",
   "/sksl/errors/AssignmentTypeMismatch.sksl",
   "/sksl/errors/BadCaps.sksl",

resources/sksl/errors/ArrayOfVoid.sksl

@@ -0,0 +1,10 @@
+// Expect 7 errors
+
+void a[2];
+void[2] b;
+
+void funcD(void d[2]) {}
+void funcE(void[2] e) {}
+void[2] funcF() {}
+void funcG() { void g[2]; }
+void funcH() { void[2] h; }

resources/sksl/errors/ArrayOfVoidInStruct.sksl

@@ -0,0 +1,7 @@
+// Expect 3 errors
+
+struct S {
+    void a[2];
+    void[2] b;
+    void[2] c[2];
+};

resources/sksl/errors/ArraySplitDimensions.sksl

@@ -0,0 +1 @@
+float[2] x[2];

resources/sksl/errors/ArraySplitDimensionsInFuncBody.sksl

@@ -0,0 +1 @@
+void func() { float[2] x[2]; }

resources/sksl/errors/ArraySplitDimensionsInFuncDecl.sksl

@@ -0,0 +1 @@
+void func(float[2] x[2]) {}

resources/sksl/errors/ArraySplitDimensionsInStruct.sksl

@@ -0,0 +1 @@
+struct S { float[2] x[2]; };  // TODO(skia:11322): flag this as an error

resources/sksl/errors/ArrayTooManyDimensions.sksl

@@ -1 +1 @@
-in int ar[2][4];
+float x[2][2];

resources/sksl/errors/ArrayTooManyDimensionsInFuncBody.sksl

@@ -0,0 +1 @@
+void main() { float x[2][2]; }

resources/sksl/errors/ArrayTooManyDimensionsInFuncDecl.sksl

@@ -0,0 +1 @@
+void func(float x[2][2]) {}

resources/sksl/errors/ArrayTooManyDimensionsInStruct.sksl

@@ -0,0 +1 @@
+struct S { float x[2][2]; };

resources/sksl/errors/ArrayTypeTooManyDimensions.sksl

@@ -0,0 +1 @@
+float[2][2] x;

resources/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.sksl

@@ -0,0 +1 @@
+void main() { float[2][2] x; }

resources/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.sksl

@@ -0,0 +1 @@
+void func(float[2][2] x) {}

resources/sksl/errors/ArrayTypeTooManyDimensionsInStruct.sksl

@@ -0,0 +1 @@
+struct S { float[2][2] x; };

src/sksl/SkSLIRGenerator.cpp

@@ -267,7 +267,7 @@ std::unique_ptr<Statement> IRGenerator::convertVarDeclarationStatement(const AST
     }
 }
 
-int IRGenerator::convertArraySize(int offset, const ASTNode& s) {
+int IRGenerator::convertArraySize(const Type& type, int offset, const ASTNode& s) {
     if (!s) {
         this->errorReporter().error(offset, "array must have a size");
         return 0;
@@ -276,14 +276,23 @@ int IRGenerator::convertArraySize(int offset, const ASTNode& s) {
     if (!size) {
         return 0;
     }
-    return this->convertArraySize(std::move(size));
+    return this->convertArraySize(type, std::move(size));
 }
 
-int IRGenerator::convertArraySize(std::unique_ptr<Expression> size) {
+int IRGenerator::convertArraySize(const Type& type, std::unique_ptr<Expression> size) {
     size = this->coerce(std::move(size), *fContext.fTypes.fInt);
     if (!size) {
         return 0;
     }
+    if (type == *fContext.fTypes.fVoid) {
+        this->errorReporter().error(size->fOffset, "type 'void' may not be used in an array");
+        return 0;
+    }
+    if (type.isOpaque()) {
+        this->errorReporter().error(
+                size->fOffset, "opaque type '" + type.name() + "' may not be used in an array");
+        return 0;
+    }
     if (!size->is<IntLiteral>()) {
         this->errorReporter().error(size->fOffset, "array size must be an integer");
         return 0;
@@ -421,13 +430,11 @@ std::unique_ptr<Statement> IRGenerator::convertVarDeclaration(int offset,
     const Type* type = baseType;
     int arraySizeValue = 0;
     if (isArray) {
-        if (type->isOpaque()) {
-            this->errorReporter().error(
-                    offset,
-                    "opaque type '" + type->name() + "' may not be used in an array");
-        }
         SkASSERT(arraySize);
-        arraySizeValue = this->convertArraySize(std::move(arraySize));
+        arraySizeValue = this->convertArraySize(*type, std::move(arraySize));
+        if (!arraySizeValue) {
+            return {};
+        }
         type = fSymbolTable->addArrayDimension(type, arraySizeValue);
     }
     auto var = std::make_unique<Variable>(offset, fModifiers->addToPool(modifiers),
@@ -1145,7 +1152,7 @@ void IRGenerator::convertFunction(const ASTNode& f) {
             return;
         }
         if (pd.fIsArray) {
-            int arraySize = this->convertArraySize(param.fOffset, *paramIter++);
+            int arraySize = this->convertArraySize(*type, param.fOffset, *paramIter++);
             if (!arraySize) {
                 return;
             }
@@ -1405,7 +1412,7 @@ std::unique_ptr<InterfaceBlock> IRGenerator::convertInterfaceBlock(const ASTNode
             // convertArraySize rejects unsized arrays. This is the one place we allow those, but
             // we've already checked for that, so this is verifying the other aspects (constant,
             // positive, not too large).
-            arraySize = this->convertArraySize(size.fOffset, size);
+            arraySize = this->convertArraySize(*type, size.fOffset, size);
             if (!arraySize) {
                 return nullptr;
             }
@@ -1531,30 +1538,18 @@ const Type* IRGenerator::convertType(const ASTNode& type, bool allowVoid) {
     }
     const Type* result = &symbol->as<Type>();
     const bool isArray = (type.begin() != type.end());
-    if (*result == *fContext.fTypes.fVoid) {
-        if (!allowVoid) {
-            this->errorReporter().error(type.fOffset,
-                                        "type '" + name + "' not allowed in this context");
-            return nullptr;
-        }
-        if (isArray) {
-            this->errorReporter().error(type.fOffset,
-                                        "type '" + name + "' may not be used in an array");
-            return nullptr;
-        }
+    if (*result == *fContext.fTypes.fVoid && !allowVoid) {
+        this->errorReporter().error(type.fOffset,
+                                    "type '" + name + "' not allowed in this context");
+        return nullptr;
     }
     if (!fIsBuiltinCode && this->typeContainsPrivateFields(*result)) {
         this->errorReporter().error(type.fOffset, "type '" + name + "' is private");
         return nullptr;
     }
-    if (isArray && result->isOpaque()) {
-        this->errorReporter().error(type.fOffset,
-                                    "opaque type '" + name + "' may not be used in an array");
-        return nullptr;
-    }
     if (isArray) {
         auto iter = type.begin();
-        int arraySize = this->convertArraySize(type.fOffset, *iter);
+        int arraySize = this->convertArraySize(*result, type.fOffset, *iter);
         if (!arraySize) {
             return nullptr;
         }
@@ -2802,11 +2797,11 @@ std::unique_ptr<Expression> IRGenerator::convertIndexExpression(const ASTNode& i
             this->errorReporter().error(index.fOffset, "array must have a size");
             return nullptr;
         }
-        int arraySize = this->convertArraySize(index.fOffset, *iter);
+        const Type* type = &base->as<TypeReference>().value();
+        int arraySize = this->convertArraySize(*type, index.fOffset, *iter);
         if (!arraySize) {
             return nullptr;
         }
-        const Type* type = &base->as<TypeReference>().value();
         type = fSymbolTable->addArrayDimension(type, arraySize);
         return std::make_unique<TypeReference>(fContext, base->fOffset, type);
     }

src/sksl/SkSLIRGenerator.h

@@ -182,8 +182,8 @@ private:
                           const ExpressionArray& arguments);
     std::unique_ptr<Expression> coerce(std::unique_ptr<Expression> expr, const Type& type);
     CoercionCost coercionCost(const Expression& expr, const Type& type);
-    int convertArraySize(int offset, const ASTNode& s);
-    int convertArraySize(std::unique_ptr<Expression> s);
+    int convertArraySize(const Type& type, int offset, const ASTNode& s);
+    int convertArraySize(const Type& type, std::unique_ptr<Expression> s);
     bool containsConstantZero(Expression& expr);
     bool dividesByZero(Token::Kind op, Expression& right);
     std::unique_ptr<Expression> convertBinaryExpression(std::unique_ptr<Expression> left,

src/sksl/SkSLParser.cpp

@@ -313,6 +313,12 @@ bool Parser::isType(StringFragment name) {
     return s && s->is<Type>();
 }
 
+bool Parser::isArrayType(ASTNode::ID type) {
+    const ASTNode& node = this->getNode(type);
+    SkASSERT(node.fKind == ASTNode::Kind::kType);
+    return node.begin() != node.end();
+}
+
 /* DIRECTIVE(#version) INT_LITERAL ("es" | "compatibility")? |
    DIRECTIVE(#extension) IDENTIFIER COLON IDENTIFIER */
 ASTNode::ID Parser::directive() {
@@ -667,9 +673,9 @@ ASTNode::ID Parser::varDeclarationEnd(Modifiers mods, ASTNode::ID type, StringFr
     this->addChild(result, this->createNode(offset, ASTNode::Kind::kModifiers, mods));
     getNode(result).addChild(type);
 
-    auto parseArrayDimensions = [this](ASTNode::ID currentVar, ASTNode::VarData* vd) -> bool {
+    auto parseArrayDimensions = [&](ASTNode::ID currentVar, ASTNode::VarData* vd) -> bool {
         while (this->checkNext(Token::Kind::TK_LBRACKET)) {
-            if (vd->fIsArray) {
+            if (vd->fIsArray || this->isArrayType(type)) {
                 this->error(this->peek(), "multi-dimensional arrays are not supported");
                 return false;
             }
@@ -753,7 +759,7 @@ ASTNode::ID Parser::parameter() {
     ASTNode::ParameterData pd(modifiers, this->text(name), 0);
     getNode(result).addChild(type);
     while (this->checkNext(Token::Kind::TK_LBRACKET)) {
-        if (pd.fIsArray) {
+        if (pd.fIsArray || this->isArrayType(type)) {
             this->error(this->peek(), "multi-dimensional arrays are not supported");
             return ASTNode::ID::Invalid();
         }

src/sksl/SkSLParser.h

@@ -152,6 +152,11 @@ private:
      */
     bool isType(StringFragment name);
 
+    /**
+     * Returns true if the passed-in ASTNode is an array type, or false if it is a non-arrayed type.
+     */
+    bool isArrayType(ASTNode::ID type);
+
     // The pointer to the node may be invalidated by modifying the fNodes vector
     ASTNode& getNode(ASTNode::ID id) {
         SkASSERT(id.fValue >= 0 && id.fValue < (int) fFile->fNodes.size());

tests/sksl/errors/ArrayOfVoid.glsl

@@ -0,0 +1,10 @@
+### Compilation failed:
+
+error: 3: type 'void' not allowed in this context
+error: 4: type 'void' not allowed in this context
+error: 6: type 'void' not allowed in this context
+error: 7: type 'void' not allowed in this context
+error: 8: type 'void' may not be used in an array
+error: 9: type 'void' not allowed in this context
+error: 10: type 'void' not allowed in this context
+7 errors

tests/sksl/errors/ArrayOfVoidInStruct.glsl

@@ -0,0 +1,6 @@
+### Compilation failed:
+
+error: 4: opaque type 'void' is not permitted in a struct
+error: 5: opaque type 'void' is not permitted in a struct
+error: 6: multi-dimensional arrays are not supported
+3 errors

tests/sksl/errors/ArraySplitDimensions.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArraySplitDimensionsInFuncBody.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArraySplitDimensionsInFuncDecl.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArraySplitDimensionsInStruct.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArrayTooManyDimensionsInFuncBody.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArrayTooManyDimensionsInFuncDecl.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArrayTooManyDimensionsInStruct.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArrayTypeTooManyDimensions.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArrayTypeTooManyDimensionsInFuncBody.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: expected ';', but found 'x'
+1 error

tests/sksl/errors/ArrayTypeTooManyDimensionsInFuncDecl.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error

tests/sksl/errors/ArrayTypeTooManyDimensionsInStruct.glsl

@@ -0,0 +1,4 @@
+### Compilation failed:
+
+error: 1: multi-dimensional arrays are not supported
+1 error