delay expand macro bang when there has indeterminate path #121589

bvanjoi · 2024-02-25T13:45:21Z

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 · 2024-03-05T16:21:26Z

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 · 2024-03-05T16:28:39Z

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

try to resolve under eager expander Fixes rust-lang#98291 I will attempt to clarify the root problem through several examples: Firstly, ```rs // 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, ```rs // 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: ```rs // 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`

bors · 2024-03-05T16:29:53Z

⌛ Trying commit ee857f7 with merge 8b0b849...

bors · 2024-03-05T17:57:58Z

☀️ Try build successful - checks-actions
Build commit: 8b0b849 (8b0b84954eb02575b87834af9171a9c93a3f1691)

rust-timer · 2024-03-05T20:28:26Z

Finished benchmarking commit (8b0b849): 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 · 2024-03-05T20:49:02Z

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

bvanjoi · 2024-03-09T12:54:57Z

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

compiler/rustc_expand/src/base.rs

bvanjoi · 2024-03-09T14:16:23Z

ci is green. @rustbot ready

compiler/rustc_expand/src/expand.rs

compiler/rustc_builtin_macros/src/compile_error.rs

compiler/rustc_builtin_macros/src/format.rs

compiler/rustc_builtin_macros/src/cfg.rs

compiler/rustc_builtin_macros/src/log_syntax.rs

petrochenkov · 2024-03-11T17:18:52Z

compiler/rustc_expand/src/base.rs

+    {
+        return ExpandResult::Retry(());
+    }
+
    // Perform eager expansion on the expression.
    // We want to be able to handle e.g., `concat!("foo", "bar")`.
    let expr = cx.expander().fully_expand_fragment(AstFragment::Expr(expr)).make_expr();


Ideally, the whole fully_expand_fragment would return Ready/Retry, but that's probably a much larger change.
The logic above is a good enough approximation.

compiler/rustc_expand/src/base.rs

bvanjoi · 2024-03-12T04:40:11Z

@rustbot ready

bors · 2024-03-12T12:57:49Z

⌛ Trying commit ab31e3e with merge 0af3b72...

delay expand macro bang when there has indeterminate path Related rust-lang#98291 I will attempt to clarify the root problem through several examples: Firstly, ```rs // 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, ```rs // 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: ```rs // 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`

bors · 2024-03-12T14:29:04Z

☀️ Try build successful - checks-actions
Build commit: 0af3b72 (0af3b723d85f7322081dc20664c34d0aadaf394a)

rust-timer · 2024-03-12T17:06:07Z

Finished benchmarking commit (0af3b72): 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%)

compiler/rustc_resolve/src/macros.rs

petrochenkov · 2024-03-12T17:43:15Z

r=me with #121589 (comment) addressed.

rustbot · 2024-03-13T02:26:05Z

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.

compiler/rustc_resolve/src/macros.rs

bvanjoi · 2024-03-13T10:04:01Z

@rustbot ready

petrochenkov · 2024-03-13T12:32:01Z

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

bors · 2024-03-13T12:32:03Z

📌 Commit 8fcdf54 has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2024-03-13T13:20:45Z

⌛ Testing commit 8fcdf54 with merge 184c5ab...

bors · 2024-03-13T15:24:26Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 184c5ab to master...

rust-timer · 2024-03-13T17:41:57Z

Finished benchmarking commit (184c5ab): 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%)

rustbot assigned petrochenkov Feb 25, 2024

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 Feb 25, 2024

bvanjoi mentioned this pull request Feb 26, 2024

Adding an attribute to a macro_exported macro and reexporting it incorrectly triggers the macro_expanded_macro_exports_accessed_by_absolute_paths error #98291

Open

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 5, 2024

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 5, 2024

This comment has been minimized.

Sign in to view

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 5, 2024

rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 5, 2024

bvanjoi force-pushed the fix-98291 branch from ee857f7 to 3a035cd Compare March 9, 2024 12:51

bvanjoi commented Mar 9, 2024

View reviewed changes

compiler/rustc_expand/src/base.rs Outdated Show resolved Hide resolved

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 Mar 9, 2024

bvanjoi changed the title ~~try to resolve under eager expander~~ delay expand macro bang when there has indeterminate path Mar 10, 2024

petrochenkov reviewed Mar 11, 2024

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 Mar 11, 2024

bvanjoi force-pushed the fix-98291 branch from 3a035cd to ab31e3e Compare March 12, 2024 03:02

petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 12, 2024

This comment has been minimized.

Sign in to view

rustbot removed S-waiting-on-perf Status: Waiting on a perf run to be completed. perf-regression Performance regression. labels Mar 12, 2024

petrochenkov reviewed Mar 12, 2024

View reviewed changes

compiler/rustc_resolve/src/macros.rs Show resolved Hide resolved

bvanjoi force-pushed the fix-98291 branch from ab31e3e to 988c172 Compare March 13, 2024 02:26

petrochenkov reviewed Mar 13, 2024

View reviewed changes

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

petrochenkov added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 13, 2024

delay expand macro bang when there has indeterminate path

8fcdf54

bvanjoi force-pushed the fix-98291 branch from 988c172 to 8fcdf54 Compare March 13, 2024 08:15

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 Mar 13, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 13, 2024

bors merged commit 184c5ab into rust-lang:master Mar 13, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Mar 13, 2024

bors mentioned this pull request Mar 13, 2024

Suggest correct path in include_bytes! #121833

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delay expand macro bang when there has indeterminate path #121589

delay expand macro bang when there has indeterminate path #121589

bvanjoi commented Feb 25, 2024 •

edited

Loading

petrochenkov commented Mar 5, 2024

petrochenkov commented Mar 5, 2024

This comment has been minimized.

bors commented Mar 5, 2024

bors commented Mar 5, 2024

This comment has been minimized.

rust-timer commented Mar 5, 2024

petrochenkov commented Mar 5, 2024

bvanjoi commented Mar 9, 2024

bvanjoi commented Mar 9, 2024

petrochenkov Mar 11, 2024

bvanjoi commented Mar 12, 2024

bors commented Mar 12, 2024

bors commented Mar 12, 2024

This comment has been minimized.

rust-timer commented Mar 12, 2024

petrochenkov commented Mar 12, 2024

rustbot commented Mar 13, 2024

bvanjoi commented Mar 13, 2024

petrochenkov commented Mar 13, 2024

bors commented Mar 13, 2024

bors commented Mar 13, 2024

bors commented Mar 13, 2024

rust-timer commented Mar 13, 2024

delay expand macro bang when there has indeterminate path #121589

delay expand macro bang when there has indeterminate path #121589

Conversation

bvanjoi commented Feb 25, 2024 • edited Loading

petrochenkov commented Mar 5, 2024

petrochenkov commented Mar 5, 2024

This comment has been minimized.

bors commented Mar 5, 2024

bors commented Mar 5, 2024

This comment has been minimized.

rust-timer commented Mar 5, 2024

Overall result: ❌ regressions - ACTION NEEDED

petrochenkov commented Mar 5, 2024

bvanjoi commented Mar 9, 2024

bvanjoi commented Mar 9, 2024

Choose a reason for hiding this comment

bvanjoi commented Mar 12, 2024

bors commented Mar 12, 2024

bors commented Mar 12, 2024

This comment has been minimized.

rust-timer commented Mar 12, 2024

Overall result: ❌ regressions - no action needed

petrochenkov commented Mar 12, 2024

rustbot commented Mar 13, 2024

bvanjoi commented Mar 13, 2024

petrochenkov commented Mar 13, 2024

bors commented Mar 13, 2024

bors commented Mar 13, 2024

bors commented Mar 13, 2024

rust-timer commented Mar 13, 2024

Overall result: no relevant changes - no action needed

bvanjoi commented Feb 25, 2024 •

edited

Loading