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

Handle projections as uncovered types during coherence check #100555

Closed
wants to merge 11 commits into from
Closed
51 changes: 33 additions & 18 deletions compiler/rustc_trait_selection/src/traits/coherence.rs
Expand Up @@ -24,7 +24,7 @@ use rustc_middle::traits::specialization_graph::OverlapMode;
use rustc_middle::ty::fast_reject::{DeepRejectCtxt, TreatParams};
use rustc_middle::ty::subst::Subst;
use rustc_middle::ty::visit::TypeVisitable;
use rustc_middle::ty::{self, ImplSubject, Ty, TyCtxt, TypeVisitor};
use rustc_middle::ty::{self, ImplSubject, Ty, TyCtxt, TypeFlags, TypeVisitor};
use rustc_span::symbol::sym;
use rustc_span::DUMMY_SP;
use std::fmt::Debug;
Expand Down Expand Up @@ -461,7 +461,7 @@ pub fn trait_ref_is_local_or_fundamental<'tcx>(

pub enum OrphanCheckErr<'tcx> {
NonLocalInputType(Vec<(Ty<'tcx>, bool /* Is this the first input type? */)>),
UncoveredTy(Ty<'tcx>, Option<Ty<'tcx>>),
UncoveredTy(Ty<'tcx>, usize, Option<Ty<'tcx>>),
}

/// Checks the coherence orphan rules. `impl_def_id` should be the
Expand Down Expand Up @@ -588,21 +588,28 @@ fn orphan_check_trait_ref<'tcx>(
}

let mut checker = OrphanChecker::new(tcx, in_crate);
match trait_ref.visit_with(&mut checker) {
ControlFlow::Continue(()) => Err(OrphanCheckErr::NonLocalInputType(checker.non_local_tys)),
ControlFlow::Break(OrphanCheckEarlyExit::ParamTy(ty)) => {
// Does there exist some local type after the `ParamTy`.
checker.search_first_local_ty = true;
if let Some(OrphanCheckEarlyExit::LocalTy(local_ty)) =
trait_ref.visit_with(&mut checker).break_value()
{
Err(OrphanCheckErr::UncoveredTy(ty, Some(local_ty)))
} else {
Err(OrphanCheckErr::UncoveredTy(ty, None))
for (idx, arg) in trait_ref.substs.types().enumerate() {
match checker.visit_ty(arg) {
ControlFlow::Continue(()) => continue,
ControlFlow::Break(OrphanCheckEarlyExit::ParamTy(ty)) => {
// Does there exist some local type after the `ParamTy`.
checker.search_first_local_ty = true;

if let Some(OrphanCheckEarlyExit::LocalTy(local_ty)) =
trait_ref.visit_with(&mut checker).break_value()
{
return Err(OrphanCheckErr::UncoveredTy(ty, idx, Some(local_ty)));
} else {
return Err(OrphanCheckErr::UncoveredTy(ty, idx, None));
}
}
ControlFlow::Break(OrphanCheckEarlyExit::LocalTy(_)) => return Ok(()),
}
ControlFlow::Break(OrphanCheckEarlyExit::LocalTy(_)) => Ok(()),
}

// If no early break occured, means we didn't find a local type
// and should return Err
return Err(OrphanCheckErr::NonLocalInputType(checker.non_local_tys));
}

struct OrphanChecker<'tcx> {
Expand Down Expand Up @@ -631,7 +638,8 @@ impl<'tcx> OrphanChecker<'tcx> {
ControlFlow::CONTINUE
}

fn found_param_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<OrphanCheckEarlyExit<'tcx>> {
#[instrument(skip(self), level = "debug")]
fn found_uncovered_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<OrphanCheckEarlyExit<'tcx>> {
if self.search_first_local_ty {
ControlFlow::CONTINUE
} else {
Expand Down Expand Up @@ -672,10 +680,17 @@ impl<'tcx> TypeVisitor<'tcx> for OrphanChecker<'tcx> {
| ty::Slice(..)
| ty::RawPtr(..)
| ty::Never
| ty::Tuple(..)
| ty::Projection(..) => self.found_non_local_ty(ty),
| ty::Tuple(..) => self.found_non_local_ty(ty),

ty::Param(..) => self.found_uncovered_ty(ty),

ty::Param(..) => self.found_param_ty(ty),
ty::Projection(proj) => {
if proj.has_type_flags(TypeFlags::HAS_TY_PARAM) {
self.found_uncovered_ty(ty)
} else {
ControlFlow::CONTINUE
}
}

ty::Placeholder(..) | ty::Bound(..) | ty::Infer(..) => match self.in_crate {
InCrate::Local => self.found_non_local_ty(ty),
Expand Down
59 changes: 42 additions & 17 deletions compiler/rustc_typeck/src/coherence/orphan.rs
Expand Up @@ -111,7 +111,7 @@ fn do_orphan_check_impl<'tcx>(
tr.path.span,
trait_ref.self_ty(),
impl_.self_ty.span,
&impl_.generics,
&impl_.of_trait.as_ref().unwrap().path.segments.last().unwrap().args().args,
err,
)?,
}
Expand Down Expand Up @@ -212,7 +212,7 @@ fn emit_orphan_check_error<'tcx>(
trait_span: Span,
self_ty: Ty<'tcx>,
Copy link
Contributor

@lcnr lcnr Aug 26, 2022

Choose a reason for hiding this comment

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

should get the whole substs for the impl header, not just the self_ty. With this you can check for fundamental types for all arguments.

Alternatively, do the "is in fundamental" check, in orphan_check_trait_ref as part of the OrphanChecker. That's probably a bit cleaner than computing it here

self_ty_span: Span,
generics: &hir::Generics<'tcx>,
generics: &[rustc_hir::GenericArg<'tcx>],
Copy link
Contributor

Choose a reason for hiding this comment

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

generics isn't quite right anymore. MAybe something like hir_trait_args

err: traits::OrphanCheckErr<'tcx>,
) -> Result<!, ErrorGuaranteed> {
Err(match err {
Expand Down Expand Up @@ -275,59 +275,84 @@ fn emit_orphan_check_error<'tcx>(
err.note("define and implement a trait or new type instead");
err.emit()
}
traits::OrphanCheckErr::UncoveredTy(param_ty, local_type) => {
let mut sp = sp;
for param in generics.params {
if param.name.ident().to_string() == param_ty.to_string() {
sp = param.span;
traits::OrphanCheckErr::UncoveredTy(param_ty, idx, local_type) => {
let sp;
let mut param_ty_in_fundamental_ty = false;

match idx {
0 => {
sp = self_ty_span;
atsuzaki marked this conversation as resolved.
Show resolved Hide resolved
match *self_ty.kind() {
ty::Ref(..) => {
param_ty_in_fundamental_ty = true;
}
ty::Adt(def, _) => {
param_ty_in_fundamental_ty = def.is_fundamental();
}
_ => (),
}
Comment on lines +285 to +293
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you only doing that if the uncovered ty is in the self type?

you can also have impl<T> Trait<Box<T>> for () which would also benefit from this message

}
_ => sp = generics[idx - 1].span(),
}

let message = match *param_ty.kind() {
ty::Projection(..) => "associated type",
_ => "type parameter",
};

let fundamental_ty_msg =
if param_ty_in_fundamental_ty { "as argument to a fundamental type " } else { "" };

match local_type {
Some(local_type) => struct_span_err!(
tcx.sess,
sp,
E0210,
"type parameter `{}` must be covered by another type \
"{} `{}` {}must be covered by another type \
Copy link
Contributor

Choose a reason for hiding this comment

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

this is existing, but these messages would probably be a lot clearer if we used inline format args, i.e.

"{message} {param_ty} {fundamental_ty_msg}must be covered by another type \

(renaming message to param_ty_kind or something like that)

when it appears before the first local type (`{}`)",
message,
param_ty,
fundamental_ty_msg,
local_type
)
.span_label(
sp,
format!(
"type parameter `{}` must be covered by another type \
when it appears before the first local type (`{}`)",
param_ty, local_type
"{} `{}` {}must be covered by another type \
when it appears before the first local type (`{}`)",
message, param_ty, fundamental_ty_msg, local_type
),
)
.note(
"implementing a foreign trait is only possible if at \
least one of the types for which it is implemented is local, \
and no uncovered type parameters appear before that first \
local type",
and no uncovered type parameters or associated types appear \
before that first local type",
)
.note(
"in this case, 'before' refers to the following order: \
`impl<..> ForeignTrait<T1, ..., Tn> for T0`, \
where `T0` is the first and `Tn` is the last",
)
.emit(),

None => struct_span_err!(
atsuzaki marked this conversation as resolved.
Show resolved Hide resolved
tcx.sess,
sp,
E0210,
"type parameter `{}` must be used as the type parameter for some \
"{} `{}` {}must be used as the type parameter for some \
local type (e.g., `MyStruct<{}>`)",
message,
param_ty,
fundamental_ty_msg,
param_ty
)
.span_label(
sp,
format!(
"type parameter `{}` must be used as the type parameter for some \
local type",
param_ty,
"{} `{}` {}must be used as the type parameter for some \
local type",
message, param_ty, fundamental_ty_msg
),
)
.note(
Expand Down
3 changes: 3 additions & 0 deletions src/test/ui/coherence/auxiliary/coherence_projection_assoc.rs
@@ -0,0 +1,3 @@
pub trait Foreign<T, U> {
type Assoc;
}
4 changes: 2 additions & 2 deletions src/test/ui/coherence/coherence-all-remote.stderr
@@ -1,8 +1,8 @@
error[E0210]: type parameter `T` must be used as the type parameter for some local type (e.g., `MyStruct<T>`)
--> $DIR/coherence-all-remote.rs:6:6
--> $DIR/coherence-all-remote.rs:6:17
|
LL | impl<T> Remote1<T> for isize { }
| ^ type parameter `T` must be used as the type parameter for some local type
| ^ type parameter `T` must be used as the type parameter for some local type
|
= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local
= note: only traits defined in the current crate can be implemented for a type parameter
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/coherence/coherence-bigint-param.stderr
@@ -1,10 +1,10 @@
error[E0210]: type parameter `T` must be covered by another type when it appears before the first local type (`BigInt`)
--> $DIR/coherence-bigint-param.rs:8:6
--> $DIR/coherence-bigint-param.rs:8:29
|
LL | impl<T> Remote1<BigInt> for T { }
| ^ type parameter `T` must be covered by another type when it appears before the first local type (`BigInt`)
| ^ type parameter `T` must be covered by another type when it appears before the first local type (`BigInt`)
|
= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters appear before that first local type
= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters or associated types appear before that first local type
= note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last

error: aborting due to previous error
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/coherence/coherence-cross-crate-conflict.stderr
@@ -1,8 +1,8 @@
error[E0210]: type parameter `A` must be used as the type parameter for some local type (e.g., `MyStruct<A>`)
--> $DIR/coherence-cross-crate-conflict.rs:9:6
--> $DIR/coherence-cross-crate-conflict.rs:9:17
|
LL | impl<A> Foo for A {
| ^ type parameter `A` must be used as the type parameter for some local type
| ^ type parameter `A` must be used as the type parameter for some local type
|
= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local
= note: only traits defined in the current crate can be implemented for a type parameter
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/coherence/coherence-lone-type-parameter.stderr
@@ -1,8 +1,8 @@
error[E0210]: type parameter `T` must be used as the type parameter for some local type (e.g., `MyStruct<T>`)
--> $DIR/coherence-lone-type-parameter.rs:6:6
--> $DIR/coherence-lone-type-parameter.rs:6:20
|
LL | impl<T> Remote for T { }
| ^ type parameter `T` must be used as the type parameter for some local type
| ^ type parameter `T` must be used as the type parameter for some local type
|
= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local
= note: only traits defined in the current crate can be implemented for a type parameter
Expand Down
24 changes: 24 additions & 0 deletions src/test/ui/coherence/coherence-projection-uncovered-ty-2.rs
@@ -0,0 +1,24 @@
// Here we expect that orphan rule is violated
// because T is an uncovered parameter appearing
// before the first local (Issue #99554)

// aux-build:coherence_projection_assoc.rs

extern crate coherence_projection_assoc as lib;
use lib::Foreign;

trait Id {
type Assoc;
}

impl<T> Id for T {
type Assoc = T;
}

pub struct B;
impl<T> Foreign<B, T> for <Vec<Vec<T>> as Id>::Assoc {
//~^ ERROR E0210
type Assoc = usize;
}

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/coherence/coherence-projection-uncovered-ty-2.stderr
@@ -0,0 +1,12 @@
error[E0210]: associated type `<Vec<Vec<T>> as Id>::Assoc` must be covered by another type when it appears before the first local type (`B`)
--> $DIR/coherence-projection-uncovered-ty-2.rs:19:27
|
LL | impl<T> Foreign<B, T> for <Vec<Vec<T>> as Id>::Assoc {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ associated type `<Vec<Vec<T>> as Id>::Assoc` must be covered by another type when it appears before the first local type (`B`)
|
= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters or associated types appear before that first local type
= note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last

error: aborting due to previous error

For more information about this error, try `rustc --explain E0210`.
20 changes: 20 additions & 0 deletions src/test/ui/coherence/coherence-projection-uncovered-ty-3.rs
@@ -0,0 +1,20 @@
// aux-build:coherence_projection_assoc.rs

extern crate coherence_projection_assoc as lib;
use lib::Foreign;

trait Id {
type Assoc;
}

impl<T> Id for T {
type Assoc = T;
}

pub struct Local<T>(T);
impl<T> Foreign<<T as Id>::Assoc, Local<T>> for () {
//~^ ERROR E0210
type Assoc = usize;
}

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/coherence/coherence-projection-uncovered-ty-3.stderr
@@ -0,0 +1,12 @@
error[E0210]: associated type `<T as Id>::Assoc` must be covered by another type when it appears before the first local type (`Local<T>`)
--> $DIR/coherence-projection-uncovered-ty-3.rs:15:17
|
LL | impl<T> Foreign<<T as Id>::Assoc, Local<T>> for () {
| ^^^^^^^^^^^^^^^^ associated type `<T as Id>::Assoc` must be covered by another type when it appears before the first local type (`Local<T>`)
|
= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local, and no uncovered type parameters or associated types appear before that first local type
= note: in this case, 'before' refers to the following order: `impl<..> ForeignTrait<T1, ..., Tn> for T0`, where `T0` is the first and `Tn` is the last

error: aborting due to previous error

For more information about this error, try `rustc --explain E0210`.
19 changes: 19 additions & 0 deletions src/test/ui/coherence/coherence-projection-uncovered-ty-4.rs
@@ -0,0 +1,19 @@
// aux-build:coherence_projection_assoc.rs

extern crate coherence_projection_assoc as lib;
use lib::Foreign;

trait Id {
type Assoc;
}

impl<T> Id for T {
type Assoc = T;
}

impl<T> Foreign<(), Vec<T>> for <T as Id>::Assoc {
//~^ ERROR E0210
type Assoc = usize;
}

fn main() {}
12 changes: 12 additions & 0 deletions src/test/ui/coherence/coherence-projection-uncovered-ty-4.stderr
@@ -0,0 +1,12 @@
error[E0210]: associated type `<T as Id>::Assoc` must be used as the type parameter for some local type (e.g., `MyStruct<<T as Id>::Assoc>`)
--> $DIR/coherence-projection-uncovered-ty-4.rs:14:33
|
LL | impl<T> Foreign<(), Vec<T>> for <T as Id>::Assoc {
| ^^^^^^^^^^^^^^^^ associated type `<T as Id>::Assoc` must be used as the type parameter for some local type
|
= note: implementing a foreign trait is only possible if at least one of the types for which it is implemented is local
= note: only traits defined in the current crate can be implemented for a type parameter

error: aborting due to previous error

For more information about this error, try `rustc --explain E0210`.
24 changes: 24 additions & 0 deletions src/test/ui/coherence/coherence-projection-uncovered-ty.rs
@@ -0,0 +1,24 @@
// Here we expect that orphan rule is violated
// because T is an uncovered parameter appearing
// before the first local (Issue #99554)

// aux-build:coherence_projection_assoc.rs

extern crate coherence_projection_assoc as lib;
use lib::Foreign;

trait Id {
type Assoc;
}

impl<T> Id for T {
type Assoc = T;
}

pub struct B;
impl<T> Foreign<B, T> for <T as Id>::Assoc {
//~^ ERROR E0210
type Assoc = usize;
}

fn main() {}