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

Rollup of 7 pull requests #137163

Merged
merged 29 commits into from
Feb 17, 2025
Merged

Rollup of 7 pull requests #137163

merged 29 commits into from
Feb 17, 2025

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

dianne and others added 29 commits February 10, 2025 04:08
Because of an ambiguity with const closures, the parser needs to ensure
that for a const item, the `const` keyword isn't followed by a `move` or
`static` keyword, as that would indicate a const closure:

```rust
fn main() {
  const move // ...
}
```

This check did not take raw identifiers into account, therefore being
unable to distinguish between `const move` and `const r#move`. The
latter is obviously not a const closure, so it should be allowed as a
const item.

This fixes the check in the parser to only treat `const ...` as a const
closure if it's followed by the *proper keyword*, and not a raw
identifier.

Additionally, this adds a large test that tests for all raw identifiers in
all kinds of positions, including `const`, to prevent issues like this
one from occurring again.
It's similar to the other limits, e.g. obtained via `get_limit`. So it
makes sense to handle it consistently with the other limits. We now use
`Limit`/`usize` in most places instead of `Option<usize>`, so we use
`Limit::new(usize::MAX)`/`usize::MAX` to emulate how `None` used to work.

The commit also adds `Limit::unlimited`.
Thanks to the previous commit, they no longer need to be separate.
It's always good to make `rustc_middle` smaller. `rustc_interface` is
the best destination, because it's the only crate that calls
`get_recursive_limit`.
For consistency with `recursion_limit`, `move_size_limit`, and
`type_length_limit`.
The `Map` trait is there for cases where `tcx` isn't available. This
isn't one of those cases, so it's simpler to just call through `tcx`
directly.
All the methods are unreachable, so make that clearer.
There is no need for the extra subdirectory, and this makes the `map`
module consistent with its sibling modules `nested_filter` and `place`.
The end goal is to eliminate `Map` altogether.

I added a `hir_` prefix to all of them, that seemed simplest. The
exceptions are `module_items` which became `hir_module_free_items` because
there was already a `hir_module_items`, and `items` which became
`hir_free_items` for consistency with `hir_module_free_items`.
First of all, note that `Map` has three different relevant meanings.
- The `intravisit::Map` trait.
- The `map::Map` struct.
- The `NestedFilter::Map` associated type.

The `intravisit::Map` trait is impl'd twice.
- For `!`, where the methods are all unreachable.
- For `map::Map`, which gets HIR stuff from the `TyCtxt`.

As part of getting rid of `map::Map`, this commit changes `impl
intravisit::Map for map::Map` to `impl intravisit::Map for TyCtxt`. It's
fairly straightforward except various things are renamed, because the
existing names would no longer have made sense.

- `trait intravisit::Map` becomes `trait intravisit::HirTyCtxt`, so named
  because it gets some HIR stuff from a `TyCtxt`.
- `NestedFilter::Map` assoc type becomes `NestedFilter::MaybeTyCtxt`,
  because it's always `!` or `TyCtxt`.
- `Visitor::nested_visit_map` becomes `Visitor::maybe_tcx`.

I deliberately made the new trait and associated type names different to
avoid the old `type Map: Map` situation, which I found confusing. We now
have `type MaybeTyCtxt: HirTyCtxt`.
It's a trivial wrapper around the `hir_crate` query with a small number
of uses.
…cjgillot

Start removing `rustc_middle::hir::map::Map`

`rustc_middle::hir::map::Map` is now just a low-value wrapper around `TyCtxt`. This PR starts removing it.

r? `@cjgillot`
…eril

Overhaul `rustc_middle::limits`

In particular, to make `pattern_complexity` work more like other limits, which then enables some other simplifications.

r? ``@Nadrieril``
…tion, r=Nadrieril

Pattern Migration 2024: clean up and comment

This follows up on rust-lang#136577 by moving the pattern migration logic to its own module, removing a bit of unnecessary complexity, and adding comments. Since there's quite a bit of pattern migration logic now (and potentially more in rust-lang#136496), I think it makes sense to keep it separate from THIR construction, at least as much as is convenient.

r? ``@Nadrieril``
…sDenton

Use `const_error!` when possible

Replace usages of `io::Error::new(io::ErrorKind::Variant, "constant string")` with `io::const_error!(io::ErrorKind::Variant, "constant string")` to avoid allocations when possible. Additionally, fix `&&str` error messages in SGX and missing/misplaced trailing commas in `const_error!`.
bootstrap: add more tracing to compiler/std/llvm flows

- Add more tracing to compiler/std/llvm flows.
- Two drive-by nits:
    1. Take `TargetSelection` by-value for `builder.is_builder_target()`. Noticed while adding tracing; follow-up to rust-lang#136767.
    2. Coalesce enzyme build logic into one branch.
- Document `COMPILER{,_FOR}` tracing targets for rust-lang#96176.
- No functional changes.

### Testing

You can play with the tracing locally with:

```
$ BOOTSTRAP_TRACING=bootstrap=debug ./x build library
$ BOOTSTRAP_TRACING=bootstrap=trace ./x build library
$ BOOTSTRAP_TRACING=bootstrap=trace,COMPILER=trace,COMPILER_FOR=trace ./x build library
```

### Previews

```
$ BOOTSTRAP_TRACING=bootstrap=debug ./x build library
```

![Screenshot 2025-02-15 230824](https://github.com/user-attachments/assets/c3b02b62-d52e-4c03-a00a-da0d95618989)

```
$ BOOTSTRAP_TRACING=bootstrap=trace,COMPILER=trace,COMPILER_FOR=trace ./x build library
```

![Screenshot 2025-02-15 233859](https://github.com/user-attachments/assets/842e4ece-4c26-4191-acbb-5f93e42de4dc)

r? ``@onur-ozkan`` (or reroll)
…Urgau

`invalid_from_utf8[_unchecked]`: also lint inherent methods

Addressing rust-lang#131114 (comment)

Also corrected a typo: "_an_ invalid literal", not "_a_ invalid literal".
…ompiler-errors

Fix const items not being allowed to be called `r#move` or `r#static`

Because of an ambiguity with const closures, the parser needs to ensure that for a const item, the `const` keyword isn't followed by a `move` or `static` keyword, as that would indicate a const closure:

```rust
fn main() {
  const move // ...
}
```

This check did not take raw identifiers into account, therefore being unable to distinguish between `const move` and `const r#move`. The latter is obviously not a const closure, so it should be allowed as a const item.

This fixes the check in the parser to only treat `const ...` as a const closure if it's followed by the *proper keyword*, and not a raw identifier.

Additionally, this adds a large test that tests for all raw identifiers in all kinds of positions, including `const`, to prevent issues like this one from occurring again.

fixes rust-lang#137128
@rustbot rustbot added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Feb 17, 2025
@rustbot rustbot added O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. rollup A PR which is a rollup labels Feb 17, 2025
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=5

@bors
Copy link
Contributor

bors commented Feb 17, 2025

📌 Commit f071099 has been approved by matthiaskrgr

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 17, 2025
@bors
Copy link
Contributor

bors commented Feb 17, 2025

⌛ Testing commit f071099 with merge 273465e...

@bors
Copy link
Contributor

bors commented Feb 17, 2025

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing 273465e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 17, 2025
@bors bors merged commit 273465e into rust-lang:master Feb 17, 2025
7 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 17, 2025
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#136466 Start removing rustc_middle::hir::map::Map 24b5ecef3da30f0cb5d4d82427072f8ac80efeee (link)
#136671 Overhaul rustc_middle::limits 7159f0b7e6d9c1a4d640ee02321cdde33598efc1 (link)
#136817 Pattern Migration 2024: clean up and comment 3ec5d8bef5e0366545f6e8f15716d81d0a4ff260 (link)
#136844 Use const_error! when possible 6e5dfcce47fac3b8be892f420243b91cd6cafc3d (link)
#137080 bootstrap: add more tracing to compiler/std/llvm flows d96c7a8f45924a7e87d281bed4f3a920f234e4b9 (link)
#137101 invalid_from_utf8[_unchecked]: also lint inherent methods 9cd5a37d86e4d52fd34d2e27bf77f6eedb136b41 (link)
#137140 Fix const items not being allowed to be called r#move or … 964a73c43e3a81b9a9659d86b10fc7f3691b2605 (link)

previous master: d5eb31c934

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (273465e): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

  • If the regression was expected or you think it can be justified,
    please write a comment with sufficient written justification, and add
    @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
  • If you think that you know of a way to resolve the regression, try to create
    a new PR with a fix for the regression.
  • If you do not understand the regression or you think that it is just noise,
    you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
    were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.1% [0.1%, 0.1%] 1
Improvements ✅
(primary)
-0.3% [-0.3%, -0.3%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.3% [-0.3%, -0.3%] 2

Max RSS (memory usage)

Results (primary 0.6%)

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.

mean range count
Regressions ❌
(primary)
3.1% [3.1%, 3.1%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.0% [-2.0%, -2.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.6% [-2.0%, 3.1%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 789.735s -> 789.915s (0.02%)
Artifact size: 350.22 MiB -> 350.21 MiB (-0.00%)

@rustbot rustbot added the perf-regression Performance regression. label Feb 17, 2025
@Kobzol
Copy link
Contributor

Kobzol commented Feb 18, 2025

Three tiny changes, the only regression is on an incr-unchanged run of a parser stress test, I don't think that this warrants further investigation.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) A-rustc-dev-guide Area: rustc-dev-guide A-rustdoc-json Area: Rustdoc JSON backend A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. O-hermit Operating System: Hermit O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-windows Operating system: Windows perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.