commit 33a8cddb2d200850377f4156ccf4f3ab1753a7eb
author Ben Clayton <bclayton@google.com> Wed Feb 24 13:31:22 2021 +0000
committer Commit Bot service account <commit-bot@chromium.org> Wed Feb 24 13:31:22 2021 +0000
tree 8cdaf6d73885d40c412f3cd1240b6553a6fb774f
parent 13dbbc6797a300043474202834ac401388d13bb0

TypeDeterminer: Error if an IdentifierExpression to a callable is not called

IdentifierExpressions that resolve to functions and intrinsics were not being assigned a semantic::Expression as there is no function / intrinsic type to give them. This lead to NPEs in the writers when these identifiers were reached and TypeOf() is called.

Adding a new tint::type::Callable is an option, but until functions become a type in the language, this seems like a very large and debatable change.

Attempting to detect IdentifierExpressions with no semantic node in the validator is another option, but this is clunky as it has to detect incomplete semantic info from the TD, and cannot identify whether this actually resolved to a function or an intrinsic.

Instead we now error in the TD if encounter an IdentifierExpression that resolves to a function or intrinsic which is not called (ident is not followed with parenthesis).

Fixed: chromium:1180544
Fixed: chromium:1180814
Change-Id: I121dd194356419f94b09c7ee1ed544a350a114b3
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/42220
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>

src/source.h

diff --git a/src/source.h b/src/source.h
index b77fa02..9b6e74c 100644
--- a/src/source.h
+++ b/src/source.h
@@ -114,9 +114,19 @@
   /// @param content the source file content
   inline Source(const Range& rng,
                 const std::string& path,
-                FileContent* content = nullptr)
+                const FileContent* content = nullptr)
       : range(rng), file_path(path), file_content(content) {}
 
+  /// @returns a Source that points to the begin range of this Source.
+  inline Source Begin() const {
+    return Source(Range{range.begin}, file_path, file_content);
+  }
+
+  /// @returns a Source that points to the end range of this Source.
+  inline Source End() const {
+    return Source(Range{range.end}, file_path, file_content);
+  }
+
   /// range is the span of text this source refers to in #file_path
   Range range;
   /// file is the optional file path this source refers to

src/type_determiner.cc

diff --git a/src/type_determiner.cc b/src/type_determiner.cc
index 5bce843..fd334c2 100644
--- a/src/type_determiner.cc
+++ b/src/type_determiner.cc
@@ -406,9 +406,6 @@
 }
 
 bool TypeDeterminer::DetermineCall(ast::CallExpression* call) {
-  if (!DetermineResultType(call->func())) {
-    return false;
-  }
   if (!DetermineResultType(call->params())) {
     return false;
   }
@@ -436,8 +433,8 @@
 
       auto callee_func_it = symbol_to_function_.find(ident->symbol());
       if (callee_func_it == symbol_to_function_.end()) {
-        diagnostics_.add_error("unable to find called function: " + name,
-                               call->source());
+        diagnostics_.add_error(
+            "v-0006: unable to find called function: " + name, call->source());
         return false;
       }
       auto* callee_func = callee_func_it->second;
@@ -558,14 +555,16 @@
 
   auto iter = symbol_to_function_.find(symbol);
   if (iter != symbol_to_function_.end()) {
-    // Identifier is to a function, which has no type (currently).
-    return true;
+    diagnostics_.add_error("missing '(' for function call",
+                           expr->source().End());
+    return false;
   }
 
   std::string name = builder_->Symbols().NameFor(symbol);
   if (MatchIntrinsicType(name) != IntrinsicType::kNone) {
-    // Identifier is to an intrinsic function, which has no type (currently).
-    return true;
+    diagnostics_.add_error("missing '(' for intrinsic call",
+                           expr->source().End());
+    return false;
   }
 
   diagnostics_.add_error(

src/type_determiner_test.cc

diff --git a/src/type_determiner_test.cc b/src/type_determiner_test.cc
index 3fd3951..801260d 100644
--- a/src/type_determiner_test.cc
+++ b/src/type_determiner_test.cc
@@ -396,9 +396,8 @@
 
   EXPECT_FALSE(td()->Determine());
 
-  EXPECT_EQ(
-      td()->error(),
-      "12:34 error: v-0006: identifier must be declared before use: func");
+  EXPECT_EQ(td()->error(),
+            "12:34 error: v-0006: unable to find called function: func");
 }
 
 TEST_F(TypeDeterminerTest, Stmt_VariableDecl) {
@@ -692,6 +691,27 @@
   EXPECT_TRUE(TypeOf(call)->Is<type::F32>());
 }
 
+TEST_F(TypeDeterminerTest, Expr_DontCall_Function) {
+  Func("func", {}, ty.void_(), {}, {});
+  auto* ident = create<ast::IdentifierExpression>(
+      Source{{Source::Location{3, 3}, Source::Location{3, 8}}},
+      Symbols().Register("func"));
+  WrapInFunction(ident);
+
+  EXPECT_FALSE(td()->Determine());
+  EXPECT_EQ(td()->error(), "3:8 error: missing '(' for function call");
+}
+
+TEST_F(TypeDeterminerTest, Expr_DontCall_Intrinsic) {
+  auto* ident = create<ast::IdentifierExpression>(
+      Source{{Source::Location{3, 3}, Source::Location{3, 8}}},
+      Symbols().Register("round"));
+  WrapInFunction(ident);
+
+  EXPECT_FALSE(td()->Determine());
+  EXPECT_EQ(td()->error(), "3:8 error: missing '(' for intrinsic call");
+}
+
 TEST_F(TypeDeterminerTest, Expr_Cast) {
   Global("name", ty.f32(), ast::StorageClass::kPrivate);