Make const eval return normalized ty

[bit-aloo]

closes: rust-lang/trait-system-refactor-initiative#286
closes: #156294

r? @lcnr

[rustbot]

Some changes occurred in const_evaluatable.rs

cc @BoxyUwU

Some changes occurred to the core trait solver

cc @rust-lang/initiative-trait-system-refactor

[bit-aloo]

I am very dicy around the old solver changes: 0d49779

[khyperia]

neat, looks good to me!

one mostly style comment I had is that the evaluate machinery is getting really annoyingly tangly and layered and messy, and this is just more layers of wonkiness on top, haha. I wonder if instead of having a double-wrapped Result like you do with this change, instead:

• remove the _with_fallible_normalization methods
• EvaluateConstErr gets a new variant, "type normalization error", whose value is a generic parameter
• try_evaluate_const has an <E> generic arg, takes a normalize: fn(..) -> Result<Ty, E> and returns Result<Const, EvaluateConstErr<E>>
• evaluate_const returns either Result<Option<Const>, E> or Result<Const, SomeNewEnum<E>> that's either... you know what, hm, evaluate_const maybe should just get nuked, maybe everything should use try_evaluate_const, since there's multiple complex failure modes of regular evaluate_const, and the complexity reduction of evaluate_const over try_evaluate_const isn't that useful anymore. idk. shrug.
• infallable normalization returns Result<Ty, Infallible> so try_evaluate_const returns EvaluateConstErr<Infallible>. (or actual ! never, I forget if we're allowed to use it here)
• fallable normalization returns with E=NoSolutionOrRerunNonErased or whatever
• try_evaluate_const_inner doesn't need to exist, nor does the weird new enum EvaluatedConst

anyway, just kind of spitballing here, totally up to you on whether you try to clean this up, it's also fine as-is (I'll probably try to clean it up post-merge though if you leave it as-is)

[bit-aloo]

neat, looks good to me!

one mostly style comment I had is that the evaluate machinery is getting really annoyingly tangly and layered and messy, and this is just more layers of wonkiness on top, haha. I wonder if instead of having a double-wrapped Result like you do with this change, instead:

• remove the _with_fallible_normalization methods
• EvaluateConstErr gets a new variant, "type normalization error", whose value is a generic parameter
• try_evaluate_const has an <E> generic arg, takes a normalize: fn(..) -> Result<Ty, E> and returns Result<Const, EvaluateConstErr<E>>
• evaluate_const returns either Result<Option<Const>, E> or Result<Const, SomeNewEnum<E>> that's either... you know what, hm, evaluate_const maybe should just get nuked, maybe everything should use try_evaluate_const, since there's multiple complex failure modes of regular evaluate_const, and the complexity reduction of evaluate_const over try_evaluate_const isn't that useful anymore. idk. shrug.
• infallable normalization returns Result<Ty, Infallible> so try_evaluate_const returns EvaluateConstErr<Infallible>. (or actual ! never, I forget if we're allowed to use it here)
• fallable normalization returns with E=NoSolutionOrRerunNonErased or whatever
• try_evaluate_const_inner doesn't need to exist, nor does the weird new enum EvaluatedConst

anyway, just kind of spitballing here, totally up to you on whether you try to clean this up, it's also fine as-is (I'll probably try to clean it up post-merge though if you leave it as-is)

I will do the clean up and let you know. Thanks for the review 😄

[BoxyUwU]

I would also try not being generic over normalization routine. I don't think it's important to have old solver support here

[bit-aloo]

Ripped out all old solver patches, changes should be eye pleasing now 😄

[bit-aloo]

r? @khyperia

[khyperia]

changes should be eye pleasing now

heck yea love me some nevers 🔥

two nits, and one thing I'm squinting at, otherwise looks fantastic!

View changes since this review

[khyperia]

this isn't your PR, but lmao, what are we doing with this match statement 💀 can just remove the match and use try_evaluate_const directly I think, but also ok to leave it

[bit-aloo]

😆 This is so funny. Collapsed it.

[khyperia]

I have a vague preference to keep #[derive(Debug)] and add E: Debug to evaluate_const<E: Debug> and try_evaluate_const<E: Debug> (i.e. add E: Debug wherever the compiler errors at you)

[bit-aloo]

I had this before, but then changed it to explicit debug. I have reverted it back now.

[khyperia]

I'm vaguely sus of this. Should this really return Ok(None) instead of Err?

Admittedly I need more coffee and time to think through this. Some potential wonky interactions with feature(checked_type_aliases) and whatnot that's too big-brained for me right now. If you're confident this should be Ok(None), great, if not, maybe we could chat on zulip a bit about this and figure it out together? At the very least, I think a comment explaining why it should/shouldn't return Ok(None) vs Err would be good here.

[bit-aloo]

Now that you mention it, I think what I did here is wrong, or atleast incomplete. Originally, this method's Err was just the rerun signal (RerunNonErased) from the erased-mode guard, while Ok(None) meant "too generic, treat as ambiguity". Normalizing the value's type adds another possible failure (NoSolutionOrRerunNonErased from self.normalize). I don't think those should be conflated. So a NoSolution here should be considered as a failure?. normalize_free_alias also propagates normalization failures with ?, so treating it as ambiguity here seems inconsistent. So I am leaning towards returning Result<Option<Const>, NoSolutionOrRerunNonErased> and letting callers ? it. The main thing I am unsure about is whether we can even hit NoSolution here. The const is fully concrete by the time we evaluate it, so its type's projections should also resolve?

[khyperia]

Yeah, the next morning's coffee in hand with a proper sleep, I think I agree, on a theoretical level a type normalization failure here should be an error, not treated as ambiguous (or, under full GCA, as rigid). I think the None case ought to only come from CTFE needing concrete types to compute values, but types ought to always be "evaluable" (even if just to some rigid version of themselves if generics are involved).

On a concrete technical level, both callers already return Result<_, NoSolutionOrRerunNonErased> (well, spelled QueryResultOrRerunNonErased<I> but whatever, same thing) and both already do ?, so on a mechanical level it's straightforward. I think the match statement here would also just go away, return self.delegate.evaluate_const directly~

Unfortunately I'm also unsure of whether it's even possible to reach this point. I spent a bit trying to manually fuzz my way into hitting it and was unable to. I'll poke in zulip to see if people who know more than I do know what's up :3

edit: after thinking some more (see zulip thread), I thiiiiink this is unreachable (I think), so maybe we should assert/ICE here, instead of propagating Err(NoSolution). It's next-solver anyway, so can't break stable with an ICE, and then we get a nice lil bug report if it does turn out to be reachable~