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

oli-obk · 2025-01-14T11:39:15Z

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 · 2025-01-14T11:41:51Z

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 · 2025-01-14T11:41:53Z

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

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.

compiler/rustc_hir_analysis/src/check/check.rs

nikomatsakis · 2025-01-15T16:14:21Z

@rfcbot reviewed

scottmcm · 2025-01-15T16:22:07Z

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 · 2025-01-15T17:14:41Z

@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 · 2025-01-15T20:05:57Z

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 · 2025-01-20T13:28:46Z

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

compiler-errors · 2025-02-12T20:02:03Z

r? lcnr since you were reviewing this

compiler/rustc_hir_analysis/src/check/check.rs

lcnr · 2025-02-14T14:21:12Z

tests/ui/traits/solver-cycles/129541-recursive-struct-and-array-impl.rs

+
+//@ error-pattern: reached the recursion limit finding the struct tail for `<[Hello] as Normalize>::Assoc`
+
+trait Bound {}


why add this test?

This was just necessary before I added the normalization.

lcnr

two nits, then r=me

oli-obk · 2025-02-25T07:35:53Z

@bors r=lcnr

bors · 2025-02-25T07:35:56Z

📌 Commit fec0385 has been approved by lcnr

It is now in the queue for this repository.

…mpls for unsized types

oli-obk · 2025-02-25T08:25:58Z

@bors r=lcnr

bors · 2025-02-25T08:26:01Z

📌 Commit ffc955b has been approved by lcnr

It is now in the queue for this repository.

…pl, r=lcnr Don't require method impls for methods with `Self:Sized` bounds for impls for unsized types Similarly to how rust-lang#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: ```rust 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`.

Rollup of 6 pull requests Successful merges: - rust-lang#135480 (Don't require method impls for methods with `Self:Sized` bounds for impls for unsized types) - rust-lang#137360 (Use `as_chunks` in `analyze_source_file_sse2`) - rust-lang#137460 (downgrade bootstrap `cc`) - rust-lang#137515 (Update `compiler-builtins` to 0.1.148) - rust-lang#137522 (use stage 2 on cargo and clippy tests when possible) - rust-lang#137597 (Revert accidental cargo submodule update) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#135480 - oli-obk:sized-method-on-unsized-impl, r=lcnr Don't require method impls for methods with `Self:Sized` bounds for impls for unsized types Similarly to how rust-lang#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: ```rust 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`.

fmease · 2025-02-25T23:28:06Z

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 · 2025-02-26T09:39:00Z

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

rustbot assigned chenyukang Jan 14, 2025

This comment was marked as resolved.

Sign in to view

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 14, 2025

oli-obk assigned compiler-errors and unassigned chenyukang Jan 14, 2025

oli-obk added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 14, 2025

rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 14, 2025

This comment has been minimized.

Sign in to view

oli-obk force-pushed the sized-method-on-unsized-impl branch from fa00d7b to f049c41 Compare January 14, 2025 11:53

oli-obk added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 14, 2025

lcnr reviewed Jan 14, 2025

View reviewed changes

compiler/rustc_hir_analysis/src/check/check.rs Outdated Show resolved Hide resolved

oli-obk force-pushed the sized-method-on-unsized-impl branch 2 times, most recently from f403d53 to d1c9c54 Compare January 14, 2025 16:05

nikomatsakis approved these changes Jan 15, 2025

View reviewed changes

rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 15, 2025

oli-obk added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2025

rustbot assigned lcnr and unassigned compiler-errors Feb 12, 2025

apiraino removed the to-announce Announce this issue on triage meeting label Feb 13, 2025

lcnr reviewed Feb 14, 2025

View reviewed changes

compiler/rustc_hir_analysis/src/check/check.rs Outdated Show resolved Hide resolved

lcnr reviewed Feb 14, 2025

View reviewed changes

oli-obk force-pushed the sized-method-on-unsized-impl branch 2 times, most recently from d6549e6 to fec0385 Compare February 25, 2025 07:35

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2025

This comment has been minimized.

Sign in to view

Don't require method impls for methods with Self:Sized bounds for i…

ffc955b

…mpls for unsized types

oli-obk force-pushed the sized-method-on-unsized-impl branch from fec0385 to ffc955b Compare February 25, 2025 08:25

This was referenced Feb 25, 2025

Rollup of 7 pull requests #137607

Closed

Rollup of 6 pull requests #137611

Merged

bors merged commit fb31487 into rust-lang:master Feb 25, 2025
6 checks passed

rustbot added this to the 1.87.0 milestone Feb 25, 2025

oli-obk mentioned this pull request Feb 25, 2025

unsized params in traits rust-lang/rfcs#3745

Open

oli-obk deleted the sized-method-on-unsized-impl branch February 25, 2025 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

oli-obk commented Jan 14, 2025

This comment was marked as resolved.

oli-obk commented Jan 14, 2025

rfcbot commented Jan 14, 2025 •

edited by spastorino

Loading

This comment has been minimized.

nikomatsakis commented Jan 15, 2025

scottmcm commented Jan 15, 2025

traviscross commented Jan 15, 2025 •

edited

Loading

compiler-errors commented Jan 15, 2025

lcnr commented Jan 20, 2025

compiler-errors commented Feb 12, 2025

lcnr Feb 14, 2025

oli-obk Feb 24, 2025

lcnr left a comment

oli-obk commented Feb 25, 2025

bors commented Feb 25, 2025

This comment has been minimized.

oli-obk commented Feb 25, 2025

bors commented Feb 25, 2025

fmease commented Feb 25, 2025 •

edited

Loading

Overall result: ❌ regressions - please read the text below

Instruction count

oli-obk commented Feb 26, 2025


		//@ error-pattern: reached the recursion limit finding the struct tail for `<[Hello] as Normalize>::Assoc`

		trait Bound {}

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

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

Conversation

oli-obk commented Jan 14, 2025

This comment was marked as resolved.

oli-obk commented Jan 14, 2025

rfcbot commented Jan 14, 2025 • edited by spastorino Loading

This comment has been minimized.

nikomatsakis commented Jan 15, 2025

scottmcm commented Jan 15, 2025

traviscross commented Jan 15, 2025 • edited Loading

compiler-errors commented Jan 15, 2025

lcnr commented Jan 20, 2025

compiler-errors commented Feb 12, 2025

lcnr Feb 14, 2025

Choose a reason for hiding this comment

oli-obk Feb 24, 2025

Choose a reason for hiding this comment

lcnr left a comment

Choose a reason for hiding this comment

oli-obk commented Feb 25, 2025

bors commented Feb 25, 2025

This comment has been minimized.

oli-obk commented Feb 25, 2025

bors commented Feb 25, 2025

fmease commented Feb 25, 2025 • edited Loading

Overall result: ❌ regressions - please read the text below

Instruction count

oli-obk commented Feb 26, 2025

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

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

rfcbot commented Jan 14, 2025 •

edited by spastorino

Loading

traviscross commented Jan 15, 2025 •

edited

Loading

fmease commented Feb 25, 2025 •

edited

Loading