Simplify the canonical clone method and the copy-like forms to copy
Fixes#128081.
The optimized clone method ends up as the following MIR:
```
_2 = copy ((*_1).0: i32);
_3 = copy ((*_1).1: u64);
_4 = copy ((*_1).2: [i8; 3]);
_0 = Foo { a: move _2, b: move _3, c: move _4 };
```
We can transform this to:
```
_0 = copy (*_1);
```
r? `@cjgillot`
Don't call closure_by_move_body_def_id on FnOnce async closures in MIR validation
Refactors the check in #129847 to not unncessarily call the `closure_by_move_body_def_id` query for async closures that don't *need* a by-move body.
Fixes#130167
- 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.
coverage: Simplify creation of sum counters
A small and self-contained improvement, extracted from some larger changes that I'm still working on.
Ultimately I want to avoid creating these sum counter-expressions in some cases (in favour of just adding physical counters directly to the nodes we care about), so a good incremental move towards that is splitting the “gather edge counters” step out from the ”build a sum of those counters” step.
Creating an extra intermediate vector should have negligible cost (and coverage isn't exercised by the benchmark suite anyway). The removed logging is redundant with the `#[instrument(..)]` logging we already have on the underlying method calls.
some const cleanup: remove unnecessary attributes, add const-hack indications
I learned that we use `FIXME(const-hack)` on top of the "const-hack" label. That seems much better since it marks the right place in the code and moves around with the code. So I went through the PRs with that label and added appropriate FIXMEs in the code. IMO this means we can then remove the label -- Cc ``@rust-lang/wg-const-eval.``
I also noticed some const stability attributes that don't do anything useful, and removed them.
r? ``@fee1-dead``
coverage: Clean up terminology in counter creation
Some of the terminology in this module is confusing, or has drifted out of sync with other parts of the coverage code.
This PR therefore renames some variables and methods, and adjusts comments and debug logging statements, to make things clearer and more consistent.
No functional changes, other than some small tweaks to debug logging.
In all cases the struct can own the relevant thing instead of having a
reference to it. This makes the code simpler, and in some cases removes
a struct lifetime.
These are all functions with a single callsite, where having a separate
function does nothing to help with readability. These changes make the
code a little shorter and easier to read.
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.
Currently it constructs two vectors `calls_to_terminated` and
`cleanups_to_remove` in the main loop, and then processes them after the
main loop. But the processing can be done in the main loop, avoiding the
need for the vectors.
Do not call query to compute coroutine layout for synthetic body of async closure
There is code in the MIR validator that attempts to prevent query cycles when inlining a coroutine into itself, and will use the coroutine layout directly from the body when it detects that's the same coroutine as the one that's being validated. After #128506, this logic didn't take into account the fact that the coroutine def id will differ if it's the "by-move body" of an async closure. This PR implements that.
Fixes#129811
coverage: Count await when the Future is immediately ready
Currently `await` is only counted towards coverage if the containing
function is suspended and resumed at least once. This is because it
expands to code which contains a branch on the discriminant of `Poll`.
By treating it like a branching macro (e.g. `assert!`), these
implementation details will be hidden from the coverage results.
I added a test to ensure the fix works in simple cases, but the heuristic of picking only the first await-related covspan might be unreliable. I plan on testing more thoroughly with a real codebase over the next couple of weeks.
closes#98712
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`).
Currently `await` is only counted towards coverage if the containing
function is suspended and resumed at least once. This is because it
expands to code which contains a branch on the discriminant of `Poll`.
By treating it like a branching macro (e.g. `assert!`), these
implementation details will be hidden from the coverage results.
Rename dump of coroutine by-move-body to be more consistent, fix ICE in dump_mir
First, we add a missing match for `DefKind::SyntheticCoroutineBody` in `dump_mir`. Fixes#129703. The second commit (directly below) serves as a test.
Second, we reorder the `dump_mir` in `coroutine_by_move_body_def_id` to be *after* we adjust the body source, and change the disambiguator so it reads more like any other MIR body. This also serves as a test for the ICE, since we're dumping the MIR of a body with `DefKind::SyntheticCoroutineBody`.
Third, we change the parenting of the synthetic MIR body to have the *coroutine-closure* (i.e. async closure) as its parent, so we don't have long strings of `{closure#0}-{closure#0}-{closure#0}`.
try-job: test-various
Move `SanityCheck` and `MirPass`
They are currently in `rustc_middle`. This PR moves them to `rustc_mir_transform`, which makes more sense.
r? ``@cjgillot``
Because that's now the only crate that uses it.
Moving stuff out of `rustc_middle` is always welcome.
I chose to use `impl crate::MirPass`/`impl crate::MirLint` (with
explicit `crate::`) everywhere because that's the only mention of
`MirPass`/`MirLint` used in all of these files. (Prior to this change,
`MirPass` was mostly imported via `use rustc_middle::mir::*` items.)
The actual implementation remains in `rustc_mir_dataflow`, but this
commit moves the `MirPass` impl to `rustc_mir_transform` and changes it
to a `MirLint` (fixing a `FIXME` comment).
(I originally tried moving the full implementation from
`rustc_mir_dataflow` but I had some trait problems with `HasMoveData`
and `RustcPeekAt` and `MaybeLiveLocals`. This commit was much smaller
and simpler, but still will allow some follow-up cleanups.)
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 `Option<!>` return types.
Several compiler functions have `Option<!>` for their return type. That's odd. The only valid return value is `None`, so why is this type used?
Because it lets you write certain patterns slightly more concisely. E.g. if you have these common patterns:
```
let Some(a) = f() else { return };
let Ok(b) = g() else { return };
```
you can shorten them to these:
```
let a = f()?;
let b = g().ok()?;
```
Huh.
An `Option` return type typically designates success/failure. How should I interpret the type signature of a function that always returns (i.e. doesn't panic), does useful work (modifying `&mut` arguments), and yet only ever fails? This idiom subverts the type system for a cute syntactic trick.
Furthermore, returning `Option<!>` from a function F makes things syntactically more convenient within F, but makes things worse at F's callsites. The callsites can themselves use `?` with F but should not, because they will get an unconditional early return, which is almost certainly not desirable. Instead the return value should be ignored. (Note that some of callsites of `process_operand`, `process_immedate`, `process_assign` actually do use `?`, though the early return doesn't matter in these cases because nothing of significance comes after those calls. Ugh.)
When I first saw this pattern I had no idea how to interpret it, and it took me several minutes of close reading to understand everything I've written above. I even started a Zulip thread about it to make sure I understood it properly. "Save a few characters by introducing types so weird that compiler devs have to discuss it on Zulip" feels like a bad trade-off to me. This commit replaces all the `Option<!>` return values and uses `else`/`return` (or something similar) to replace the relevant `?` uses. The result is slightly more verbose but much easier to understand.
r? ``````@cjgillot``````
Several compiler functions have `Option<!>` for their return type.
That's odd. The only valid return value is `None`, so why is this type
used?
Because it lets you write certain patterns slightly more concisely. E.g.
if you have these common patterns:
```
let Some(a) = f() else { return };
let Ok(b) = g() else { return };
```
you can shorten them to these:
```
let a = f()?;
let b = g().ok()?;
```
Huh.
An `Option` return type typically designates success/failure. How should
I interpret the type signature of a function that always returns (i.e.
doesn't panic), does useful work (modifying `&mut` arguments), and yet
only ever fails? This idiom subverts the type system for a cute
syntactic trick.
Furthermore, returning `Option<!>` from a function F makes things
syntactically more convenient within F, but makes things worse at F's
callsites. The callsites can themselves use `?` with F but should not,
because they will get an unconditional early return, which is almost
certainly not desirable. Instead the return value should be ignored.
(Note that some of callsites of `process_operand`, `process_immedate`,
`process_assign` actually do use `?`, though the early return doesn't
matter in these cases because nothing of significance comes after those
calls. Ugh.)
When I first saw this pattern I had no idea how to interpret it, and it
took me several minutes of close reading to understand everything I've
written above. I even started a Zulip thread about it to make sure I
understood it properly. "Save a few characters by introducing types so
weird that compiler devs have to discuss it on Zulip" feels like a bad
trade-off to me. This commit replaces all the `Option<!>` return values
and uses `else`/`return` (or something similar) to replace the relevant
`?` uses. The result is slightly more verbose but much easier to
understand.
By making it own the index maps, instead of holding references to them.
This requires moving the free function `find_candidate` into
`Candidate::reset_and_find`. It lets the `'alloc` lifetime be removed
everywhere that still has it.
LLVM uses the word "code" to refer to a particular kind of coverage mapping.
This unrelated usage of the word is confusing, and makes it harder to introduce
types whose names correspond to the LLVM classification of coverage kinds.
When deduplicating unreachable blocks, erase the source information.
After deduplication the block conceptually belongs to multiple locations in the source. Although these blocks are unreachable, in #123341 we did come across a real side effect, an unreachable block that survives into the compiled code can cause a debugger to set a breakpoint on the wrong instruction. Erasing the source information ensures that a debugger will never be misled into thinking that the unreachable block is worth setting a breakpoint on, especially after #128627.
Technically we don't need to erase the source information if all the deduplicated blocks have identical source information, but tracking that seems like more effort than it's worth.
I'll let njn redirect this one too. r? `@nnethercote`
Fix projections when parent capture is by-ref but child capture is by-value in the `ByMoveBody` pass
This fixes a somewhat strange bug where we build the incorrect MIR in #129074. This one is weird, but I don't expect it to actually matter in practice since it almost certainly results in a move error in borrowck. However, let's not ICE.
Given the code:
```
#![feature(async_closure)]
// NOT copy.
struct Ty;
fn hello(x: &Ty) {
let c = async || {
*x;
//~^ ERROR cannot move out of `*x` which is behind a shared reference
};
}
fn main() {}
```
The parent coroutine-closure captures `x: &Ty` by-ref, resulting in an upvar of `&&Ty`. The child coroutine captures `x` by-value, resulting in an upvar of `&Ty`. When constructing the by-move body for the coroutine-closure, we weren't applying an additional deref projection to convert the parent capture into the child capture, resulting in an type error in assignment, which is a validation ICE.
As I said above, this only occurs (AFAICT) in code that eventually results in an error, because it is only triggered by HIR that attempts to move a non-copy value out of a ref. This doesn't occur if `Ty` is `Copy`, since we'd instead capture `x` by-ref in the child coroutine.
Fixes#129074
Use `append` instead of `extend(drain(..))`
The first commit adds `IndexVec::append` that forwards to `Vec::append`, and uses it in a couple places.
The second commit updates `indexmap` for its new `IndexMap::append`, and also uses that in a couple places.
These changes are similar to what [`clippy::extend_with_drain`](https://rust-lang.github.io/rust-clippy/master/index.html#/extend_with_drain) would suggest, just for other collection types.
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`
Use more slice patterns inside the compiler
Nothing super noteworthy. Just replacing the common 'fragile' pattern of "length check followed by indexing or unwrap" with slice patterns for legibility and 'robustness'.
r? ghost
Fix `ElaborateBoxDerefs` on debug varinfo
Slightly simplifies the `ElaborateBoxDerefs` pass to fix cases where it was applying the wrong projections to debug var infos containing places that deref boxes.
From what I can tell[^1], we don't actually have any tests (or code anywhere, really) that exercise `debug x => *(...: Box<T>)`, and it's very difficult to trigger this in surface Rust, so I wrote a custom MIR test.
What happens is that the pass was turning `*(SOME_PLACE: Box<T>)` into `*(*((((SOME_PLACE).0: Unique<T>).0: NonNull<T>).0: *const T))` in debug var infos. In particular, notice the *double deref*, which was wrong.
This is the root cause of #128554, so this PR fixes#128554 as well. The reason that async closures was affected is because of the way that we compute the [`ByMove` body](https://github.com/rust-lang/rust/blob/master/compiler/rustc_mir_transform/src/coroutine/by_move_body.rs), which resulted in `*(...: Box<T>)` in debug var info. But this really has nothing to do with async closures.
[^1]: Validated by literally replacing the `if elem == PlaceElem::Deref && base_ty.is_box() { ... }` innards with a `panic!()`, which compiled all of stage2 without panicking.
