SkSL now detects modifiers used in incorrect contexts

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

gm/runtimecolorfilter.cpp

@@ -44,6 +44,7 @@ DEF_SIMPLE_GM(runtimecolorfilter, canvas, 256 * 3, 256) {
 
     for (auto src : { gLumaSrc, gLumaSrcWithCoords }) {
         sk_sp<SkRuntimeEffect> effect = std::get<0>(SkRuntimeEffect::Make(SkString(src)));
+        SkASSERT(effect);
         SkPaint p;
         p.setColorFilter(effect->makeColorFilter(nullptr));
         canvas->translate(256, 0);

gm/vertices.cpp

@@ -306,6 +306,9 @@ DEF_SIMPLE_GM(vertices_data, canvas, 512, 256) {
             }
         )";
         auto[effect, errorText] = SkRuntimeEffect::Make(SkString(gProg));
+        if (!effect) {
+            SK_ABORT("RuntimeEffect error: %s\n", errorText.c_str());
+        }
         paint.setShader(effect->makeShader(nullptr, nullptr, 0, nullptr, true));
         canvas->drawVertices(builder.detach(), paint);
         canvas->translate(r.width(), 0);

src/sksl/SkSLIRGenerator.cpp

@@ -377,6 +377,17 @@ std::unique_ptr<VarDeclarations> IRGenerator::convertVarDeclarations(const ASTNo
             fErrors.error(decls.fOffset, "'varying' must be float scalar or vector");
         }
     }
+    int permitted = Modifiers::kConst_Flag;
+    if (storage == Variable::kGlobal_Storage) {
+        permitted |= Modifiers::kIn_Flag | Modifiers::kOut_Flag | Modifiers::kUniform_Flag |
+                     Modifiers::kFlat_Flag | Modifiers::kVarying_Flag |
+                     Modifiers::kNoPerspective_Flag | Modifiers::kPLS_Flag |
+                     Modifiers::kPLSIn_Flag | Modifiers::kPLSOut_Flag |
+                     Modifiers::kRestrict_Flag | Modifiers::kVolatile_Flag |
+                     Modifiers::kReadOnly_Flag | Modifiers::kWriteOnly_Flag |
+                     Modifiers::kCoherent_Flag | Modifiers::kBuffer_Flag;
+    }
+    this->checkModifiers(decls.fOffset, modifiers, permitted);
     for (; iter != decls.end(); ++iter) {
         const ASTNode& varDecl = *iter;
         if (modifiers.fLayout.fLocation == 0 && modifiers.fLayout.fIndex == 0 &&
@@ -836,6 +847,36 @@ private:
 
 template <typename T> AutoClear(T* c) -> AutoClear<T>;
 
+void IRGenerator::checkModifiers(int offset, const Modifiers& modifiers, int permitted) {
+    int flags = modifiers.fFlags;
+    #define CHECK(flag, name)                                              \
+        if (!flags) return;                                                \
+        if (flags & flag) {                                                \
+            if (!(permitted & flag)) {                                     \
+                fErrors.error(offset, "'" name "' is not permitted here"); \
+            }                                                              \
+            flags &= ~flag;                                                \
+        }
+    CHECK(Modifiers::kConst_Flag,          "const")
+    CHECK(Modifiers::kIn_Flag,             "in")
+    CHECK(Modifiers::kOut_Flag,            "out")
+    CHECK(Modifiers::kUniform_Flag,        "uniform")
+    CHECK(Modifiers::kFlat_Flag,           "flat")
+    CHECK(Modifiers::kNoPerspective_Flag,  "noperspective")
+    CHECK(Modifiers::kReadOnly_Flag,       "readonly")
+    CHECK(Modifiers::kWriteOnly_Flag,      "writeonly")
+    CHECK(Modifiers::kCoherent_Flag,       "coherent")
+    CHECK(Modifiers::kVolatile_Flag,       "volatile")
+    CHECK(Modifiers::kRestrict_Flag,       "restrict")
+    CHECK(Modifiers::kBuffer_Flag,         "buffer")
+    CHECK(Modifiers::kHasSideEffects_Flag, "sk_has_side_effects")
+    CHECK(Modifiers::kPLS_Flag,            "__pixel_localEXT")
+    CHECK(Modifiers::kPLSIn_Flag,          "__pixel_local_inEXT")
+    CHECK(Modifiers::kPLSOut_Flag,         "__pixel_local_outEXT")
+    CHECK(Modifiers::kVarying_Flag,        "varying")
+    SkASSERT(flags == 0);
+}
+
 void IRGenerator::convertFunction(const ASTNode& f) {
     AutoClear clear(&fReferencedIntrinsics);
     auto iter = f.begin();
@@ -859,11 +900,14 @@ void IRGenerator::convertFunction(const ASTNode& f) {
         return;
     }
     const ASTNode::FunctionData& fd = f.getFunctionData();
+    this->checkModifiers(f.fOffset, fd.fModifiers, Modifiers::kHasSideEffects_Flag);
     std::vector<const Variable*> parameters;
     for (size_t i = 0; i < fd.fParameterCount; ++i) {
         const ASTNode& param = *(iter++);
         SkASSERT(param.fKind == ASTNode::Kind::kParameter);
         ASTNode::ParameterData pd = param.getParameterData();
+        this->checkModifiers(param.fOffset, pd.fModifiers, Modifiers::kIn_Flag |
+                                                           Modifiers::kOut_Flag);
         auto paramIter = param.begin();
         const Type* type = this->convertType(*(paramIter++));
         if (!type) {
@@ -1082,14 +1126,6 @@ std::unique_ptr<InterfaceBlock> IRGenerator::convertInterfaceBlock(const ASTNode
                 fErrors.error(decl->fOffset,
                               "initializers are not permitted on interface block fields");
             }
-            if (vd.fVar->fModifiers.fFlags & (Modifiers::kIn_Flag |
-                                              Modifiers::kOut_Flag |
-                                              Modifiers::kUniform_Flag |
-                                              Modifiers::kBuffer_Flag |
-                                              Modifiers::kConst_Flag)) {
-                fErrors.error(decl->fOffset,
-                              "interface block fields may not have storage qualifiers");
-            }
             if (vd.fVar->fType.kind() == Type::kArray_Kind &&
                 vd.fVar->fType.columns() == -1) {
                 haveRuntimeArray = true;

src/sksl/SkSLIRGenerator.h

@@ -80,6 +80,7 @@ private:
     void pushSymbolTable();
     void popSymbolTable();
 
+    void checkModifiers(int offset, const Modifiers& modifiers, int permitted);
     std::unique_ptr<VarDeclarations> convertVarDeclarations(const ASTNode& decl,
                                                             Variable::Storage storage);
     void convertFunction(const ASTNode& f);

tests/SkSLErrorTest.cpp

@@ -360,7 +360,7 @@ DEF_TEST(SkSLTernaryMismatch, r) {
 DEF_TEST(SkSLInterfaceBlockStorageModifiers, r) {
     test_failure(r,
                  "uniform foo { out int x; };",
-                 "error: 1: interface block fields may not have storage qualifiers\n1 error\n");
+                 "error: 1: 'out' is not permitted here\n1 error\n");
 }
 
 DEF_TEST(SkSLUseWithoutInitialize, r) {
@@ -605,3 +605,55 @@ DEF_TEST(SkSLMustBeConstantIntegralEnum, r) {
     test_success(r,
                  "enum class E { a = 1 }; void main() {}");
 }
+
+DEF_TEST(SkSLBadModifiers, r) {
+    test_failure(r,
+                 "const in out uniform flat noperspective readonly writeonly coherent volatile "
+                 "restrict buffer sk_has_side_effects __pixel_localEXT __pixel_local_inEXT "
+                 "__pixel_local_outEXT varying void main() {}",
+                 "error: 1: 'const' is not permitted here\n"
+                 "error: 1: 'in' is not permitted here\n"
+                 "error: 1: 'out' is not permitted here\n"
+                 "error: 1: 'uniform' is not permitted here\n"
+                 "error: 1: 'flat' is not permitted here\n"
+                 "error: 1: 'noperspective' is not permitted here\n"
+                 "error: 1: 'readonly' is not permitted here\n"
+                 "error: 1: 'writeonly' is not permitted here\n"
+                 "error: 1: 'coherent' is not permitted here\n"
+                 "error: 1: 'volatile' is not permitted here\n"
+                 "error: 1: 'restrict' is not permitted here\n"
+                 "error: 1: 'buffer' is not permitted here\n"
+                 "error: 1: '__pixel_localEXT' is not permitted here\n"
+                 "error: 1: '__pixel_local_inEXT' is not permitted here\n"
+                 "error: 1: '__pixel_local_outEXT' is not permitted here\n"
+                 "error: 1: 'varying' is not permitted here\n"
+                 "16 errors\n");
+    test_failure(r,
+                 "void test(const in out uniform flat noperspective readonly writeonly coherent "
+                 "volatile restrict buffer sk_has_side_effects __pixel_localEXT "
+                 "__pixel_local_inEXT __pixel_local_outEXT varying float test) {}",
+                 "error: 1: 'const' is not permitted here\n"
+                 "error: 1: 'uniform' is not permitted here\n"
+                 "error: 1: 'flat' is not permitted here\n"
+                 "error: 1: 'noperspective' is not permitted here\n"
+                 "error: 1: 'readonly' is not permitted here\n"
+                 "error: 1: 'writeonly' is not permitted here\n"
+                 "error: 1: 'coherent' is not permitted here\n"
+                 "error: 1: 'volatile' is not permitted here\n"
+                 "error: 1: 'restrict' is not permitted here\n"
+                 "error: 1: 'buffer' is not permitted here\n"
+                 "error: 1: 'sk_has_side_effects' is not permitted here\n"
+                 "error: 1: '__pixel_localEXT' is not permitted here\n"
+                 "error: 1: '__pixel_local_inEXT' is not permitted here\n"
+                 "error: 1: '__pixel_local_outEXT' is not permitted here\n"
+                 "error: 1: 'varying' is not permitted here\n"
+                 "15 errors\n");
+    test_failure(r,
+                 "const in out uniform flat noperspective readonly writeonly coherent volatile "
+                 "restrict buffer sk_has_side_effects __pixel_localEXT "
+                 "__pixel_local_inEXT __pixel_local_outEXT varying float test;",
+                 "error: 1: 'in uniform' variables only permitted within fragment processors\n"
+                 "error: 1: 'varying' is only permitted in runtime effects\n"
+                 "error: 1: 'sk_has_side_effects' is not permitted here\n"
+                 "3 errors\n");
+}