[type-determiner] Fixup handling of unknown identifiers.

This Cl updates the identifier type determination check to fail if the
identifier is not found and is not an intrinsic method.

Bug: tint:139
Change-Id: I332dd7fb42dae62bdee459c4a8819bdb5685c903
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/30081
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Reviewed-by: Sarah Mashayekhi <sarahmashay@google.com>

src/type_determiner.cc

@@ -369,7 +369,6 @@ bool TypeDeterminer::DetermineBitcast(ast::BitcastExpression* expr) {
   if (!DetermineResultType(expr->expr())) {
     return false;
   }
-
   expr->set_result_type(expr->type());
   return true;
 }
@@ -787,12 +786,15 @@ bool TypeDeterminer::DetermineIdentifier(ast::IdentifierExpression* expr) {
     return true;
   }
 
-  SetIntrinsicIfNeeded(expr);
+  if (!SetIntrinsicIfNeeded(expr)) {
-
+    set_error(expr->source(),
+              "v-0006: identifier must be declared before use: " + name);
+    return false;
+  }
   return true;
 }
 
-void TypeDeterminer::SetIntrinsicIfNeeded(ast::IdentifierExpression* ident) {
+bool TypeDeterminer::SetIntrinsicIfNeeded(ast::IdentifierExpression* ident) {
   if (ident->name() == "abs") {
     ident->set_intrinsic(ast::Intrinsic::kAbs);
   } else if (ident->name() == "acos") {
@@ -927,7 +929,10 @@ void TypeDeterminer::SetIntrinsicIfNeeded(ast::IdentifierExpression* ident) {
     ident->set_intrinsic(ast::Intrinsic::kTextureSampleLevel);
   } else if (ident->name() == "trunc") {
     ident->set_intrinsic(ast::Intrinsic::kTrunc);
+  } else {
+    return false;
   }
+  return true;
 }
 
 bool TypeDeterminer::DetermineMemberAccessor(

src/type_determiner.h

@@ -108,7 +108,8 @@ class TypeDeterminer {
 
   /// Sets the intrinsic data information for the identifier if needed
   /// @param ident the identifier expression
-  void SetIntrinsicIfNeeded(ast::IdentifierExpression* ident);
+  /// @returns true if an intrinsic was set
+  bool SetIntrinsicIfNeeded(ast::IdentifierExpression* ident);
 
  private:
   void set_error(const Source& src, const std::string& msg);

src/type_determiner_test.cc

@@ -400,7 +400,7 @@ TEST_F(TypeDeterminerTest, Stmt_Call_undeclared) {
   ast::type::F32Type f32;
   ast::ExpressionList call_params;
   auto call_expr = std::make_unique<ast::CallExpression>(
-      Source{12, 34}, std::make_unique<ast::IdentifierExpression>("func"),
+      std::make_unique<ast::IdentifierExpression>(Source{12, 34}, "func"),
       std::move(call_params));
   ast::VariableList params0;
   auto func_main =
@@ -419,7 +419,7 @@ TEST_F(TypeDeterminerTest, Stmt_Call_undeclared) {
 
   EXPECT_FALSE(td()->Determine()) << td()->error();
   EXPECT_EQ(td()->error(),
-            "12:34: v-0005: function must be declared before use: 'func'");
+            "12:34: v-0006: identifier must be declared before use: func");
 }
 
 TEST_F(TypeDeterminerTest, Stmt_VariableDecl) {
@@ -615,6 +615,9 @@ TEST_F(TypeDeterminerTest, Expr_Bitcast) {
   ast::BitcastExpression bitcast(
       &f32, std::make_unique<ast::IdentifierExpression>("name"));
 
+  ast::Variable v("name", ast::StorageClass::kPrivate, &f32);
+  td()->RegisterVariableForTesting(&v);
+
   EXPECT_TRUE(td()->DetermineResultType(&bitcast));
   ASSERT_NE(bitcast.result_type(), nullptr);
   EXPECT_TRUE(bitcast.result_type()->IsF32());
@@ -688,9 +691,11 @@ TEST_F(TypeDeterminerTest, Expr_Cast) {
 
   ast::ExpressionList params;
   params.push_back(std::make_unique<ast::IdentifierExpression>("name"));
-
   ast::TypeConstructorExpression cast(&f32, std::move(params));
 
+  ast::Variable v("name", ast::StorageClass::kPrivate, &f32);
+  td()->RegisterVariableForTesting(&v);
+
   EXPECT_TRUE(td()->DetermineResultType(&cast));
   ASSERT_NE(cast.result_type(), nullptr);
   EXPECT_TRUE(cast.result_type()->IsF32());
@@ -852,6 +857,11 @@ TEST_F(TypeDeterminerTest, Expr_Identifier_Function) {
   EXPECT_TRUE(ident.result_type()->IsF32());
 }
 
+TEST_F(TypeDeterminerTest, Expr_Identifier_Unknown) {
+  ast::IdentifierExpression a("a");
+  EXPECT_FALSE(td()->DetermineResultType(&a));
+}
+
 TEST_F(TypeDeterminerTest, Function_RegisterInputOutputVariables) {
   ast::type::F32Type f32;
 
@@ -1005,8 +1015,11 @@ TEST_F(TypeDeterminerTest, Function_NotRegisterFunctionVariable) {
 
   mod()->AddFunction(std::move(func));
 
+  ast::Variable v("var", ast::StorageClass::kFunction, &f32);
+  td()->RegisterVariableForTesting(&v);
+
   // Register the function
-  EXPECT_TRUE(td()->Determine());
+  EXPECT_TRUE(td()->Determine()) << td()->error();
 
   EXPECT_EQ(func_ptr->referenced_module_variables().size(), 0u);
 }
@@ -2478,7 +2491,7 @@ TEST_P(IntrinsicDataTest, Lookup) {
   auto param = GetParam();
 
   ast::IdentifierExpression ident(param.name);
-  td()->SetIntrinsicIfNeeded(&ident);
+  EXPECT_TRUE(td()->SetIntrinsicIfNeeded(&ident));
   EXPECT_EQ(ident.intrinsic(), param.intrinsic);
   EXPECT_TRUE(ident.IsIntrinsic());
 }
@@ -2558,7 +2571,7 @@ INSTANTIATE_TEST_SUITE_P(
 
 TEST_F(TypeDeterminerTest, IntrinsicNotSetIfNotMatched) {
   ast::IdentifierExpression ident("not_intrinsic");
-  td()->SetIntrinsicIfNeeded(&ident);
+  EXPECT_FALSE(td()->SetIntrinsicIfNeeded(&ident));
   EXPECT_EQ(ident.intrinsic(), ast::Intrinsic::kNone);
   EXPECT_FALSE(ident.IsIntrinsic());
 }
@@ -4726,6 +4739,17 @@ TEST_F(TypeDeterminerTest, Function_EntryPoints_StageDecoration) {
   mod()->AddFunction(std::move(ep_1));
   mod()->AddFunction(std::move(ep_2));
 
+  mod()->AddGlobalVariable(std::make_unique<ast::Variable>(
+      "first", ast::StorageClass::kPrivate, &f32));
+  mod()->AddGlobalVariable(std::make_unique<ast::Variable>(
+      "second", ast::StorageClass::kPrivate, &f32));
+  mod()->AddGlobalVariable(std::make_unique<ast::Variable>(
+      "call_a", ast::StorageClass::kPrivate, &f32));
+  mod()->AddGlobalVariable(std::make_unique<ast::Variable>(
+      "call_b", ast::StorageClass::kPrivate, &f32));
+  mod()->AddGlobalVariable(std::make_unique<ast::Variable>(
+      "call_c", ast::StorageClass::kPrivate, &f32));
+
   // Register the functions and calculate the callers
   ASSERT_TRUE(td()->Determine()) << td()->error();
 

src/validator_test.cc

@@ -87,9 +87,9 @@ TEST_F(ValidatorTest, UsingUndefinedVariable_Fail) {
   auto assign = std::make_unique<ast::AssignmentStatement>(
       Source{12, 34}, std::move(lhs), std::move(rhs));
 
-  EXPECT_TRUE(td()->DetermineResultType(assign.get())) << td()->error();
+  EXPECT_FALSE(td()->DetermineResultType(assign.get()));
-  EXPECT_FALSE(v()->ValidateStatement(assign.get()));
+  EXPECT_EQ(td()->error(),
-  EXPECT_EQ(v()->error(), "12:34: v-0006: 'b' is not declared");
+            "12:34: v-0006: identifier must be declared before use: b");
 }
 
 TEST_F(ValidatorTest, UsingUndefinedVariableInBlockStatement_Fail) {
@@ -106,9 +106,9 @@ TEST_F(ValidatorTest, UsingUndefinedVariableInBlockStatement_Fail) {
   body->append(std::make_unique<ast::AssignmentStatement>(
       Source{12, 34}, std::move(lhs), std::move(rhs)));
 
-  EXPECT_TRUE(td()->DetermineStatements(body.get())) << td()->error();
+  EXPECT_FALSE(td()->DetermineStatements(body.get()));
-  EXPECT_FALSE(v()->ValidateStatements(body.get()));
+  EXPECT_EQ(td()->error(),
-  EXPECT_EQ(v()->error(), "12:34: v-0006: 'b' is not declared");
+            "12:34: v-0006: identifier must be declared before use: b");
 }
 
 TEST_F(ValidatorTest, AssignCompatibleTypes_Pass) {

src/writer/hlsl/generator_impl_intrinsic_test.cc

@@ -108,6 +108,9 @@ TEST_F(HlslGeneratorImplTest_Intrinsic, Intrinsic_Bad_Name) {
 }
 
 TEST_F(HlslGeneratorImplTest_Intrinsic, Intrinsic_Call) {
+  ast::type::F32Type f32;
+  ast::type::VectorType vec(&f32, 3);
+
   ast::ExpressionList params;
   params.push_back(std::make_unique<ast::IdentifierExpression>("param1"));
   params.push_back(std::make_unique<ast::IdentifierExpression>("param2"));
@@ -115,6 +118,12 @@ TEST_F(HlslGeneratorImplTest_Intrinsic, Intrinsic_Call) {
   ast::CallExpression call(std::make_unique<ast::IdentifierExpression>("dot"),
                            std::move(params));
 
+  ast::Variable v1("param1", ast::StorageClass::kFunction, &vec);
+  ast::Variable v2("param2", ast::StorageClass::kFunction, &vec);
+
+  td().RegisterVariableForTesting(&v1);
+  td().RegisterVariableForTesting(&v2);
+
   ASSERT_TRUE(td().DetermineResultType(&call)) << td().error();
 
   gen().increment_indent();

src/writer/msl/generator_impl_intrinsic_test.cc

@@ -112,6 +112,9 @@ TEST_F(MslGeneratorImplTest, Intrinsic_Bad_Name) {
 }
 
 TEST_F(MslGeneratorImplTest, Intrinsic_Call) {
+  ast::type::F32Type f32;
+  ast::type::VectorType vec(&f32, 3);
+
   ast::ExpressionList params;
   params.push_back(std::make_unique<ast::IdentifierExpression>("param1"));
   params.push_back(std::make_unique<ast::IdentifierExpression>("param2"));
@@ -122,6 +125,13 @@ TEST_F(MslGeneratorImplTest, Intrinsic_Call) {
   Context ctx;
   ast::Module m;
   TypeDeterminer td(&ctx, &m);
+
+  ast::Variable v1("param1", ast::StorageClass::kFunction, &vec);
+  ast::Variable v2("param2", ast::StorageClass::kFunction, &vec);
+
+  td.RegisterVariableForTesting(&v1);
+  td.RegisterVariableForTesting(&v2);
+
   ASSERT_TRUE(td.DetermineResultType(&call)) << td.error();
 
   GeneratorImpl g(&m);

src/writer/spirv/builder_constructor_expression_test.cc

@@ -2465,6 +2465,14 @@ TEST_F(BuilderTest, IsConstructorConst_GlobalVector_WithIdent) {
   Context ctx;
   ast::Module mod;
   TypeDeterminer td(&ctx, &mod);
+
+  ast::Variable var_a("a", ast::StorageClass::kPrivate, &f32);
+  ast::Variable var_b("b", ast::StorageClass::kPrivate, &f32);
+  ast::Variable var_c("c", ast::StorageClass::kPrivate, &f32);
+  td.RegisterVariableForTesting(&var_a);
+  td.RegisterVariableForTesting(&var_b);
+  td.RegisterVariableForTesting(&var_c);
+
   ASSERT_TRUE(td.DetermineResultType(&t)) << td.error();
 
   Builder b(&mod);
@@ -2614,6 +2622,14 @@ TEST_F(BuilderTest, IsConstructorConst_Vector_WithIdent) {
   Context ctx;
   ast::Module mod;
   TypeDeterminer td(&ctx, &mod);
+
+  ast::Variable var_a("a", ast::StorageClass::kPrivate, &f32);
+  ast::Variable var_b("b", ast::StorageClass::kPrivate, &f32);
+  ast::Variable var_c("c", ast::StorageClass::kPrivate, &f32);
+  td.RegisterVariableForTesting(&var_a);
+  td.RegisterVariableForTesting(&var_b);
+  td.RegisterVariableForTesting(&var_c);
+
   ASSERT_TRUE(td.DetermineResultType(&t)) << td.error();
 
   Builder b(&mod);
@@ -2821,6 +2837,12 @@ TEST_F(BuilderTest, IsConstructorConst_Struct_WithIdentSubExpression) {
   Context ctx;
   ast::Module mod;
   TypeDeterminer td(&ctx, &mod);
+
+  ast::Variable var_a("a", ast::StorageClass::kPrivate, &f32);
+  ast::Variable var_b("b", ast::StorageClass::kPrivate, &f32);
+  td.RegisterVariableForTesting(&var_a);
+  td.RegisterVariableForTesting(&var_b);
+
   ASSERT_TRUE(td.DetermineResultType(&t)) << td.error();
 
   Builder b(&mod);