delay expand macro bang when there has indeterminate path

[bvanjoi]

Related #98291

I will attempt to clarify the root problem through several examples:

Firstly,

// rustc code.rs --edition=2018

macro_rules! wrap {
    () => {
        macro_rules! _a {
            () => {
                "Hello world"
            };
        }
    };
}

wrap!();

use _a as a;

fn main() {
    format_args!(_a!());
}

The above case will compile successfully because _a is defined after the wrap expaned, ensuring _a can be resolved without any issues.

And,

// rustc code.rs --edition=2018

macro_rules! wrap {
    () => {
        macro_rules! _a {
            () => {
                "Hello world"
            };
        }
    };
}

wrap!();

use _a as a;

fn main() {
    format_args!("{}", a!());
}

The above example will also compile successfully because the parse_args in expand_format_args_impl will return a value MacroInput { fmtstr: Expr::Lit::Str, args: [Expr::MacroCall]}. Since the graph for args will be build lately, a will eventually be resolved.

However, in the case of:

// rustc code.rs --edition=2018

macro_rules! wrap {
    () => {
        macro_rules! _a {
            () => {
                "Hello world"
            };
        }
    };
}

wrap!();

use _a as a;

fn main() {
    format_args!(a!());
}

The result of parse_args is MacroInput {fmtstr: Expr::Lit::Macro, args: [] }, we attempt to expand fmtstr eagerly within expr_to_spanned_string. Although we have recorded (root, _a) into resolutions, use _a as a is an indeterminate import, which will not try to resolve under the conditions of expander.monotonic = false.

Therefore, I've altered the strategy for resolving indeterminate imports, ensuring it will also resolve during eager expansion. This could be a significant change to the resolution infra. However, I think it's acceptable if the goal of avoiding resolution under eager expansion is to save time.

r? @petrochenkov

[petrochenkov]

The right strategy here is for println! (and other similar macros) to do what #[cfg_accessible(path)] currently does

• Attempt to resolve a (or rather attempt to expand a!())
• If the attempt returns indeterminacy, then println! should return indeterminacy as well (ExpandResult::Retry)
• After that println! will return into the queue of unexpanded macros and we will try to expand it again some time later, when the resolution for use _a as a may already be available.

[petrochenkov]

What this PR does may be helpful too, but it's sort of luck based. We try to resolve imports one last time just before eagerly expanding a macro, and if it helps, then it helps, but if it's not (the necessary import is not yet read) then we are back to the same issue.

Let's actually try to benchmark this solution. @bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

:hourglass: Trying commit ee857f7808f54aa50b41b1990164de27f94ce38d with merge 8b0b84954eb02575b87834af9171a9c93a3f1691...

[bors]

:sunny: Try build successful - checks-actions Build commit: 8b0b84954eb02575b87834af9171a9c93a3f1691 (8b0b84954eb02575b87834af9171a9c93a3f1691)

[rust-timer]

Queued 8b0b84954eb02575b87834af9171a9c93a3f1691 with parent c7beecf3e3cef7a8226a99aec4e4f6bfc114ba8e, future comparison URL. There is currently 1 preceding artifact in the queue. It will probably take at least ~1.3 hours until the benchmark run finishes.

[rust-timer]

Finished benchmarking commit (8b0b84954eb02575b87834af9171a9c93a3f1691): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never @rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	6.8%	[0.2%, 84.6%]	149
Regressions ❌ (secondary)	0.4%	[0.2%, 1.0%]	10
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	6.8%	[0.2%, 84.6%]	149

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.8%	[-3.8%, -3.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.8%	[-3.8%, -3.8%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	6.9%	[1.1%, 43.7%]	63
Regressions ❌ (secondary)	5.8%	[5.8%, 5.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.0%	[-3.1%, -0.9%]	2
All ❌✅ (primary)	6.9%	[1.1%, 43.7%]	63

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.0%	[1.2%, 2.9%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[1.2%, 2.9%]	8

Bootstrap: 644.654s -> 666.022s (3.31%) Artifact size: 175.03 MiB -> 174.43 MiB (-0.34%)

[petrochenkov]

So, the current approach clearly won't work. @rustbot author

[bvanjoi]

Update: It will retry expand for macro_rules! when an indeterminate path is encountered

[bvanjoi]

ci is green. @rustbot ready

[bvanjoi]

@rustbot ready

[petrochenkov]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

:hourglass: Trying commit ab31e3e52e498e566e7668823b429ae6f0a89a7c with merge 0af3b723d85f7322081dc20664c34d0aadaf394a...

[bors]

:sunny: Try build successful - checks-actions Build commit: 0af3b723d85f7322081dc20664c34d0aadaf394a (0af3b723d85f7322081dc20664c34d0aadaf394a)

[rust-timer]

Queued 0af3b723d85f7322081dc20664c34d0aadaf394a with parent 3b85d2c7fc6d1698e68b94f7bc1a5c9633f2554d, future comparison URL. There are currently 4 preceding artifacts in the queue. It will probably take at least ~4.8 hours until the benchmark run finishes.

[rust-timer]

Finished benchmarking commit (0af3b723d85f7322081dc20664c34d0aadaf394a): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never @rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.7%, 0.7%]	1

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.0%	[2.0%, 2.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 674.911s -> 674.04s (-0.13%) Artifact size: 310.31 MiB -> 310.30 MiB (-0.00%)

[petrochenkov]

r=me with https://github.com/rust-lang/rust/pull/121589#discussion_r1521886941 addressed.

[rustbot]

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged. Otherwise, you can ignore this comment.

[bvanjoi]

@rustbot ready

[petrochenkov]

Thanks! Fixing this was in my todo list for quite some time. @bors r+

[bors]

:pushpin: Commit 8fcdf54a6b98c129e951caf3a97cbf20db677ee3 has been approved by petrochenkov

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 8fcdf54a6b98c129e951caf3a97cbf20db677ee3 with merge 184c5ab1807a5e605cc8235d3734a97552768c22...

[bors]

:sunny: Test successful - checks-actions Approved by: petrochenkov Pushing 184c5ab1807a5e605cc8235d3734a97552768c22 to master...

[rust-timer]

Finished benchmarking commit (184c5ab1807a5e605cc8235d3734a97552768c22): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 675.369s -> 675.199s (-0.03%) Artifact size: 310.83 MiB -> 310.80 MiB (-0.01%)