Fix modules with same name as builtins causing ICE (#3315) #3437

liamnaddell · 2025-02-14T04:24:21Z

@powerboat9 I can update to move the language prelude to rust-forever-stack.hxx if you still want the change

gcc/rust/ChangeLog:

	* resolve/rust-forever-stack.h: Add a dedicated prelude node for the Language prelude
	* resolve/rust-forever-stack.hxx: Add support code for the prelude node
	* resolve/rust-late-name-resolver-2.0.cc (Late::visit): Move language prelude builtins to the prelude context
	* resolve/rust-name-resolution-context.cc: Add code for handling the prelude corner case
	* resolve/rust-rib.h: Add a special Prelude rib type

gcc/testsuite/ChangeLog:

	* rust/compile/issue-3315-1.rs: Add test for module with same name as builtin
	* rust/compile/issue-3315-2.rs: Test with utilization of i32 type
	* rust/compile/nr2/exclude: issue-3315-2.rs Does not work with NR2.0

Thank you for making Rust GCC better!

If your PR fixes an issue, you can add "Fixes #issue_number" into this
PR description and the git commit message. This way the issue will be
automatically closed when your PR is merged. If your change addresses
an issue but does not fully fix it please mark it as "Addresses #issue_number"
in the git commit message.

Here is a checklist to help you with your PR.

GCC development requires copyright assignment or the Developer's Certificate of Origin sign-off, see https://gcc.gnu.org/contribute.html or https://gcc.gnu.org/dco.html
Read contributing guidlines
make check-rust passes locally
Run clang-format
Added any relevant test cases to gcc/testsuite/rust/

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

*Please write a comment explaining your change. This is the message
that will be part of the merge commit.

powerboat9 · 2025-02-15T22:29:19Z

gcc/rust/resolve/rust-late-name-resolver-2.0.cc

+	// we're at the root :) hopefully! Globbed declaration allows
+	// user-defined items in the `types` namespace
+	//  to shadow builtins. Probably unwise, but allowed by rustc!
+	auto ok = ctx.types.insert_globbed (builtin.name, builtin.node_id);


Since it's in a separate rib, insert, insert_shadowable, or insert_globbed would all work fine here

powerboat9 · 2025-02-15T22:35:56Z

gcc/rust/resolve/rust-name-resolution-context.cc

@@ -107,6 +107,11 @@ NameResolutionContext::scoped (Rib::Kind rib_kind, NodeId id,
 			       std::function<void (void)> lambda,
 			       tl::optional<Identifier> path)
 {
+  // Prelude doesn't have an access path
+  // TODO: Assert current cursor is at the root


This should already hit an assertion, so you could remove the comment or add another assertion

powerboat9 · 2025-02-15T22:36:58Z

gcc/rust/resolve/rust-name-resolution-context.cc

+  // Prelude doesn't have an access path
+  // TODO: Assert current cursor is at the root
+  if (rib_kind == Rib::Kind::Prelude)
+    rust_assert (!path);


This could be moved to push_inner or copied there, but an assertion like this should probably be in push_inner

powerboat9 · 2025-02-15T22:37:53Z

@powerboat9 I can update to move the language prelude to rust-forever-stack.hxx if you still want the change

It's alright, it looks good

liamnaddell · 2025-02-15T22:53:53Z

@powerboat9 Thank you for the reviews, and sorry about the TODO: comments -- I forgot about those.

I think I fixed all of the issues you pointed out.

philberty

Looks like a regression for:

Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/borrowck/borrowck.exp ...
Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/compile.exp ...
Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/macros/builtin/builtin_macro.exp ...
Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/macros/mbe/mbe_macro.exp ...
Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/macros/proc/proc_macro.exp ...
Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/nr2/compile.exp ...
FAIL: /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/macros/mbe/macro54.rs: nr2 failure: FAIL: rust/compile/macros/mbe/macro54.rs -frust-name-resolution-2.0 (test for excess errors)

liamnaddell · 2025-02-24T01:43:01Z

Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/macros/mbe/mbe_macro.exp ...
Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/macros/proc/proc_macro.exp ...
Running /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/nr2/compile.exp ...
FAIL: /home/runner/work/gccrs/gccrs/gcc/testsuite/rust/compile/macros/mbe/macro54.rs: nr2 failure: FAIL: rust/compile/macros/mbe/macro54.rs -frust-name-resolution-2.0 (test for excess errors)

For some reason after rebasing these fail now. I'll take a look at these

liamnaddell · 2025-02-25T02:57:58Z

I think the strategy I chose to implement this doesn't work, since the test involves resolving something like u32::VALUE.

I'll try to re-implement

liamnaddell · 2025-03-02T20:49:27Z

@powerboat9 @philberty I added some code to get_segments to handle the prelude rib as a special case. I think my PR should work now

powerboat9

Otherwise, looks great

powerboat9 · 2025-03-02T22:07:06Z

gcc/rust/resolve/rust-forever-stack.hxx

+    {
+      // If you push_inner into the prelude from outside the root, you will pop
+      // back into the
+      //  root, which could screw up a traversal.


Should probably adjust the formatting on this comment

powerboat9 · 2025-03-02T22:10:24Z

gcc/rust/resolve/rust-forever-stack.hxx

@@ -487,6 +519,7 @@ ForeverStack<N>::resolve_segments (
       *


Should probably add an entry to the numbered list in the above comment, mentioning changing searched_prelude from false to true

I added a comment for this, let me know if it needs modification.

powerboat9 · 2025-03-02T22:23:05Z

gcc/rust/resolve/rust-forever-stack.h

+   * It has the root node as a parent, and acts as a "special case" for name
+   * resolution
+   */
+  Node prelude;


I probably should have noticed and commented on this earlier, but this should really be called something like language_prelude, given that we'll have to deal with multiple preludes

I considered that as well, I figured that we would also dump the std::prelude and core::prelude in here as well

I figured we'd keep some sort of reference to the standard library prelude -- since that should already be a module with its own Node, we shouldn't need to try to copy entries into another Node

I think the comment there clarifies that it's currently only used for the language prelude (language builtins). If it needs to be beefed up with a better explanation I can do that.

powerboat9 · 2025-03-02T22:43:16Z

gcc/rust/resolve/rust-name-resolution-context.cc

@@ -107,6 +107,7 @@ NameResolutionContext::scoped (Rib::Kind rib_kind, NodeId id,
 			       std::function<void (void)> lambda,
 			       tl::optional<Identifier> path)
 {
+  // NOTE: You should be at the root node when calling this function.


You should be at the root node, if pushing a Prelude rib kind

I'll update to clarify that we only need to be at the root for the prelude rib pushes

powerboat9 · 2025-03-02T22:48:13Z

It looks like rustc has some backtracking capability. We may have to implement something like that later, for full parity, but that's probably outside the scope of this PR

gcc/rust/ChangeLog: * resolve/rust-forever-stack.h: Add a dedicated prelude node for the Language prelude * resolve/rust-forever-stack.hxx: Add support code for the prelude node * resolve/rust-late-name-resolver-2.0.cc (Late::visit): Move language prelude builtins to the prelude context * resolve/rust-name-resolution-context.cc: Add code for handling the prelude corner case * resolve/rust-rib.h: Add a special Prelude rib type gcc/testsuite/ChangeLog: * rust/compile/issue-3315-1.rs: Add test for module with same name as builtin * rust/compile/issue-3315-2.rs: Test with utilization of i32 type * rust/compile/nr2/exclude: issue-3315-2.rs Does not work with NR2.0 Signed-off-by: Liam Naddell <liamnprg@gmail.com>

liamnaddell force-pushed the modname branch 5 times, most recently from 44e6e4e to 98f7902 Compare February 15, 2025 18:42

powerboat9 reviewed Feb 15, 2025

View reviewed changes

liamnaddell force-pushed the modname branch 2 times, most recently from c9a02b2 to 20e456a Compare February 15, 2025 22:53

powerboat9 approved these changes Feb 15, 2025

View reviewed changes

philberty added the name resolution label Feb 16, 2025

philberty requested review from P-E-P and CohenArthur February 16, 2025 16:43

philberty added this to the Name Resolution 2.0 rework milestone Feb 16, 2025

liamnaddell force-pushed the modname branch 3 times, most recently from 933eba4 to 05aaeb8 Compare February 23, 2025 15:02

philberty reviewed Feb 23, 2025

View reviewed changes

philberty assigned liamnaddell Feb 23, 2025

liamnaddell force-pushed the modname branch 5 times, most recently from a865b40 to dbc0b7d Compare March 2, 2025 20:48

powerboat9 reviewed Mar 2, 2025

View reviewed changes

liamnaddell force-pushed the modname branch from dbc0b7d to cf479cb Compare March 2, 2025 23:10

powerboat9 approved these changes Mar 2, 2025

View reviewed changes

liamnaddell requested a review from philberty March 5, 2025 01:14

liamnaddell force-pushed the modname branch from cf479cb to b142d83 Compare March 7, 2025 02:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix modules with same name as builtins causing ICE (#3315) #3437

Fix modules with same name as builtins causing ICE (#3315) #3437

liamnaddell commented Feb 14, 2025 •

edited

Loading

powerboat9 Feb 15, 2025

powerboat9 Feb 15, 2025

powerboat9 Feb 15, 2025

powerboat9 commented Feb 15, 2025

liamnaddell commented Feb 15, 2025

philberty left a comment

liamnaddell commented Feb 24, 2025

liamnaddell commented Feb 25, 2025

liamnaddell commented Mar 2, 2025

powerboat9 left a comment

powerboat9 Mar 2, 2025

liamnaddell Mar 2, 2025

powerboat9 Mar 2, 2025

liamnaddell Mar 2, 2025

powerboat9 Mar 2, 2025

liamnaddell Mar 2, 2025

powerboat9 Mar 2, 2025

liamnaddell Mar 2, 2025 •

edited

Loading

powerboat9 Mar 2, 2025

liamnaddell Mar 2, 2025

liamnaddell Mar 2, 2025

powerboat9 commented Mar 2, 2025 •

edited

Loading

Fix modules with same name as builtins causing ICE (#3315) #3437

Are you sure you want to change the base?

Fix modules with same name as builtins causing ICE (#3315) #3437

Conversation

liamnaddell commented Feb 14, 2025 • edited Loading

Note that you can skip the above if you are just opening a WIP PR in order to get feedback.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

powerboat9 commented Feb 15, 2025

liamnaddell commented Feb 15, 2025

philberty left a comment

Choose a reason for hiding this comment

liamnaddell commented Feb 24, 2025

liamnaddell commented Feb 25, 2025

liamnaddell commented Mar 2, 2025

powerboat9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liamnaddell Mar 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

powerboat9 commented Mar 2, 2025 • edited Loading

liamnaddell commented Feb 14, 2025 •

edited

Loading

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

liamnaddell Mar 2, 2025 •

edited

Loading

powerboat9 commented Mar 2, 2025 •

edited

Loading