Eliminate print_expr_maybe_paren function from pretty printers
This PR is part of backporting Syn's expression precedence design into rustc. (See #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 https://github.com/dtolnay/syn/pull/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``
Mark visionOS as supporting `std`
Cargo's -Zbuild-std has recently started checking this field, which causes it to fail to compile even though we have full support for the standard library on these targets.
[Example of failed build](https://github.com/rust-random/getrandom/actions/runs/12069033154/job/33655430622).
Affected targets: `aarch64-apple-visionos` and `aarch64-apple-visionos-sim`.
r? Noratrieb (because you've worked with `rustc` target metadata IIRC)
``@rustbot`` label O-visionos
use stores of the correct size to set discriminants
Resolves an old HACK /FIXME.
Note that I haven't worked much with codegen so I'm not sure if I'm using the functions correctly and I was surprised seeing out-of-range values being fed into `const_uint_big` but apparently they're wrapped implicitly? By making it explicit we can pass in-range values instead.
Support input/output in vector registers of PowerPC inline assembly
This extends currently clobber-only vector registers (`vreg`) support to allow passing `#[repr(simd)]` types as input/output.
| Architecture | Register class | Target feature | Allowed types |
| ------------ | -------------- | -------------- | -------------- |
| PowerPC | `vreg` | `altivec` | `i8x16`, `i16x8`, `i32x4`, `f32x4` |
| PowerPC | `vreg` | `vsx` | `f32`, `f64`, `i64x2`, `f64x2` |
In addition to floats and `core::simd` types listed above, `core::arch` types and custom `#[repr(simd)]` types of the same size and type are also allowed. All allowed types and relevant target features are currently unstable.
r? `@Amanieu`
`@rustbot` label +O-PowerPC +A-inline-assembly
Improve span handling in `parse_expr_bottom`.
`parse_expr_bottom` stores `this.token.span` in `lo`, but then fails to use it in many places where it could. This commit fixes that, and likewise (to a smaller extent) in `parse_ty_common`.
r? ``@spastorino``
Simplify hir_typeck_pass_to_variadic_function
r? ``@compiler-errors``
This reworks a bit how the diagnostic is generated so that it does the same as #133538
The `help` is useless now so I removed it
Add `+forced-atomics` feature to esp32s2 no_std target
Similar to https://github.com/rust-lang/rust/pull/114499 but for the Xtensa backend. The ESP32-S2 doesn't have native atomic support, but can have atomic load/stores as part of the ISA with this LLVM codegen feature.
Note: The current rev of LLVM that rustc is using doesn't contain the `+forced-atomics` feature for Xtensa, but I'm pushing this now to remove the patch from our fork in `esp-rs/rust`.
r? ``@Amanieu`` because you reviewed the related RISC-V PR
Fix target_feature handling in freg of LoongArch inline assembly
In LoongArch inline assembly, freg currently always accepts f32/f64 as input/output.
9b4d7c6a40/compiler/rustc_target/src/asm/loongarch.rs (L41)
However, these types actually require f/d target features as in RISC-V.
Otherwise, an (ugly) compile error will occur: https://godbolt.org/z/K61Gq1E9E
f32/f64 without f:
```
error: couldn't allocate output register for constraint '{$f1}'
--> <source>:12:11
|
12 | asm!("", in("$f1") x, lateout("$f1") y);
| ^
```
f64 with f but without d:
```
error: scalar-to-vector conversion failed, possible invalid constraint for vector type
--> <source>:19:11
|
19 | asm!("", in("$f1") x, lateout("$f1") y);
| ^
```
cc ``@heiher``
r? ``@Amanieu``
``@rustbot`` label +O-LoongArch +A-inline-assembly
Do not call `extern_crate` on current trait on crate mismatch errors
When we encounter an error caused by traits/types of different versions of the same crate, filter out the current crate when collecting spans to add to the context so we don't call `extern_crate` on the `DefId` of the current crate, which is meaningless and ICEs.
Produced output with this filter:
```
error[E0277]: the trait bound `foo::Struct: Trait` is not satisfied
--> y.rs:13:19
|
13 | check_trait::<foo::Struct>();
| ^^^^^^^^^^^ the trait `Trait` is not implemented for `foo::Struct`
|
note: there are multiple different versions of crate `foo` in the dependency graph
--> y.rs:7:1
|
4 | extern crate foo;
| ----------------- one version of crate `foo` is used here, as a direct dependency of the current crate
5 |
6 | pub struct Struct;
| ----------------- this type implements the required trait
7 | pub trait Trait {}
| ^^^^^^^^^^^^^^^ this is the required trait
|
::: x.rs:4:1
|
4 | pub struct Struct;
| ----------------- this type doesn't implement the required trait
5 | pub trait Trait {}
| --------------- this is the found trait
= note: two types coming from two different versions of the same crate are different types even if they look the same
= help: you can use `cargo tree` to explore your dependency tree
note: required by a bound in `check_trait`
--> y.rs:10:19
|
10 | fn check_trait<T: Trait>() {}
| ^^^^^ required by this bound in `check_trait`
```
Fix#133563.
[AIX] handle libunwind native_libs
AIX should follow a similar path here to other libunwind platforms, with regards to system vs in-tree libunwind and the native lib search directories.
Having the right native lib search directories here is also required to get the correct default library search paths, due to some quirks of the AIX linker.
Stabilize `extended_varargs_abi_support`
I think that is everything? If there is any documentation regarding `extern` and/or varargs to correct, let me know, some quick greps suggest that there might be none.
Tracking issue: https://github.com/rust-lang/rust/issues/100189
Bump boostrap compiler to new beta
Currently failing due to something about the const stability checks and `panic!`. I'm not sure why though since I wasn't able to see any PRs merged in the past few days that would result in a `cfg(bootstrap)` that shouldn't be removed. cc `@RalfJung` #131349
Support `clobber_abi` in AVR inline assembly
This PR implements the `clobber_abi` part necessary to eventually stabilize the inline assembly for AVR. This is tracked in #93335.
This is heavily inspired by the sibling-PR #131310 for the MSP430. I've explained my reasoning in the first commit message in detail, which is reproduced below for easier reviewing:
This follows the [ABI documentation] of AVR-GCC:
> The [...] call-clobbered general purpose registers (GPRs) are registers that might be destroyed (clobbered) by a function call.
>
> - **R18–R27, R30, R31**
>
> These GPRs are call clobbered. An ordinary function may use them without restoring the contents. [...]
>
> - **R0, T-Flag**
>
> The temporary register and the T-flag in SREG are also call-clobbered, but this knowledge is not exposed explicitly to the compiler (R0 is a fixed register).
Therefore this commit lists the aforementioned registers `r18–r27`, `r30` and `r31` as clobbered registers. Since the `r0` register (listed above as well) is not available in inline assembly at all (potentially because the AVR-GCC considers it a fixed register causing the register to never be used in register allocation and LLVM adopting this), there is no need to list it in the clobber list (the `r0`-variant is not even available). A comment was added to ensure, that the `r0` gets added to the clobber-list once the register gets usable in inline ASM.
Since the SREG is normally considered clobbered anyways (unless the user supplies the `preserve_flags`-option), there is no need to explicitly list a bit in this register (which is not possible to list anyways).
Note, that this commit completely ignores the case of interrupts (that are described in the ABI-specification), since every register touched in an ISR need to be saved anyways.
[ABI documentation]: https://gcc.gnu.org/wiki/avr-gcc#Call-Used_Registers
r? ``@Amanieu``
``@rustbot`` label +O-AVR
Rollup of 7 pull requests
Successful merges:
- #132782 (improvements on initial sysroot and libdir finding logics)
- #133466 (Fix typos in pin.rs)
- #133492 (bootstrap: allow skipping steps with start of path)
- #133501 (support revealing defined opaque post borrowck)
- #133530 (Use consistent wording in docs, use is zero instead of is 0)
- #133538 (Better diagnostic for fn items in variadic functions)
- #133590 (Rename `-Zparse-only`)
r? `@ghost`
`@rustbot` modify labels: rollup
support revealing defined opaque post borrowck
By adding a new `TypingMode::PostBorrowckAnalysis`. Currently only supported with the new solver and I didn't look into the way we replace `ReErased`. ``@compiler-errors`` mentioned that always using existentials may be unsound.
r? ``@compiler-errors``
Remove `HybridBitSet`
`HybridBitSet` was introduced under the name `HybridIdxSetBuf` way back in #53383 where it was a big win for NLL borrow checker performance. In #93984 the more flexible `ChunkedBitSet` was added. Uses of `HybridBitSet` have gradually disappeared (e.g. #116152) and there are now few enough that they can be replaced with `BitSet` or `ChunkedBitSet`, and `HybridBitSet` can be removed, cutting more than 700 lines of code.
r? `@Mark-Simulacrum`
As part of the larger goal of reducing `HybridBitSet` use in general.
This code is for debugging only and isn't performance sensitive, so
`ChunkedBitSet` should be fine.
The current implementation is slow because it does an operation for
every bit in the set, even zero bits. So if you have a large bitset with
many zero bits (which is common) it's very slow.
This commit improves the iterator to skip over `Zeros` chunks in a
single step, and uses the fast `BitIter` for `Mixed` chunks. It also
removes the existing `fold` implementation, which was only there because
the old iterator was slow.
- Fix a typo in a comment.
- Remove unnecessary `Chunk::` qualifiers.
- Rename `ChunkedBitIter::bitset` as `ChunkedBitIter::bit_set`, because
`bit_set` is the form used everywhere else.
- Avoid some unnecessary local variables.
`ChunkedBitSet::is_empty` currently does an unnecessary check. This
commit removes that check and adds clarifying comments and an assertion
that demonstrate why it's unnecessary.
Revert #133418 (Store coverage source regions as `Span`) due to regression #133606
This reverts commit adf9b5fcd1, reversing changes made to af1ca153d4.
Reverting #133418 due to regressions reported at #133606.
r? jieyouxu
ensure JSON-defined targets are consistent
We have a `check_consistency` check that ensures some invariants which (presumably) the rest of the compiler relies on. However, JSON targets can easily be written in a way that violates those invariants. So this PR applies the same consistency check to JSON targets that we already enforce for built-in targets.
I have converted many of the assertions in that function to new macros that show a nice error instead of a panic; if people are okay with the general approach here, I can do that for the rest of the checks as well.
Enable -Zshare-generics for inline(never) functions
This avoids inlining cross-crate generic items when possible that are
already marked inline(never), implying that the author is not intending
for the function to be inlined by callers. As such, having a local copy
may make it easier for LLVM to optimize but mostly just adds to binary
bloat and codegen time. In practice our benchmarks indicate this is
indeed a win for larger compilations, where the extra cost in dynamic
linking to these symbols is diminished compared to the advantages in
fewer copies that need optimizing in each binary.
It might also make sense it expand this with other heuristics (e.g.,
`#[cold]`) in the future, but this seems like a good starting point.
FWIW, I expect that doing cleanup in where we make the decision
what should/shouldn't be shared is also a good idea. Way too
much code needed to be tweaked to check this. But I'm hoping
to leave that for a follow-up PR rather than blocking this on it.
this is funny though! apparently tidy parsed `.gitignore`, but did not
recognize unignore lines (`!...`), so tidy was ignoring `rustc_mir_build`
this whole time (at least for some lints?).
I was surprised to find that running with `-Zparse-only` only parses the
crate root file. Other files aren't parsed because that happens later
during expansion.
This commit renames the option and updates the help message to make this
clearer.
This reduces code sizes and better respects programmer intent when
marking inline(never). Previously such a marking was essentially ignored
for generic functions, as we'd still inline them in remote crates.
