Rollup of 6 pull requests #133684
Merged
Rollup of 6 pull requests #133684
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Signed-off-by: onur-ozkan <[email protected]>
…ck, r=RalfJung 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.
…ratrieb 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
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``
…e, r=clubby789 Remove unused code
bump hashbrown version This pulls in rust-lang/hashbrown#586, in preparation for rust-lang#102575. Cc ``@Amanieu``
replace hard coded error id with `ErrorKind::DirectoryNotEmpty` Resolves an internal bootstrap FIXME.
rustbot
added
A-run-make
|
@bors r+ rollup=never p=5 |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
📌 Perf builds for each rolled up PR:
previous master: f981b2e27a In the case of a perf regression, run the following command for each PR you suspect might be the cause: |
Merged
|
Finished benchmarking commit (7442931): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 2.0%, secondary -1.7%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 2.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 774.827s -> 774.339s (-0.06%) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Successful merges:
std#133571 (Mark visionOS as supportingstd)ErrorKind::DirectoryNotEmpty#133673 (replace hard coded error id withErrorKind::DirectoryNotEmpty)r? @ghost
@rustbot modify labels: rollup
Create a similar rollup