validation: Reject struct builtins on wrong stages

Change-Id: I047f28c5be6a7d878b9f7ad37b6accc842b4fe98
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/65840
Auto-Submit: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>

src/resolver/builtins_validation_test.cc

@@ -217,7 +217,7 @@ TEST_F(ResolverBuiltinsValidationTest, FragDepthIsInput_Fail) {
             "fragment pipeline stage");
 }
 
-TEST_F(ResolverBuiltinsValidationTest, FragDepthIsInputStruct_Ignored) {
+TEST_F(ResolverBuiltinsValidationTest, FragDepthIsInputStruct_Fail) {
   // struct MyInputs {
   //   [[builtin(frag_depth)]] ff: f32;
   // };
@@ -231,8 +231,28 @@ TEST_F(ResolverBuiltinsValidationTest, FragDepthIsInputStruct_Ignored) {
 
   Func("fragShader", {Param("arg", ty.Of(s))}, ty.f32(), {Return(1.0f)},
        {Stage(ast::PipelineStage::kFragment)}, {Location(0)});
+  EXPECT_FALSE(r()->Resolve());
+  EXPECT_EQ(r()->error(),
+            "12:34 error: builtin(frag_depth) cannot be used in input of "
+            "fragment pipeline stage\n"
+            "note: while analysing entry point 'fragShader'");
+}
+
+TEST_F(ResolverBuiltinsValidationTest, StructBuiltinInsideEntryPoint_Ignored) {
+  // struct S {
+  //   [[builtin(vertex_index)]] idx: u32;
+  // };
+  // [[stage(fragment)]]
+  // fn fragShader() { var s : S; }
+
+  Structure("S",
+            {Member("idx", ty.u32(), {Builtin(ast::Builtin::kVertexIndex)})});
+
+  Func("fragShader", {}, ty.void_(), {Decl(Var("s", ty.type_name("S")))},
+       {Stage(ast::PipelineStage::kFragment)});
   EXPECT_TRUE(r()->Resolve());
 }
+
 }  // namespace StageTest
 
 TEST_F(ResolverBuiltinsValidationTest, PositionNotF32_Struct_Fail) {

src/resolver/resolver.cc

@@ -1277,8 +1277,7 @@ bool Resolver::ValidateFunctionParameter(const ast::Function* func,
 
 bool Resolver::ValidateBuiltinDecoration(const ast::BuiltinDecoration* deco,
                                          const sem::Type* storage_type,
-                                         const bool is_input,
+                                         const bool is_input) {
-                                         const bool is_struct_member) {
   auto* type = storage_type->UnwrapRef();
   const auto stage = current_function_
                          ? current_function_->declaration->pipeline_stage()
@@ -1386,16 +1385,12 @@ bool Resolver::ValidateBuiltinDecoration(const ast::BuiltinDecoration* deco,
       break;
   }
 
-  // ignore builtin attribute on struct members to facillate data movement
+  if (is_stage_mismatch) {
-  // between stages
+    AddError(deco_to_str(deco) + " cannot be used in " +
-  if (!is_struct_member) {
+                 (is_input ? "input of " : "output of ") + stage_name.str() +
-    if (is_stage_mismatch) {
+                 " pipeline stage",
-      AddError(deco_to_str(deco) + " cannot be used in " +
+             deco->source());
-                   (is_input ? "input of " : "output of ") + stage_name.str() +
+    return false;
-                   " pipeline stage",
-               deco->source());
-      return false;
-    }
   }
 
   return true;
@@ -1556,10 +1551,9 @@ bool Resolver::ValidateEntryPoint(const ast::Function* func,
               return false;
             }
 
-            if (!ValidateBuiltinDecoration(
+            if (!ValidateBuiltinDecoration(builtin, ty,
-                    builtin, ty,
+                                           /* is_input */ param_or_ret ==
-                    /* is_input */ param_or_ret == ParamOrRetType::kParameter,
+                                               ParamOrRetType::kParameter)) {
-                    /* is_struct_member */ is_struct_member)) {
               return false;
             }
             builtins.emplace(builtin->value());
@@ -4098,8 +4092,7 @@ bool Resolver::ValidateStructure(const sem::Struct* str) {
         }
       } else if (auto* builtin = deco->As<ast::BuiltinDecoration>()) {
         if (!ValidateBuiltinDecoration(builtin, member->Type(),
-                                       /* is_input */ false,
+                                       /* is_input */ false)) {
-                                       /* is_struct_member */ true)) {
           return false;
         }
         if (builtin->value() == ast::Builtin::kPosition) {

src/resolver/resolver.h

@@ -283,8 +283,7 @@ class Resolver {
   bool ValidateAssignment(const ast::AssignmentStatement* a);
   bool ValidateBuiltinDecoration(const ast::BuiltinDecoration* deco,
                                  const sem::Type* storage_type,
-                                 const bool is_input,
+                                 const bool is_input);
-                                 const bool is_struct_member);
   bool ValidateCall(ast::CallExpression* call);
   bool ValidateCallStatement(ast::CallStatement* stmt);
   bool ValidateEntryPoint(const ast::Function* func, const FunctionInfo* info);

src/resolver/struct_pipeline_stage_use_test.cc

@@ -140,8 +140,8 @@ TEST_F(ResolverPipelineStageUseTest, StructUsedMultipleStages) {
   auto* s = Structure(
       "S", {Member("a", ty.vec4<f32>(), {Builtin(ast::Builtin::kPosition)})});
 
-  Func("vert_main", {Param("param", ty.Of(s))}, ty.Of(s),
+  Func("vert_main", {}, ty.Of(s), {Return(Construct(ty.Of(s)))},
-       {Return(Construct(ty.Of(s)))}, {Stage(ast::PipelineStage::kVertex)});
+       {Stage(ast::PipelineStage::kVertex)});
 
   Func("frag_main", {Param("param", ty.Of(s))}, ty.void_(), {},
        {Stage(ast::PipelineStage::kFragment)});
@@ -151,8 +151,7 @@ TEST_F(ResolverPipelineStageUseTest, StructUsedMultipleStages) {
   auto* sem = TypeOf(s)->As<sem::Struct>();
   ASSERT_NE(sem, nullptr);
   EXPECT_THAT(sem->PipelineStageUses(),
-              UnorderedElementsAre(sem::PipelineStageUsage::kVertexInput,
+              UnorderedElementsAre(sem::PipelineStageUsage::kVertexOutput,
-                                   sem::PipelineStageUsage::kVertexOutput,
                                    sem::PipelineStageUsage::kFragmentInput));
 }