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

Test rustc's automatic parenthesis insertion #1788

Closed
wants to merge 1 commit into from
Closed

Conversation

dtolnay
Copy link
Owner

@dtolnay dtolnay commented Nov 29, 2024

Using the same technique as #1787, but on rustc expressions instead of syn.

This reveals numerous parenthesization false negatives in rustc_ast_pretty's algorithm and other bugs. A selection of examples:

FAIL tests/rust/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
BEFORE:
for expr in expr_finder.clones {
    if let hir::ExprKind::MethodCall(_, rcvr, _, span) = expr.kind
        && let Some(rcvr_ty) = typeck_results.node_type_opt(rcvr.hir_id)
        && let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
        && rcvr_ty == ty
        && let ty::Ref (_, inner, _) = rcvr_ty.kind()
        && let inner = inner.peel_refs()
        && (Holds { ty: inner }).visit_ty(local_ty).is_break()
        && let None = self.infcx.type_implements_trait_shallow(clone, inner, self.param_env)
    {
        err.span_label(
            span,
            format!("this call doesn't do anything, the result is still `{rcvr_ty}` because `{inner}` doesn't implement `Clone`"),
        );
        types_to_constrain.insert(inner);
    }
}
AFTER:
for expr in expr_finder.clones {
    if (let hir::ExprKind::MethodCall(_, rcvr, _, span) = expr.kind &&
                                        let Some(rcvr_ty) =
                                            typeck_results.node_type_opt(rcvr.hir_id) &&
                                    let Some(ty) = typeck_results.node_type_opt(expr.hir_id) &&
                                rcvr_ty == ty && let ty::Ref(_, inner, _) = rcvr_ty.kind()
                        && let inner = inner.peel_refs() &&
                    Holds { ty: inner }.visit_ty(local_ty).is_break() &&
                let None =
                    self.infcx.type_implements_trait_shallow(clone, inner,
                        self.param_env)) {
            err.span_label(span,
                format!("this call doesn't do anything, the result is still `{rcvr_ty}` \
                             because `{inner}` doesn't implement `Clone`",));
            types_to_constrain.insert(inner);
        }
}

FAIL tests/rust/tests/ui/issues/issue-39548.rs
BEFORE:
((1 < 2) == false) as usize
AFTER:
(1 < 2 == false) as usize

FAIL tests/rust/tests/pretty/ast-stmt-expr-attr.rs
BEFORE:
(#[attr] foo).bar
AFTER:
#[attr] foo.bar

FAIL tests/rust/tests/pretty/ast-stmt-expr-attr.rs
BEFORE:
(#[attr] foo).0
AFTER:
#[attr] foo.0

FAIL tests/rust/tests/pretty/ast-stmt-expr-attr.rs
BEFORE:
(#[attr] foo) [bar]
AFTER:
#[attr] foo[bar]

FAIL tests/rust/src/tools/rust-analyzer/crates/parser/test_data/parser/inline/ok/let_expr.rs
BEFORE:
while 1 == 5 && (let None = None) {}
AFTER:
while 1 == 5 && let None = None {}

FAIL tests/rust/tests/pretty/stmt_expr_attributes.rs
BEFORE:
(#[rustc_dummy] s).data
AFTER:
#[rustc_dummy] s.data

FAIL tests/rust/tests/pretty/stmt_expr_attributes.rs
BEFORE:
(#[rustc_dummy] t).0
AFTER:
#[rustc_dummy] t.0

FAIL tests/rust/tests/pretty/stmt_expr_attributes.rs
BEFORE:
(#[rustc_dummy] v) [0]
AFTER:
#[rustc_dummy] v[0]

FAIL tests/rust/compiler/rustc_expand/src/proc_macro_server.rs
BEFORE:
if start > u32::MAX as usize
    || end > u32::MAX as usize
    || (u32::MAX - start as u32) < span.lo().to_u32()
    || (u32::MAX - end as u32) < span.lo().to_u32()
    || start >= end
    || end > length
{
    return None;
}
AFTER:
if start > u32::MAX as usize || end > u32::MAX as usize ||
                        u32::MAX - start as u32 < span.lo().to_u32() ||
                    u32::MAX - end as u32 < span.lo().to_u32() || start >= end
            || end > length {
        return None;
    }

FAIL tests/rust/src/tools/clippy/tests/ui/bool_comparison.rs
BEFORE:
((1 < 2) == false) as usize
AFTER:
(1 < 2 == false) as usize

FAIL tests/rust/src/tools/clippy/tests/ui/bool_comparison.rs
BEFORE:
((1 < 2) == !m!(func)) as usize
AFTER:
(1 < 2 == !m!(func)) as usize

FAIL tests/rust/tests/ui/lint/unused/issue-90807-unused-paren.rs
BEFORE:
for _ in (1..{ 2 }) {}
AFTER:
for _ in 1..{ 2 } {}

FAIL tests/rust/src/tools/rust-analyzer/crates/syntax/test_data/parser/validation/invalid_let_expr.rs
BEFORE:
if true { (let _ = None) }
AFTER:
if true { let _ = None }

FAIL tests/rust/src/tools/rust-analyzer/crates/syntax/test_data/parser/validation/invalid_let_expr.rs
BEFORE:
if true && (let _ = None) {
    (let _ = None);
    while let _ = None {
        match None {
            _ if let _ = None => {
                let _ = None;
            }
        }
    }
}
AFTER:
if true && let _ = None {
        let _ = None;
        while let _ = None {
            match None { _ if let _ = None => { let _ = None; } }
        }
    }

FAIL tests/rust/tests/ui/attrs-resolution.rs
BEFORE:
match foo_struct {
    FooStruct { #[rustfmt::skip] bar } => {}
}
AFTER:
match foo_struct { FooStruct { bar } => {} }

@dtolnay dtolnay marked this pull request as draft November 29, 2024 23:57
@dtolnay dtolnay closed this Nov 29, 2024
@dtolnay dtolnay deleted the rustcparentest branch November 29, 2024 23:57
RalfJung added a commit to RalfJung/rust that referenced this pull request Nov 30, 2024
Eliminate print_expr_maybe_paren function from pretty printers

This PR is part of backporting Syn's expression precedence design into rustc. (See rust-lang#133603 for other work on this.)

In Syn, our version of `print_expr_cond_paren` is called `print_subexpression` and it is called from 19 places. Of those calls, 12 of them need a "custom" behavior for the `needs_paren` argument, whereas only 7 use a "standard" behavior resembling `print_subexpression($e, $e.precedence() < Precedence::$Variant, ...)`. In other words the behavior that rustc_ast_pretty's `print_expr_maybe_paren` implements is actually not what you want most of the time. The current usage you see in rustc is overuse.

<details>
<summary>Aside: am I confident about the correctness of Syn's parenthesization? Yes. Click for details.</summary>

---

The behavior is constrained by the following pair of tests which both run over every Rust source file of rustc and the standard library and tools and test suites:

- To rule out **false positives**: for every expression in every source file, print the expression, parse it back, and verify that not a single new parenthesis got added. Since these are expressions parsed from source code, not macro-generated syntax trees, we know they must never need automatic parenthesis insertion. Rustc's pretty printer does not pass this.

    Pseudocode: `assert(expr == parse(print(expr)))`

- To rule out **false negatives**: for every expression in every source file, replace every Expr::Paren node in the syntax tree with just its contents, i.e. stripping the parentheses but otherwise preserving the syntax tree structure. Then print the stripped expression performing parenthesis insertion wherever needed, and reparse it. Verify that the reparsed expression has identical structure to the original, despite there being no parentheses in the original prior to printing, i.e. all the right parentheses got re-inserted by the printer to preserve the expression's structure. Rustc's pretty printer does not pass this. See dtolnay/syn#1788 which reveals multiple rustc_ast_pretty bugs.

    Pseudocode: `assert(unparenthesize(expr) == unparenthesize(parse(print(unparenthesize(expr)))))`

---
</details>

If `print_expr_maybe_paren` is usually not correct, is there harm in keeping it for the minority of cases where it is correct? I think the answer is yes and Syn doesn't use any equivalent of this helper function. The problems with it are:

- Having both `print_expr_maybe_paren` and `print_expr_cond_paren` applies counterproductive inertia against moving from the first to the second. When looking at a call site like `print_expr_maybe_paren(e, Precedence::$Variant, ...)` with parentheses not being inserted where they should be, anyone's first inclination would be to solve the bug by tweaking $Variant because that is the only knob that visibly appears in the function call. For example to pass "prec + 1", like tweaking the code to conditionally pass `Precedence::Prefix` instead of `Precedence::Cast`.

    Experience in Syn shows this is (almost?) never what you want the person to do. In a call `print_expr_cond_paren(e, e.precedence() < ExprPrecedence::$Variant, ...)` almost always the best fix involves one of:

    - Changing `e.precedence()`, e.g. to `fixup.leading_precedence(e)` and `fixup.trailing_precedence(e)` in cases of asymmetrical precedence (`(return 1) + 1` vs `1 + return 1`).

    - Changing `<` to `<=`, to handle associativity and other grammar restrictions like chained comparisons (which rustc gets wrong today).

    - Adding `||` and/or `&&` clauses to the condition.

    By using these 3 better knobs instead of $Variant, it upholds the property that any time we talk about precedence, it is always the precedence of some actual expression that our code is actively manipulating, instead of a value standing in for some imaginary precedence level that would exist between two consecutive [real levels](https://doc.rust-lang.org/1.83.0/reference/expressions.html#expression-precedence). For example consider that "`Cast` + 1" might be `Prefix` today, but only until some new Rust syntax ends up adding a level between those.

- The `print_expr_maybe_paren` call sites look shorter, but they are not clearer. For myself, a function argument that says "does this subexpression need parenthesization" is a concrete thing that is easy to think about, while a function argument that is "what is the effective precedence level associated with this subexpression's placement inside its parent expression" is abstract and tricky to even state a precise meaning for. I expect that for someone less familiar with the pretty printer working on adding a new expression kind (like postfix match, recently), having every subexpression consistently printed using `print_expr_cond_paren` will be more beneficial, for the same reason, than having `print_expr_maybe_paren` available.

r? `@lcnr`
RalfJung added a commit to RalfJung/rust that referenced this pull request Nov 30, 2024
Eliminate print_expr_maybe_paren function from pretty printers

This PR is part of backporting Syn's expression precedence design into rustc. (See rust-lang#133603 for other work on this.)

In Syn, our version of `print_expr_cond_paren` is called `print_subexpression` and it is called from 19 places. Of those calls, 12 of them need a "custom" behavior for the `needs_paren` argument, whereas only 7 use a "standard" behavior resembling `print_subexpression($e, $e.precedence() < Precedence::$Variant, ...)`. In other words the behavior that rustc_ast_pretty's `print_expr_maybe_paren` implements is actually not what you want most of the time. The current usage you see in rustc is overuse.

<details>
<summary>Aside: am I confident about the correctness of Syn's parenthesization? Yes. Click for details.</summary>

---

The behavior is constrained by the following pair of tests which both run over every Rust source file of rustc and the standard library and tools and test suites:

- To rule out **false positives**: for every expression in every source file, print the expression, parse it back, and verify that not a single new parenthesis got added. Since these are expressions parsed from source code, not macro-generated syntax trees, we know they must never need automatic parenthesis insertion. Rustc's pretty printer does not pass this.

    Pseudocode: `assert(expr == parse(print(expr)))`

- To rule out **false negatives**: for every expression in every source file, replace every Expr::Paren node in the syntax tree with just its contents, i.e. stripping the parentheses but otherwise preserving the syntax tree structure. Then print the stripped expression performing parenthesis insertion wherever needed, and reparse it. Verify that the reparsed expression has identical structure to the original, despite there being no parentheses in the original prior to printing, i.e. all the right parentheses got re-inserted by the printer to preserve the expression's structure. Rustc's pretty printer does not pass this. See dtolnay/syn#1788 which reveals multiple rustc_ast_pretty bugs.

    Pseudocode: `assert(unparenthesize(expr) == unparenthesize(parse(print(unparenthesize(expr)))))`

---
</details>

If `print_expr_maybe_paren` is usually not correct, is there harm in keeping it for the minority of cases where it is correct? I think the answer is yes and Syn doesn't use any equivalent of this helper function. The problems with it are:

- Having both `print_expr_maybe_paren` and `print_expr_cond_paren` applies counterproductive inertia against moving from the first to the second. When looking at a call site like `print_expr_maybe_paren(e, Precedence::$Variant, ...)` with parentheses not being inserted where they should be, anyone's first inclination would be to solve the bug by tweaking $Variant because that is the only knob that visibly appears in the function call. For example to pass "prec + 1", like tweaking the code to conditionally pass `Precedence::Prefix` instead of `Precedence::Cast`.

    Experience in Syn shows this is (almost?) never what you want the person to do. In a call `print_expr_cond_paren(e, e.precedence() < ExprPrecedence::$Variant, ...)` almost always the best fix involves one of:

    - Changing `e.precedence()`, e.g. to `fixup.leading_precedence(e)` and `fixup.trailing_precedence(e)` in cases of asymmetrical precedence (`(return 1) + 1` vs `1 + return 1`).

    - Changing `<` to `<=`, to handle associativity and other grammar restrictions like chained comparisons (which rustc gets wrong today).

    - Adding `||` and/or `&&` clauses to the condition.

    By using these 3 better knobs instead of $Variant, it upholds the property that any time we talk about precedence, it is always the precedence of some actual expression that our code is actively manipulating, instead of a value standing in for some imaginary precedence level that would exist between two consecutive [real levels](https://doc.rust-lang.org/1.83.0/reference/expressions.html#expression-precedence). For example consider that "`Cast` + 1" might be `Prefix` today, but only until some new Rust syntax ends up adding a level between those.

- The `print_expr_maybe_paren` call sites look shorter, but they are not clearer. For myself, a function argument that says "does this subexpression need parenthesization" is a concrete thing that is easy to think about, while a function argument that is "what is the effective precedence level associated with this subexpression's placement inside its parent expression" is abstract and tricky to even state a precise meaning for. I expect that for someone less familiar with the pretty printer working on adding a new expression kind (like postfix match, recently), having every subexpression consistently printed using `print_expr_cond_paren` will be more beneficial, for the same reason, than having `print_expr_maybe_paren` available.

r? ``@lcnr``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 30, 2024
Rollup merge of rust-lang#133655 - dtolnay:maybeparen, r=lcnr

Eliminate print_expr_maybe_paren function from pretty printers

This PR is part of backporting Syn's expression precedence design into rustc. (See rust-lang#133603 for other work on this.)

In Syn, our version of `print_expr_cond_paren` is called `print_subexpression` and it is called from 19 places. Of those calls, 12 of them need a "custom" behavior for the `needs_paren` argument, whereas only 7 use a "standard" behavior resembling `print_subexpression($e, $e.precedence() < Precedence::$Variant, ...)`. In other words the behavior that rustc_ast_pretty's `print_expr_maybe_paren` implements is actually not what you want most of the time. The current usage you see in rustc is overuse.

<details>
<summary>Aside: am I confident about the correctness of Syn's parenthesization? Yes. Click for details.</summary>

---

The behavior is constrained by the following pair of tests which both run over every Rust source file of rustc and the standard library and tools and test suites:

- To rule out **false positives**: for every expression in every source file, print the expression, parse it back, and verify that not a single new parenthesis got added. Since these are expressions parsed from source code, not macro-generated syntax trees, we know they must never need automatic parenthesis insertion. Rustc's pretty printer does not pass this.

    Pseudocode: `assert(expr == parse(print(expr)))`

- To rule out **false negatives**: for every expression in every source file, replace every Expr::Paren node in the syntax tree with just its contents, i.e. stripping the parentheses but otherwise preserving the syntax tree structure. Then print the stripped expression performing parenthesis insertion wherever needed, and reparse it. Verify that the reparsed expression has identical structure to the original, despite there being no parentheses in the original prior to printing, i.e. all the right parentheses got re-inserted by the printer to preserve the expression's structure. Rustc's pretty printer does not pass this. See dtolnay/syn#1788 which reveals multiple rustc_ast_pretty bugs.

    Pseudocode: `assert(unparenthesize(expr) == unparenthesize(parse(print(unparenthesize(expr)))))`

---
</details>

If `print_expr_maybe_paren` is usually not correct, is there harm in keeping it for the minority of cases where it is correct? I think the answer is yes and Syn doesn't use any equivalent of this helper function. The problems with it are:

- Having both `print_expr_maybe_paren` and `print_expr_cond_paren` applies counterproductive inertia against moving from the first to the second. When looking at a call site like `print_expr_maybe_paren(e, Precedence::$Variant, ...)` with parentheses not being inserted where they should be, anyone's first inclination would be to solve the bug by tweaking $Variant because that is the only knob that visibly appears in the function call. For example to pass "prec + 1", like tweaking the code to conditionally pass `Precedence::Prefix` instead of `Precedence::Cast`.

    Experience in Syn shows this is (almost?) never what you want the person to do. In a call `print_expr_cond_paren(e, e.precedence() < ExprPrecedence::$Variant, ...)` almost always the best fix involves one of:

    - Changing `e.precedence()`, e.g. to `fixup.leading_precedence(e)` and `fixup.trailing_precedence(e)` in cases of asymmetrical precedence (`(return 1) + 1` vs `1 + return 1`).

    - Changing `<` to `<=`, to handle associativity and other grammar restrictions like chained comparisons (which rustc gets wrong today).

    - Adding `||` and/or `&&` clauses to the condition.

    By using these 3 better knobs instead of $Variant, it upholds the property that any time we talk about precedence, it is always the precedence of some actual expression that our code is actively manipulating, instead of a value standing in for some imaginary precedence level that would exist between two consecutive [real levels](https://doc.rust-lang.org/1.83.0/reference/expressions.html#expression-precedence). For example consider that "`Cast` + 1" might be `Prefix` today, but only until some new Rust syntax ends up adding a level between those.

- The `print_expr_maybe_paren` call sites look shorter, but they are not clearer. For myself, a function argument that says "does this subexpression need parenthesization" is a concrete thing that is easy to think about, while a function argument that is "what is the effective precedence level associated with this subexpression's placement inside its parent expression" is abstract and tricky to even state a precise meaning for. I expect that for someone less familiar with the pretty printer working on adding a new expression kind (like postfix match, recently), having every subexpression consistently printed using `print_expr_cond_paren` will be more beneficial, for the same reason, than having `print_expr_maybe_paren` available.

r? ``@lcnr``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant