take 2
open up coroutines
tweak the wordings
the lint works up until 2021
We were missing one case, for ADTs, which was
causing `Result` to yield incorrect results.
only include field spans with significant types
deduplicate and eliminate field spans
switch to emit spans to impl Drops
Co-authored-by: Niko Matsakis <nikomat@amazon.com>
collect drops instead of taking liveness diff
apply some suggestions and add explantory notes
small fix on the cache
let the query recurse through coroutine
new suggestion format with extracted variable name
fine-tune the drop span and messages
bugfix on runtime borrows
tweak message wording
filter out ecosystem types earlier
apply suggestions
clippy
check lint level at session level
further restrict applicability of the lint
translate bid into nop for stable mir
detect cycle in type structure
There is an `Rc<UniversalRegions>` within `UniversalRegionRelations`,
and yet the two types get passed around in tandem a lot.
This commit makes `UniversalRegionRelations` own `UniversalRegions`,
removing the `Rc` (which wasn't truly needed) and the tandem-passing.
This requires adding a `universal_relations` method to
`UniversalRegionRelations`, and renaming a couple of existing methods
producing iterators to avoid a name clash.
the behavior of the type system not only depends on the current
assumptions, but also the currentnphase of the compiler. This is
mostly necessary as we need to decide whether and how to reveal
opaque types. We track this via the `TypingMode`.
`suggest_borrow_generic_arg`: instantiate clauses properly
This simplifies and fixes the way `suggest_borrow_generic_arg` instantiates callees' predicates when testing them to see if a moved argument can instead be borrowed. Previously, it would ICE if the moved argument's type included a region variable, since it was getting passed to a call of `EarlyBinder::instantiate`. This makes the instantiation much more straightforward, which also fixes the ICE.
Fixes#133118
This also modifies `tests/ui/moves/moved-value-on-as-ref-arg.rs` to have more useful bounds on the tests for suggestions to borrow `Borrow` and `BorrowMut` arguments. With its old tautological `T: BorrowMut<T>` bound, this fix would make it suggest a shared borrow for that argument.
Fixes issue 133118.
This also modifies `tests/ui/moves/moved-value-on-as-ref-arg.rs` to have more
useful bounds on the tests for suggestions to borrow `Borrow` and `BorrowMut`
arguments. With its old tautological `T: BorrowMut<T>` bound, this fix would
make it suggest a shared borrow for that argument.
This subsumes the suggestions to borrow arguments with `AsRef`/`Borrow` bounds and those to borrow
arguments with `Fn` and `FnMut` bounds. It works for other traits implemented on references as well,
such as `std::io::Read`, `std::io::Write`, and `core::fmt::Write`.
Incidentally, by making the logic for suggesting borrowing closures general, this removes some
spurious suggestions to mutably borrow `FnMut` closures in assignments, as well as an unhelpful
suggestion to add a `Clone` constraint to an `impl Fn` argument.
This is setup for unifing the logic for suggestions to borrow arguments in generic positions.
As of this commit, it's still special cases for `AsRef`/`Borrow`-like traits and `Fn`-like traits.
This also downgrades its applicability to MaybeIncorrect. Its suggestion can result in ill-typed
code when the type parameter it suggests providing a different generic argument for appears
elsewhere in the callee's signature or predicates.
Remove unnecessary pub enum glob-imports from `rustc_middle::ty`
We used to have an idiom in the compiler where we'd prefix or suffix all the variants of an enum, for example `BoundRegionKind`, with something like `Br`, and then *glob-import* that enum variant directly.
`@noratrieb` brought this up, and I think that it's easier to read when we just use the normal style `EnumName::Variant`.
This PR is a bit large, but it's just naming.
The only somewhat opinionated change that this PR does is rename `BorrowKind::Imm` to `BorrowKind::Immutable` and same for the other variants. I think these enums are used sparingly enough that the extra length is fine.
r? `@noratrieb` or reassign
Now that `Results` is the only impl of `ResultsVisitable`, the trait can
be removed. This simplifies things by removining unnecessary layers of
indirection and abstraction.
- `ResultsVisitor` is simpler.
- Its type parameter changes from `R` (an analysis result) to the
simpler `A` (an analysis).
- It no longer needs the `Domain` associated type, because it can use
`A::Domain`.
- Occurrences of `R` become `Results<'tcx, A>`, because there is now
only one kind of analysis results.
- `save_as_intervals` also changes type parameter from `R` to `A`.
- The `results.reconstruct_*` method calls are replaced with
`results.analysis.apply_*` method calls, which are equivalent.
- `Direction::visit_results_in_block` is simpler, with a single generic
param (`A`) instead of two (`D` and `R`/`F`, with a bound connecting
them). Likewise for `visit_results`.
- The `ResultsVisitor` impls for `MirBorrowCtxt` and
`StorageConflictVisitor` are now specific about the type of the
analysis results they work with. They both used to have a type param
`R` but they weren't genuinely generic. In both cases there was only a
single results type that made sense to instantiate them with.
The results of most analyses end up in a `Results<'tcx, A>`, where `A`
is the analysis. It's then possible to traverse the results via a
`ResultsVisitor`, which relies on the `ResultsVisitable` trait. (That
trait ends up using the same `apply_*` methods that were used when
computing the analysis, albeit indirectly.)
This pattern of "compute analysis results, then visit them" is common.
But there is one exception. For borrow checking we compute three
separate analyses (`Borrows`, `MaybeUninitializedPlaces`, and
`EverInitializedPlaces`), combine them into a single `BorrowckResults`,
and then do a single visit of that `BorrowckResults` with
`MirBorrowckResults`. `BorrowckResults` is just different enough from
`Results` that it requires the existence of `ResultsVisitable`, which
abstracts over the traversal differences between `Results` and
`BorrowckResults`.
This commit changes things by introducing `Borrowck` and bundling the
three borrowck analysis results into a standard `Results<Borrowck>`
instead of the exceptional `BorrowckResults`. Once that's done, the
results can be visited like any other analysis results.
`BorrowckResults` is removed, as is `impl ResultsVisitable for
BorrowckResults`. (It's instructive to see how similar the added `impl
Analysis for Borrowck` is to the removed `impl ResultsVisitable for
BorrowckResults`. They're both doing exactly the same things.)
Overall this increases the number of lines of code and might not seem
like a win. But it enables the removal of `ResultsVisitable` in the next
commit, which results in many simplifications.
- Store a mut ref to a `BorrowckDiags` in `MirBorrowckCtxt` instead of
owning it, to save having to pass ownership in and out of
`promoted_mbcx`.
- Use `buffer_error` in a couple of suitable places.
Because there is no real reason for it to be a separate struct.
- It has no methods.
- It's easy to confuse with the nearby `BorrowckInferContext` (which
does have methods).
- The `mut` ref to it in `TypeChecker` makes it seem like any of the
fields within might be mutable, but only two (`all_facts` and
`constraints`) actually are.
- Two of the fields are `pub(crate)` but can be private.
This change makes a lot of code more concise and readable.
It's strange to have a struct that contains a single anonymous field
that is an enum. This commit merges them. This does require increasing
the visibility of `TypeOfInfo` to `pub(crate)`, but that seems
worthwhile.
Remove region from adjustments
It's not necessary to store this region, because it's only used in THIR and MemCat/ExprUse, both of which already basically only deal with erased regions anyways.
This is a standard pattern:
```
MyAnalysis.into_engine(tcx, body).iterate_to_fixpoint()
```
`into_engine` and `iterate_to_fixpoint` are always called in pairs, but
sometimes with a builder-style `pass_name` call between them. But a
builder-style interface is overkill here. This has been bugging me a for
a while.
This commit:
- Merges `Engine::new` and `Engine::iterate_to_fixpoint`. This removes
the need for `Engine` to have fields, leaving it as a trivial type
that the next commit will remove.
- Renames `Analysis::into_engine` as `Analysis::iterate_to_fixpoint`,
gives it an extra argument for the optional pass name, and makes it
call `Engine::iterate_to_fixpoint` instead of `Engine::new`.
This turns the pattern from above into this:
```
MyAnalysis.iterate_to_fixpoint(tcx, body, None)
```
which is shorter at every call site, and there's less plumbing required
to support it.
Deeply normalize `TypeTrace` when reporting type error in new solver
Normalize the values that come from the `TypeTrace` for various type mismatches.
Side-note: We can't normalize the `TypeError` itself bc it may come from instantiated binders, so it may reference values from within the probe...
r? lcnr
Continue to get rid of `ty::Const::{try_}eval*`
This PR mostly does:
* Removes all of the `try_eval_*` and `eval_*` helpers from `ty::Const`, and replace their usages with `try_to_*`.
* Remove `ty::Const::eval`.
* Rename `ty::Const::normalize` to `ty::Const::normalize_internal`. This function is still used in the normalization code itself.
* Fix some weirdness around the `TransmuteFrom` goal.
I'm happy to split it out further; for example, I could probably land the first part which removes the helpers, or the changes to codegen which are more obvious than the changes to tools.
r? BoxyUwU
Part of https://github.com/rust-lang/rust/issues/130704
Remove `GenKillAnalysis`
There are two kinds of dataflow analysis in the compiler: `Analysis`, which is the basic kind, and `GenKillAnalysis`, which is a more specialized kind for gen/kill analyses that is intended as an optimization. However, it turns out that `GenKillAnalysis` is actually a pessimization! It's faster (and much simpler) to do all the gen/kill analyses via `Analysis`. This lets us remove `GenKillAnalysis`, and `GenKillSet`, and a few other things, and also merge `AnalysisDomain` into `Analysis`. The PR removes 500 lines of code and improves performance.
r? `@tmiasko`
Use `ThinVec` for PredicateObligation storage
~~I noticed while profiling clippy on a project that a large amount of time is being spent allocating `Vec`s for `PredicateObligation`, and the `Vec`s are often quite small. This is an attempt to optimise this by using SmallVec to avoid heap allocations for these common small Vecs.~~
This PR turns all the `Vec<PredicateObligation>` into a single type alias while avoiding referring to `Vec` around it, then swaps the type over to `ThinVec<PredicateObligation>` and fixes the fallout. This also contains an implementation of `ThinVec::extract_if`, copied from `Vec::extract_if` and currently being upstreamed to https://github.com/Gankra/thin-vec/pull/66.
This leads to a small (0.2-0.7%) performance gain in the latest perf run.
`GenKillAnalysis` has very similar methods to `Analysis`, but the first
two have a notable difference: the second argument is `&mut impl
GenKill<Self::Idx>` instead of `&mut Self::Domain`. But thanks to the
previous commit, this difference is no longer necessary.
stabilize `ci_rustc_if_unchanged_logic` test
Makes `ci_rustc_if_unchanged_logic` test more stable and re-enables it. Previously, it was expecting CI-rustc to be used all the time when there were no changes, which wasn’t always the case. Purpose of this test is making sure we don't use CI-rustc while there are changes in compiler and/or library, but we don't really need to cover cases where CI-rustc is not enabled.
Second commit was pushed for making a change in the compiler tree, so `ci_rustc_if_unchanged_logic` can be tested properly in merge CI.
- fix for divergence
- fix error message
- fix another cranelift test
- fix some cranelift things
- don't set the NORETURN option for naked asm
- fix use of naked_asm! in doc comment
- fix use of naked_asm! in run-make test
- use `span_bug` in unreachable branch
Make opaque types regular HIR nodes
Having opaque types as HIR owner introduces all sorts of complications. This PR proposes to make them regular HIR nodes instead.
I haven't gone through all the test changes yet, so there may be a few surprises.
Many thanks to `@camelid` for the first draft.
Fixes https://github.com/rust-lang/rust/issues/129023Fixes#129099Fixes#125843Fixes#119716Fixes#121422
Stabilize the `map`/`value` methods on `ControlFlow`
And fix the stability attribute on the `pub use` in `core::ops`.
libs-api in https://github.com/rust-lang/rust/issues/75744#issuecomment-2231214910 seemed reasonably happy with naming for these, so let's try for an FCP.
Summary:
```rust
impl<B, C> ControlFlow<B, C> {
pub fn break_value(self) -> Option<B>;
pub fn map_break<T>(self, f: impl FnOnce(B) -> T) -> ControlFlow<T, C>;
pub fn continue_value(self) -> Option<C>;
pub fn map_continue<T>(self, f: impl FnOnce(C) -> T) -> ControlFlow<B, T>;
}
```
Resolves#75744
``@rustbot`` label +needs-fcp +t-libs-api -t-libs
---
Aside, in case it keeps someone else from going down the same dead end: I looked at the `{break,continue}_value` methods and tried to make them `const` as part of this, but that's disallowed because of not having `const Drop`, so put it back to not even unstably-const.
The `regioncx` and `borrow_set` fields can be references instead of
`Rc`. They use the existing `'a` lifetime. This avoids some heap
allocations and is a bit simpler.
Add `File` constructors that return files wrapped with a buffer
In addition to the light convenience, these are intended to raise visibility that buffering is something you should consider when opening a file, since unbuffered I/O is a common performance footgun to Rust newcomers.
ACP: https://github.com/rust-lang/libs-team/issues/446
Tracking Issue: #130804
This changes the remaining span for the cast, because the new `Cast`
category has a higher priority (lower `Ord`) than the old `Coercion`
category, so we no longer report the region error for the "unsizing"
coercion from `*const Trait` to itself.
This commit adds basic support for reborrowing `Pin` types in argument
position. At the moment it only supports reborrowing `Pin<&mut T>` as
`Pin<&mut T>` by inserting a call to `Pin::as_mut()`, and only in
argument position (not as the receiver in a method call).
- Replace non-standard names like 's, 'p, 'rg, 'ck, 'parent, 'this, and
'me with vanilla 'a. These are cases where the original name isn't
really any more informative than 'a.
- Replace names like 'cx, 'mir, and 'body with vanilla 'a when the lifetime
applies to multiple fields and so the original lifetime name isn't
really accurate.
- Put 'tcx last in lifetime lists, and 'a before 'b.
Rescope temp lifetime in if-let into IfElse with migration lint
Tracking issue #124085
This PR shortens the temporary lifetime to cover only the pattern matching and consequent branch of a `if let`.
At the expression location, means that the lifetime is shortened from previously the deepest enclosing block or statement in Edition 2021. This warrants an Edition change.
Coming with the Edition change, this patch also implements an edition lint to warn about the change and a safe rewrite suggestion to preserve the 2021 semantics in most cases.
Related to #103108.
Related crater runs: https://github.com/rust-lang/rust/pull/129466.
Simplify some nested `if` statements
Applies some but not all instances of `clippy::collapsible_if`. Some ended up looking worse afterwards, though, so I left those out. Also applies instances of `clippy::collapsible_else_if`
Review with whitespace disabled please.
There are four related dataflow structs: `MaybeInitializedPlaces`,
`MaybeUninitializedPlaces`, and `EverInitializedPlaces`,
`DefinitelyInitializedPlaces`. They all have a `&Body` and a
`&MoveData<'tcx>` field. The first three use different lifetimes for the
two fields, but the last one uses the same lifetime for both.
This commit changes the first three to use the same lifetime, removing
the need for one of the lifetimes. Other structs that also lose a
lifetime as a result of this are `LivenessContext`, `LivenessResults`,
`InitializationData`.
It then does similar things in various other structs.
Make `Ty::boxed_ty` return an `Option`
Looks like a good place to use Rust's type system.
---
Most of 4ac7bcbaad/compiler/rustc_middle/src/ty/sty.rs (L971-L1963) looks like it could be moved to `TyKind` (then I guess `Ty` should be made to deref to `TyKind`).
Check WF of source type's signature on fn pointer cast
This PR patches the implied bounds holes slightly for #129005, #25860.
Like most implied bounds related unsoundness fixes, this isn't complete w.r.t. higher-ranked function signatures, but I believe it implements a pretty good heuristic for now.
### What does this do?
This PR makes a partial patch for a soundness hole in a `FnDef` -> `FnPtr` "reifying" pointer cast where we were never checking that the signature we are casting *from* is actually well-formed. Because of this, and because `FnDef` doesn't require its signature to be well-formed (just its predicates must hold), we are essentially allowed to "cast away" implied bounds that are assumed within the body of the `FnDef`:
```
fn foo<'a, 'b, T>(_: &'a &'b (), v: &'b T) -> &'a T { v }
fn bad<'short, T>(x: &'short T) -> &'static T {
let f: fn(_, &'short T) -> &'static T = foo;
f(&&(), x)
}
```
In this example, subtyping ends up casting the `_` type (which should be `&'static &'short ()`) to some other type that no longer serves as a "witness" to the lifetime relationship `'short: 'static` which would otherwise be required for this call to be WF. This happens regardless of if `foo`'s lifetimes are early- or late-bound.
This PR implements two checks:
1. We check that the signature of the `FnDef` is well-formed *before* casting it. This ensures that there is at least one point in the MIR where we ensure that the `FnDef`'s implied bounds are actually satisfied by the caller.
2. Implements a special case where if we're casting from a higher-ranked `FnDef` to a non-higher-ranked, we instantiate the binder of the `FnDef` with *infer vars* and ensure that it is a supertype of the target of the cast.
The (2.) is necessary to validate that these pointer casts are valid for higher-ranked `FnDef`. Otherwise, the example above would still pass even if `help`'s `'a` lifetime were late-bound.
### Further work
The WF checks for function calls are scattered all over the MIR. We check the WF of args in call terminators, we check the WF of `FnDef` when we create a `const` operand referencing it, and we check the WF of the return type in #115538, to name a few.
One way to make this a bit cleaner is to simply extend #115538 to always check that the signature is WF for `FnDef` types. I may do this as a follow-up, but I wanted to keep this simple since this leads to some pretty bad NLL diagnostics regressions, and AFAICT this solution is *complete enough*.
### Crater triage
Done here: https://github.com/rust-lang/rust/pull/129021#issuecomment-2297702647
r? lcnr
Remove `#[macro_use] extern crate tracing`, round 4
Because explicit importing of macros via use items is nicer (more standard and readable) than implicit importing via #[macro_use]. Continuing the work from #124511, #124914, and #125434. After this PR no `rustc_*` crates use `#[macro_use] extern crate tracing` except for `rustc_codegen_gcc` which is a special case and I will do separately.
r? ```@jieyouxu```
Remove Duplicate E0381 Label
Aims to resolve https://github.com/rust-lang/rust/issues/129274, and adds a test for the case.
Essentially, we are duplicating this span for some reason. For now, I'm just using a set to collect the spans rather than the vec. I imagine there's probably no real reason to inspect duplicates in this area, but if I'm wrong I can adjust to collect "seen spans" in just the point where this label is applied.
I'm not sure why it's producing duplicate spans. Looks like this has been this way for a while? I think it gives the duplicate label on 1.75.0 for example.
Add `#[warn(unreachable_pub)]` to a bunch of compiler crates
By default `unreachable_pub` identifies things that need not be `pub` and tells you to make them `pub(crate)`. But sometimes those things don't need any kind of visibility. So they way I did these was to remove the visibility entirely for each thing the lint identifies, and then add `pub(crate)` back in everywhere the compiler said it was necessary. (Or occasionally `pub(super)` when context suggested that was appropriate.) Tedious, but results in more `pub` removal.
There are plenty more crates to do but this seems like enough for a first PR.
r? `@compiler-errors`
Record the correct target type when coercing fn items/closures to pointers
Self-explanatory. We were previously not recording the *target* type of a coercion as the output of an adjustment. This should remedy that.
We must also modify the function pointer casts in MIR typeck to use subtyping, since those broke since #118247.
r? lcnr
Shrink `TyKind::FnPtr`.
By splitting the `FnSig` within `TyKind::FnPtr` into `FnSigTys` and `FnHeader`, which can be packed more efficiently. This reduces the size of the hot `TyKind` type from 32 bytes to 24 bytes on 64-bit platforms. This reduces peak memory usage by a few percent on some benchmarks. It also reduces cache misses and page faults similarly, though this doesn't translate to clear cycles or wall-time improvements on CI.
r? `@compiler-errors`
Normalize struct tail properly for `dyn` ptr-to-ptr casting in new solver
Realized that the new solver didn't handle ptr-to-ptr casting correctly.
r? lcnr
Built on #128694
