fresh binding should shadow the def in expand #143141

bvanjoi · 2025-06-28T13:53:43Z

Fixes #95237

r? @petrochenkov or @cjgillot

compiler/rustc_resolve/src/late.rs

tests/ui/resolve/fresh-should-shallow-definitation-after-macro-expand.rs

compiler/rustc_resolve/src/late.rs

petrochenkov · 2025-06-30T18:27:07Z

I don't understand why this works and how it fixes the issue.
The LookAheadMacroDefinition are inserted for all macro definitions, regardless of whether it's macro or macro_rules, and nothing is done for use items.
The reversed rib walk in fn apply_pattern_bindings can also encounter arbitrary ribs before encountering a LookAheadMacroDefinition.

petrochenkov · 2025-06-30T18:32:33Z

Also "shallow" -> "shadow" in the PR/commit messages and file names.

bvanjoi · 2025-07-01T13:49:57Z

The reversed rib walk in fn apply_pattern_bindings can also encounter arbitrary ribs before encountering a LookAheadMacroDefinition.

There may be a bug if LookAheadMacroDefinition fails to apply correctly.

I don't understand why this works and how it fixes the issue.

LookAheadMacroDefinition stores the macro expansion result and serves as a fallback in resolve_ident_in_lexical_scope, ensuring it takes the correct resolution path:

rust/compiler/rustc_resolve/src/ident.rs

Lines 320 to 328 in 86e05cd

    
           // The ident resolves to a type parameter or local variable. 
        
           return Some(LexicalScopeBinding::Res(self.validate_res_from_ribs( 
        
               i, 
        
               rib_ident, 
        
               *res, 
        
               finalize.map(|finalize| finalize.path_span), 
        
               *original_rib_ident_def, 
        
               ribs, 
        
           )));

bvanjoi · 2025-07-01T16:29:50Z

@rustbot ready

petrochenkov · 2025-07-12T09:11:03Z

@bvanjoi
Are you sure this cannot successfully resolve some names that should not be resolved?
(I started reviewing yesterday, but it was late so I'll finish today or on Monday.)

bvanjoi · 2025-07-12T09:31:52Z

Are you sure this cannot successfully resolve some names that should not be resolved?

I need to test this with additional cases across different rib contexts. @rustbot author

rustbot · 2025-07-12T09:31:55Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

tests/ui/resolve/fresh-should-shadow-definitation-in-macro-expand.rs

tests/ui/resolve/fresh-should-shadow-definitation-in-decl-macro-expand.rs

tests/ui/resolve/fresh-should-shadow-definitation-in-macro-expand.rs

tests/ui/resolve/fresh-should-shadow-definitation-in-macro-expand-in-block.rs

bvanjoi · 2025-08-05T17:08:38Z

Update: The key change in this patch introduces RibKind::Block, which consolidates both fresh bindings and macro definitions within a block scope, while verifying their accessibility at macro call sites.

@rustbot ready

bvanjoi · 2025-08-05T17:09:58Z

compiler/rustc_resolve/src/late.rs

        // Descend into the block.
        for stmt in &block.stmts {
            if let StmtKind::Item(ref item) = stmt.kind
                && let ItemKind::MacroDef(..) = item.kind
            {
                num_macro_definition_ribs += 1;
                let res = self.r.local_def_id(item.id).to_def_id();
-                self.ribs[ValueNS].push(Rib::new(RibKind::MacroDefinition(res)));


I think we can remove RibKind::MacroDefinition in the future since all macro bindings can be accessed through RibKind::Block.

If the new scheme is already implemented, then it's better to remove it now to make sure everything is migrated correctly.

I see, it's still there due to labels, but labels are supposed to be resolved just like local variables, so they can migrate to the new scheme too.

bvanjoi · 2025-08-05T17:11:48Z

compiler/rustc_resolve/src/late.rs

@@ -4658,6 +4729,7 @@ impl<'a, 'ast, 'ra, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
        // Move down in the graph, if there's an anonymous module rooted here.
        let orig_module = self.parent_scope.module;
        let anonymous_module = self.r.block_map.get(&block.id).cloned(); // clones a reference
+        let is_module_block = anonymous_module.is_some();


I think we can remove RibKind::Module in the future and use something like RibKind::Block { id: NodeId, module: Option<Module> }.

Let's do this, and only add Block ribs for blocks, not Module ribs or Normal ribs.
I'd rather do it before making changes from this PR.

petrochenkov · 2025-08-06T18:38:31Z

compiler/rustc_resolve/src/late.rs

+        );
+    }
+
+    fn collect_fresh_binding_in_pat(&mut self, pat: &ast::Pat) {


Could you inline these 3 methods?
They are all small and are called only once.

compiler/rustc_resolve/src/late.rs

petrochenkov · 2025-08-06T18:41:58Z

compiler/rustc_resolve/src/late.rs

@@ -4669,14 +4741,15 @@ impl<'a, 'ast, 'ra, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
            self.ribs[ValueNS].push(Rib::new(RibKind::Normal));
        }

+        self.ribs[ValueNS]
+            .push(Rib::new(RibKind::Block { id: block.id, is_module: is_module_block }));


Suggested change

.push(Rib::new(RibKind::Block { id: block.id, is_module: is_module_block }));

.push(Rib::new(RibKind::Block { id: block.id, is_module }));

Nit: rename for brevity.

petrochenkov · 2025-08-06T18:44:07Z

compiler/rustc_resolve/src/late.rs

        // Descend into the block.
        for stmt in &block.stmts {
            if let StmtKind::Item(ref item) = stmt.kind
                && let ItemKind::MacroDef(..) = item.kind
            {
                num_macro_definition_ribs += 1;
                let res = self.r.local_def_id(item.id).to_def_id();
-                self.ribs[ValueNS].push(Rib::new(RibKind::MacroDefinition(res)));
                self.label_ribs.push(Rib::new(RibKind::MacroDefinition(res)));


Labels have the same hygiene as local variables and are also resolved at macro definition site.

petrochenkov · 2025-08-06T18:51:17Z

compiler/rustc_resolve/src/late.rs

            self.label_ribs.pop();
        }
-        self.last_block_rib = self.ribs[ValueNS].pop();
+        let block_rib = self.ribs[ValueNS].pop(); // pop `RibKind::Block`
+        if !is_module_block {


Modules are indeed not created for some blocks, but that's only a performance optimization.
There should be zero semantic difference between module blocks and non-module blocks, but this PR seems to add some.

If I remove the optimization and run ui tests some diagnostics change.
That's not very good, it means there are some pre-existing issues with this.

but this PR seems to add some

They're only to generate last_block_rib which is only used for diagnostics, specifically to display information about non-module blocks:

rust/compiler/rustc_resolve/src/late/diagnostics.rs

Lines 853 to 870 in 6bcdcc7

// Try to find in last block rib

if let Some(rib) = &self.last_block_rib

&& let RibKind::Normal = rib.kind

{

for (ident, &res) in &rib.bindings {

if let Res::Local(_) = res

&& path.len() == 1

&& ident.span.eq_ctxt(path[0].ident.span)

&& ident.name == path[0].ident.name

{

err.span_help(

ident.span,

format!("the binding `{path_str}` is available in a different scope in the same function"),

);

return (true, suggested_candidates, candidates);

}

}

}

specifically to display information about non-module blocks

That's exactly what I'm talking about - a pre-existing bug.
This code should work for all blocks, regardless of whether those blocks potentially have items in them or not.

petrochenkov · 2025-08-06T18:56:32Z

(I didn't review everything yet, will continue later this week.)

petrochenkov · 2025-08-07T15:46:55Z

compiler/rustc_resolve/src/late.rs

+                    }
+                    LookaheadItemInBlock::MacroDef { bindings: macro_bindings, .. } => {
+                        let bindings =
+                            bindings.last().unwrap().1.iter().filter_map(|(name, res)| {


Suggested change

bindings.last().unwrap().1.iter().filter_map(|(name, res)| {

bindings.last().unwrap().1.iter().copied().filter(|(name, _)| !need_removed.contains(name))

petrochenkov · 2025-08-07T15:51:54Z

compiler/rustc_resolve/src/late.rs

+
+        if let Some(last_pat_id) = last_pat_id
+            && let RibKind::Block { id: block, .. } = self.ribs[ValueNS].last_mut().unwrap().kind
+            && let Some(items) = self.r.lookahead_items_in_block.get_mut(&block)


So, in

let a = 0; { let b = 1; macro_rules! m { ... } }

only b will be added to LookaheadItemInBlock::MacroDef bindings for m, but not a.

Is this ok?
If yes, why is this ok?

petrochenkov · 2025-08-07T15:59:55Z

tests/ui/resolve/fresh-should-shadow-definitation-in-decl-macro-expand.rs

+    let a0: BindingF = m!();        //~ NOTE in this expansion of m!
+    let f = || -> BindingF { 42 };
+    let a1: BindingF = m!();
+    macro m() { f() }               //~ ERROR cannot find function `f` in this scope


f can be found at m's definition site, we should report a different error in this case, like "f is not yet alive at point of use" or something.

petrochenkov · 2025-08-07T16:14:13Z

Could you also add a test case demonstrating what happens with this example?

fn f() {
    let a = 10;
    
    #[macro_export]
    macro_rules! m { () => { a } }
}

fn main() {
    fn a() {}

    crate::m!();
}

bors · 2025-08-08T09:37:21Z

☔ The latest upstream changes (presumably #145077) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned petrochenkov Jun 28, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 28, 2025

bvanjoi commented Jun 28, 2025

View reviewed changes

compiler/rustc_resolve/src/late.rs Outdated Show resolved Hide resolved

bvanjoi commented Jun 28, 2025

View reviewed changes

tests/ui/resolve/fresh-should-shallow-definitation-after-macro-expand.rs Outdated Show resolved Hide resolved

bvanjoi commented Jun 28, 2025

View reviewed changes

tests/ui/resolve/fresh-should-shallow-definitation-after-macro-expand.rs Outdated Show resolved Hide resolved

petrochenkov reviewed Jun 30, 2025

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 30, 2025

bvanjoi force-pushed the issue-95237 branch from 4d7a3eb to 3bfa817 Compare July 1, 2025 13:39

bvanjoi changed the title ~~fresh binding should shallow the def after expand~~ fresh binding should shadow the def in expand Jul 1, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 1, 2025

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2025

petrochenkov reviewed Jul 14, 2025

View reviewed changes

bvanjoi force-pushed the issue-95237 branch from 3bfa817 to ec8d2bf Compare July 14, 2025 16:18

bvanjoi force-pushed the issue-95237 branch from ec8d2bf to a86854c Compare August 2, 2025 18:46

This comment has been minimized.

Sign in to view

bvanjoi marked this pull request as draft August 3, 2025 14:56

bvanjoi force-pushed the issue-95237 branch from a86854c to 3af335e Compare August 3, 2025 17:11

This comment has been minimized.

Sign in to view

bvanjoi force-pushed the issue-95237 branch from 3af335e to 842d9ca Compare August 5, 2025 15:42

bvanjoi marked this pull request as ready for review August 5, 2025 17:06

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 5, 2025

fresh binding should shadow the def after expand

92f5d0c

bvanjoi force-pushed the issue-95237 branch from 842d9ca to 92f5d0c Compare August 5, 2025 17:07

bvanjoi commented Aug 5, 2025

View reviewed changes

petrochenkov reviewed Aug 6, 2025

View reviewed changes

petrochenkov reviewed Aug 7, 2025

View reviewed changes

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 7, 2025

bvanjoi mentioned this pull request Aug 7, 2025

add block kind during name resolution #145058

Closed

petrochenkov mentioned this pull request Aug 7, 2025

resolve: Introduce RibKind::Block #145065

Open

	.push(Rib::new(RibKind::Block { id: block.id, is_module: is_module_block }));
	.push(Rib::new(RibKind::Block { id: block.id, is_module }));

	// Try to find in last block rib
	if let Some(rib) = &self.last_block_rib
	&& let RibKind::Normal = rib.kind
	{
	for (ident, &res) in &rib.bindings {
	if let Res::Local(_) = res
	&& path.len() == 1
	&& ident.span.eq_ctxt(path[0].ident.span)
	&& ident.name == path[0].ident.name
	{
	err.span_help(
	ident.span,
	format!("the binding `{path_str}` is available in a different scope in the same function"),
	);
	return (true, suggested_candidates, candidates);
	}
	}
	}

	bindings.last().unwrap().1.iter().filter_map(\|(name, res)\| {
	bindings.last().unwrap().1.iter().copied().filter(\|(name, _)\| !need_removed.contains(name))

fresh binding should shadow the def in expand #143141

Are you sure you want to change the base?

fresh binding should shadow the def in expand #143141

Conversation

bvanjoi commented Jun 28, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petrochenkov commented Jun 30, 2025

Uh oh!

petrochenkov commented Jun 30, 2025

Uh oh!

bvanjoi commented Jul 1, 2025

Uh oh!

bvanjoi commented Jul 1, 2025

Uh oh!

petrochenkov commented Jul 12, 2025

Uh oh!

bvanjoi commented Jul 12, 2025

Uh oh!

rustbot commented Jul 12, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

bvanjoi commented Aug 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Aug 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Aug 7, 2025

Uh oh!

bors commented Aug 8, 2025

Uh oh!

Uh oh!

bvanjoi commented Jun 28, 2025 •

edited by rustbot

Loading