From: Enes Cevik <[email protected]>
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 <[email protected]>
---
This change was merged into the gccrs repository and is posted here for
upstream visibility and potential drive-by review, as requested by GCC
release managers.
Each commit email contains a link to its details on github from where you can
find the Pull-Request and associated discussions.
Commit on github:
https://github.com/Rust-GCC/gccrs/commit/83aefd6ad71221c6fb492b21635e482a476a03d1
The commit has NOT been mentioned in any issue.
The commit has been mentioned in the following pull-request(s):
- https://github.com/Rust-GCC/gccrs/pull/4551
gcc/rust/checks/lints/rust-lint-marklive.cc | 7 ++-
.../lints/unused/rust-unused-checker.cc | 22 +++++----
.../lints/unused/rust-unused-collector.cc | 11 +++--
.../lints/unused/rust-unused-collector.h | 48 +++++++++++++++----
.../resolve/rust-early-name-resolver-2.0.cc | 8 ++--
.../resolve/rust-name-resolution-context.hxx | 15 ++++--
gcc/rust/rust-session-manager.cc | 6 ---
.../typecheck/rust-hir-type-check-pattern.cc | 8 ++--
8 files changed, 84 insertions(+), 41 deletions(-)
diff --git a/gcc/rust/checks/lints/rust-lint-marklive.cc
b/gcc/rust/checks/lints/rust-lint-marklive.cc
index 3998cb70f..8fc81b363 100644
--- a/gcc/rust/checks/lints/rust-lint-marklive.cc
+++ b/gcc/rust/checks/lints/rust-lint-marklive.cc
@@ -165,8 +165,11 @@ MarkLive::visit_path_segment (HIR::PathExprSegment seg)
// We should mark them alive all and ignoring other kind of segments.
// If the segment we dont care then just return false is fine
// TODO: Should we look that up in all namespaces?
- if (auto id = resolver.lookup (ast_node_id, Resolver2_0::Namespace::Values))
- ref_node_id = *id;
+
+ if (auto nslookup
+ = resolver.lookup (ast_node_id, Resolver2_0::Namespace::Values,
+ Resolver2_0::Namespace::Types))
+ ref_node_id = nslookup->id;
else
return false;
if (auto hid = mappings.lookup_node_to_hir (ref_node_id))
diff --git a/gcc/rust/checks/lints/unused/rust-unused-checker.cc
b/gcc/rust/checks/lints/unused/rust-unused-checker.cc
index 06bbfd446..571e31e43 100644
--- a/gcc/rust/checks/lints/unused/rust-unused-checker.cc
+++ b/gcc/rust/checks/lints/unused/rust-unused-checker.cc
@@ -105,21 +105,25 @@ UnusedChecker::visit (HIR::IdentifierPattern &pattern)
"variable %qs should have a snake case name",
var_name.c_str ());
}
-void
+void
UnusedChecker::visit (HIR::AssignmentExpr &expr)
-
{
const auto &lhs = expr.get_lhs ();
auto var_name = lhs.to_string ();
NodeId ast_node_id = lhs.get_mappings ().get_nodeid ();
- NodeId def_id
- = nr_context.lookup (ast_node_id, Resolver2_0::Namespace::Values).value ();
- HirId id = mappings.lookup_node_to_hir (def_id).value ();
- if (unused_context.is_variable_assigned (id, lhs.get_mappings ().get_hirid
())
- && var_name[0] != '_')
- rust_warning_at (lhs.get_locus (), OPT_Wunused_variable,
- "unused assignment %qs", var_name.c_str ());
+ if (auto def_id
+ = nr_context.lookup (ast_node_id, Resolver2_0::Namespace::Values))
+ {
+ if (auto id = mappings.lookup_node_to_hir (*def_id))
+ {
+ if (unused_context.is_variable_assigned (
+ *id, lhs.get_mappings ().get_hirid ())
+ && var_name[0] != '_')
+ rust_warning_at (lhs.get_locus (), OPT_Wunused_variable,
+ "unused assignment %qs", var_name.c_str ());
+ }
+ }
}
void
diff --git a/gcc/rust/checks/lints/unused/rust-unused-collector.cc
b/gcc/rust/checks/lints/unused/rust-unused-collector.cc
index 5596dd97a..96920efb9 100644
--- a/gcc/rust/checks/lints/unused/rust-unused-collector.cc
+++ b/gcc/rust/checks/lints/unused/rust-unused-collector.cc
@@ -62,10 +62,13 @@ UnusedCollector::visit (HIR::StructExprFieldIdentifier
&ident)
void
UnusedCollector::visit (HIR::AssignmentExpr &expr)
{
- auto def_id = get_def_id (expr.get_lhs (), Resolver2_0::Namespace::Values);
- HirId id = expr.get_lhs ().get_mappings ().get_hirid ();
- unused_context.remove_mut (def_id);
- unused_context.add_assign (def_id, id);
+ if (auto def_id
+ = get_def_id (expr.get_lhs (), Resolver2_0::Namespace::Values))
+ {
+ HirId id = expr.get_lhs ().get_mappings ().get_hirid ();
+ unused_context.remove_mut (*def_id);
+ unused_context.add_assign (*def_id, id);
+ }
visit_outer_attrs (expr);
expr.get_rhs ().accept_vis (*this);
}
diff --git a/gcc/rust/checks/lints/unused/rust-unused-collector.h
b/gcc/rust/checks/lints/unused/rust-unused-collector.h
index 5059f7689..a506bff80 100644
--- a/gcc/rust/checks/lints/unused/rust-unused-collector.h
+++ b/gcc/rust/checks/lints/unused/rust-unused-collector.h
@@ -57,25 +57,55 @@ private:
virtual void visit (HIR::ContinueExpr &expr) override;
template <typename T>
- HirId get_def_id (T &path_expr, Resolver2_0::Namespace ns)
+ tl::optional<HirId> get_def_id (T &path_expr, Resolver2_0::Namespace ns)
{
NodeId ast_node_id = path_expr.get_mappings ().get_nodeid ();
- NodeId id = nr_context.lookup (ast_node_id, ns).value ();
- HirId def_id = mappings.lookup_node_to_hir (id).value ();
- return def_id;
+
+ if (auto id = nr_context.lookup (ast_node_id, ns))
+ {
+ if (auto def_id = mappings.lookup_node_to_hir (*id))
+ {
+ return *def_id;
+ }
+ }
+ return tl::nullopt;
+ }
+
+ template <typename T>
+ tl::optional<HirId> get_def_id (T &path_expr, Resolver2_0::Namespace ns1,
+ Resolver2_0::Namespace ns2)
+ {
+ NodeId ast_node_id = path_expr.get_mappings ().get_nodeid ();
+
+ if (auto nslookup = nr_context.lookup (ast_node_id, ns1, ns2))
+ {
+ NodeId id = nslookup->id;
+
+ if (auto def_id = mappings.lookup_node_to_hir (id))
+ {
+ return *def_id;
+ }
+ }
+
+ return tl::nullopt;
}
template <typename T> void mark_path_used (T &path_expr)
{
- auto def_id = get_def_id (path_expr, Resolver2_0::Namespace::Values);
- unused_context.add_variable (def_id);
- unused_context.remove_assign (def_id);
+ if (auto def_id = get_def_id (path_expr, Resolver2_0::Namespace::Values,
+ Resolver2_0::Namespace::Types))
+ {
+ unused_context.add_variable (*def_id);
+ unused_context.remove_assign (*def_id);
+ }
}
template <typename T> void mark_label_used (T &path_expr)
{
- auto def_id = get_def_id (path_expr, Resolver2_0::Namespace::Labels);
- unused_context.add_label (def_id);
+ if (auto def_id = get_def_id (path_expr, Resolver2_0::Namespace::Labels))
+ {
+ unused_context.add_label (*def_id);
+ }
}
};
} // namespace Analysis
diff --git a/gcc/rust/resolve/rust-early-name-resolver-2.0.cc
b/gcc/rust/resolve/rust-early-name-resolver-2.0.cc
index 148385300..1d63b6392 100644
--- a/gcc/rust/resolve/rust-early-name-resolver-2.0.cc
+++ b/gcc/rust/resolve/rust-early-name-resolver-2.0.cc
@@ -467,7 +467,8 @@ Early::finalize_simple_import (const Early::ImportPair
&mapping)
Definition (definition.definition.get_node_id ()),
definition.ns);
- toplevel.insert_or_error_out (identifier, import.get_locus (), import_id,
+ toplevel.insert_or_error_out (identifier, import.get_locus (),
+ definition.definition.get_node_id (),
definition.ns);
dirty = dirty || toplevel.is_dirty ();
@@ -523,7 +524,7 @@ Early::finalize_rebind_import (const Early::ImportPair
&mapping)
{
// Erroneous `self` or `{self}` use declaration
if (segments.size () == 1)
- break;
+ return;
declared_name = segments[segments.size () - 2].as_string ();
import_id = segments[segments.size () - 2].get_node_id ();
}
@@ -548,7 +549,8 @@ Early::finalize_rebind_import (const Early::ImportPair
&mapping)
Definition (definition.definition.get_node_id ()),
definition.ns);
- toplevel.insert_or_error_out (declared_name, path.get_locus (),
import_id,
+ toplevel.insert_or_error_out (declared_name, path.get_locus (),
+ definition.definition.get_node_id (),
definition.ns);
dirty = dirty || toplevel.is_dirty ();
diff --git a/gcc/rust/resolve/rust-name-resolution-context.hxx
b/gcc/rust/resolve/rust-name-resolution-context.hxx
index c16bae874..359dd9c65 100644
--- a/gcc/rust/resolve/rust-name-resolution-context.hxx
+++ b/gcc/rust/resolve/rust-name-resolution-context.hxx
@@ -353,13 +353,20 @@ NameResolutionContext::resolve_segments (
.lookup_glob_container (rib_lookup->get_node_id ())
.has_value ())
{
- auto leaf_module
+ NodeId leaf_module
= stack.find_leaf_definition (rib_lookup->get_node_id ())
- .value ()
+ .value_or (Definition (rib_lookup->get_node_id ()))
.id;
- child = stack.dfs_node (stack.root, leaf_module).value ();
- break;
+ if (auto new_child = stack.dfs_node (stack.root, leaf_module))
+ {
+ child = new_child.value ();
+ break;
+ }
+ else
+ {
+ return tl::nullopt;
+ }
}
else
{
diff --git a/gcc/rust/rust-session-manager.cc b/gcc/rust/rust-session-manager.cc
index b3bc99249..5eda91da9 100644
--- a/gcc/rust/rust-session-manager.cc
+++ b/gcc/rust/rust-session-manager.cc
@@ -764,15 +764,9 @@ Session::compile_crate (const char *filename)
if (last_step == CompileOptions::CompileStep::NameResolution)
return;
- // FIXME: Or run it within Late at the end?
- name_resolution_ctx.flatten ();
-
// resolution pipeline stage
Resolver2_0::Late (name_resolution_ctx).go (parsed_crate);
- // FIXME: Or run it within Late at the end?
- name_resolution_ctx.flatten ();
-
if (options.dump_option_enabled (CompileOptions::RESOLUTION_DUMP))
dump_name_resolution (name_resolution_ctx);
diff --git a/gcc/rust/typecheck/rust-hir-type-check-pattern.cc
b/gcc/rust/typecheck/rust-hir-type-check-pattern.cc
index 8c99790f4..7d12bc7b4 100644
--- a/gcc/rust/typecheck/rust-hir-type-check-pattern.cc
+++ b/gcc/rust/typecheck/rust-hir-type-check-pattern.cc
@@ -57,11 +57,11 @@ TypeCheckPattern::visit (HIR::PathInExpression &pattern)
auto &nr_ctx = Resolver2_0::FinalizedNameResolutionContext::get ();
- // FIXME: Not sure if this is enough to look up enum variants
- if (auto id = nr_ctx.lookup (pattern.get_mappings ().get_nodeid (),
- Resolver2_0::Namespace::Values))
+ if (auto nslookup = nr_ctx.lookup (pattern.get_mappings ().get_nodeid (),
+ Resolver2_0::Namespace::Values,
+ Resolver2_0::Namespace::Types))
{
- ref_node_id = *id;
+ ref_node_id = nslookup->id;
maybe_item = true;
}
--
2.54.0
