Skip to content

EvaluatedToUnknown -> EvaluatedToAmbigStackDependent, EvaluatedToRecur -> EvaluatedToErrStackDependent #118685

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
Dec 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
20 changes: 10 additions & 10 deletions compiler/rustc_middle/src/traits/select.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,8 @@ pub enum SelectionCandidate<'tcx> {
///
/// The evaluation results are ordered:
/// - `EvaluatedToOk` implies `EvaluatedToOkModuloRegions`
/// implies `EvaluatedToAmbig` implies `EvaluatedToUnknown`
/// - `EvaluatedToErr` implies `EvaluatedToRecur`
/// implies `EvaluatedToAmbig` implies `EvaluatedToAmbigStackDependent`
/// - `EvaluatedToErr` implies `EvaluatedToErrStackDependent`
Comment on lines +183 to +184
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 doesn't seem to agree with the ordering, unless I am misunderstanding what "implies" means in both the positive and negative case.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, it seems wrong.

Though unifying EvaluatedToAmbigStackDependent and EvaluatedToAmbig should result in EvaluatedToAmbigStackDependent 🤔 at least when unifying the result of multiple nested goals

I believe the comment to be right and the order here to be wrong 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I thought so too.

/// - the "union" of evaluation results is equal to their maximum -
/// all the "potential success" candidates can potentially succeed,
/// so they are noops when unioned with a definite error, and within
Expand All @@ -199,16 +199,16 @@ pub enum EvaluationResult {
/// Evaluation is known to be ambiguous -- it *might* hold for some
/// assignment of inference variables, but it might not.
///
/// While this has the same meaning as `EvaluatedToUnknown` -- we can't
/// While this has the same meaning as `EvaluatedToAmbigStackDependent` -- we can't
/// know whether this obligation holds or not -- it is the result we
/// would get with an empty stack, and therefore is cacheable.
EvaluatedToAmbig,
/// Evaluation failed because of recursion involving inference
/// variables. We are somewhat imprecise there, so we don't actually
/// know the real result.
///
/// This can't be trivially cached for the same reason as `EvaluatedToRecur`.
EvaluatedToUnknown,
/// This can't be trivially cached for the same reason as `EvaluatedToErrStackDependent`.
EvaluatedToAmbigStackDependent,
/// Evaluation failed because we encountered an obligation we are already
/// trying to prove on this branch.
///
Expand Down Expand Up @@ -247,12 +247,12 @@ pub enum EvaluationResult {
/// does not hold, because of the bound (which can indeed be satisfied
/// by `SomeUnsizedType` from another crate).
//
// FIXME: when an `EvaluatedToRecur` goes past its parent root, we
// FIXME: when an `EvaluatedToErrStackDependent` goes past its parent root, we
// ought to convert it to an `EvaluatedToErr`, because we know
// there definitely isn't a proof tree for that obligation. Not
// doing so is still sound -- there isn't any proof tree, so the
// branch still can't be a part of a minimal one -- but does not re-enable caching.
EvaluatedToRecur,
EvaluatedToErrStackDependent,
/// Evaluation failed.
EvaluatedToErr,
}
Expand All @@ -276,15 +276,15 @@ impl EvaluationResult {
| EvaluatedToOk
| EvaluatedToOkModuloRegions
| EvaluatedToAmbig
| EvaluatedToUnknown => true,
| EvaluatedToAmbigStackDependent => true,

EvaluatedToErr | EvaluatedToRecur => false,
EvaluatedToErr | EvaluatedToErrStackDependent => false,
}
}

pub fn is_stack_dependent(self) -> bool {
match self {
EvaluatedToUnknown | EvaluatedToRecur => true,
EvaluatedToAmbigStackDependent | EvaluatedToErrStackDependent => true,

EvaluatedToOkModuloOpaqueTypes
| EvaluatedToOk
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub trait InferCtxtExt<'tcx> {
/// - the parameter environment
///
/// Invokes `evaluate_obligation`, so in the event that evaluating
/// `Ty: Trait` causes overflow, EvaluatedToRecur (or EvaluatedToUnknown)
/// `Ty: Trait` causes overflow, EvaluatedToErrStackDependent (or EvaluatedToAmbigStackDependent)
/// will be returned.
fn type_implements_trait(
&self,
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,8 @@ pub enum TreatInductiveCycleAs {
impl From<TreatInductiveCycleAs> for EvaluationResult {
fn from(treat: TreatInductiveCycleAs) -> EvaluationResult {
match treat {
TreatInductiveCycleAs::Ambig => EvaluatedToUnknown,
TreatInductiveCycleAs::Recur => EvaluatedToRecur,
TreatInductiveCycleAs::Ambig => EvaluatedToAmbigStackDependent,
TreatInductiveCycleAs::Recur => EvaluatedToErrStackDependent,
}
}
}
Expand Down Expand Up @@ -1231,7 +1231,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
})
{
debug!("evaluate_stack --> unbound argument, recursive --> giving up",);
return Ok(EvaluatedToUnknown);
return Ok(EvaluatedToAmbigStackDependent);
}

match self.candidate_from_obligation(stack) {
Expand Down