Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add expansion_growth_limit attr as another expansion limit #103029

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vincenzopalazzo
Copy link
Member

@vincenzopalazzo vincenzopalazzo commented Oct 13, 2022

This PR required a little bit of content, as discussed on Zulip

The following code triggers a recursion that will keep adding tokens inside the list of tokens, as described here

/// Code that cause the rust issue #95698
macro_rules! from_cow_impls {
    ($( $from: ty ),+ $(,)? ) => {
        // recursion call
        from_cow_impls!(
            $( $from, Cow::from ),+
        );
    };

    ($( $from: ty, $normalizer: expr ),+ $(,)? ) => {
        $( impl<'a> From<$from> for LhsValue<'a> {
            fn from(val: $from) -> Self {
                LhsValue::Bytes($normalizer(val))
            }
        } )+
    };
}

from_cow_impls!(
    &'a [u8], /*callback*/
    Vec<u8>, /*callback*/
);

This PR adds a new attribute expansion_growth_limit which allows us to fix this corner case where this happens and also can catch some performance problems in case of macros resolution.

Closes #95698

@rustbot r? @Aaron1011

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 13, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2022
@vincenzopalazzo vincenzopalazzo force-pushed the macros/expansion_grow_limit branch 3 times, most recently from e01efdc to 897095d Compare October 14, 2022 09:21
@rust-log-analyzer

This comment has been minimized.

@vincenzopalazzo vincenzopalazzo force-pushed the macros/expansion_grow_limit branch from 897095d to c419f77 Compare October 14, 2022 13:23
@vincenzopalazzo vincenzopalazzo changed the title feat: add expansion_grow_limit attr as another expansion limit feat: add expansion_growth_limit attr as another expansion limit Oct 15, 2022
@vincenzopalazzo
Copy link
Member Author

@rustbot r? rust-lang/compiler

@rustbot rustbot assigned compiler-errors and unassigned Aaron1011 Nov 22, 2022
@compiler-errors
Copy link
Member

Not going to be able to review this for a while.

r? parser

@bors

This comment was marked as resolved.

@vincenzopalazzo vincenzopalazzo force-pushed the macros/expansion_grow_limit branch from c419f77 to f41a9e0 Compare December 20, 2022 16:39
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Dec 20, 2022
@rustbot
Copy link
Collaborator

rustbot commented Dec 20, 2022

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

@vincenzopalazzo vincenzopalazzo force-pushed the macros/expansion_grow_limit branch from 95bb7d6 to 031b127 Compare December 20, 2022 20:38
@rust-log-analyzer

This comment has been minimized.

@vincenzopalazzo vincenzopalazzo force-pushed the macros/expansion_grow_limit branch 2 times, most recently from bf2f70c to c7d9291 Compare December 20, 2022 22:45
@wesleywiser
Copy link
Member

r? rust-lang/compiler

@rustbot rustbot assigned oli-obk and unassigned petrochenkov Dec 22, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Dec 25, 2022

I'm on vacation and will review in 2-3 weeks

@petrochenkov
Copy link
Contributor

I need to be aware of what happens here, but right now I'm taking some break from reviewing, I'll hope to return to this some time later in January.

Right now this PR seems to set a limit on number of tokens produced by a macro (*), which is not right because macros can simply produce a lot of code without any recursion.

(*) Only a fn-like macro, but not an attribute or derive macro, which also doesn't look correct.
@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2023
@petrochenkov 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 8, 2023
@WaffleLapkin
Copy link
Member

One day I'll learn how to use crater properly 😓 (it needs a try build...)

@bors try

@bors
Copy link
Contributor

bors commented Jun 8, 2023

⌛ Trying commit 7ddb63a with merge d42a007e04d1d7df0de258d39ac2402f35aba932...

@bors
Copy link
Contributor

bors commented Jun 8, 2023

☀️ Try build successful - checks-actions
Build commit: d42a007e04d1d7df0de258d39ac2402f35aba932 (d42a007e04d1d7df0de258d39ac2402f35aba932)

@WaffleLapkin
Copy link
Member

@craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-103029/retry-regressed-list.txt

@craterbot
Copy link
Collaborator

👌 Experiment pr-103029-3 created and queued.
🤖 Automatically detected try build d42a007e04d1d7df0de258d39ac2402f35aba932
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-103029-3 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-103029-3 is completed!
📊 1 regressed and 0 fixed (125 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jun 8, 2023
@petrochenkov 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 9, 2023
@Dylan-DPC
Copy link
Member

@vincenzopalazzo any updates on this?

@vincenzopalazzo
Copy link
Member Author

@Dylan-DPC thinking how to move on on it, I guess it need a MCP and a compile team discussion

@Dylan-DPC
Copy link
Member

@vincenzopalazzo any updates on this?

@vincenzopalazzo
Copy link
Member Author

any updates on this?

Now really @Dylan-DPC I was unable to work on anything in the past few months, so I think I will start the discussion again.

Now that we have the @rust-lang/wg-macros I think the discussion should start from there before creating the MCP

@apiraino
Copy link
Contributor

I'll set this to S-blocked to signal that it needs design work

@rustbot S-blocked

@apiraino apiraino added S-blocked Status: Blocked on something else such as an RFC or other implementation work. needs-mcp This change is large enough that it needs a major change proposal before starting work. labels Jun 17, 2024
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic needs-mcp This change is large enough that it needs a major change proposal before starting work. S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustc_expand can be tricked into infinite loops