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 9 pull requests #120767

Merged
merged 30 commits into from
Feb 8, 2024
Merged

Rollup of 9 pull requests #120767

merged 30 commits into from
Feb 8, 2024

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

Nadrieril and others added 30 commits January 25, 2024 02:35
It avoids a lot of repetition.
…rors

resolve: Unload speculatively resolved crates before freezing cstore

Name resolution sometimes loads additional crates to improve diagnostics (e.g. suggest imports).
Not all of these diagnostics result in errors, sometimes they are just warnings, like in rust-lang#117772.

If additional crates loaded speculatively stay and gets listed by things like `query crates` then they may produce further errors like duplicated lang items, because lang items from speculatively loaded crates are as good as from non-speculatively loaded crates.
They can probably do things like adding unintended impls from speculatively loaded crates to method resolution as well.
The extra crates will also get into the crate's metadata as legitimate dependencies.

In this PR I remove the speculative crates from cstore when name resolution is finished and cstore is frozen.
This is better than e.g. filtering away speculative crates in `query crates` because things like `DefId`s referring to these crates and leaking to later compilation stages can produce ICEs much easier, allowing to detect them.

The unloading could potentially be skipped if any errors were reported (to allow using `DefId`s from speculatively loaded crates for recovery), but I didn't do it in this PR because I haven't seen such cases of recovery. We can reconsider later if any relevant ICEs are reported.

Unblocks rust-lang#117772.
…oli-obk

Make it so that async-fn-in-trait is compatible with a concrete future in implementation

There's no technical reason why an AFIT like `async fn foo()` cannot be satisfied with an implementation signature like `fn foo() -> Pin<Box<dyn Future<Output = ()> + 'static>>`.

We rejected this previously because we were uncertain about how AFITs worked with refinement, but I don't believe this needs to be a restriction any longer.

r? oli-obk
…rrors

hir: Make sure all `HirId`s have corresponding HIR `Node`s

And then remove `tcx.opt_hir_node(hir_id)` in favor of `tcx.hir_node(hir_id)`.
match lowering: consistently lower bindings deepest-first

Currently when lowering match expressions to MIR, we do a funny little dance with the order of bindings. I attempt to explain it in the third commit: we handle refutable (i.e. needing a test) patterns differently than irrefutable ones. This leads to inconsistencies, as reported in rust-lang#120210. The reason we need a dance at all is for situations like:

```rust
fn foo1(x: NonCopyStruct) {
    let y @ NonCopyStruct { copy_field: z } = x;
    // the above should turn into
    let z = x.copy_field;
    let y = x;
}
```

Here the `y ```````@```````` binding will move out of `x`, so we need to copy the field first.

I believe that the inconsistency came about when we fixed rust-lang#69971, and didn't notice that the fix didn't extend to refutable patterns. My guess then is that ordering bindings by "deepest-first, otherwise source order" is a sound choice. This PR implements that (at least I hope, match lowering is hard to follow 🥲).

Fixes rust-lang#120210

r? ```````@oli-obk``````` since you merged the original fix to rust-lang#69971
cc ```````@matthewjasper```````
GVN: also turn moves into copies with projections

Fixes rust-lang#120613
docs: also check the inline stmt during redundant link check

Fixes rust-lang#120444

This issue was brought about by querying `root::webdavfs::A`, a key that doesn't exist in `doc_link_resolutions`. To avoid a panic, I've altered the gating mechanism to allow this lint pass to be skipped.

I'm not certain if this is the best solution. An alternative approach might be to leverage other info from the name resolutions instead of `doc_link_resolutions`. After all, all we need is to get the resolution from a combination of `(module, name)`. However, I believe they would yield the same outcome, both skipping this lint.
…ompiler-errors

exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered"

There was an exception when reporting integer ranges as missing, it's been there for as long as I can remember. This PR removes it. I think it's nicer to report "`0..MAX` not covered" than "`_` not covered". This also makes it consistent with enums, where we report individual enum variants in this case (as showcased in the rest of the `empty-match.rs` test).

r? ``@estebank``
…, r=compiler-errors

Add `SubdiagnosticMessageOp` as a trait alias.

It avoids a lot of repetition.

r? matthewjasper
…r-errors

improve pretty printing for associated items in trait objects

* Don't print a binder in front of associated items, because it's not valid syntax.
  * e.g. print `dyn for<'a> Trait<'a, Assoc = &'a u8>` instead of `dyn for<'a> Trait<'a, for<'a> Assoc = &'a u8>`.
* Don't print associated items that are implied by a supertrait bound.
  * e.g. if we have `trait Sub: Super<Assoc = u8> {}`, then just print `dyn Sub` instead of `dyn Sub<Assoc = u8>`.

I've added the test in the first commit, so you can see the diff of the compiler output in the second commit.
@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler 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. rollup A PR which is a rollup labels Feb 8, 2024
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=9

@bors
Copy link
Contributor

bors commented Feb 8, 2024

📌 Commit a059dd8 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 8, 2024
@bors
Copy link
Contributor

bors commented Feb 8, 2024

⌛ Testing commit a059dd8 with merge 1280928...

@bors
Copy link
Contributor

bors commented Feb 8, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 8, 2024
@bors bors merged commit 1280928 into rust-lang:master Feb 8, 2024
12 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 8, 2024
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#119592 resolve: Unload speculatively resolved crates before freezi… 0263fa6b03933925db4d4458b79dd6bd0a285c05 (link)
#120103 Make it so that async-fn-in-trait is compatible with a conc… cc0aec1ae1a126f6a21cb1a71313ec347a9cc6c1 (link)
#120206 hir: Make sure all HirIds have corresponding HIR Nodes 8a04ceeb309e44a2fdeb1bc192c5229fffbcb4a1 (link)
#120214 match lowering: consistently lower bindings deepest-first f8e5f062a5a7802a2762b79a5763920562f7f38a (link)
#120688 GVN: also turn moves into copies with projections 690b2339bb6d488dcf9917168eab1f226347cd4f (link)
#120702 docs: also check the inline stmt during redundant link check 1462f5bca72ea3b5fcc2c6ddb5da0032a48d6814 (link)
#120727 exhaustiveness: Prefer "0..MAX not covered" to "_ not c… 9ff31ab4b1e21969ebeacc66d46c50e2c20cf34d (link)
#120734 Add SubdiagnosticMessageOp as a trait alias. 71c7662c7262fe0696a06e1e72fcc1df96f6257f (link)
#120739 improve pretty printing for associated items in trait objec… ba9fbae264895c534bd5c1bee2fb366c285a3653 (link)

previous master: af88f7db51

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 (1280928): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

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

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.3%, 0.4%] 2
Regressions ❌
(secondary)
0.3% [0.3%, 0.3%] 1
Improvements ✅
(primary)
-0.2% [-0.3%, -0.2%] 13
Improvements ✅
(secondary)
-0.4% [-0.8%, -0.2%] 25
All ❌✅ (primary) -0.2% [-0.3%, 0.4%] 15

Max RSS (memory usage)

Results

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)
6.5% [2.9%, 10.1%] 2
Regressions ❌
(secondary)
2.4% [2.4%, 2.4%] 1
Improvements ✅
(primary)
-1.5% [-3.4%, -0.7%] 20
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.8% [-3.4%, 10.1%] 22

Cycles

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

Binary size

Results

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)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.0% [-0.0%, -0.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.0%, -0.0%] 1

Bootstrap: 663.188s -> 662.316s (-0.13%)
Artifact size: 308.26 MiB -> 308.26 MiB (-0.00%)

@Kobzol
Copy link
Contributor

Kobzol commented Feb 13, 2024

Many more wins than regressions.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 13, 2024
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 26, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#119592 (resolve: Unload speculatively resolved crates before freezing cstore)
 - rust-lang#120103 (Make it so that async-fn-in-trait is compatible with a concrete future in implementation)
 - rust-lang#120206 (hir: Make sure all `HirId`s have corresponding HIR `Node`s)
 - rust-lang#120214 (match lowering: consistently lower bindings deepest-first)
 - rust-lang#120688 (GVN: also turn moves into copies with projections)
 - rust-lang#120702 (docs: also check the inline stmt during redundant link check)
 - rust-lang#120727 (exhaustiveness: Prefer "`0..MAX` not covered" to "`_` not covered")
 - rust-lang#120734 (Add `SubdiagnosticMessageOp` as a trait alias.)
 - rust-lang#120739 (improve pretty printing for associated items in trait objects)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr deleted the rollup-0k8ib1c branch March 16, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. 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-compiler Relevant to the compiler 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.