Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow impl on projection #107263

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion compiler/rustc_hir_analysis/src/coherence/inherent_impls.rs
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,13 @@ impl<'tcx> InherentCollect<'tcx> {
return;
};

let self_ty = self.tcx.type_of(item.owner_id);
let mut self_ty = self.tcx.type_of(item.owner_id);
if matches!(self_ty.kind(), ty::Alias(ty::AliasKind::Projection, _)) {
let param_env = self.tcx.param_env(item.owner_id);
if let Ok(new_ty) = self.tcx.try_normalize_erasing_regions(param_env, self_ty) {
self_ty = new_ty;
}
}
match *self_ty.kind() {
ty::Adt(def, _) => {
self.check_def_id(item, self_ty, def.did());
Expand Down
10 changes: 9 additions & 1 deletion compiler/rustc_privacy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,15 @@ trait VisibilityLike: Sized {
effective_visibilities: &EffectiveVisibilities,
) -> Self {
let mut find = FindMin { tcx, effective_visibilities, min: Self::MAX };
find.visit(tcx.type_of(def_id));
let mut ty = tcx.type_of(def_id);
let param_env = tcx.param_env(def_id);
// We need this normalization when we check the visibility of a projection so it is applied
// to the item behind the projection and not to the projection itself.
ty = match tcx.try_normalize_erasing_regions(param_env, ty) {
Ok(new_ty) => new_ty,
Err(_) => ty,
};
find.visit(ty);
if let Some(trait_ref) = tcx.impl_trait_ref(def_id) {
find.visit_trait(trait_ref.subst_identity());
}
Expand Down
7 changes: 4 additions & 3 deletions tests/ui/privacy/private-in-public-ill-formed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ mod aliases_pub {
}

impl <Priv as PrivTr>::AssocAlias {
//~^ ERROR no nominal type found for inherent implementation
pub fn f(arg: Priv) {} // private type `aliases_pub::Priv` in public interface
pub fn f(arg: Priv) {}
//~^ ERROR private type `aliases_pub::Priv` in public interface
//~| ERROR private type `aliases_pub::Priv` in public interface
//~| ERROR private trait `aliases_pub::PrivTr` in public interface
}
}

Expand All @@ -29,7 +31,6 @@ mod aliases_priv {
}

impl <Priv as PrivTr>::AssocAlias {
//~^ ERROR no nominal type found for inherent implementation
pub fn f(arg: Priv) {} // OK
}
}
Expand Down
38 changes: 25 additions & 13 deletions tests/ui/privacy/private-in-public-ill-formed.stderr
Original file line number Diff line number Diff line change
@@ -1,19 +1,31 @@
error[E0118]: no nominal type found for inherent implementation
--> $DIR/private-in-public-ill-formed.rs:14:10
error[E0446]: private type `aliases_pub::Priv` in public interface
--> $DIR/private-in-public-ill-formed.rs:15:9
|
LL | impl <Priv as PrivTr>::AssocAlias {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl requires a nominal type
|
= note: either implement a trait on it or create a newtype to wrap it instead
LL | struct Priv;
| ----------- `aliases_pub::Priv` declared as private
...
LL | pub fn f(arg: Priv) {}
| ^^^^^^^^^^^^^^^^^^^ can't leak private type

error[E0118]: no nominal type found for inherent implementation
--> $DIR/private-in-public-ill-formed.rs:31:10
error[E0445]: private trait `aliases_pub::PrivTr` in public interface
--> $DIR/private-in-public-ill-formed.rs:15:9
|
LL | impl <Priv as PrivTr>::AssocAlias {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ impl requires a nominal type
LL | trait PrivTr {
| ------------ `aliases_pub::PrivTr` declared as private
...
LL | pub fn f(arg: Priv) {}
| ^^^^^^^^^^^^^^^^^^^ can't leak private trait

error[E0446]: private type `aliases_pub::Priv` in public interface
--> $DIR/private-in-public-ill-formed.rs:15:9
|
= note: either implement a trait on it or create a newtype to wrap it instead
LL | struct Priv;
| ----------- `aliases_pub::Priv` declared as private
...
LL | pub fn f(arg: Priv) {}
| ^^^^^^^^^^^^^^^^^^^ can't leak private type

error: aborting due to 2 previous errors
error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0118`.
Some errors have detailed explanations: E0445, E0446.
For more information about an error, try `rustc --explain E0445`.
13 changes: 13 additions & 0 deletions tests/ui/projection/impl-foreign.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#![crate_type = "lib"]

pub trait Identity {
type Identity: ?Sized;
}

impl<T: ?Sized> Identity for T {
type Identity = Self;
}

impl <String as Identity>::Identity { //~ ERROR
pub fn foo(&self) {}
}
13 changes: 13 additions & 0 deletions tests/ui/projection/impl-foreign.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
error[E0116]: cannot define inherent `impl` for a type outside of the crate where the type is defined
--> $DIR/impl-foreign.rs:11:1
|
LL | / impl <String as Identity>::Identity {
LL | | pub fn foo(&self) {}
LL | | }
| |_^ impl for type defined outside of crate.
|
= note: define and implement a trait or new type instead

error: aborting due to previous error

For more information about this error, try `rustc --explain E0116`.
15 changes: 15 additions & 0 deletions tests/ui/projection/impl-on-assoc-const.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#![crate_type = "lib"]

pub trait Identity {
const Identity: u32;
}

impl<T: ?Sized> Identity for T {
const Identity: u32 = 0;
}

pub struct S;

impl <S as Identity>::Identity { //~ ERROR
pub fn foo(&self) {}
}
9 changes: 9 additions & 0 deletions tests/ui/projection/impl-on-assoc-const.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
error[E0575]: expected associated type, found associated constant `Identity::Identity`
--> $DIR/impl-on-assoc-const.rs:13:6
|
LL | impl <S as Identity>::Identity {
| ^^^^^^^^^^^^^^^^^^^^^^^^^ not a associated type

error: aborting due to previous error

For more information about this error, try `rustc --explain E0575`.
16 changes: 16 additions & 0 deletions tests/ui/projection/impl-on-weird-projection.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
//~ ERROR
#![crate_type = "lib"]

pub trait Identity {
type Identity: ?Sized;
}

impl<T: ?Sized> Identity for T {
type Identity = Self;
}

pub struct I8<const F: i8>;

impl <I8<{i8::MIN}> as Identity>::Identity {
pub fn foo(&self) {}
}
76 changes: 76 additions & 0 deletions tests/ui/projection/impl-on-weird-projection.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
error[E0391]: cycle detected when finding all inherent impls defined in crate
|
= note: ...which requires normalizing `<I8<{i8::MIN}> as Identity>::Identity`...
note: ...which requires evaluating type-level constant...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires const-evaluating + checking `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires const-evaluating + checking `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires caching MIR for CTFE of the const argument `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires elaborating drops for `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires borrow-checking the const argument`<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires processing MIR for the const argument `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires const checking the const argument `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires preparing the const argument `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}` for borrow checking...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires unsafety-checking the const argument `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires building MIR for `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires building THIR for `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
note: ...which requires type-checking the const argument `<impl at $DIR/impl-on-weird-projection.rs:14:1: 14:43>::{constant#0}`...
--> $DIR/impl-on-weird-projection.rs:14:10
|
LL | impl <I8<{i8::MIN}> as Identity>::Identity {
| ^^^^^^^^^
= note: ...which requires collecting all inherent impls for `IntSimplifiedType(I8)`...
= note: ...which requires collecting all impls for a type in a crate...
= note: ...which again requires finding all inherent impls defined in crate, completing the cycle
= note: cycle used when running analysis passes on this crate

error: aborting due to previous error

For more information about this error, try `rustc --explain E0391`.
15 changes: 15 additions & 0 deletions tests/ui/projection/impl-projection-on-primitive.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#![crate_type = "lib"]

pub trait Identity {
type Identity: ?Sized;
}

impl<T: ?Sized> Identity for T {
type Identity = ();
}

pub struct S;

impl <S as Identity>::Identity { //~ ERROR
pub fn foo(&self) {}
}
11 changes: 11 additions & 0 deletions tests/ui/projection/impl-projection-on-primitive.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error[E0390]: cannot define inherent `impl` for primitive types
--> $DIR/impl-projection-on-primitive.rs:13:6
|
LL | impl <S as Identity>::Identity {
compiler-errors marked this conversation as resolved.
Show resolved Hide resolved
| ^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider using an extension trait instead

error: aborting due to previous error

For more information about this error, try `rustc --explain E0390`.
23 changes: 23 additions & 0 deletions tests/ui/projection/impl-projection.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// check-pass

#![crate_type = "lib"]

pub trait Identity {
type Identity: ?Sized;
}

impl<T: ?Sized> Identity for T {
type Identity = Self;
}

pub struct S;

impl <S as Identity>::Identity {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this is what I wanted to mention. I'm not sure if we want to be able to normalize impl headers when considering inherent impls.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Jan 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also something I wondered but after talking with @oli-obk, they seemed to think it was ok so I made this PR. I think it'll require an FCP in any case so you can see with your team whether or not it's something you want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My words were "why should we not allow it", I don't know the answer to that. Mostly I don't know the use case for it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion. Well in any case, I'll let your team discuss about it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember having written code exactly like that before and then being surprised that it didn't compile. I can't remember where that was and I must've found a different approach, but it seems like something that is consistent with how other parts of the language works. And it's kind of surprising/unexpected that it doesn't compile.

pub fn foo(&self) {}
}

impl S {
pub fn bar(&self) {
self.foo();
}
}
24 changes: 24 additions & 0 deletions tests/ui/projection/impl-projection2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// check-pass

#![crate_type = "lib"]

pub trait Identity {
type Identity: ?Sized;
}

impl<T: ?Sized> Identity for T {
type Identity = S;
}

pub struct S;
pub struct Bar;

impl <Bar as Identity>::Identity {
pub fn foo(&self) {}
}

impl S {
pub fn bar(&self) {
self.foo();
}
}