-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
fix: Unexpected trait bound not satisfied in HRTB and Associated Type #103695
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,7 @@ use super::{ | |
|
||
use crate::infer::{InferCtxt, InferOk, TypeFreshener}; | ||
use crate::traits::error_reporting::TypeErrCtxtExt; | ||
use crate::traits::project::try_normalize_with_depth_to; | ||
use crate::traits::project::ProjectAndUnifyResult; | ||
use crate::traits::project::ProjectionCacheKeyExt; | ||
use crate::traits::ProjectionCacheKey; | ||
|
@@ -1017,7 +1018,51 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { | |
return Ok(cycle_result); | ||
} | ||
|
||
let (result, dep_node) = self.in_task(|this| this.evaluate_stack(&stack)); | ||
let (result, dep_node) = self.in_task(|this| { | ||
let mut result = this.evaluate_stack(&stack)?; | ||
|
||
// fix issue #103563, we don't normalize | ||
// nested obligations which produced by `TraitDef` candidate | ||
// (i.e. using bounds on assoc items as assumptions). | ||
// because we don't have enough information to | ||
// normalize these obligations before evaluating. | ||
// so we will try to normalize the obligation and evaluate again. | ||
// we will replace it with new solver in the future. | ||
if EvaluationResult::EvaluatedToErr == result | ||
&& fresh_trait_pred.has_projections() | ||
&& fresh_trait_pred.is_global() | ||
{ | ||
let mut nested_obligations = Vec::new(); | ||
let predicate = try_normalize_with_depth_to( | ||
this, | ||
param_env, | ||
obligation.cause.clone(), | ||
obligation.recursion_depth + 1, | ||
obligation.predicate, | ||
&mut nested_obligations, | ||
); | ||
if predicate != obligation.predicate { | ||
let mut nested_result = EvaluationResult::EvaluatedToOk; | ||
for obligation in nested_obligations { | ||
nested_result = cmp::max( | ||
this.evaluate_predicate_recursively(stack.list(), obligation)?, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. afaict this uses the stack which already includes the current goal, so we get a coinductive cycle in #108897. Have to pop the element pushed in line 988 before recursing so that the stack isn't invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks~ |
||
nested_result, | ||
); | ||
} | ||
|
||
if nested_result.must_apply_modulo_regions() { | ||
let obligation = obligation.with(this.tcx(), predicate); | ||
result = cmp::max( | ||
nested_result, | ||
this.evaluate_trait_predicate_recursively(stack.list(), obligation)?, | ||
); | ||
} | ||
} | ||
} | ||
|
||
Ok::<_, OverflowError>(result) | ||
}); | ||
|
||
let result = result?; | ||
|
||
if !result.must_apply_modulo_regions() { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
// build-pass | ||
|
||
fn main() { | ||
let mut log_service = LogService { inner: Inner }; | ||
log_service.call(()); | ||
} | ||
|
||
pub trait Service<Request> { | ||
type Response; | ||
|
||
fn call(&mut self, req: Request) -> Self::Response; | ||
} | ||
|
||
pub struct LogService<S> { | ||
inner: S, | ||
} | ||
|
||
impl<T, U, S> Service<T> for LogService<S> | ||
where | ||
S: Service<T, Response = U>, | ||
U: Extension + 'static, | ||
for<'a> U::Item<'a>: std::fmt::Debug, | ||
{ | ||
type Response = S::Response; | ||
|
||
fn call(&mut self, req: T) -> Self::Response { | ||
self.inner.call(req) | ||
} | ||
} | ||
|
||
pub struct Inner; | ||
|
||
impl Service<()> for Inner { | ||
type Response = Resp; | ||
|
||
fn call(&mut self, req: ()) -> Self::Response { | ||
Resp::A(req) | ||
} | ||
} | ||
|
||
pub trait Extension { | ||
type Item<'a>; | ||
|
||
fn touch<F>(self, f: F) -> Self | ||
where | ||
for<'a> F: Fn(Self::Item<'a>); | ||
} | ||
|
||
pub enum Resp { | ||
A(()), | ||
} | ||
|
||
impl Extension for Resp { | ||
type Item<'a> = RespItem<'a>; | ||
fn touch<F>(self, _f: F) -> Self | ||
where | ||
for<'a> F: Fn(Self::Item<'a>), | ||
{ | ||
match self { | ||
Self::A(a) => Self::A(a), | ||
} | ||
} | ||
} | ||
|
||
pub enum RespItem<'a> { | ||
A(&'a ()), | ||
} | ||
|
||
impl<'a> std::fmt::Debug for RespItem<'a> { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
match self { | ||
Self::A(arg0) => f.debug_tuple("A").field(arg0).finish(), | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't feel like the right place to do this. The caller of evaluate should ideally have already normalized the predicate.
I feel like we're missing a normalize call somewhere and that this change is merely hiding that issue.
Haven't read the discussion in this PR so this may have already been brought up, sorry in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright 😅 actually looked at the previous comments and saw this already mentioned. We intend to replace evaluate with a new solver in the near future at which point this issue should be fixed without needing any additional work.
Until then I am always a bit concerned when making evaluate stronger as this provides more places where the specific behavior of evaluate could be more powerful than the new solver making it more difficult to switch. I don't think this PR adds any such issues cause it should just work™ in the new solver.