fresh binding should shadow the def in expand #143141
Conversation
tests/ui/resolve/fresh-should-shallow-definitation-after-macro-expand.rs
Outdated
Show resolved
Hide resolved
tests/ui/resolve/fresh-should-shallow-definitation-after-macro-expand.rs
Outdated
Show resolved
Hide resolved
| macro_rules! m {() => ( f() )} | ||
| use m; | ||
| let b: i16 = m!(); | ||
| } |
There was a problem hiding this comment.
It would be good to test all permutations of
fn f
let f
macro_rules! m
use m
in all possible orders.
There should be 12 of them (the ones with use m going before macro_rules! m are impossible).
The testing statements let a/b/c/... can then be inserted between all of them.
let a
fn f
let b
let f
let c
macro_rules! m
let d
use m
let e
Then we'll get exhaustive testing.
Upd: i8 -> FnF, i16 -> LetF.
There was a problem hiding this comment.
Some more test cases:
{
m!(); // in a block before the import
}
macro_rules! m { ... }
use m;There was a problem hiding this comment.
As a note, we should add additional test cases like without_decl_f and without_closure_f for better coverage.
There was a problem hiding this comment.
note on cases in f0~f23:
- For
let a: i8 = m!()inf8~f23, the macro expansion's f refers to function f. There are two distinct scenarios:- Using the declared function when found at macro definition site:
This behaves similarly to:fn f20() { macro_rules! m {() => ( f() )} // Only resolves to `fn f` at this position use m; fn f() -> i8 { 42 } let f = || -> i16 { 42 }; let a: i8 = m!(); }
fn f() { let x = 0; macro_rules! foo { () => { assert_eq!(x, 0); } } let x = 1; foo!(); }
- Using the declared function when the closure is unavailable:
fn b12() { let a: i8 = m!(); fn f() -> i8 { 42 } let f = || -> i16 { 42 }; macro_rules! m {() => ( f() )} // Although both exist, `let a` can only use `fn f` use m; }
- In other cases, it refers to the closure
f.
There was a problem hiding this comment.
Could you please review whether these new test cases match the expected behavior? I'm uncertain if this might conflict with the intended design. The LookaheadMacro may incorrectly if conflicts occur.
There was a problem hiding this comment.
And I thought 12 test cases were a bit too much, and now there are 24 * 3 of them :D
i8->FnF,i16->LetF
Could you apply this ^^^ suggestion? Otherwise it's just painful to read all the tests cases carefully.
There was a problem hiding this comment.
Apologies for the oversight. I've now added all combinations(4 * 3 * 2 * 1)🤦. Please let me know if any duplicate cases that should be removed.
Could you apply this ^^^ suggestion
done
|
I don't understand why this works and how it fixes the issue. |
|
Also "shallow" -> "shadow" in the PR/commit messages and file names. |
There may be a bug if
rust/compiler/rustc_resolve/src/ident.rs Lines 320 to 328 in 86e05cd |
|
@rustbot ready |
|
@bvanjoi |
|
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 |
|
Reminder, once the PR becomes ready for a review, use |
| let f = || -> i16 { 42 }; | ||
| let a: i8 = m!(); | ||
| fn f() -> i8 { 42 } | ||
| } |
There was a problem hiding this comment.
f22 and f23 can be merged into a single function.
Same with all other test case pairs.
fn f12() {
macro_rules! m {() => ( f() )}
use m;
let f = || -> i16 { 42 };
let a: i8 = m!();
fn f() -> i8 { 42 }
let b: i8 = m!();
}| let a: i8 = m!(); | ||
| fn f() -> i8 { 42 } | ||
| let f = || -> i16 { 42 }; | ||
| macro m() { f() } |
There was a problem hiding this comment.
This is incorrect, f is the closure at the macro's definition site.
This should either produce an error during name resolution, or resolve to the closure and produce some kind of borrow checker error later because f-the-closure is clearly not yet live at the point of use.
We do not currently treat this correctly because the whole algorithm in resolve_ident_in_lexical_scope works in a very questionable way that doesn't really match th
28C5
e notion of "definition site resolution" for macros.
There was a problem hiding this comment.
I see. I will investigate it further.
There was a problem hiding this comment.
btw, I would like to verify whether the test case passes as expected (though I believe it should):
type FnF = i8;
type LetF = i16;
fn f() {
let a: FnF = f();
fn f() -> FnF { 42 }
let f = || -> LetF { 42 };
}| } | ||
|
|
||
| fn f13() { | ||
| let a: i8 = m!(); |
There was a problem hiding this comment.
Same as the previous comment.
|
|
||
| fn f8() { | ||
| fn f() -> i8 { 42 } | ||
| let a: i8 = m!(); |
There was a problem hiding this comment.
Same as the previous comment.
| fn b8() { | ||
| fn f() -> i8 { 42 } | ||
| { | ||
| let a: i8 = m!(); |
There was a problem hiding this comment.
Same as the previous comment.
Fixes #95237
r? @petrochenkov or @cjgillot