resolver: Error when calling shadowed functions
The fix involves unifying the dependency logic of DependencyScanner, which also cleans up the code quite a bit.
Also:
* Correctly resolve parameter type before declaring the parameter variable.
* Fix bonkers typos in utils::Lookup.
Fixed: tint:1442
Change-Id: I77b548e148b461f87ec10e8272c398f6fee297bd
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/81102
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: Ben Clayton <bclayton@google.com>
diff --git a/src/resolver/call_validation_test.cc b/src/resolver/call_validation_test.cc
index e4a479b..71cca02 100644
--- a/src/resolver/call_validation_test.cc
+++ b/src/resolver/call_validation_test.cc
@@ -265,6 +265,24 @@
note: 'v' declared here)");
}
+TEST_F(ResolverCallValidationTest, CallVariableShadowsFunction) {
+ // fn x() {}
+ // fn f() {
+ // var x : i32;
+ // x();
+ // }
+ Func("x", {}, ty.void_(), {});
+ Func("f", {}, ty.void_(),
+ {
+ Decl(Var(Source{{56, 78}}, "x", ty.i32())),
+ CallStmt(Call(Source{{12, 34}}, "x")),
+ });
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(), R"(error: cannot call variable 'x'
+56:78 note: 'x' declared here)");
+}
+
} // namespace
} // namespace resolver
} // namespace tint
diff --git a/src/resolver/dependency_graph.cc b/src/resolver/dependency_graph.cc
index d3ac5f4..cbb574d 100644
--- a/src/resolver/dependency_graph.cc
+++ b/src/resolver/dependency_graph.cc
@@ -173,6 +173,16 @@
/// Traverses the function, performing symbol resolution and determining
/// global dependencies.
void TraverseFunction(const ast::Function* func) {
+ // Perform symbol resolution on all the parameter types before registering
+ // the parameters themselves. This allows the case of declaring a parameter
+ // with the same identifier as its type.
+ for (auto* param : func->params) {
+ TraverseType(param->type);
+ }
+ // Resolve the return type
+ TraverseType(func->return_type);
+
+ // Push the scope stack for the parameters and function body.
scope_stack_.Push();
TINT_DEFER(scope_stack_.Pop());
@@ -181,12 +191,10 @@
graph_.shadows.emplace(param, shadows);
}
Declare(param->symbol, param);
- TraverseType(param->type);
}
if (func->body) {
TraverseStatements(func->body->statements);
}
- TraverseType(func->return_type);
}
/// Traverses the statements, performing symbol resolution and determining
@@ -295,38 +303,15 @@
ast::TraverseExpressions(
root, diagnostics_, [&](const ast::Expression* expr) {
if (auto* ident = expr->As<ast::IdentifierExpression>()) {
- auto* node = scope_stack_.Get(ident->symbol);
- if (node == nullptr) {
- if (!IsBuiltin(ident->symbol)) {
- UnknownSymbol(ident->symbol, ident->source, "identifier");
- }
- return ast::TraverseAction::Descend;
- }
- auto global_it = globals_.find(ident->symbol);
- if (global_it != globals_.end() &&
- node == global_it->second->node) {
- AddGlobalDependency(ident, ident->symbol, "identifier",
- "references");
- } else {
- graph_.resolved_symbols.emplace(ident, node);
- }
+ AddDependency(ident, ident->symbol, "identifier", "references");
}
if (auto* call = expr->As<ast::CallExpression>()) {
if (call->target.name) {
- if (!IsBuiltin(call->target.name->symbol)) {
- AddGlobalDependency(call->target.name,
- call->target.name->symbol, "function",
- "calls");
- graph_.resolved_symbols.emplace(
- call,
- utils::Lookup(graph_.resolved_symbols, call->target.name));
- }
+ AddDependency(call->target.name, call->target.name->symbol,
+ "function", "calls");
}
if (call->target.type) {
TraverseType(call->target.type);
- graph_.resolved_symbols.emplace(
- call,
- utils::Lookup(graph_.resolved_symbols, call->target.type));
}
}
if (auto* cast = expr->As<ast::BitcastExpression>()) {
@@ -360,7 +345,7 @@
return;
}
if (auto* tn = ty->As<ast::TypeName>()) {
- AddGlobalDependency(tn, tn->name, "type", "references");
+ AddDependency(tn, tn->name, "type", "references");
return;
}
if (auto* vec = ty->As<ast::Vector>()) {
@@ -416,24 +401,31 @@
UnhandledNode(diagnostics_, attr);
}
- /// Adds the dependency to the currently processed global
- void AddGlobalDependency(const ast::Node* from,
- Symbol to,
- const char* use,
- const char* action) {
- auto global_it = globals_.find(to);
- if (global_it != globals_.end()) {
- auto* global = global_it->second;
+ /// Adds the dependency from `from` to `to`, erroring if `to` cannot be
+ /// resolved.
+ void AddDependency(const ast::Node* from,
+ Symbol to,
+ const char* use,
+ const char* action) {
+ auto* resolved = scope_stack_.Get(to);
+ if (!resolved) {
+ if (!IsBuiltin(to)) {
+ UnknownSymbol(to, from->source, use);
+ return;
+ }
+ }
+
+ if (auto* global = utils::Lookup(globals_, to);
+ global && global->node == resolved) {
if (dependency_edges_
.emplace(DependencyEdge{current_global_, global},
DependencyInfo{from->source, action})
.second) {
current_global_->deps.emplace_back(global);
}
- graph_.resolved_symbols.emplace(from, global->node);
- } else {
- UnknownSymbol(to, from->source, use);
}
+
+ graph_.resolved_symbols.emplace(from, resolved);
}
/// @returns true if `name` is the name of a builtin function
diff --git a/src/resolver/dependency_graph_test.cc b/src/resolver/dependency_graph_test.cc
index 2329d86..102d996 100644
--- a/src/resolver/dependency_graph_test.cc
+++ b/src/resolver/dependency_graph_test.cc
@@ -993,9 +993,9 @@
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
- R"(3:1 error: cyclic dependency found: 'S' -> 'A' -> 'S'
-3:10 note: struct 'S' references alias 'A' here
+ R"(2:1 error: cyclic dependency found: 'A' -> 'S' -> 'A'
2:10 note: alias 'A' references struct 'S' here
+3:10 note: struct 'S' references alias 'A' here
4:1 error: cyclic dependency found: 'Z' -> 'L' -> 'Z'
4:10 note: var 'Z' references let 'L' here
5:10 note: let 'L' references var 'Z' here)");
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index 9869755..f2f516a 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -268,8 +268,9 @@
return builder_->create<sem::ExternalTexture>();
},
[&](Default) -> sem::Type* {
+ auto* resolved = ResolvedSymbol(ty);
return Switch(
- ResolvedSymbol(ty), //
+ resolved, //
[&](sem::Type* type) { return type; },
[&](sem::Variable* var) {
auto name =
@@ -291,7 +292,10 @@
},
[&](Default) {
TINT_UNREACHABLE(Resolver, diagnostics_)
- << "Unhandled ast::Type: " << ty->TypeInfo().name;
+ << "Unhandled resolved type '"
+ << (resolved ? resolved->TypeInfo().name : "<null>")
+ << "' resolved from ast::Type '" << ty->TypeInfo().name
+ << "'";
return nullptr;
});
});
diff --git a/src/utils/map.h b/src/utils/map.h
index db1b153..a84c9ba 100644
--- a/src/utils/map.h
+++ b/src/utils/map.h
@@ -29,9 +29,9 @@
/// @return the map item value, or `if_missing` if the map does not contain the
/// given key
template <typename K, typename V, typename H, typename C, typename KV = K>
-V Lookup(std::unordered_map<K, V, H, C>& map,
+V Lookup(const std::unordered_map<K, V, H, C>& map,
const KV& key,
- const KV& if_missing = {}) {
+ const V& if_missing = {}) {
auto it = map.find(key);
return it != map.end() ? it->second : if_missing;
}
