Test that we require implementing trait items whose bounds don't hold in the current impl #112929

oli-obk · 2023-06-22T15:58:47Z

I initially tried to make most of these pass, but that's a big can of worms, so I'm just adding them as tests, considering we have no tests for these things.

compiler-errors · 2023-06-22T16:10:40Z

compiler/rustc_hir_analysis/src/check/check.rs

@@ -805,7 +805,19 @@ fn check_impl_items_against_trait<'tcx>(
                .is_some_and(|node_item| node_item.item.defaultness(tcx).has_value());

            if !is_implemented && tcx.defaultness(impl_id).is_final() {
-                missing_items.push(tcx.associated_item(trait_item_id));
+                let infcx = tcx.infer_ctxt().build();


This probably should use intercrate mode - #20021 (comment)

That's necessary, but not sufficient. If there are no impls at all for a foreign trait, we'll still error. If we wanted to do this, it could get ugly, because we'd need to invoke some of coherence ourselves or sth (making sure either the type or the trait is local).

erroring even when there are no impls for a foreign trait seems like correct behaviour to me, downstream crates could add impls for the foreign trait for a type they declare

compiler/rustc_hir_analysis/src/check/check.rs

BoxyUwU · 2023-06-22T16:14:03Z

this is probably very very unsound right now, two things:

You need to do this in intercrate mode, the following should fail to compile:

// crate a
trait Other {}
trait Trait { fn foo() where Self: Other; }

// crate b
// `u8: Other` happens to not hold righ tnow
impl Trait for u8 {}

or else adding a u8: Other impl will be a semver breaking change. also without intercrate set we may just return NoSolution for things that are actually provable which is very bad for this.

Second you also cannot be using params here subst_identity on the impl trait ref is wrong:

trait Trait<T> { fn foo() where T: Clone }

impl<T> Trait<T> {}

this shouldn't compile but will if you are attempting to prove T: Clone in the T: Sized env since that returns NoSolution.

BoxyUwU · 2023-06-22T16:19:33Z

I don't think we should do this without the new solver to be honest, the ideal impl here is probably to use a list of infer vars as the substs for the impl and paramenv, supporting infer vars in the paramenv in the old solver seems like a large change so not something we should do. An easier thing might be to only support cases where your predicates dont mention any generics which conveniently sidesteps that impl requirement.

The other thing is that the current solver is not super sound with intercrate enabled which might result in unsoundness in this check and would open us up to more opportunities for back compat issues with new solver. (I don't know how much of an issue this actually is if you're only trying to check predicates that dont have infer vars/params, probably much less of an opportunity for issues but still have some for the infer_projection hack.)

oli-obk · 2023-06-22T20:19:03Z

Yea, I don't think this is possible at present in general (see #112929 (comment)).

What we could do is limit it to predicates that are for Self and for a local trait and ignore all other predicates. But even then, I guess there could be situations where those Self: LocalTrait impls rely on other crates' traits and types...

oli-obk · 2023-06-23T16:07:22Z

r? @compiler-errors this just now adds tests and doesn't change compiler behaviour

tests/ui/traits/trivial_impl4.rs

tests/ui/traits/trivial_impl3.rs

tests/ui/traits/trivial_impl4.rs

oli-obk · 2023-06-26T09:39:22Z

@rustbot ready

… of effect on impls

compiler-errors · 2023-06-29T00:58:39Z

@bors r+ rollup (only tests)

bors · 2023-06-29T00:58:41Z

📌 Commit 042f605 has been approved by compiler-errors

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#112670 (privacy: Type privacy lints fixes and cleanups) - rust-lang#112929 (Test that we require implementing trait items whose bounds don't hold in the current impl) - rust-lang#113054 (Make `rustc_on_unimplemented` std-agnostic) - rust-lang#113137 (don't suggest `move` for borrows that aren't closures) - rust-lang#113139 (style-guide: Clarify let-else further) - rust-lang#113140 (style-guide: Add an example of formatting a multi-line attribute) - rust-lang#113143 (style-guide: Narrow guidance about references and dereferencing) r? `@ghost` `@rustbot` modify labels: rollup

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 Jun 22, 2023

compiler-errors reviewed Jun 22, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

BoxyUwU added the T-types Relevant to the types team, which will review and decide on the PR/issue. label Jun 22, 2023

oli-obk force-pushed the what_if_an_impl_item_just_doesnt_wanna_be_impld branch from edb4d45 to 055ee29 Compare June 22, 2023 21:55

This comment has been minimized.

Sign in to view

oli-obk force-pushed the what_if_an_impl_item_just_doesnt_wanna_be_impld branch from 055ee29 to 0ddf2f7 Compare June 23, 2023 14:30

This comment has been minimized.

Sign in to view

oli-obk force-pushed the what_if_an_impl_item_just_doesnt_wanna_be_impld branch 2 times, most recently from 915cdd3 to 1715ad6 Compare June 23, 2023 16:05

oli-obk changed the title ~~Don't require implementing trait items whose bounds don't hold in the current impl~~ Test that we require implementing trait items whose bounds don't hold in the current impl Jun 23, 2023

rustbot assigned compiler-errors Jun 23, 2023

compiler-errors approved these changes Jun 23, 2023

View reviewed changes

BoxyUwU reviewed Jun 23, 2023

View reviewed changes

tests/ui/traits/trivial_impl4.rs Show resolved Hide resolved

compiler-errors reviewed Jun 24, 2023

View reviewed changes

tests/ui/traits/trivial_impl3.rs Outdated Show resolved Hide resolved

tests/ui/traits/trivial_impl4.rs Outdated Show resolved Hide resolved

tests/ui/traits/trivial_impl4.rs Outdated Show resolved Hide resolved

compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2023

oli-obk force-pushed the what_if_an_impl_item_just_doesnt_wanna_be_impld branch from 1715ad6 to f69e04f Compare June 26, 2023 09:39

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 26, 2023

This comment has been minimized.

Sign in to view

Add some tests around where bounds on associated items and their lack…

042f605

… of effect on impls

oli-obk force-pushed the what_if_an_impl_item_just_doesnt_wanna_be_impld branch from f69e04f to 042f605 Compare June 26, 2023 09:56

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 Jun 29, 2023

matthiaskrgr mentioned this pull request Jun 29, 2023

Rollup of 7 pull requests #113146

Merged

bors merged commit 1963688 into rust-lang:master Jun 29, 2023
11 checks passed

rustbot added this to the 1.72.0 milestone Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test that we require implementing trait items whose bounds don't hold in the current impl #112929

Test that we require implementing trait items whose bounds don't hold in the current impl #112929

oli-obk commented Jun 22, 2023 •

edited

compiler-errors Jun 22, 2023

oli-obk Jun 22, 2023

BoxyUwU Jun 23, 2023 •

edited

This comment has been minimized.

BoxyUwU commented Jun 22, 2023

BoxyUwU commented Jun 22, 2023 •

edited

oli-obk commented Jun 22, 2023 •

edited

This comment has been minimized.

This comment has been minimized.

oli-obk commented Jun 23, 2023

oli-obk commented Jun 26, 2023

This comment has been minimized.

compiler-errors commented Jun 29, 2023

bors commented Jun 29, 2023

Test that we require implementing trait items whose bounds don't hold in the current impl #112929

Test that we require implementing trait items whose bounds don't hold in the current impl #112929

Conversation

oli-obk commented Jun 22, 2023 • edited

compiler-errors Jun 22, 2023

Choose a reason for hiding this comment

oli-obk Jun 22, 2023

Choose a reason for hiding this comment

BoxyUwU Jun 23, 2023 • edited

Choose a reason for hiding this comment

This comment has been minimized.

BoxyUwU commented Jun 22, 2023

BoxyUwU commented Jun 22, 2023 • edited

oli-obk commented Jun 22, 2023 • edited

This comment has been minimized.

This comment has been minimized.

oli-obk commented Jun 23, 2023

oli-obk commented Jun 26, 2023

This comment has been minimized.

compiler-errors commented Jun 29, 2023

bors commented Jun 29, 2023

oli-obk commented Jun 22, 2023 •

edited

BoxyUwU Jun 23, 2023 •

edited

BoxyUwU commented Jun 22, 2023 •

edited

oli-obk commented Jun 22, 2023 •

edited