Skip to content

Restrict parse_maybe_literal_minus #129289

New issue

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

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

Already on GitHub? # to your account

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
Collaborator

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
Collaborator

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
@petrochenkov
Copy link
Contributor

Still waiting on the author to answer.
@nnethercote Do you plan to return to this?

@nnethercote
Copy link
Contributor Author

@nnethercote Do you plan to return to this?

Unsure. The current behaviour isn't great, but fixing it turns out to be more complex than I expected, so this PR has fallen a long way down my todo list :(

# for free to join this conversation on GitHub. Already have an account? # 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