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 #95662

Merged
merged 23 commits into from Apr 5, 2022
Merged

Rollup of 7 pull requests #95662

merged 23 commits into from Apr 5, 2022

Conversation

Dylan-DPC
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

compiler-errors and others added 23 commits April 2, 2022 15:29
When encountering an unsatisfied trait bound, if there are no other
suggestions, mention all the types that *do* implement that trait:

```
error[E0277]: the trait bound `f32: Foo` is not satisfied
  --> $DIR/impl_wf.rs:22:6
   |
LL | impl Baz<f32> for f32 { }
   |      ^^^^^^^^ the trait `Foo` is not implemented for `f32`
   |
   = help: the following other types implement trait `Foo`:
             Option<T>
             i32
             str
note: required by a bound in `Baz`
  --> $DIR/impl_wf.rs:18:31
   |
LL | trait Baz<U: ?Sized> where U: Foo { }
   |                               ^^^ required by this bound in `Baz`
```

Mention implementers of traits in `ImplObligation`s.

Do not mention other `impl`s for closures, ranges and `?`.
…ied-trait, r=davidtwco

Mention implementers of unsatisfied trait

When encountering an unsatisfied trait bound, if there are no other
suggestions, mention all the types that *do* implement that trait:

```
error[E0277]: the trait bound `f32: Foo` is not satisfied
  --> $DIR/impl_wf.rs:22:6
   |
LL | impl Baz<f32> for f32 { }
   |      ^^^^^^^^ the trait `Foo` is not implemented for `f32`
   |
   = help: the trait `Foo` is implemented for `i32`
note: required by a bound in `Baz`
  --> $DIR/impl_wf.rs:18:31
   |
LL | trait Baz<U: ?Sized> where U: Foo { }
   |                               ^^^ required by this bound in `Baz`
```
```
error[E0277]: the trait bound `u32: Foo` is not satisfied
  --> $DIR/associated-types-path-2.rs:29:5
   |
LL |     f1(2u32, 4u32);
   |     ^^ the trait `Foo` is not implemented for `u32`
   |
   = help: the trait `Foo` is implemented for `i32`
note: required by a bound in `f1`
  --> $DIR/associated-types-path-2.rs:13:14
   |
LL | pub fn f1<T: Foo>(a: T, x: T::A) {}
   |              ^^^ required by this bound in `f1`
```

Suggest dereferencing in more cases.

Fix rust-lang#87437, fix rust-lang#90970.
explicitly distinguish pointer::addr and pointer::expose_addr

``@bgeron`` pointed out that the current docs promise that `ptr.addr()` and `ptr as usize` are equivalent. I don't think that is a promise we want to make. (Conceptually, `ptr as usize` might 'escape' the provenance to enable future `usize as ptr` casts, but `ptr.addr()` dertainly does not do that.)

So I propose we word the docs a bit more carefully here. ``@Gankra`` what do you think?
Fix late-bound ICE in `dyn` return type suggestion

This fixes the root-cause of the attached issues -- the root problem is that we're using the return type from a signature with late-bound instead of early-bound regions. The change on line 1087 (`let Some(liberated_sig) = typeck_results.liberated_fn_sigs().get(fn_hir_id) else { return false; };`) makes sure we're grabbing the _right_ return type for this suggestion to check the `dyn` predicates with.

Fixes rust-lang#91801
Fixes rust-lang#91803

This fix also includes some drive-by changes, specifically:

1. Don't suggest boxing when we have `-> dyn Trait` and are already returning `Box<T>` where `T: Trait` (before we always boxed the value).
2. Suggestion applies even when the return type is a type alias (e.g. `type Foo = dyn Trait`). This does cause the suggestion to expand to the aliased type, but I think it's still beneficial.
3. Split up the multipart suggestion because there's a 6-line max in the printed output...

I am open to splitting out the above changes, if we just want to fix the ICE first.

cc: ```@terrarier2111``` and rust-lang#92289
interpret: remove MemoryExtra in favor of giving access to the Machine

The Miri PR for this is upcoming.

r? ``@oli-obk``
…Jung

Update `NonNull` pointer provenance methods' documentation

 - Add links to equivalent methods on raw pointers
…locks, r=davidtwco

Refactor: remove unnecessary nested blocks
`CandidateSource::XCandidate` -> `CandidateSource::X`
@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. rollup A PR which is a rollup labels Apr 4, 2022
@Dylan-DPC
Copy link
Member Author

@bors r+ rollup=never p=5

@bors
Copy link
Contributor

bors commented Apr 4, 2022

📌 Commit b3c3eda has been approved by Dylan-DPC

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 4, 2022
@bors
Copy link
Contributor

bors commented Apr 5, 2022

⌛ Testing commit b3c3eda with merge c4b0e69efdf3ee0b46e722e8353a4daa83045899...

@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 Apr 5, 2022
@bors
Copy link
Contributor

bors commented Apr 5, 2022

⌛ Testing commit b3c3eda with merge a22cf2a...

@bors
Copy link
Contributor

bors commented Apr 5, 2022

☀️ Test successful - checks-actions
Approved by: Dylan-DPC
Pushing a22cf2a to master...

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (a22cf2a): comparison url.

Summary:

  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found. 19 results were found to be statistically significant but too small to be relevant.
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 20 13 0 6 20
mean2 0.8% 0.4% N/A -0.4% 0.8%
max 2.0% 0.7% N/A -0.5% 2.0%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

@rustbot rustbot added the perf-regression Performance regression. label Apr 5, 2022
@rylev
Copy link
Member

rylev commented Apr 5, 2022

@Dylan-DPC this unfortunately causes some pretty concerning regression in real world crates (an average of 0.8% regression across 20 different impacted test cases). I ran cachegrind and it seems that register_predicate_obligation is being called a lot more than previously. This would seem to point to #91873 or #95603. I'm not sure if @estebank or @compiler-errors have any thoughts here.

Here's what I ran locally from my rustc-perf checkout:
./target/release/collector profile_local cachegrind +60e50fc1cfe0bb693a5f4f93eb83ef70854531e3 --rustc2 +a22cf2af0510b3ec4cbb19c3de11d3d8291349d9 --include diesel-1.4.8 --profiles Check --scenarios Full

I got the following output: https://gist.github.com/rylev/50efc253c7538ee6125b01357dbe2196

@estebank
Copy link
Contributor

estebank commented Apr 5, 2022

It doesn't feel like #91873 would cause that (for code that compiles successfully), but I can't rule it out.

Edit: looking at #95603 it seems even less likely (there're multiple predicates being checked, but they only happen in suggestion code and they already existed).

Edit 2: having said that, I am not surprised that it'd be Diesel that got affected by more predicates being registered :)

@compiler-errors
Copy link
Member

Same for #95603, it should only affect code paths that end in errors...

@estebank
Copy link
Contributor

estebank commented Apr 5, 2022

@rylev can we run perf on the merged PRs directly so that we don't have to informed-guess that much?

@compiler-errors
Copy link
Member

@estebank: since this is merged, I will put up a perf revert experiment for our two PRs. If they show a performance gain, then I think we can continue to investigate.

@Dylan-DPC
Copy link
Member Author

@compiler-errors i'm already working on reverting #91873

@compiler-errors
Copy link
Member

Agh, just committed a branch for it.

@rylev
Copy link
Member

rylev commented Apr 5, 2022

@estebank as an FYI we do have some experimental support for automatically reverting rollup PRs and running perf on them, but it's unfortunately pretty buggy.

@estebank
Copy link
Contributor

estebank commented Apr 8, 2022

So neither revert PR yielded useful info?

@rylev
Copy link
Member

rylev commented Apr 8, 2022

It seems not 🙁. None of the other PRs really make sense. The only other ones that touch compiler code are #95642 (which refactors an enum's variant names), #95620 (which touches const eval), and #95631 (which refactors some nested if/let blocks to use if let && if let syntax).

Given it seems that the main offended is in trait selection, none of the remaining candidates seem to make much sense....

(FYI: there was discussion in Zulip and a lot of head scratching)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet