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 10 pull requests #73367

Merged
merged 30 commits into from
Jun 15, 2020
Merged

Rollup of 10 pull requests #73367

merged 30 commits into from
Jun 15, 2020

Conversation

RalfJung
Copy link
Member

Successful merges:

Failed merges:

r? @ghost

matthew-mcallister and others added 30 commits May 24, 2020 17:54
Fixes rust-lang#69446

When we encounter a region error involving an `FnMut` closure, we
display a specialized error message. However, we currently do not
tell the user which upvar was captured. This makes it difficult to
determine the cause of the error, especially when the closure is large.

This commit records marks constraints involving closure upvars
with `ConstraintCategory::ClosureUpvar`. When we decide to 'blame'
a `ConstraintCategory::Return`, we additionall store
the captured upvar if we found a `ConstraintCategory::ClosureUpvar` in
the path.

When generating an error message, we point to relevant spans if we have
closure upvar information available. We further customize the message if
an `async` closure is being returned, to make it clear that the captured
variable is being returned indirectly.
Also, implement this query for the local crate, not just foreign crates.
It was applied to a `use` item, not to the module
This commit adds a `Closure` variant to `NonStructuralMatchTy` in
`structural_match`, fixing an ICE which can occur when
`impl_trait_in_bindings` is used with constants.

Signed-off-by: David Wood <[email protected]>
The existing error documentation did not show how to use a child module's functions if the types used in those functions are private. These are some other places this problem has popped up that did not present a solution (these are from before the solution existed, 2016-2017. The solution was released in the Rust 2018 edition. However these were the places I was pointed to when I encountered the problem myself):
rust-lang#30905
https://stackoverflow.com/questions/39334430/how-to-reference-private-types-from-public-functions-in-private-modules/62374958#62374958
…p-elab, r=oli-obk

Check for live drops in constants after drop elaboration

Resolves rust-lang#66753.

This PR splits the MIR "optimization" pass series in two and introduces a query–`mir_drops_elaborated_and_const_checked`–that holds the result of the `post_borrowck_cleanup` analyses and checks for live drops. This query is invoked in `rustc_interface` for all items requiring const-checking, which means we now do `post_borrowck_cleanup` for items even if they are unused in the crate.

As a result, we are now more precise about when drops are live. This is because drop elaboration can e.g. eliminate drops of a local when all its fields are moved from. This does not mean we are doing value-based analysis on move paths, however; Storing a `Some(CustomDropImpl)` into a field of a local will still set the qualifs for that entire local.

r? @oli-obk
… r=nikomatsakis

Explain move errors that occur due to method calls involving `self`

When calling a method that takes `self` (e.g. `vec.into_iter()`), the method receiver is moved out of. If the method receiver is used again, a move error will be emitted::

```rust
fn main() {
    let a = vec![true];
    a.into_iter();
    a;
}
```

emits

```
error[E0382]: use of moved value: `a`
 --> src/main.rs:4:5
  |
2 |     let a = vec![true];
  |         - move occurs because `a` has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
3 |     a.into_iter();
  |     - value moved here
4 |     a;
  |     ^ value used here after move
```

However, the error message doesn't make it clear that the move is caused by the call to `into_iter`.

This PR adds additional messages to move errors when the move is caused by using a value as the receiver of a `self` method::

```
error[E0382]: use of moved value: `a`
   --> vec.rs:4:5
    |
2   |     let a = vec![true];
    |         - move occurs because `a` has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
3   |     a.into_iter();
    |     ------------- value moved due to this method call
4   |     a;
    |     ^ value used here after move
    |
note: this function takes `self`, which moves the receiver
   --> /home/aaron/repos/rust/src/libcore/iter/traits/collect.rs:239:5
    |
239 |     fn into_iter(self) -> Self::IntoIter;
```

TODO:

- [x] Add special handling for `FnOnce/FnMut/Fn` - we probably don't want to point at the unstable trait methods
- [x] Consider adding additional context for operations (e.g. `Shr::shr`) when the call was generated using the operator syntax (e.g. `a >> b`)
- [x] Consider pointing to the method parent (impl or trait block) in addition to the method itself.
…rent-impl, r=estebank

Fix trait alias inherent impl resolution

Fixes rust-lang#60021 and fixes rust-lang#72415.

Obviously, the fix was very easy, but getting started with the testing and debugging rust compiler was an interesting experience. Now I can cross it off my bucket list!
Stabilize vec::Drain::as_slice

and add `AsRef<[T]> for Drain<'_, T>`.

Tracking issue: rust-lang#58957. Does not stabilize `slice::IterMut::as_slice` yet. cc @cuviper
This PR proposes stabilizing just the `vec::Drain::as_slice` part of that tracking issue.

My ultimate goal here: being able to use `for<T, I: Iterator<Item=T>   AsRef<[T]>> I` to refer to `vec::IntoIter`, `vec::Drain`, and eventually `array::IntoIter`, as an approximation of the set of by-value iterators that can be "previewed" as by-ref iterators. (Actually expressing that as a trait requires GAT.)
…n, r=nikomatsakis

Display information about captured variable in `FnMut` error

Fixes rust-lang#69446

When we encounter a region error involving an `FnMut` closure, we
display a specialized error message. However, we currently do not
tell the user which upvar was captured. This makes it difficult to
determine the cause of the error, especially when the closure is large.

This commit records marks constraints involving closure upvars
with `ConstraintCategory::ClosureUpvar`. When we decide to 'blame'
a `ConstraintCategory::Return`, we additionall store
the captured upvar if we found a `ConstraintCategory::ClosureUpvar` in
the path.

When generating an error message, we point to relevant spans if we have
closure upvar information available. We further customize the message if
an `async` closure is being returned, to make it clear that the captured
variable is being returned indirectly.
Group `Pattern::strip_*` method together
_match.rs: fix module doc comment

It was applied to a `use` item, not to the module
Fix iterator copied() documentation example code

The documentation for copied() gives example code with variable v_cloned instead of v_copied. This seems like a copy/paste error from cloned() and it would be clearer to use v_copied.
Update E0446.md

The existing error documentation did not show how to use a child module's functions if the types used in those functions are private. These are some other places this problem has popped up that did not present a solution (these are from before the solution existed, 2016-2017. The solution was released in the Rust 2018 edition. However these were the places I was pointed to when I encountered the problem myself):
rust-lang#30905
https://stackoverflow.com/questions/39334430/how-to-reference-private-types-from-public-functions-in-private-modules/62374958#62374958
…l-match-ty-closures, r=varkor

structural_match: non-structural-match ty closures

Fixes rust-lang#73003.

This PR adds a `Closure` variant to `NonStructuralMatchTy` in `structural_match`, fixing an ICE which can occur when `impl_trait_in_bindings` is used with constants.
@RalfJung
Copy link
Member Author

@rustbot modify labels: rollup
@bors r rollup=never p=10

@bors
Copy link
Contributor

bors commented Jun 15, 2020

📌 Commit bca9e90 has been approved by RalfJung

@rustbot rustbot added the rollup A PR which is a rollup label Jun 15, 2020
@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 Jun 15, 2020
@bors
Copy link
Contributor

bors commented Jun 15, 2020

⌛ Testing commit bca9e90 with merge d4ecf31...

@bors
Copy link
Contributor

bors commented Jun 15, 2020

☀️ Test successful - checks-azure
Approved by: RalfJung
Pushing d4ecf31 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 15, 2020
@bors bors merged commit d4ecf31 into rust-lang:master Jun 15, 2020
@RalfJung RalfJung deleted the rollup-4ewvk9b branch June 15, 2020 17:08
@nnethercote
Copy link
Contributor

One or more PRs in this rollup caused a perf regression of up to 3.5%.

@ecstatic-morse, @Aaron1011, @matthew-mcallister, @CAD97, @lzutao, @jonas-schievink, @schteve, @gnodarse, @davidtwco: any idea which PR(s) may have been the cause?

@nnethercote
Copy link
Contributor

@ecstatic-morse: #71824 would be my guess just by looking at the PR names, but I could easily be wrong about that.

@RalfJung: #71824 had 8 commits, modified 16 files, and was (to my eye) non-trivial. What's the criteria for including a PR in a rollup? That feels to me like a PR that would have been better to land by itself.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jun 16, 2020

Because I marked it rollup=always. It only affects const items and only when a feature gate is enabled. It does add an intermediate MIR query that gets stolen from, but I'd be surprised if that was the culprit.

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jun 16, 2020

The largest regressions are in the unchanged incremental benchmarks, and mostly in HIR lowering. I suspect one of the diagnostics improvements PRs made a frequently copied data structure larger.

@nnethercote
Copy link
Contributor

@Aaron1011: what do you think?

@Aaron1011
Copy link
Member

Aaron1011 commented Jun 16, 2020

The regression appears to be in hir_lowering, so it seems possible that these for-loop desguaring changes in #72389 are the cause.

@RalfJung
Copy link
Member Author

@nnethercote

#71824 had 8 commits, modified 16 files, and was (to my eye) non-trivial. What's the criteria for including a PR in a rollup? That feels to me like a PR that would have been better to land by itself.

It's a <1k line diff, so I would have considered rolling it up even without @ecstatic-morse's annotation. We have a huge queue so aggressively rolling up is our only option to keep PRs landing faster than they are approved. We can maybe land 2-3 PRs by themselves per day currently, everything else should be rollups of 10 PRs or more.

@RalfJung
Copy link
Member Author

The regression appears to be in hir_lowering, so it seems possible that these for-loop desguaring changes in #72389 are the cause.

What would be really useful for cases like this is being able to run rust-timer on a PR after it already landed... @rust-lang/infra is that something we can do?

@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jun 19, 2020

@nnethercote, #72598 caused a 1.4% increase in instruction count for full builds of wg-grammar. #72389 caused a small increase across the board for all patched/unchanged incremental benchmarks, with keccak being the worst.

I don't think we need to revert PRs that cause small regressions in unchanged incremental benchmarks (which run quite fast) or in a single uncommon crate. These two PRs both added non-trivial functionality; they weren't just refactoring. That said, I wonder if @Aaron1011 has any ideas to speed these PRs up?

@cuviper cuviper added this to the 1.46 milestone May 2, 2024
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. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.