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

Restrict parse_maybe_literal_minus #129289

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

Conversation

nnethercote
Copy link
Contributor

parse_literal_maybe_minus currently accepts to many kinds of NtExpr. This PR starts the process of restricting it.

r? @petrochenkov

It covers some patterns that are relevant for upcoming changes to
`parse_literal_maybe_minus`.
The end goal is to only allow `ExprKind::Lit` and
`ExprKind::Unary`/`Unop::Neg` interpolated expressions. We can't do that
yet, because we still need to allow `ExprKind::Path` and
`ExprKind::MacCall`.

Some tests have their output changed, because some invalid code is now
rejected during parsing instead of during HIR lowering. Each error
location moves from a macro call site to a macro body, which I think is
an improvement.
@rustbot 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 Aug 20, 2024
@nnethercote
Copy link
Contributor Author

The original version of #126571 tried to ban all NtExprs other than literals and unary minus, but a crater run
showed that ExprKind::Path and ExprKind::MacCall are still needed, which is why they are still allowed in this PR. To double-check this, we could re-run the failures from that first crater run, or even do a full crater run if we want to be extra careful.

There are follow-ups to be done to remove ExprKind::Path and Expr::MacCall from parse_literal_maybe_minus, but this PR is a sufficient first step.

@petrochenkov
Copy link
Contributor

@bors try

@petrochenkov petrochenkov added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 20, 2024
@bors
Copy link
Contributor

bors commented Aug 20, 2024

⌛ Trying commit 21efb72 with merge 79097c2...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2024
…minus-1, r=<try>

Restrict `parse_maybe_literal_minus`

`parse_literal_maybe_minus` currently accepts to many kinds of `NtExpr`. This PR starts the process of restricting it.

r? `@petrochenkov`
@bors
Copy link
Contributor

bors commented Aug 20, 2024

☀️ Try build successful - checks-actions
Build commit: 79097c2 (79097c2a1baf756f8e4297787c180a9db4e42301)

@petrochenkov
Copy link
Contributor

@craterbot
Copy link
Collaborator

🚨 Error: missing desired crates: {"htmx-rs"}

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

👌 Experiment pr-129289 created and queued.
🤖 Automatically detected try build 79097c2
🔍 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
Copy link
Collaborator

🚧 Experiment pr-129289 is now running

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

@craterbot
Copy link
Collaborator

🎉 Experiment pr-129289 is completed!
📊 1 regressed and 0 fixed (926 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 Aug 22, 2024
@nnethercote
Copy link
Contributor Author

There is one real regression. If I allow NtExpr(...ExprKind::Tup...) in parse_literal_maybe_minus that fixes the regression. I need to investigate some more to understand what's happening.

@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 Aug 22, 2024
@nnethercote
Copy link
Contributor Author

Here's an interesting test case:

macro_rules! p2 {
    ($m:pat) => {}
}   

macro_rules! p1 {
    ($m:expr) => { p2!($m) };
}   
    
fn main() {
    // Reasonable expression patterns, that are currently accepted, and
    // show up in a crater run.
    p1!(());
    p1!((3,4));
    
    // Reasonable expression patterns, that are currently accepted, but
    // dont't show up in a crater run.
    p1!([3,4]);
    p1!(&x);
    p1!(..);
    p1!(Foo(x, y));
    p1!(Foo { x, y });
    
    // Unreasonable expression patterns, that are currently accepted, but
    // don't show up in a crater run.
    p1!(f());
    p1!(x.f());
    p1!(1 + 1);
    p1!(x as u32);
    p1!(if a { b } else { c });
    p1!(loop {});
    p1!(x = 3);
    p1!((3)); // maybe a reasonable pattern?
}

Currently this compiles without any problem because parse_literal_maybe_minus accepts any kind of NtExpr. With this PR's current code all of these lines would fail with syntax errors of the form expected pattern, found expression (). @petrochenkov, do you have opinions about which of these should be accepted?

@petrochenkov
Copy link
Contributor

do you have opinions about which of these should be accepted?

After migrating to the token model - everything that fits into both the expression and the pattern syntax as tokens.
That would be all the examples except x.f(), 1 + 1, x as u32, if a { b } else { c }, loop {} and x = 3.

Before the migration - no strong opinion, just not more than after the migration, and no large breakage.

@alex-semenyuk alex-semenyuk 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

6 participants