[Jules-Bertholet]

View all comments

Except inside closures, where we need to keep capture inference stable. Gets rid of MIR bloat, and improves diagnostics.

See also #163717 (would allow doing this even inside closures), #163494 (would allow doing this for mutable references outside closures).

r? compiler

@rustbot label A-coercions

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

[Jules-Bertholet]

To make tests/codegen-llvm/lib-optimizations/append-elements.rs happy. That test is really fragile…

View changes since the review

[jieyouxu]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: d77c4b4 (d77c4b4f551a87fc763f35c94b4177493a25ab83)
Base parent: 0f6e5bf (0f6e5bf4c63a0ab336cc62de446b77123effaffc)

[rust-timer]

Finished benchmarking commit (d77c4b4): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

@bors rollup=never rustc-perf
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.2%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-3.9%, -0.1%]	175
Improvements ✅ (secondary)	-1.2%	[-7.0%, -0.1%]	151
All ❌✅ (primary)	-0.7%	[-3.9%, 0.2%]	176

Max RSS (memory usage)

Results (primary -2.1%, secondary -2.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.1%	[2.1%, 4.2%]	2
Improvements ✅ (primary)	-2.1%	[-6.2%, -0.8%]	16
Improvements ✅ (secondary)	-3.7%	[-6.7%, -0.7%]	16
All ❌✅ (primary)	-2.1%	[-6.2%, -0.8%]	16

Cycles

Results (primary -2.7%, secondary -4.6%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	7.1%	[7.1%, 7.1%]	1
Improvements ✅ (primary)	-2.7%	[-3.1%, -2.2%]	5
Improvements ✅ (secondary)	-5.4%	[-8.9%, -1.8%]	15
All ❌✅ (primary)	-2.7%	[-3.1%, -2.2%]	5

Binary size

Results (primary 0.1%, secondary -0.2%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 1.1%]	27
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.3%	[-1.0%, -0.0%]	22
Improvements ✅ (secondary)	-0.2%	[-0.5%, -0.1%]	12
All ❌✅ (primary)	0.1%	[-1.0%, 1.1%]	49

Bootstrap: 486.533s -> 486.402s (-0.03%)
Artifact size: 406.44 MiB -> 406.63 MiB (0.05%)

[theemathas]

Does this have implications on what Miri can catch?

[Jules-Bertholet]

Does this have implications on what Miri can catch?

It shouldn't. Miri does retags on every typed copy.

[adwinwhite]

I think the intention of this PR is to skip more no-op reborrows?
Some explanation on what the "more" part is would be awesome.
As someone who's not super familiar with coercions, this isn't too obvious :>

View changes since this review

[adwinwhite]

This change looks very good to me. But I'm not totally confident about its implication for borrowck. Could we accidentally allow some unsound code to compile now that we have less lifetime shortening? Hope someone more experienced with borrowck can confirm it's okay. cc @BoxyUwU @lcnr

View changes since this review

[adwinwhite]

This covers the skipping in coercion, right? It's more general, since it removes all redundant adjustments, whereas the coercion skipping only removes one autoborrow + deref pair?
It also skips adjustments from method confirmation 🤔

vibe: how about we only keep this skipping in THIR building and drop those in coercion?
This shouldn't affect the major perf win in borrowck, and it reduces duplicated logic for this optimization.

[Jules-Bertholet]

Not doing HIR skipping might lose some of the Clippy and diagnostics improvements. I will check

[Jules-Bertholet]

Can confirm, we lose the Clippy wins. Which is quite minor in this PR, but a bigger loss with #163717 (lose all the redundant_closure improvements)

[adwinwhite]

Could you link some examples? I didn't find tests mentioning redundant_closure or redundant_clone in #163717. Only found one redundant_closure_for_method_call.

Curious about why not doing HIR skipping affects them.

[Jules-Bertholet]

Look at the clippy UI test diffs of 3fe31e3#diff-1a22849d98bd10df7627f6e869d3aac2a384b02629660ab698b7bcb0fe2ca452

[Jules-Bertholet]

These lints look at a closure's HIR to determine "does it do anything other than a function call", and coercion counts as "anything"

[Jules-Bertholet]

I just realized—this does have an effect on borrowcheck. The following goes from compile fail → pass:

fn main() {
    let x: &'static ();
    let _: &_ = /* there used to be an implicit reborrow here */ x;
}

Personally, I think we should accept this, but worth discussing.

Note that the following compiles today:

fn main() {
    let x: &'static ();
    let _ = x;
}

Zulip thread

What semantics do we want here? @rustbot label I-opsem-nominated T-opsem T-types

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.