Skip to content

Don't call type_of on TAIT in defining scope in new solver #112825

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

Merged
merged 1 commit into from
Jul 7, 2023
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
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/solve/eval_ctxt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
}
}

pub(super) fn can_define_opaque_ty(&mut self, def_id: LocalDefId) -> bool {
pub(super) fn can_define_opaque_ty(&self, def_id: LocalDefId) -> bool {
self.infcx.opaque_type_origin(def_id).is_some()
}

Expand Down
27 changes: 27 additions & 0 deletions compiler/rustc_trait_selection/src/solve/trait_goals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use rustc_hir::{LangItem, Movability};
use rustc_infer::traits::query::NoSolution;
use rustc_infer::traits::util::supertraits;
use rustc_middle::traits::solve::{CanonicalResponse, Certainty, Goal, QueryResult};
use rustc_middle::traits::Reveal;
use rustc_middle::ty::fast_reject::{DeepRejectCtxt, TreatParams, TreatProjections};
use rustc_middle::ty::{self, ToPredicate, Ty, TyCtxt};
use rustc_middle::ty::{TraitPredicate, TypeVisitableExt};
Expand Down Expand Up @@ -118,6 +119,32 @@ impl<'tcx> assembly::GoalKind<'tcx> for TraitPredicate<'tcx> {
return result;
}

// Don't call `type_of` on a local TAIT that's in the defining scope,
// since that may require calling `typeck` on the same item we're
// currently type checking, which will result in a fatal cycle that
// ideally we want to avoid, since we can make progress on this goal
// via an alias bound or a locally-inferred hidden type instead.
//
// Also, don't call `type_of` on a TAIT in `Reveal::All` mode, since
// we already normalize the self type in
// `assemble_candidates_after_normalizing_self_ty`, and we'd
// just be registering an identical candidate here.
//
// Returning `Err(NoSolution)` here is ok in `SolverMode::Coherence`
// since we'll always be registering an ambiguous candidate in
// `assemble_candidates_after_normalizing_self_ty` due to normalizing
// the TAIT.
Comment on lines +122 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally don't like this perspective on why we do this. Imo the query cycles are a side effect of something more fundamental:

  • instantiate_constituent_tys_for_auto_trait for opaque types is a hacky, pseudo normalization-routine which does not check item bounds of the opaque
  • if we're able to normalize the opaque in a different way, either because we're able to define it or because we're in Reveal::All, using regular normalization is strictly preferable to auto trait leakage

I would like you to rewrite this comment to reflect that, but also fine to wait and chat about it in sync a bit if you don't consider this perspective to be helpful or maybe even correct

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not particularly attached to any specific comment here, though I think the best way to settle this is for you to just write what you'd like for this comment to say as an inline suggestion.

I'm not sure if we share the exact mental model for why this "hack" is not as much of a hack as it seems, but what I wrote above is how I see it at least from an implementation perspective.

Copy link
Contributor

Choose a reason for hiding this comment

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

the comment I would like to write would also require some general code changes, put that in my notes and will look at it at some later point and open a separate PR

if let ty::Alias(ty::Opaque, opaque_ty) = goal.predicate.self_ty().kind() {
if matches!(goal.param_env.reveal(), Reveal::All)
|| opaque_ty
.def_id
.as_local()
.is_some_and(|def_id| ecx.can_define_opaque_ty(def_id))
{
return Err(NoSolution);
}
}

ecx.probe_and_evaluate_goal_for_constituent_tys(
goal,
structural_traits::instantiate_constituent_tys_for_auto_trait,
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/traits/new-solver/dont-remap-tait-substs.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// compile-flags: -Ztrait-solver=next
// known-bug: #112825
// check-pass

// Makes sure we don't prepopulate the MIR typeck of `define`
// with `Foo<T, U> = T`, but instead, `Foo<B, A> = B`, so that
Expand Down
99 changes: 0 additions & 99 deletions tests/ui/traits/new-solver/dont-remap-tait-substs.stderr

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
error[E0283]: type annotations needed: cannot satisfy `Foo: Send`
--> $DIR/dont-type_of-tait-in-defining-scope.rs:16:5
|
LL | needs_send::<Foo>();
| ^^^^^^^^^^^^^^^^^
|
= note: cannot satisfy `Foo: Send`
note: required by a bound in `needs_send`
--> $DIR/dont-type_of-tait-in-defining-scope.rs:13:18
|
LL | fn needs_send<T: Send>() {}
| ^^^^ required by this bound in `needs_send`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0283`.
22 changes: 22 additions & 0 deletions tests/ui/traits/new-solver/dont-type_of-tait-in-defining-scope.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// revisions: is_send not_send
// compile-flags: -Ztrait-solver=next
//[is_send] check-pass

#![feature(type_alias_impl_trait)]

#[cfg(is_send)]
type Foo = impl Send;

#[cfg(not_send)]
type Foo = impl Sized;

fn needs_send<T: Send>() {}

fn test() {
needs_send::<Foo>();
//[not_send]~^ ERROR type annotations needed: cannot satisfy `Foo: Send`
}

fn main() {
let _: Foo = ();
}