Skip to content

Commit

Permalink
Normalize trait ref before orphan check
Browse files Browse the repository at this point in the history
  • Loading branch information
fmease committed Oct 28, 2023
1 parent 9ab0749 commit 878f995
Show file tree
Hide file tree
Showing 14 changed files with 275 additions and 29 deletions.
8 changes: 4 additions & 4 deletions compiler/rustc_hir_analysis/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -370,13 +370,13 @@ hir_analysis_transparent_non_zero_sized_enum = the variant of a transparent {$de
.label = needs at most one field with non-trivial size or alignment, but has {$field_count}
.labels = this field has non-zero size or requires alignment
hir_analysis_ty_param_first_local = type parameter `{$param_ty}` must be covered by another type when it appears before the first local type (`{$local_type}`)
.label = type parameter `{$param_ty}` must be covered by another type when it appears before the first local type (`{$local_type}`)
hir_analysis_ty_param_first_local = type parameter `{$param}` must be covered by another type when it appears before the first local type (`{$local_type}`)
.label = type parameter `{$param}` must be covered by another type when it appears before the first local type (`{$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
.case_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
hir_analysis_ty_param_some = type parameter `{$param_ty}` must be used as the type parameter for some local type (e.g., `MyStruct<{$param_ty}>`)
.label = type parameter `{$param_ty}` must be used as the type parameter for some local type
hir_analysis_ty_param_some = type parameter `{$param}` must be used as the type parameter for some local type (e.g., `MyStruct<{$param}>`)
.label = type parameter `{$param}` 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
.only_note = only traits defined in the current crate can be implemented for a type parameter
Expand Down
47 changes: 37 additions & 10 deletions compiler/rustc_hir_analysis/src/coherence/orphan.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Orphan checker: every impl either implements a trait defined in this
//! crate or pertains to a type defined in this crate.

use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::{DelayDm, ErrorGuaranteed};
use rustc_hir as hir;
use rustc_middle::ty::util::CheckRegions;
Expand All @@ -12,7 +12,7 @@ use rustc_middle::ty::{
};
use rustc_session::lint;
use rustc_span::def_id::{DefId, LocalDefId};
use rustc_span::Span;
use rustc_span::{Span, Symbol};
use rustc_trait_selection::traits;
use std::ops::ControlFlow;

Expand Down Expand Up @@ -424,22 +424,49 @@ fn emit_orphan_check_error<'tcx>(
};
tcx.sess.emit_err(err_struct)
}
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(uncovered_ty, local_type) => {
struct TyParamFinder {
ty_params: FxHashMap<Symbol, Span>,
}

// FIXME: This only reports the first uncovered type parameter it finds when in fact
// there could be multiple. E.g., in `<Type<T, U> as Trait>::Assoc` for `<T, U>`.
impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for TyParamFinder {
type BreakTy = (Symbol, Span);

fn visit_ty(&mut self, ty: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
// FIXME: This doesn't respect macro hygiene.
if let ty::Param(param_ty) = ty.kind()
&& let Some(&span) = self.ty_params.get(&param_ty.name)
{
return ControlFlow::Break((param_ty.name, span));
}

ty.super_visit_with(self)
}
}

let mut visitor = TyParamFinder {
ty_params: generics
.params
.iter()
.filter(|param| matches!(param.kind, hir::GenericParamKind::Type { .. }))
.map(|param| (param.name.ident().name, param.span))
.collect(),
};

let ControlFlow::Break((param, span)) = uncovered_ty.visit_with(&mut visitor) else {
bug!("failed to find ty param in {uncovered_ty}");
};

match local_type {
Some(local_type) => tcx.sess.emit_err(errors::TyParamFirstLocal {
span: sp,
span,
note: (),
param_ty,
param,
local_type,
}),
None => tcx.sess.emit_err(errors::TyParamSome { span: sp, note: (), param_ty }),
None => tcx.sess.emit_err(errors::TyParamSome { span, note: (), param }),
}
}
})
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_hir_analysis/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1198,20 +1198,20 @@ pub struct TyParamFirstLocal<'a> {
pub span: Span,
#[note(hir_analysis_case_note)]
pub note: (),
pub param_ty: Ty<'a>,
pub param: Symbol,
pub local_type: Ty<'a>,
}

#[derive(Diagnostic)]
#[diag(hir_analysis_ty_param_some, code = "E0210")]
#[note]
pub struct TyParamSome<'a> {
pub struct TyParamSome {
#[primary_span]
#[label]
pub span: Span,
#[note(hir_analysis_only_note)]
pub note: (),
pub param_ty: Ty<'a>,
pub param: Symbol,
}

#[derive(Diagnostic)]
Expand Down
59 changes: 49 additions & 10 deletions compiler/rustc_trait_selection/src/traits/coherence.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use crate::traits::{
};
use rustc_data_structures::fx::FxIndexSet;
use rustc_errors::Diagnostic;
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_hir::def_id::DefId;
use rustc_infer::infer::{DefineOpaqueTypes, InferCtxt, TyCtxtInferExt};
use rustc_infer::traits::{util, TraitEngine};
use rustc_middle::traits::query::NoSolution;
Expand Down Expand Up @@ -620,7 +620,7 @@ pub fn trait_ref_is_local_or_fundamental<'tcx>(
tcx: TyCtxt<'tcx>,
trait_ref: ty::TraitRef<'tcx>,
) -> bool {
trait_ref.def_id.krate == LOCAL_CRATE || tcx.has_attr(trait_ref.def_id, sym::fundamental)
trait_ref.def_id.is_local() || tcx.has_attr(trait_ref.def_id, sym::fundamental)
}

#[derive(Debug)]
Expand All @@ -637,7 +637,7 @@ pub enum OrphanCheckErr<'tcx> {
/// 2. Some local type must appear in `Self`.
#[instrument(level = "debug", skip(tcx), ret)]
pub fn orphan_check(tcx: TyCtxt<'_>, impl_def_id: DefId) -> Result<(), OrphanCheckErr<'_>> {
// We only except this routine to be invoked on implementations
// We only accept this routine to be invoked on implementations
// of a trait, not inherent implementations.
let trait_ref = tcx.impl_trait_ref(impl_def_id).unwrap().instantiate_identity();
debug!(?trait_ref);
Expand All @@ -648,7 +648,42 @@ pub fn orphan_check(tcx: TyCtxt<'_>, impl_def_id: DefId) -> Result<(), OrphanChe
return Ok(());
}

orphan_check_trait_ref::<!>(trait_ref, InCrate::Local, |ty| Ok(ty)).unwrap()
let delay_bug = || {
tcx.sess.delay_span_bug(
tcx.def_span(impl_def_id),
format!(
"orphan check: failed to normalize `{trait_ref}` while checking {impl_def_id:?}"
),
)
};

let infcx = tcx.infer_ctxt().intercrate(true).build();
let cause = ObligationCause::dummy();
let param_env = tcx.param_env(impl_def_id);

let ocx = ObligationCtxt::new(&infcx);
let trait_ref = ocx.normalize(&cause, param_env, trait_ref);
let trait_ref = infcx.resolve_vars_if_possible(trait_ref);
if !ocx.select_where_possible().is_empty() {
delay_bug();
}

orphan_check_trait_ref::<!>(trait_ref, InCrate::Local, |ty| {
Ok(if infcx.next_trait_solver() && let ty::Alias(..) = ty.kind() {
let mut fulfill_cx = <dyn TraitEngine<'_>>::new(&infcx);
// FIXME(-Ztrait-solver=next): This normalizes unnormalizable projections to
// infer vars (which can't be resolved here yet) (in intercrate mode) and it
// instantiates ty params to infer vars. This prevents us from syntactically
// checking if the result is a projection that contains ty params.
match infcx.at(&cause, param_env).structurally_normalize(ty, &mut *fulfill_cx) {
Ok(ty) => ty,
_ => Ty::new_error(tcx, delay_bug()),
}
} else {
ty
})
})
.unwrap()
}

/// Checks whether a trait-ref is potentially implementable by a crate.
Expand Down Expand Up @@ -754,7 +789,7 @@ fn orphan_check_trait_ref<'tcx, E: Debug>(
Ok(match trait_ref.visit_with(&mut checker) {
ControlFlow::Continue(()) => Err(OrphanCheckErr::NonLocalInputType(checker.non_local_tys)),
ControlFlow::Break(OrphanCheckEarlyExit::NormalizationFailure(err)) => return Err(err),
ControlFlow::Break(OrphanCheckEarlyExit::ParamTy(ty)) => {
ControlFlow::Break(OrphanCheckEarlyExit::UncoveredTy(ty)) => {
// Does there exist some local type after the `ParamTy`.
checker.search_first_local_ty = true;
if let Some(OrphanCheckEarlyExit::LocalTy(local_ty)) =
Expand Down Expand Up @@ -798,11 +833,11 @@ where
ControlFlow::Continue(())
}

fn found_param_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<OrphanCheckEarlyExit<'tcx, E>> {
fn found_uncovered_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<OrphanCheckEarlyExit<'tcx, E>> {
if self.search_first_local_ty {
ControlFlow::Continue(())
} else {
ControlFlow::Break(OrphanCheckEarlyExit::ParamTy(t))
ControlFlow::Break(OrphanCheckEarlyExit::UncoveredTy(t))
}
}

Expand All @@ -816,7 +851,7 @@ where

enum OrphanCheckEarlyExit<'tcx, E> {
NormalizationFailure(E),
ParamTy(Ty<'tcx>),
UncoveredTy(Ty<'tcx>),
LocalTy(Ty<'tcx>),
}

Expand Down Expand Up @@ -851,10 +886,14 @@ where
| ty::Never
| ty::Tuple(..)
| ty::Alias(ty::Projection | ty::Inherent | ty::Weak, ..) => {
self.found_non_local_ty(ty)
if ty.has_type_flags(ty::TypeFlags::HAS_TY_PARAM) {
self.found_uncovered_ty(ty)
} else {
ControlFlow::Continue(())
}
}

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

ty::Placeholder(..) | ty::Bound(..) | ty::Infer(..) => match self.in_crate {
InCrate::Local => self.found_non_local_ty(ty),
Expand Down
2 changes: 2 additions & 0 deletions tests/ui/associated-types/issue-38821.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ pub trait Column: Expression {}

#[derive(Debug, Copy, Clone)]
//~^ ERROR the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
//~| ERROR the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
//~| ERROR the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
pub enum ColumnInsertValue<Col, Expr> where
Col: Column,
Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>,
Expand Down
40 changes: 39 additions & 1 deletion tests/ui/associated-types/issue-38821.stderr
Original file line number Diff line number Diff line change
@@ -1,3 +1,22 @@
error[E0277]: the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
--> $DIR/issue-38821.rs:23:10
|
LL | #[derive(Debug, Copy, Clone)]
| ^^^^^ the trait `NotNull` is not implemented for `<Col as Expression>::SqlType`
|
note: required for `<Col as Expression>::SqlType` to implement `IntoNullable`
--> $DIR/issue-38821.rs:9:18
|
LL | impl<T: NotNull> IntoNullable for T {
| ------- ^^^^^^^^^^^^ ^
| |
| unsatisfied trait bound introduced here
= note: this error originates in the derive macro `Debug` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider further restricting the associated type
|
LL | Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull,
| +++++++++++++++++++++++++++++++++++++++

error[E0277]: the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
--> $DIR/issue-38821.rs:23:17
|
Expand All @@ -17,6 +36,25 @@ help: consider further restricting the associated type
LL | Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull,
| +++++++++++++++++++++++++++++++++++++++

error: aborting due to previous error
error[E0277]: the trait bound `<Col as Expression>::SqlType: NotNull` is not satisfied
--> $DIR/issue-38821.rs:23:23
|
LL | #[derive(Debug, Copy, Clone)]
| ^^^^^ the trait `NotNull` is not implemented for `<Col as Expression>::SqlType`
|
note: required for `<Col as Expression>::SqlType` to implement `IntoNullable`
--> $DIR/issue-38821.rs:9:18
|
LL | impl<T: NotNull> IntoNullable for T {
| ------- ^^^^^^^^^^^^ ^
| |
| unsatisfied trait bound introduced here
= note: this error originates in the derive macro `Clone` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider further restricting the associated type
|
LL | Expr: Expression<SqlType=<Col::SqlType as IntoNullable>::Nullable>, <Col as Expression>::SqlType: NotNull,
| +++++++++++++++++++++++++++++++++++++++

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0277`.
2 changes: 2 additions & 0 deletions tests/ui/coherence/auxiliary/parametrized-trait.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
pub trait Trait0<T, U, V> {}
pub trait Trait1<T, U> {}
24 changes: 24 additions & 0 deletions tests/ui/coherence/orphan-check-projection-does-cover.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Projections can cover type parameters if they normalize to a (local) type that covers them.
// This ensures that we don't perform an overly strict check on
// projections like in closed PR #100555 which did a syntactic
// check for type parameters in projections without normalizing
// first which would've lead to real-word regressions.

// check-pass
// revisions: classic next
//[next] compile-flags: -Ztrait-solver=next

// aux-crate:foreign=parametrized-trait.rs
// edition:2021

trait Project { type Output; }

impl<T> Project for T {
type Output = Local;
}

struct Local;

impl<T> foreign::Trait1<Local, T> for <T as Project>::Output {}

fn main() {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
error[E0210]: type parameter `T` must be covered by another type when it appears before the first local type (`Local`)
--> $DIR/orphan-check-projection-doesnt-cover.rs:25:6
|
LL | impl<T> foreign::Trait0<Local, T, ()> for <T as Identity>::Output {}
| ^ type parameter `T` must be covered by another type when it appears before the first local type (`Local`)
|
= 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: 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[E0210]: type parameter `T` must be covered by another type when it appears before the first local type (`Local`)
--> $DIR/orphan-check-projection-doesnt-cover.rs:28:6
|
LL | impl<T> foreign::Trait0<<T as Identity>::Output, Local, T> for Option<T> {}
| ^ type parameter `T` must be covered by another type when it appears before the first local type (`Local`)
|
= 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: 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[E0210]: type parameter `T` must be covered by another type when it appears before the first local type (`Local`)
--> $DIR/orphan-check-projection-doesnt-cover.rs:40:6
|
LL | impl<T: Deferred> foreign::Trait1<Local, T> for <T as Deferred>::Output {}
| ^ type parameter `T` must be covered by another type when it appears before the first local type (`Local`)
|
= 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: 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 3 previous errors

For more information about this error, try `rustc --explain E0210`.
41 changes: 41 additions & 0 deletions tests/ui/coherence/orphan-check-projection-doesnt-cover.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Regression test for issue #99554.
// Projections might not cover type parameters.

// revisions: classic next
//[next] compile-flags: -Ztrait-solver=next

// FIXME(-Ztrait-solver=next): This currently passes in the next solver but it shouldn't.
//[next] check-pass
//[next] known-bug: unknown

// compile-flags: --crate-type=lib
// aux-crate:foreign=parametrized-trait.rs
// edition:2021

trait Identity {
type Output;
}

impl<T> Identity for T {
type Output = T;
}

struct Local;

impl<T> foreign::Trait0<Local, T, ()> for <T as Identity>::Output {}
//[classic]~^ ERROR type parameter `T` must be covered by another type

impl<T> foreign::Trait0<<T as Identity>::Output, Local, T> for Option<T> {}
//[classic]~^ ERROR type parameter `T` must be covered by another type

pub trait Deferred {
type Output;
}

// A downstream user could implement
//
// impl<T> Deferred for Type<T> { type Output = T; }
// struct Type<T>(T);
//
impl<T: Deferred> foreign::Trait1<Local, T> for <T as Deferred>::Output {}
//[classic]~^ ERROR type parameter `T` must be covered by another type
Loading

0 comments on commit 878f995

Please sign in to comment.