Don't require method impls for methods with Self:Sized bounds for impls for unsized types

[oli-obk]

Similarly to how #112319 doesn't require specifying associated types with Self: Sized bounds on dyn Trait, we now don't require assoc items with Self: Sized bounds to be in impls of for unsized types.

Additionally we lint assoc items with Self: Sized bounds that are in such impls:

trait Foo {
    fn foo() where Self: Sized;
}

impl Foo for () {
    fn foo() {}
}

impl Foo for i32 {}
//~^ ERROR: not all trait items implemented, missing: `foo`

impl Foo for dyn std::fmt::Debug {}

#[deny(dead_code)]
impl Foo for dyn std::fmt::Display {
    fn foo() {}
    //~^ ERROR this item cannot be used as its where bounds are not satisfied
}

Note that this works with the same Self: Sized specific logic we already have for dyn Trait, so no new capabilities like avoiding assoc items with Self: Copy bounds on impls for String or such are added here. Specifying where ConcreteType: Sized in a trait and implementing the trait for ConcreteType also does not work, it must be exactly Self: Sized.

[oli-obk]

As per #112319 (comment) I am not adding @rust-lang/lang to the FCP, but just pinging them and nominating so they'll see it and can raise concerns.

@rfcbot merge

[rfcbot]

Team member @oli-obk has proposed to merge this. The next step is review by the rest of the tagged team members:

• @BoxyUwU
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot reviewed

[scottmcm]

This seems at least plausible, but I wonder if there's anything that would make it more complex if we did Hierarchy of Sized traits by @davidtwco (or similar)? Is there anything about what's happening here with Sized that depends on it not having a super-trait? Is there bound weakening that we want to be semver compatible that could start breaking with this, if Sized is special?

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this in the lang call today, and we're OK with this going forward according to our earlier guidance for this in #112319 (comment).

This did actually generate substantial discussion, though. We thought through the compatibility with this and the ongoing work on the hierarchy of Sized traits, RFC 3729 (cc @davidtwco), we discussed the more general case, e.g. with T: Copy (and how ideally, that should probably work too), and we talked about when adding or removing such bounds is a breaking change for the trait.

[compiler-errors]

This is special-cased specifically for just Sized and built-in unsized types because adding those bounds is already breaking -- you can rely on dyn Trait: Sized not holding for coherence reasons, so we could never have a future where these bounds were changed, I think. I don't think this matters for any Sized hierarchy change.

[lcnr]

I very much think we can and should do this for trait objects. The check currently looks at the struct tail, so it would even allow such impls for a foreign type struct Foo(dyn Trait);. I don't think it's currently a breaking change to go make a type sized, so that feels suboptimal. I feel like similar to inhabitness checks relying on visibility, we should either do so for this sized check as well or limit it to trait objects (and str? 🤷) for now

[lcnr]

two nits, then r=me

[oli-obk]

@bors r=lcnr

[bors]

📌 Commit fec0385 has been approved by lcnr

It is now in the queue for this repository.

[oli-obk]

@bors r=lcnr

[bors]

📌 Commit ffc955b has been approved by lcnr

It is now in the queue for this repository.

[fmease]

This PR lead to perf regressions: #137611 (comment)

Overall result: ❌ regressions - please read the text below

Instruction count

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.1%, 0.3%]	15
Regressions ❌ (secondary)	0.2%	[0.1%, 0.7%]	39
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[0.1%, 0.3%]	15

[oli-obk]

It looks like they exactly revert the instruction count reduction from #133832 weird.