rust icon indicating copy to clipboard operation
rust copied to clipboard

Restrict `parse_maybe_literal_minus`

Open nnethercote opened this issue 1 year ago • 13 comments

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

r? @petrochenkov

nnethercote avatar Aug 20 '24 01:08 nnethercote

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.

nnethercote avatar Aug 20 '24 01:08 nnethercote

@bors try

petrochenkov avatar Aug 20 '24 12:08 petrochenkov

:hourglass: Trying commit 21efb726ba6f306121e1af18e2186e51d6c4d8d0 with merge 79097c2a1baf756f8e4297787c180a9db4e42301...

bors avatar Aug 20 '24 12:08 bors

:sunny: Try build successful - checks-actions Build commit: 79097c2a1baf756f8e4297787c180a9db4e42301 (79097c2a1baf756f8e4297787c180a9db4e42301)

bors avatar Aug 20 '24 14:08 bors

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

petrochenkov avatar Aug 20 '24 15:08 petrochenkov

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

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

craterbot avatar Aug 20 '24 15:08 craterbot

@craterbot check p=1 crates=https://gist.githubusercontent.com/petrochenkov/d33e774c2065457db57d1dee3713f55a/raw/c43de2eb8109a15dce33173973a03ffc424e58b9/gistfile1.txt

petrochenkov avatar Aug 20 '24 15:08 petrochenkov

:ok_hand: Experiment pr-129289 created and queued. :robot: Automatically detected try build 79097c2a1baf756f8e4297787c180a9db4e42301 :mag: You can check out the queue and this experiment's details.

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

craterbot avatar Aug 20 '24 15:08 craterbot

:construction: Experiment pr-129289 is now running

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

craterbot avatar Aug 22 '24 03:08 craterbot

:tada: Experiment pr-129289 is completed! :bar_chart: 1 regressed and 0 fixed (926 total) :newspaper: Open the full report.

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

craterbot avatar Aug 22 '24 03:08 craterbot

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.

nnethercote avatar Aug 22 '24 07:08 nnethercote

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?

nnethercote avatar Aug 26 '24 03:08 nnethercote

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.

petrochenkov avatar Aug 26 '24 12:08 petrochenkov

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

petrochenkov avatar Feb 19 '25 16:02 petrochenkov

@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 :(

nnethercote avatar Feb 20 '25 02:02 nnethercote