Implement asymmetrical precedence for closures and jumps

[dtolnay]

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;
}

[rustbot]

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

[fmease]

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

[dtolnay]

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:

rust/tests/ui-fulldeps/pprust-expr-roundtrip.rs

Lines 231 to 232 in ceb0441

	// 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]

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

[dtolnay]

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

[wesleywiser]

Hi @fmease, could you take another look at this PR when you get a chance? Thanks!

[dtolnay]

Rebased to resolve conflicts with:

• Make AssocOp more like ExprKind #136846
• Tighten up assignment operator representations. #138017
• Add support for postfix yield expressions #138435

[bors]

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

[dtolnay]

• Rebased to resolve conflict with Improve test coverage of HIR pretty printing. #140430

[wesleywiser]

r? rust-lang/compiler

[fmease]

I'm very sorry for the long wait. The impl looks good, thanks!

I've also performed a lot of testing locally and everything seems to check out.

[fmease]

I keep wondering if this could be relaxed to ExprPrecedence::Unambiguous. I wasn't able to find any counterexamples for doing that so far but I'm sure I'm missing some very obvious cases

[dtolnay]

Prefix and Unambiguous are interchangeable on this line. The only difference between Prefix and Unambiguous is for subexpressions that are not a rightmost subexpression:

$a.method()
// needs parens if $a is `&x` (Prefix)
// no parens if $a is `loop {}` (Unambiguous)

and if something is not a rightmost subexpression, then by definition next_operator_can_continue_expr would be true, because the outer expression literally continues after that subexpression, so we would not be in this code.

But I still think Prefix is the more natural value to return here compared to Unambiguous if you consider it against the $a.method() example — a subexpression like return false behaves more like &a than like loop {} in that (return false).method() needs the parenthesization, even if that behavior is ultimately not controlled by this return value (because next_operator_can_continue_expr would be true).

[fmease]

@bors r+ rollup

[bors]

📌 Commit 6cca4ca has been approved by fmease

It is now in the queue for this repository.