validation: cleaning up name uniqueness rules
Bug: tint:353
Change-Id: I01e5b63937e218d7c42e41f7c0e9a7910aacaa90
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/56320
Reviewed-by: Antonio Maiorano <amaiorano@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Auto-Submit: Sarah Mashayekhi <sarahmashay@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/resolver/function_validation_test.cc b/src/resolver/function_validation_test.cc
index 5d55377..9e03a47 100644
--- a/src/resolver/function_validation_test.cc
+++ b/src/resolver/function_validation_test.cc
@@ -42,8 +42,8 @@
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
- R"(12:34 error: duplicate function named 'func'
-note: first function declared here)");
+ "12:34 error: redefinition of 'func'\nnote: previous definition "
+ "is here");
}
TEST_F(ResolverFunctionValidationTest,
@@ -74,8 +74,8 @@
EXPECT_FALSE(r()->Resolve()) << r()->error();
EXPECT_EQ(r()->error(),
- "12:34 error: duplicate declaration 'foo'\n56:78 note: 'foo' first "
- "declared here:");
+ "12:34 error: redefinition of 'foo'\n56:78 note: previous "
+ "definition is here");
}
TEST_F(ResolverFunctionValidationTest,
@@ -85,14 +85,14 @@
Func(Source{Source::Location{12, 34}}, "foo", ast::VariableList{}, ty.void_(),
ast::StatementList{}, ast::DecorationList{});
- auto* global_var =
- Var("foo", ty.f32(), ast::StorageClass::kPrivate, Expr(3.14f));
+ auto* global_var = Var(Source{Source::Location{56, 78}}, "foo", ty.f32(),
+ ast::StorageClass::kPrivate, Expr(3.14f));
AST().AddGlobalVariable(global_var);
EXPECT_FALSE(r()->Resolve()) << r()->error();
EXPECT_EQ(r()->error(),
- "error: duplicate declaration 'foo'\n12:34 note: 'foo' first "
- "declared here:");
+ "56:78 error: redefinition of 'foo'\n12:34 note: previous "
+ "definition is here");
}
TEST_F(ResolverFunctionValidationTest, FunctionUsingSameVariableName_Pass) {
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index c9876d3..75560d0 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -615,10 +615,8 @@
}
bool Resolver::GlobalVariable(ast::Variable* var) {
- if (variable_stack_.has(var->symbol())) {
- AddError("redeclared global identifier '" +
- builder_->Symbols().NameFor(var->symbol()) + "'",
- var->source());
+ if (!ValidateNoDuplicateDefinition(var->symbol(), var->source(),
+ /* check_global_scope_only */ true)) {
return false;
}
@@ -673,17 +671,6 @@
}
bool Resolver::ValidateGlobalVariable(const VariableInfo* info) {
- auto duplicate_func = symbol_to_function_.find(info->declaration->symbol());
- if (duplicate_func != symbol_to_function_.end()) {
- AddError("duplicate declaration '" +
- builder_->Symbols().NameFor(info->declaration->symbol()) + "'",
- info->declaration->source());
- AddNote("'" + builder_->Symbols().NameFor(info->declaration->symbol()) +
- "' first declared here:",
- duplicate_func->second->declaration->source());
- return false;
- }
-
if (!ValidateNoDuplicateDecorations(info->declaration->decorations())) {
return false;
}
@@ -1063,30 +1050,11 @@
bool Resolver::ValidateFunction(const ast::Function* func,
const FunctionInfo* info) {
- auto func_it = symbol_to_function_.find(func->symbol());
- if (func_it != symbol_to_function_.end()) {
- AddError("duplicate function named '" +
- builder_->Symbols().NameFor(func->symbol()) + "'",
- func->source());
- AddNote("first function declared here",
- func_it->second->declaration->source());
+ if (!ValidateNoDuplicateDefinition(func->symbol(), func->source(),
+ /* check_global_scope_only */ true)) {
return false;
}
- bool is_global = false;
- VariableInfo* var;
- if (variable_stack_.get(func->symbol(), &var, &is_global)) {
- if (is_global) {
- AddError("duplicate declaration '" +
- builder_->Symbols().NameFor(func->symbol()) + "'",
- func->source());
- AddNote("'" + builder_->Symbols().NameFor(func->symbol()) +
- "' first declared here:",
- var->declaration->source());
- return false;
- }
- }
-
auto stage_deco_count = 0;
auto workgroup_deco_count = 0;
for (auto* deco : func->decorations()) {
@@ -2857,11 +2825,7 @@
ast::Variable* var = stmt->variable();
Mark(var);
- bool is_global = false;
- if (variable_stack_.get(var->symbol(), nullptr, &is_global)) {
- AddError("redeclared identifier '" +
- builder_->Symbols().NameFor(var->symbol()) + "'",
- var->source());
+ if (!ValidateNoDuplicateDefinition(var->symbol(), var->source())) {
return false;
}
@@ -3772,6 +3736,39 @@
return true;
}
+bool Resolver::ValidateNoDuplicateDefinition(Symbol sym,
+ const Source& source,
+ bool check_global_scope_only) {
+ if (check_global_scope_only) {
+ bool is_global = false;
+ VariableInfo* var;
+ if (variable_stack_.get(sym, &var, &is_global)) {
+ if (is_global) {
+ AddError("redefinition of '" + builder_->Symbols().NameFor(sym) + "'",
+ source);
+ AddNote("previous definition is here", var->declaration->source());
+ return false;
+ }
+ }
+ auto it = symbol_to_function_.find(sym);
+ if (it != symbol_to_function_.end()) {
+ AddError("redefinition of '" + builder_->Symbols().NameFor(sym) + "'",
+ source);
+ AddNote("previous definition is here", it->second->declaration->source());
+ return false;
+ }
+ } else {
+ VariableInfo* var;
+ if (variable_stack_.get(sym, &var)) {
+ AddError("redefinition of '" + builder_->Symbols().NameFor(sym) + "'",
+ source);
+ AddNote("previous definition is here", var->declaration->source());
+ return false;
+ }
+ }
+ return true;
+}
+
bool Resolver::ValidateNoDuplicateDecorations(
const ast::DecorationList& decorations) {
std::unordered_map<const TypeInfo*, Source> seen;
diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h
index 8eb2ed2..f1d663d 100644
--- a/src/resolver/resolver.h
+++ b/src/resolver/resolver.h
@@ -283,6 +283,10 @@
const sem::Matrix* matrix_type);
bool ValidateFunctionParameter(const ast::Function* func,
const VariableInfo* info);
+ bool ValidateNoDuplicateDefinition(Symbol sym,
+ const Source& source,
+ bool check_global_scope_only = false);
+ bool ValidateParameter(const ast::Function* func, const VariableInfo* info);
bool ValidateReturn(const ast::ReturnStatement* ret);
bool ValidateStatements(const ast::StatementList& stmts);
bool ValidateStorageTexture(const ast::StorageTexture* t);
diff --git a/src/resolver/var_let_validation_test.cc b/src/resolver/var_let_validation_test.cc
index e03e6e7..959b2ca 100644
--- a/src/resolver/var_let_validation_test.cc
+++ b/src/resolver/var_let_validation_test.cc
@@ -124,7 +124,9 @@
WrapInFunction(v1, v2);
EXPECT_FALSE(r()->Resolve());
- EXPECT_EQ(r()->error(), "12:34 error: redeclared identifier 'v'");
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: redefinition of 'v'\nnote: previous definition is here");
}
TEST_F(ResolverVarLetValidationTest, LocalLetRedeclared) {
@@ -135,7 +137,9 @@
WrapInFunction(l1, l2);
EXPECT_FALSE(r()->Resolve());
- EXPECT_EQ(r()->error(), "12:34 error: redeclared identifier 'l'");
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: redefinition of 'l'\nnote: previous definition is here");
}
TEST_F(ResolverVarLetValidationTest, GlobalVarRedeclared) {
@@ -145,8 +149,9 @@
Global(Source{{12, 34}}, "v", ty.i32(), ast::StorageClass::kPrivate);
EXPECT_FALSE(r()->Resolve());
- EXPECT_EQ(r()->error(),
- "12:34 error: redeclared global identifier 'v'");
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: redefinition of 'v'\nnote: previous definition is here");
}
TEST_F(ResolverVarLetValidationTest, GlobalLetRedeclared) {
@@ -156,8 +161,9 @@
GlobalConst(Source{{12, 34}}, "l", ty.i32(), Expr(0));
EXPECT_FALSE(r()->Resolve());
- EXPECT_EQ(r()->error(),
- "12:34 error: redeclared global identifier 'l'");
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: redefinition of 'l'\nnote: previous definition is here");
}
TEST_F(ResolverVarLetValidationTest, GlobalVarRedeclaredAsLocal) {
@@ -173,7 +179,9 @@
Expr(2.0f)));
EXPECT_FALSE(r()->Resolve()) << r()->error();
- EXPECT_EQ(r()->error(), "12:34 error: redeclared identifier 'v'");
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: redefinition of 'v'\nnote: previous definition is here");
}
TEST_F(ResolverVarLetValidationTest, VarRedeclaredInInnerBlock) {
@@ -190,7 +198,9 @@
WrapInFunction(outer_body);
EXPECT_FALSE(r()->Resolve());
- EXPECT_EQ(r()->error(), "12:34 error: redeclared identifier 'v'");
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: redefinition of 'v'\nnote: previous definition is here");
}
TEST_F(ResolverVarLetValidationTest, VarRedeclaredInIfBlock) {
@@ -213,7 +223,9 @@
WrapInFunction(outer_body);
EXPECT_FALSE(r()->Resolve());
- EXPECT_EQ(r()->error(), "12:34 error: redeclared identifier 'v'");
+ EXPECT_EQ(
+ r()->error(),
+ "12:34 error: redefinition of 'v'\nnote: previous definition is here");
}
TEST_F(ResolverVarLetValidationTest, InferredPtrStorageAccessMismatch) {
