Enforce Copy bounds for repeat elements while considering lifetimes

[oli-obk]

fixes #95477

this is a breaking change in order to fix a soundness bug.

Before this PR we only checked whether the repeat element type had an impl Copy, but not whether that impl also had the appropriate lifetimes. E.g. if the impl was for YourType<'static> and not a general 'a, then copying any type other than a 'static one should have been rejected, but wasn't.

r? @lcnr

[compiler-errors]

Crater perhaps?

[oli-obk]

@bors try

Oh yea, I knew I had forgotten some step here, thanks!

[bors]

⌛ Trying commit 26a448bb57ca0923995c6b71ff0f1a28e03fb750 with merge 378b6a8a384b3bd0818af5f1745e5747a98bff7b...

[bors]

☀️ Try build successful - checks-actions
Build commit: 378b6a8a384b3bd0818af5f1745e5747a98bff7b (378b6a8a384b3bd0818af5f1745e5747a98bff7b)

[oli-obk]

@craterbot check

[craterbot]

👌 Experiment pr-95819 created and queued.
🤖 Automatically detected try build 378b6a8a384b3bd0818af5f1745e5747a98bff7b
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[scottmcm]

I think we're good to do this under the usual "it was a bug" exception.

Hopefully crater says we can just do this, without a future compat dance.

[craterbot]

🚧 Experiment pr-95819 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[lcnr]

I there something blocking you from using self.cx.prove_trait_ref here?

[oli-obk]

Yes, that function will ICE if the bound doesn't hold. Hmm.. I don't remember whether I tried it or not. I think I did, but i'll try again. Maybe I discarded it as a possibility just by looking at the code and misinterpreting the fact that it'll only ICE if there's no copy impl at all, which typeck should've already prevented

[oli-obk]

Ah yes, I remember these ICEs, I did try this before, but get lots of failures like

--- stderr -------------------------------
error: internal compiler error: no errors encountered even though `delay_span_bug` issued

error: internal compiler error: broken MIR in DefId(0:7 ~ repeat_to_run_dtor_twice[6a18]::main) (NoSolution): could not prove Binder(TraitPredicate(<Foo as std::marker::Copy>, polarity:Positive), [])
  --> /home/ubuntu/rust5/src/test/ui/repeat-to-run-dtor-twice.rs:17:13
   |
LL |     let _ = [ a; 5 ];
   |             ^^^^^^^^
   |
   = note: delayed at compiler/rustc_borrowck/src/type_check/canonical.rs:149:13

Which is happening because we are trying to borrowck a function that tries to copy a value that is never copy, irrespective of the lifetimes. So... I guess we can use prove_trait_ref if we patch typeck to at least do a minimal check.

This is going to get into MCP territory, but what if we just stopped trying to prove things in HIR typeck? Basically we do the bare minimum needed for building MIR. During MIR typeck/borrowck to do the real checks and can avoid any of the modulo-lifetimes checks.

I mean... we were already doing this right here, we just didn't check lifetimes

[lcnr]

So... I guess we can use prove_trait_ref if we patch typeck to at least do a minimal check.

would really prefer that 👍 I assume that checking whether something is copy can influence type inference? 🤔 I am kind of wary of completely moving any checks to mir typeck

[lcnr]

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=9d20dd6e1989c761a16255a4f73ca78b

#[derive(Clone, Default)]
struct MaybeCopy<T>(T);

impl Copy for MaybeCopy<u8> {}

fn is_copy<T: Copy>(x: T) {
    println!("{}", std::any::type_name::<T>());
} 

fn main() {
    is_copy(MaybeCopy::default())
    // [MaybeCopy::default(); 13]
    // ^ doesn't work cause `Copy` is only checked in the mir here
}

[lcnr]

that function is now unused, so you can remove it

[lcnr]

you may want to add Res::Err here as well, to avoid Copy bounds if the repeat length uses an invalid path

[estebank]

+1

[lcnr]

impl r=me

I personally think this is strictly a bugfix and doesn't need t-lang or t-compiler signoff? 🤔

[oli-obk]

Yea, once crater is clean i'll assign to a lang team member for language level review

[craterbot]

🎉 Experiment pr-95819 is completed!
📊 8 regressed and 5 fixed (225074 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[oli-obk]

All regressions are definitely spurious

r? @nikomatsakis as you commented on the issue

This only breaks user code that is already unsound, and crater found no cases of people relying on this. We were only checking if a type has a Copy impl, not if it actually applies for the given lifetimes. So if the Copy impl was only for 'static, but we had a non-static lifetime, the code was accepted.

[estebank]

The only nitpick I'd have with the output would be to point at the whole array, But I think that'd be useful only in uncommon code, so it's not worth it to change it in this PR, particularly with the verbosity tradeoff:

let headers = [
    Header { value: &[] };
    128
];

[oli-obk]

We had this before. I could revert the spans to point at the entire array again, but I thought it was actually neat that we were just pointing to the repeat element now.

[estebank]

Nono, what we have now it's good.

[estebank]

Wait, I'm confused as to why this works. Is Default: Copy?

[estebank]

Oh, wait. This is because MabeCopy::default(): ~const, right?

[oli-obk]

This worked because there was only one type T for which MaybeCopy<T> could be Copy. If we also had impl Copy for MaybeCopy<i8> {} then this would stop working: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=a4fa25d4154048a62eca08aedcceb090

[estebank]

Ah! Wow! Hadn't realized that that's how it works.

[estebank]

Should we move that entire block to its own maybe_require_copy_value (or something) function? That might make skimming easier.

[oli-obk]

Damn I love rust analyzer. Just selected the code and clicked "extract into fn". Add some lifetimes to types and remove the generated tcx argument done

[estebank]

r=me with the nitpicks addressed

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[oli-obk]

/me pets the rfcbot, you have :chefkissfingers: timing

[oli-obk]

@bors r=estebank

[bors]

📌 Commit 67ce547 has been approved by estebank

[bors]

⌛ Testing commit 67ce547 with merge a707f40...

[bors]

☀️ Test successful - checks-actions
Approved by: estebank
Pushing a707f40 to master...

[rust-timer]

Finished benchmarking commit (a707f40): comparison url.

Summary:

• Primary benchmarks: 😿 relevant regressions found
• Secondary benchmarks: no relevant changes found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	2	0	0	0	2
mean2	0.8%	N/A	N/A	N/A	0.8%
max	1.0%	N/A	N/A	N/A	1.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 ↩

[oli-obk]

This does add new dependency edges from typeck to other queries, so I'm not surprised there's more being marked dirty in incremental. I'll Loki at the benchmark itself to see what it is, but likely simply the fact that we now evaluate an array length in typeck.