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

fix: Unexpected trait bound not satisfied in HRTB and Associated Type #103695

Merged
merged 1 commit into from
Feb 14, 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
47 changes: 46 additions & 1 deletion compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Copy link
Contributor

@lcnr lcnr Jan 10, 2023

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.

Copy link
Contributor

@lcnr lcnr Jan 10, 2023

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.

&& 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)?,
Copy link
Contributor

@lcnr lcnr Mar 8, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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() {
Expand Down
75 changes: 75 additions & 0 deletions tests/ui/traits/issue-103563.rs
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(),
}
}
}