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

Implement asymmetrical precedence for closures and jumps #134847

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Dec 28, 2024

I have been through a series of asymmetrical precedence designs in Syn, and finally have one that I like and is worth backporting into rustc. It is based on just 2 bits of state: next_operator_can_begin_expr and next_operator_can_continue_expr.

Asymmetrical precedence is the thing that enables (return 1) + 1 to require parentheses while 1 + return 1 does not, despite + always having stronger precedence than return according to the Rust Reference. This is facilitated by next_operator_can_continue_expr.

Relatedly, it is the thing that enables (return) - 1 to require parentheses while return + 1 does not, despite + and - having exactly the same precedence. This is facilitated by next_operator_can_begin_expr.

Example:

macro_rules! repro {
    ($e:expr) => {
        $e - $e;
        $e + $e;
    };
}

fn main() {
    repro!{return}
    repro!{return 1}
}

-Zunpretty=expanded Before:

fn main() {
    (return) - (return);
    (return) + (return);
    (return 1) - (return 1);
    (return 1) + (return 1);
}

After:

fn main() {
    (return) - return;
    return + return;
    (return 1) - return 1;
    (return 1) + return 1;
}

@dtolnay dtolnay added the A-pretty Area: Pretty printing (including `-Z unpretty`) label Dec 28, 2024
@rustbot
Copy link
Collaborator

rustbot commented Dec 28, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Dec 28, 2024
@fmease
Copy link
Member

fmease commented Dec 28, 2024

I'm going to look at this in a few hours.

@dtolnay
Copy link
Member Author

dtolnay commented Dec 28, 2024

One note on testing: the best approach I've found in Syn for thoroughly testing the pretty-printing algorithm and parenthesis insertion is the following pair of tests:

  • The test from Add pretty-printer parenthesis insertion test #133730. This test is able to assert that for any expression parsed from source code, when re-printing it, the pretty-printer's parenthesis insertion never kicks in. We know this because we just parsed that expression; extra parenthesis cannot be necessary to disambiguate it. In Syn we run this test by scraping every expression from every Rust file in the rust-lang/rust repo. In rustc, for now it just runs over the short list of expression edge cases in that test file. This test is designed to rule out false positives (parenthesis inserted where it shouldn't need to be). The implementation in rustc is augmented with a step to also check for false negatives to make the same edge cases list more useful for both kinds of fixes.

  • A test similar to pprust-expr-roundtrip.rs, except actually rigorous. In Syn this is test_permutations. This test programmatically generates all possible improperly parenthesized expression trees, and asserts that they can be parsed back after printing. This test is designed to rule out false negatives (missing parenthesis).

For the second test, a major difference between Syn's and rustc's is this:

// Ignore expressions with chained comparisons that fail to parse
if let Some(mut parsed) = parse_expr(&psess, &printed) {

Rustc's test just throws up its hands and says "sometimes the printer will just generate invalid syntax". (It is not just the one case mentioned in the comment.)

Once the pretty-printer is in better shape, the goal will be to turn this into an unwrap, effectively asserting that the pretty-printer always produces syntactically valid Rust output for any arbitrary syntax tree input that a macro could have generated. That will provide exhaustive coverage of the edge cases. Syn's printer is tested this way.

In rustc, for now there are still too many false positives and false negatives to easily make an exhaustive test of the set of expressions that currently happen to work. Fixing false positives (like this PR) is an important prerequisite to being able to fix certain false negatives without indiscriminately inserting way too many parentheses. Meanwhile, PRs like #134661 and #134600 have been fixing false negatives.

@bors
Copy link
Contributor

bors commented Feb 11, 2025

☔ The latest upstream changes (presumably #127541) made this pull request unmergeable. Please resolve the merge conflicts.

@dtolnay
Copy link
Member Author

dtolnay commented Feb 18, 2025

Rebased to resolve conflict with #127541 in tests/ui/invalid/invalid-rustc_legacy_const_generics-issue-123077.stderr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-pretty Area: Pretty printing (including `-Z unpretty`) 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants