Convert ScopeStack over to symbols.

This CL converts the ScopeStack to use a Symbol instead of a string as
the accessor.

Change-Id: I2893003bc119c86c4822732ef36c7393e4be1e79
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/36580
Reviewed-by: Sarah Mashayekhi <sarahmashay@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: dan sinclair <dsinclair@chromium.org>
Auto-Submit: dan sinclair <dsinclair@chromium.org>

src/validator/validator.cc

@@ -18,15 +18,14 @@
 
 namespace tint {
 
-Validator::Validator() : impl_(std::make_unique<tint::ValidatorImpl>()) {}
+Validator::Validator() = default;
 
 Validator::~Validator() = default;
 
 bool Validator::Validate(const ast::Module* module) {
-  bool ret = impl_->Validate(module);
-
-  diags_ = impl_->diagnostics();
-
+  ValidatorImpl impl(module);
+  bool ret = impl.Validate();
+  diags_ = impl.diagnostics();
   return ret;
 }
 

src/validator/validator.h

@@ -27,8 +27,6 @@
 
 namespace tint {
 
-class ValidatorImpl;
-
 /// Determines if the module is complete and valid
 class Validator {
  public:
@@ -52,7 +50,6 @@ class Validator {
   const diag::List& diagnostics() const { return diags_; }
 
  private:
-  std::unique_ptr<ValidatorImpl> impl_;
   diag::List diags_;
 };
 

src/validator/validator_function_test.cc

@@ -52,7 +52,7 @@ TEST_F(ValidateFunctionTest, VoidFunctionEndWithoutReturnStatement_Pass) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_TRUE(v()->Validate(mod));
+  EXPECT_TRUE(v()->Validate());
 }
 
 TEST_F(ValidateFunctionTest,
@@ -68,7 +68,7 @@ TEST_F(ValidateFunctionTest,
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_TRUE(v()->Validate(mod));
+  EXPECT_TRUE(v()->Validate());
 }
 
 TEST_F(ValidateFunctionTest, FunctionEndWithoutReturnStatement_Fail) {
@@ -86,7 +86,7 @@ TEST_F(ValidateFunctionTest, FunctionEndWithoutReturnStatement_Fail) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(),
             "12:34 v-0002: non-void function must end with a return statement");
 }
@@ -99,7 +99,7 @@ TEST_F(ValidateFunctionTest, FunctionEndWithoutReturnStatementEmptyBody_Fail) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(),
             "12:34 v-0002: non-void function must end with a return statement");
 }
@@ -132,7 +132,7 @@ TEST_F(ValidateFunctionTest, FunctionTypeMustMatchReturnStatementType_fail) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   // TODO(sarahM0): replace 000y with a rule number
   EXPECT_EQ(v()->error(),
             "12:34 v-000y: return statement type must match its function "
@@ -150,7 +150,7 @@ TEST_F(ValidateFunctionTest, FunctionTypeMustMatchReturnStatementTypeF32_fail) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   // TODO(sarahM0): replace 000y with a rule number
   EXPECT_EQ(v()->error(),
             "12:34 v-000y: return statement type must match its function "
@@ -177,7 +177,7 @@ TEST_F(ValidateFunctionTest, FunctionNamesMustBeUnique_fail) {
   mod->AddFunction(func_copy);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(), "12:34 v-0016: function names must be unique 'func'");
 }
 
@@ -196,7 +196,7 @@ TEST_F(ValidateFunctionTest, RecursionIsNotAllowed_Fail) {
   mod->AddFunction(func0);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod)) << v()->error();
+  EXPECT_FALSE(v()->Validate()) << v()->error();
   EXPECT_EQ(v()->error(), "12:34 v-0004: recursion is not allowed: 'func'");
 }
 
@@ -217,7 +217,7 @@ TEST_F(ValidateFunctionTest, RecursionIsNotAllowedExpr_Fail) {
   mod->AddFunction(func0);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod)) << v()->error();
+  EXPECT_FALSE(v()->Validate()) << v()->error();
   EXPECT_EQ(v()->error(), "12:34 v-0004: recursion is not allowed: 'func'");
 }
 
@@ -235,7 +235,7 @@ TEST_F(ValidateFunctionTest, Function_WithPipelineStage_NotVoid_Fail) {
 
   mod->AddFunction(func);
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(),
             "12:34 v-0024: Entry point function must return void: 'vtx_main'");
 }
@@ -257,7 +257,7 @@ TEST_F(ValidateFunctionTest, Function_WithPipelineStage_WithParams_Fail) {
 
   mod->AddFunction(func);
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(),
             "12:34 v-0023: Entry point function must accept no parameters: "
             "'vtx_func'");
@@ -279,7 +279,7 @@ TEST_F(ValidateFunctionTest, PipelineStage_MustBeUnique_Fail) {
 
   mod->AddFunction(func);
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(
       v()->error(),
       "12:34 v-0020: only one stage decoration permitted per entry point");
@@ -299,7 +299,7 @@ TEST_F(ValidateFunctionTest, OnePipelineStageFunctionMustBePresent_Pass) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_TRUE(v()->Validate(mod)) << v()->error();
+  EXPECT_TRUE(v()->Validate()) << v()->error();
 }
 
 TEST_F(ValidateFunctionTest, OnePipelineStageFunctionMustBePresent_Fail) {
@@ -312,7 +312,7 @@ TEST_F(ValidateFunctionTest, OnePipelineStageFunctionMustBePresent_Fail) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(),
             "v-0003: At least one of vertex, fragment or compute shader must "
             "be present");

src/validator/validator_impl.cc

@@ -37,7 +37,7 @@
 
 namespace tint {
 
-ValidatorImpl::ValidatorImpl() = default;
+ValidatorImpl::ValidatorImpl(const ast::Module* module) : module_(*module) {}
 
 ValidatorImpl::~ValidatorImpl() = default;
 
@@ -60,21 +60,18 @@ void ValidatorImpl::add_error(const Source& src, const std::string& msg) {
   diags_.add(std::move(diag));
 }
 
-bool ValidatorImpl::Validate(const ast::Module* module) {
-  if (!module) {
-    return false;
-  }
+bool ValidatorImpl::Validate() {
   function_stack_.push_scope();
-  if (!ValidateGlobalVariables(module->global_variables())) {
+  if (!ValidateGlobalVariables(module_.global_variables())) {
     return false;
   }
-  if (!ValidateConstructedTypes(module->constructed_types())) {
+  if (!ValidateConstructedTypes(module_.constructed_types())) {
     return false;
   }
-  if (!ValidateFunctions(module->functions())) {
+  if (!ValidateFunctions(module_.functions())) {
     return false;
   }
-  if (!ValidateEntryPoint(module->functions())) {
+  if (!ValidateEntryPoint(module_.functions())) {
     return false;
   }
   function_stack_.pop_scope();
@@ -113,9 +110,10 @@ bool ValidatorImpl::ValidateConstructedTypes(
 bool ValidatorImpl::ValidateGlobalVariables(
     const ast::VariableList& global_vars) {
   for (auto* var : global_vars) {
-    if (variable_stack_.has(var->name())) {
+    if (variable_stack_.has(var->symbol())) {
       add_error(var->source(), "v-0011",
-                "redeclared global identifier '" + var->name() + "'");
+                "redeclared global identifier '" +
+                    module_.SymbolToName(var->symbol()) + "'");
       return false;
     }
     if (!var->is_const() && var->storage_class() == ast::StorageClass::kNone) {
@@ -129,20 +127,21 @@ bool ValidatorImpl::ValidateGlobalVariables(
                 "global constants shouldn't have a storage class");
       return false;
     }
-    variable_stack_.set_global(var->name(), var);
+    variable_stack_.set_global(var->symbol(), var);
   }
   return true;
 }
 
 bool ValidatorImpl::ValidateFunctions(const ast::FunctionList& funcs) {
   for (auto* func : funcs) {
-    if (function_stack_.has(func->name())) {
+    if (function_stack_.has(func->symbol())) {
       add_error(func->source(), "v-0016",
-                "function names must be unique '" + func->name() + "'");
+                "function names must be unique '" +
+                    module_.SymbolToName(func->symbol()) + "'");
       return false;
     }
 
-    function_stack_.set(func->name(), func);
+    function_stack_.set(func->symbol(), func);
     current_function_ = func;
     if (!ValidateFunction(func)) {
       return false;
@@ -197,7 +196,7 @@ bool ValidatorImpl::ValidateFunction(const ast::Function* func) {
   variable_stack_.push_scope();
 
   for (auto* param : func->params()) {
-    variable_stack_.set(param->name(), param);
+    variable_stack_.set(param->symbol(), param);
     if (!ValidateParameter(param)) {
       return false;
     }
@@ -265,18 +264,18 @@ bool ValidatorImpl::ValidateStatements(const ast::BlockStatement* block) {
 
 bool ValidatorImpl::ValidateDeclStatement(
     const ast::VariableDeclStatement* decl) {
-  auto name = decl->variable()->name();
+  auto symbol = decl->variable()->symbol();
   bool is_global = false;
-  if (variable_stack_.get(name, nullptr, &is_global)) {
+  if (variable_stack_.get(symbol, nullptr, &is_global)) {
     const char* error_code = "v-0014";
     if (is_global) {
       error_code = "v-0013";
     }
     add_error(decl->source(), error_code,
-              "redeclared identifier '" + name + "'");
+              "redeclared identifier '" + module_.SymbolToName(symbol) + "'");
     return false;
   }
-  variable_stack_.set(name, decl->variable());
+  variable_stack_.set(symbol, decl->variable());
   if (auto* arr =
           decl->variable()->type()->UnwrapAll()->As<ast::type::Array>()) {
     if (arr->IsRuntimeArray()) {
@@ -403,18 +402,20 @@ bool ValidatorImpl::ValidateCallExpr(const ast::CallExpression* expr) {
   }
 
   if (auto* ident = expr->func()->As<ast::IdentifierExpression>()) {
-    auto func_name = ident->name();
     if (ident->IsIntrinsic()) {
       // TODO(sarahM0): validate intrinsics - tied with type-determiner
     } else {
-      if (!function_stack_.has(func_name)) {
+      auto symbol = ident->symbol();
+      if (!function_stack_.has(symbol)) {
         add_error(expr->source(), "v-0005",
-                  "function must be declared before use: '" + func_name + "'");
+                  "function must be declared before use: '" +
+                      module_.SymbolToName(symbol) + "'");
         return false;
       }
-      if (func_name == current_function_->name()) {
-        add_error(expr->source(), "v-0004",
-                  "recursion is not allowed: '" + func_name + "'");
+      if (symbol == current_function_->symbol()) {
+        add_error(
+            expr->source(), "v-0004",
+            "recursion is not allowed: '" + module_.SymbolToName(symbol) + "'");
         return false;
       }
     }
@@ -450,10 +451,11 @@ bool ValidatorImpl::ValidateConstant(const ast::AssignmentStatement* assign) {
 
   if (auto* ident = assign->lhs()->As<ast::IdentifierExpression>()) {
     ast::Variable* var;
-    if (variable_stack_.get(ident->name(), &var)) {
+    if (variable_stack_.get(ident->symbol(), &var)) {
       if (var->is_const()) {
         add_error(assign->source(), "v-0021",
-                  "cannot re-assign a constant: '" + ident->name() + "'");
+                  "cannot re-assign a constant: '" +
+                      module_.SymbolToName(ident->symbol()) + "'");
         return false;
       }
     }
@@ -495,9 +497,10 @@ bool ValidatorImpl::ValidateExpression(const ast::Expression* expr) {
 
 bool ValidatorImpl::ValidateIdentifier(const ast::IdentifierExpression* ident) {
   ast::Variable* var;
-  if (!variable_stack_.get(ident->name(), &var)) {
-    add_error(ident->source(), "v-0006",
-              "'" + ident->name() + "' is not declared");
+  if (!variable_stack_.get(ident->symbol(), &var)) {
+    add_error(
+        ident->source(), "v-0006",
+        "'" + module_.SymbolToName(ident->symbol()) + "' is not declared");
     return false;
   }
   return true;

src/validator/validator_impl.h

@@ -39,13 +39,13 @@ namespace tint {
 class ValidatorImpl {
  public:
   /// Constructor
-  ValidatorImpl();
+  /// @param module the module to validate
+  explicit ValidatorImpl(const ast::Module* module);
   ~ValidatorImpl();
 
   /// Runs the validator
-  /// @param module the module to validate
   /// @returns true if the validation was successful
-  bool Validate(const ast::Module* module);
+  bool Validate();
 
   /// @returns the diagnostic messages
   const diag::List& diagnostics() const { return diags_; }
@@ -148,6 +148,7 @@ class ValidatorImpl {
       const std::vector<ast::type::Type*>& constructed_types);
 
  private:
+  const ast::Module& module_;
   diag::List diags_;
   ScopeStack<ast::Variable*> variable_stack_;
   ScopeStack<ast::Function*> function_stack_;

src/validator/validator_test.cc

@@ -217,7 +217,7 @@ TEST_F(ValidatorTest, GlobalVariableNoStorageClass_Fail) {
                              ast::StorageClass::kNone, ty.f32, nullptr,
                              ast::VariableDecorationList{}));
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(),
             "12:34 v-0022: global variables must have a storage class");
 }
@@ -228,7 +228,7 @@ TEST_F(ValidatorTest, GlobalConstantWithStorageClass_Fail) {
                                ast::StorageClass::kInput, ty.f32, nullptr,
                                ast::VariableDecorationList{}));
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(
       v()->error(),
       "12:34 v-global01: global constants shouldn't have a storage class");
@@ -240,7 +240,7 @@ TEST_F(ValidatorTest, GlobalConstNoStorageClass_Pass) {
                                ast::StorageClass::kNone, ty.f32, nullptr,
                                ast::VariableDecorationList{}));
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod)) << v()->error();
+  EXPECT_FALSE(v()->Validate()) << v()->error();
 }
 
 TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariable_Fail) {
@@ -263,7 +263,7 @@ TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariable_Fail) {
                     ast::FunctionDecorationList{});
   mod->AddFunction(func);
 
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(), "12:34 v-0006: 'not_global_var' is not declared");
 }
 
@@ -290,7 +290,7 @@ TEST_F(ValidatorTest, UsingUndefinedVariableGlobalVariable_Pass) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_TRUE(v()->Validate(mod)) << v()->error();
+  EXPECT_TRUE(v()->Validate()) << v()->error();
 }
 
 TEST_F(ValidatorTest, UsingUndefinedVariableInnerScope_Fail) {
@@ -435,7 +435,7 @@ TEST_F(ValidatorTest, GlobalVariableFunctionVariableNotUnique_Fail) {
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
   EXPECT_TRUE(td()->DetermineFunction(func)) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod)) << v()->error();
+  EXPECT_FALSE(v()->Validate()) << v()->error();
   EXPECT_EQ(v()->error(), "12:34 v-0013: redeclared identifier 'a'");
 }
 
@@ -462,7 +462,7 @@ TEST_F(ValidatorTest, RedeclaredIndentifier_Fail) {
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
   EXPECT_TRUE(td()->DetermineFunction(func)) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(), "12:34 v-0014: redeclared identifier 'a'");
 }
 
@@ -552,7 +552,7 @@ TEST_F(ValidatorTest, RedeclaredIdentifierDifferentFunctions_Pass) {
   mod->AddFunction(func1);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_TRUE(v()->Validate(mod)) << v()->error();
+  EXPECT_TRUE(v()->Validate()) << v()->error();
 }
 
 TEST_F(ValidatorTest, VariableDeclNoConstructor_Pass) {

src/validator/validator_test_helper.cc

@@ -20,7 +20,7 @@ namespace tint {
 
 ValidatorTestHelper::ValidatorTestHelper() {
   td_ = std::make_unique<TypeDeterminer>(mod);
-  v_ = std::make_unique<ValidatorImpl>();
+  v_ = std::make_unique<ValidatorImpl>(mod);
 }
 
 ValidatorTestHelper::~ValidatorTestHelper() = default;

src/validator/validator_type_test.cc

@@ -160,7 +160,7 @@ TEST_F(ValidatorTypeTest, RuntimeArrayInFunction_Fail) {
   mod->AddFunction(func);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(),
             "12:34 v-0015: runtime arrays may only appear as the last member "
             "of a struct");
@@ -192,7 +192,7 @@ TEST_F(ValidatorTypeTest, RuntimeArrayAsParameter_Fail) {
   mod->AddFunction(main);
 
   EXPECT_TRUE(td()->Determine()) << td()->error();
-  EXPECT_FALSE(v()->Validate(mod));
+  EXPECT_FALSE(v()->Validate());
   EXPECT_EQ(v()->error(),
             "12:34 v-0015: runtime arrays may only appear as the last member "
             "of a struct");