Skip to content

Commit 48a6925

Browse files
nsvkeCohenArthur
nsvke
authored and
CohenArthur
committed
resolve: Use definition IDs directly and fix ICEs
This patch fixes multiple ICEs and test failures in the Name Resolution 2.0 - Map directly to definition IDs instead of intermediate import IDs. - Remove redundant `flatten()` calls. Flattening stripped generic arguments from aliases like `Self`, causing typecheck errors. - Prevent ICEs on invalid `use self;` imports by adding early returns. - Add `Namespace::Types` to pattern lookups to correctly emit E0532 instead of crashing. - Remove an unsafe debug loop in `Rib::get` that caused ICEs on ambiguous names. - Use `value_or` for safe optional unwrapping in module resolution. gcc/rust/ChangeLog: * checks/lints/rust-lint-marklive.cc (MarkLive::visit_path_segment): Add Types namespace lookup. * checks/lints/unused/rust-unused-checker.cc (UnusedChecker::visit): Use safe optional unwrapping. * checks/lints/unused/rust-unused-collector.cc (UnusedCollector::visit): Likewise. * checks/lints/unused/rust-unused-collector.h: Use helper for multiple namespace lookups. * resolve/rust-early-name-resolver-2.0.cc (Early::finalize_simple_import): Prevent ICE on single self imports. (Early::finalize_rebind_import): Return early to prevent ICE. * resolve/rust-name-resolution-context.hxx: Use value_or for safe leaf module lookup. * rust-session-manager.cc (Session::compile_crate): Remove obsolete flatten() calls. * typecheck/rust-hir-type-check-pattern.cc (TypeCheckPattern::visit): Look up enum variants in Types namespace. Signed-off-by: Enes Cevik <nsvke@proton.me>
1 parent 9d75e01 commit 48a6925

8 files changed

Lines changed: 84 additions & 41 deletions

‎gcc/rust/checks/lints/rust-lint-marklive.cc‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -165,8 +165,11 @@ MarkLive::visit_path_segment (HIR::PathExprSegment seg)
165165
// We should mark them alive all and ignoring other kind of segments.
166166
// If the segment we dont care then just return false is fine
167167
// TODO: Should we look that up in all namespaces?
168-
if (auto id = resolver.lookup (ast_node_id, Resolver2_0::Namespace::Values))
169-
ref_node_id = *id;
168+
169+
if (auto nslookup
170+
= resolver.lookup (ast_node_id, Resolver2_0::Namespace::Values,
171+
Resolver2_0::Namespace::Types))
172+
ref_node_id = nslookup->id;
170173
else
171174
return false;
172175
if (auto hid = mappings.lookup_node_to_hir (ref_node_id))

‎gcc/rust/checks/lints/unused/rust-unused-checker.cc‎

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -105,21 +105,25 @@ UnusedChecker::visit (HIR::IdentifierPattern &pattern)
105105
"variable %qs should have a snake case name",
106106
var_name.c_str ());
107107
}
108-
void
109108

109+
void
110110
UnusedChecker::visit (HIR::AssignmentExpr &expr)
111-
112111
{
113112
const auto &lhs = expr.get_lhs ();
114113
auto var_name = lhs.to_string ();
115114
NodeId ast_node_id = lhs.get_mappings ().get_nodeid ();
116-
NodeId def_id
117-
= nr_context.lookup (ast_node_id, Resolver2_0::Namespace::Values).value ();
118-
HirId id = mappings.lookup_node_to_hir (def_id).value ();
119-
if (unused_context.is_variable_assigned (id, lhs.get_mappings ().get_hirid ())
120-
&& var_name[0] != '_')
121-
rust_warning_at (lhs.get_locus (), OPT_Wunused_variable,
122-
"unused assignment %qs", var_name.c_str ());
115+
if (auto def_id
116+
= nr_context.lookup (ast_node_id, Resolver2_0::Namespace::Values))
117+
{
118+
if (auto id = mappings.lookup_node_to_hir (*def_id))
119+
{
120+
if (unused_context.is_variable_assigned (
121+
*id, lhs.get_mappings ().get_hirid ())
122+
&& var_name[0] != '_')
123+
rust_warning_at (lhs.get_locus (), OPT_Wunused_variable,
124+
"unused assignment %qs", var_name.c_str ());
125+
}
126+
}
123127
}
124128

125129
void

‎gcc/rust/checks/lints/unused/rust-unused-collector.cc‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,13 @@ UnusedCollector::visit (HIR::StructExprFieldIdentifier &ident)
6262
void
6363
UnusedCollector::visit (HIR::AssignmentExpr &expr)
6464
{
65-
auto def_id = get_def_id (expr.get_lhs (), Resolver2_0::Namespace::Values);
66-
HirId id = expr.get_lhs ().get_mappings ().get_hirid ();
67-
unused_context.remove_mut (def_id);
68-
unused_context.add_assign (def_id, id);
65+
if (auto def_id
66+
= get_def_id (expr.get_lhs (), Resolver2_0::Namespace::Values))
67+
{
68+
HirId id = expr.get_lhs ().get_mappings ().get_hirid ();
69+
unused_context.remove_mut (*def_id);
70+
unused_context.add_assign (*def_id, id);
71+
}
6972
visit_outer_attrs (expr);
7073
expr.get_rhs ().accept_vis (*this);
7174
}

‎gcc/rust/checks/lints/unused/rust-unused-collector.h‎

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -57,25 +57,55 @@ class UnusedCollector : public HIR::DefaultHIRVisitor
5757
virtual void visit (HIR::ContinueExpr &expr) override;
5858

5959
template <typename T>
60-
HirId get_def_id (T &path_expr, Resolver2_0::Namespace ns)
60+
tl::optional<HirId> get_def_id (T &path_expr, Resolver2_0::Namespace ns)
6161
{
6262
NodeId ast_node_id = path_expr.get_mappings ().get_nodeid ();
63-
NodeId id = nr_context.lookup (ast_node_id, ns).value ();
64-
HirId def_id = mappings.lookup_node_to_hir (id).value ();
65-
return def_id;
63+
64+
if (auto id = nr_context.lookup (ast_node_id, ns))
65+
{
66+
if (auto def_id = mappings.lookup_node_to_hir (*id))
67+
{
68+
return *def_id;
69+
}
70+
}
71+
return tl::nullopt;
72+
}
73+
74+
template <typename T>
75+
tl::optional<HirId> get_def_id (T &path_expr, Resolver2_0::Namespace ns1,
76+
Resolver2_0::Namespace ns2)
77+
{
78+
NodeId ast_node_id = path_expr.get_mappings ().get_nodeid ();
79+
80+
if (auto nslookup = nr_context.lookup (ast_node_id, ns1, ns2))
81+
{
82+
NodeId id = nslookup->id;
83+
84+
if (auto def_id = mappings.lookup_node_to_hir (id))
85+
{
86+
return *def_id;
87+
}
88+
}
89+
90+
return tl::nullopt;
6691
}
6792

6893
template <typename T> void mark_path_used (T &path_expr)
6994
{
70-
auto def_id = get_def_id (path_expr, Resolver2_0::Namespace::Values);
71-
unused_context.add_variable (def_id);
72-
unused_context.remove_assign (def_id);
95+
if (auto def_id = get_def_id (path_expr, Resolver2_0::Namespace::Values,
96+
Resolver2_0::Namespace::Types))
97+
{
98+
unused_context.add_variable (*def_id);
99+
unused_context.remove_assign (*def_id);
100+
}
73101
}
74102

75103
template <typename T> void mark_label_used (T &path_expr)
76104
{
77-
auto def_id = get_def_id (path_expr, Resolver2_0::Namespace::Labels);
78-
unused_context.add_label (def_id);
105+
if (auto def_id = get_def_id (path_expr, Resolver2_0::Namespace::Labels))
106+
{
107+
unused_context.add_label (*def_id);
108+
}
79109
}
80110
};
81111
} // namespace Analysis

‎gcc/rust/resolve/rust-early-name-resolver-2.0.cc‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -467,7 +467,8 @@ Early::finalize_simple_import (const Early::ImportPair &mapping)
467467
Definition (definition.definition.get_node_id ()),
468468
definition.ns);
469469

470-
toplevel.insert_or_error_out (identifier, import.get_locus (), import_id,
470+
toplevel.insert_or_error_out (identifier, import.get_locus (),
471+
definition.definition.get_node_id (),
471472
definition.ns);
472473

473474
dirty = dirty || toplevel.is_dirty ();
@@ -523,7 +524,7 @@ Early::finalize_rebind_import (const Early::ImportPair &mapping)
523524
{
524525
// Erroneous `self` or `{self}` use declaration
525526
if (segments.size () == 1)
526-
break;
527+
return;
527528
declared_name = segments[segments.size () - 2].as_string ();
528529
import_id = segments[segments.size () - 2].get_node_id ();
529530
}
@@ -548,7 +549,8 @@ Early::finalize_rebind_import (const Early::ImportPair &mapping)
548549
Definition (definition.definition.get_node_id ()),
549550
definition.ns);
550551

551-
toplevel.insert_or_error_out (declared_name, path.get_locus (), import_id,
552+
toplevel.insert_or_error_out (declared_name, path.get_locus (),
553+
definition.definition.get_node_id (),
552554
definition.ns);
553555

554556
dirty = dirty || toplevel.is_dirty ();

‎gcc/rust/resolve/rust-name-resolution-context.hxx‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -353,13 +353,20 @@ NameResolutionContext::resolve_segments (
353353
.lookup_glob_container (rib_lookup->get_node_id ())
354354
.has_value ())
355355
{
356-
auto leaf_module
356+
NodeId leaf_module
357357
= stack.find_leaf_definition (rib_lookup->get_node_id ())
358-
.value ()
358+
.value_or (Definition (rib_lookup->get_node_id ()))
359359
.id;
360360

361-
child = stack.dfs_node (stack.root, leaf_module).value ();
362-
break;
361+
if (auto new_child = stack.dfs_node (stack.root, leaf_module))
362+
{
363+
child = new_child.value ();
364+
break;
365+
}
366+
else
367+
{
368+
return tl::nullopt;
369+
}
363370
}
364371
else
365372
{

‎gcc/rust/rust-session-manager.cc‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -764,15 +764,9 @@ Session::compile_crate (const char *filename)
764764
if (last_step == CompileOptions::CompileStep::NameResolution)
765765
return;
766766

767-
// FIXME: Or run it within Late at the end?
768-
name_resolution_ctx.flatten ();
769-
770767
// resolution pipeline stage
771768
Resolver2_0::Late (name_resolution_ctx).go (parsed_crate);
772769

773-
// FIXME: Or run it within Late at the end?
774-
name_resolution_ctx.flatten ();
775-
776770
if (options.dump_option_enabled (CompileOptions::RESOLUTION_DUMP))
777771
dump_name_resolution (name_resolution_ctx);
778772

‎gcc/rust/typecheck/rust-hir-type-check-pattern.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,11 @@ TypeCheckPattern::visit (HIR::PathInExpression &pattern)
5757

5858
auto &nr_ctx = Resolver2_0::FinalizedNameResolutionContext::get ();
5959

60-
// FIXME: Not sure if this is enough to look up enum variants
61-
if (auto id = nr_ctx.lookup (pattern.get_mappings ().get_nodeid (),
62-
Resolver2_0::Namespace::Values))
60+
if (auto nslookup = nr_ctx.lookup (pattern.get_mappings ().get_nodeid (),
61+
Resolver2_0::Namespace::Values,
62+
Resolver2_0::Namespace::Types))
6363
{
64-
ref_node_id = *id;
64+
ref_node_id = nslookup->id;
6565
maybe_item = true;
6666
}
6767

0 commit comments

Comments
 (0)