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

Detect stricter constraints on gats where clauses in impls vs trait #88336

Merged
merged 2 commits into from
Sep 12, 2021
Merged
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
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

use crate::infer::error_reporting::nice_region_error::NiceRegionError;
use crate::infer::lexical_region_resolve::RegionResolutionError;
use crate::infer::{Subtype, ValuePairs};
use crate::infer::{SubregionOrigin, Subtype, ValuePairs};
use crate::traits::ObligationCauseCode::CompareImplMethodObligation;
use rustc_errors::ErrorReported;
use rustc_hir as hir;
Expand All @@ -11,44 +11,53 @@ use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::Visitor;
use rustc_middle::ty::error::ExpectedFound;
use rustc_middle::ty::{self, Ty, TyCtxt};
use rustc_span::{MultiSpan, Span};
use rustc_span::{MultiSpan, Span, Symbol};

impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
/// Print the error message for lifetime errors when the `impl` doesn't conform to the `trait`.
pub(super) fn try_report_impl_not_conforming_to_trait(&self) -> Option<ErrorReported> {
if let Some(ref error) = self.error {
debug!("try_report_impl_not_conforming_to_trait {:?}", error);
if let RegionResolutionError::SubSupConflict(
_,
var_origin,
sub_origin,
_sub,
sup_origin,
_sup,
) = error.clone()
{
if let (&Subtype(ref sup_trace), &Subtype(ref sub_trace)) =
(&sup_origin, &sub_origin)
let error = self.error.as_ref()?;
debug!("try_report_impl_not_conforming_to_trait {:?}", error);
if let RegionResolutionError::SubSupConflict(
_,
var_origin,
sub_origin,
_sub,
sup_origin,
_sup,
) = error.clone()
{
if let (&Subtype(ref sup_trace), &Subtype(ref sub_trace)) = (&sup_origin, &sub_origin) {
if let (
ValuePairs::Types(sub_expected_found),
ValuePairs::Types(sup_expected_found),
CompareImplMethodObligation { trait_item_def_id, .. },
) = (&sub_trace.values, &sup_trace.values, &sub_trace.cause.code)
Comment on lines +30 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove one level of nesting by merging the second if let check with the first one.

{
if let (
ValuePairs::Types(sub_expected_found),
ValuePairs::Types(sup_expected_found),
CompareImplMethodObligation { trait_item_def_id, .. },
) = (&sub_trace.values, &sup_trace.values, &sub_trace.cause.code)
{
if sup_expected_found == sub_expected_found {
self.emit_err(
var_origin.span(),
sub_expected_found.expected,
sub_expected_found.found,
*trait_item_def_id,
);
return Some(ErrorReported);
}
if sup_expected_found == sub_expected_found {
self.emit_err(
var_origin.span(),
sub_expected_found.expected,
sub_expected_found.found,
*trait_item_def_id,
);
return Some(ErrorReported);
}
}
}
}
if let RegionResolutionError::ConcreteFailure(origin, _, _) = error.clone() {
if let SubregionOrigin::CompareImplTypeObligation {
span,
item_name,
impl_item_def_id,
trait_item_def_id,
} = origin
{
self.emit_associated_type_err(span, item_name, impl_item_def_id, trait_item_def_id);
jackh726 marked this conversation as resolved.
Show resolved Hide resolved
return Some(ErrorReported);
}
}
None
}

Expand Down Expand Up @@ -107,6 +116,25 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
}
err.emit();
}

fn emit_associated_type_err(
&self,
span: Span,
item_name: Symbol,
impl_item_def_id: DefId,
trait_item_def_id: DefId,
) {
let impl_sp = self.tcx().def_span(impl_item_def_id);
let trait_sp = self.tcx().def_span(trait_item_def_id);
let mut err = self
.tcx()
.sess
.struct_span_err(span, &format!("`impl` associated type signature for `{}` doesn't match `trait` associated type signature", item_name));
err.span_label(impl_sp, &format!("found"));
err.span_label(trait_sp, &format!("expected"));

err.emit();
}
}

struct TypeParamSpanVisitor<'tcx> {
Expand Down
18 changes: 18 additions & 0 deletions compiler/rustc_infer/src/infer/error_reporting/note.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
"...so that the definition in impl matches the definition from the trait",
);
}
infer::CompareImplTypeObligation { span, .. } => {
label_or_note(
span,
"...so that the definition in impl matches the definition from the trait",
);
}
}
}

Expand Down Expand Up @@ -356,6 +362,18 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
trait_item_def_id,
&format!("`{}: {}`", sup, sub),
),
infer::CompareImplTypeObligation {
span,
item_name,
impl_item_def_id,
trait_item_def_id,
} => self.report_extra_impl_obligation(
span,
item_name,
impl_item_def_id,
trait_item_def_id,
&format!("`{}: {}`", sup, sub),
),
}
}

Expand Down
21 changes: 21 additions & 0 deletions compiler/rustc_infer/src/infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,15 @@ pub enum SubregionOrigin<'tcx> {
impl_item_def_id: DefId,
trait_item_def_id: DefId,
},

/// Comparing the signature and requirements of an impl associated type
/// against the containing trait
CompareImplTypeObligation {
span: Span,
item_name: Symbol,
impl_item_def_id: DefId,
trait_item_def_id: DefId,
},
}

// `SubregionOrigin` is used a lot. Make sure it doesn't unintentionally get bigger.
Expand Down Expand Up @@ -1810,6 +1819,7 @@ impl<'tcx> SubregionOrigin<'tcx> {
ReferenceOutlivesReferent(_, a) => a,
CallReturn(a) => a,
CompareImplMethodObligation { span, .. } => span,
CompareImplTypeObligation { span, .. } => span,
}
}

Expand All @@ -1833,6 +1843,17 @@ impl<'tcx> SubregionOrigin<'tcx> {
trait_item_def_id,
},

traits::ObligationCauseCode::CompareImplTypeObligation {
item_name,
impl_item_def_id,
trait_item_def_id,
} => SubregionOrigin::CompareImplTypeObligation {
span: cause.span,
item_name,
impl_item_def_id,
trait_item_def_id,
},

_ => default(),
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/generic-associated-types/impl_bounds.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ impl<T> Foo for Fooy<T> {
type A<'a> where Self: 'static = (&'a ());
//~^ ERROR the parameter type `T` may not live long enough
type B<'a, 'b> where 'b: 'a = (&'a(), &'b ());
//~^ ERROR lifetime bound not satisfied
//~^ ERROR `impl` associated type
//~| ERROR lifetime bound not satisfied
type C where Self: Copy = String;
//~^ ERROR the trait bound `T: Copy` is not satisfied
Expand Down
20 changes: 6 additions & 14 deletions src/test/ui/generic-associated-types/impl_bounds.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,16 @@ LL | type A<'a> where Self: 'static = (&'a ());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: consider adding an explicit lifetime bound `T: 'static`...
= note: ...so that the type `Fooy<T>` will meet its required lifetime bounds
= note: ...so that the definition in impl matches the definition from the trait
Copy link
Member Author

Choose a reason for hiding this comment

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

So this is a weird diagnostic. The trait has Self: 'a, but the impl has Self: 'static. So we should really only be saying that the impl has stricter requirements. But we're also seeming emitting a diagnostic that T doesn't live long enough.


error[E0478]: lifetime bound not satisfied
error: `impl` associated type signature for `B` doesn't match `trait` associated type signature
--> $DIR/impl_bounds.rs:17:5
|
LL | type B<'a, 'b> where 'a: 'b;
| ---------------------------- expected
...
LL | type B<'a, 'b> where 'b: 'a = (&'a(), &'b ());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
note: lifetime parameter instantiated with the lifetime `'b` as defined on the associated item at 17:16
--> $DIR/impl_bounds.rs:17:16
|
LL | type B<'a, 'b> where 'b: 'a = (&'a(), &'b ());
| ^^
note: but lifetime parameter must outlive the lifetime `'a` as defined on the associated item at 17:12
--> $DIR/impl_bounds.rs:17:12
|
LL | type B<'a, 'b> where 'b: 'a = (&'a(), &'b ());
| ^^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ found

error[E0478]: lifetime bound not satisfied
--> $DIR/impl_bounds.rs:17:5
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/generic-associated-types/issue-86787.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ LL | | <Left as HasChildrenOf>::T: 'a,
LL | | <Right as HasChildrenOf>::T: 'a
| | - help: consider adding a where clause: `, <Left as HasChildrenOf>::T: 'a`
LL | | = Either<&'a Left::T, &'a Right::T>;
| |________________________________________^ ...so that the type `<Left as HasChildrenOf>::T` will meet its required lifetime bounds
| |________________________________________^ ...so that the definition in impl matches the definition from the trait
Copy link
Member Author

Choose a reason for hiding this comment

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

This is also a bad diagnostic (still), but I wouldn't argue that it's any worse.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe point at the trait's assoc type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, but I'm planning to fix this diagnostic at some point anyways.


error[E0309]: the associated type `<Right as HasChildrenOf>::T` may not live long enough
--> $DIR/issue-86787.rs:23:5
Expand All @@ -22,7 +22,7 @@ LL | | <Left as HasChildrenOf>::T: 'a,
LL | | <Right as HasChildrenOf>::T: 'a
| | - help: consider adding a where clause: `, <Right as HasChildrenOf>::T: 'a`
LL | | = Either<&'a Left::T, &'a Right::T>;
| |________________________________________^ ...so that the type `<Right as HasChildrenOf>::T` will meet its required lifetime bounds
| |________________________________________^ ...so that the definition in impl matches the definition from the trait

error: aborting due to 2 previous errors

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// check-fail

#![feature(generic_associated_types)]

trait Foo {
type Assoc<'a, 'b>;
}
impl Foo for () {
type Assoc<'a, 'b> where 'a: 'b = ();
//~^ `impl` associated type
}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
error: `impl` associated type signature for `Assoc` doesn't match `trait` associated type signature
--> $DIR/missing-where-clause-on-trait.rs:9:5
|
LL | type Assoc<'a, 'b>;
| ------------------- expected
...
LL | type Assoc<'a, 'b> where 'a: 'b = ();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ found
Comment on lines +1 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

For future work, I'd like us to point out the exact divergence (in this case the whole where clause).


error: aborting due to previous error