Handle a specific providing ImplWitnessAccess for a symbolic binding used as a type #6201
Conversation
toolchain/check/eval.cpp
Outdated
| // Copy the updated constant field values into `bind`. | ||
| bind = inst.As<SemIR::SymbolicBindingType>(); |
There was a problem hiding this comment.
Why does this copy the value into bind? It looks it's not an output parameter; whereas before there was conditional assignment, you've made that unconditional. Can you elaborate here, or declare a separate variable to make it clearer that updating bind has no real effect?
There was a problem hiding this comment.
I'd suggest removing bind and calling Is in the two places it's used. Each time we convert it, we use it exactly once, and keeping it around as a duplicate of inst seems error-prone. (We should pass inst directly to MakeConstantResult rather than converting back from SymbolicBindingType.)
There was a problem hiding this comment.
makes sense, yeah. Done.
| return ConvertEvalResultToConstantId( | ||
| eval_context.context(), | ||
| EvalConstantInst(eval_context.context(), access), | ||
| ComputeInstPhase(eval_context.context(), access)); |
There was a problem hiding this comment.
Could shortcut this a little:
| ComputeInstPhase(eval_context.context(), access)); | |
| GetPhase(eval_context.constant_values(), value)); |
... though either way seems OK to me.
There was a problem hiding this comment.
That's true... though I decided to leave it, since it will not break this way, if we change FacetAccessType someday.
toolchain/check/eval.cpp
Outdated
| // Copy the updated constant field values into `bind`. | ||
| bind = inst.As<SemIR::SymbolicBindingType>(); |
There was a problem hiding this comment.
I'd suggest removing bind and calling Is in the two places it's used. Each time we convert it, we use it exactly once, and keeping it around as a duplicate of inst seems error-prone. (We should pass inst directly to MakeConstantResult rather than converting back from SymbolicBindingType.)
…e#6201; drop this when that lands.
3 participants
SymbolicBindingType evaluates to the type component of a symbolic facet value (a type/witnesses pair), and that symbolic facet value has its constant value replaced by a specific. That specific can provide a FacetValue, in which case it just evaluates to that FacetValue's type component. It can provide a BindSymbolicName of another binding, in which case it points to that entity instead and awaits a further specific. Currently the code only handles these two cases, and they match the behaviour of the evaluation of FacetAccessType itself.
However FacetAccessType evaluation also handles cases beyond these, as there are other instructions that occur as facet values, such as ImplWitnessAccess, when accessing an associated constant of an interface that has a facet type as its type.
Currently eval then crashes in this scenario. Instead of furthering to reproduce the contents of FacetAccessType's evaluation, defer to calling the
EvalConstantInst()overload for it when evaluating SymbolicBindingType against a new value from a specific. This means SymbolicBindingType can evaluate back into a FacetAccessType, when it was originally a FacetAccessType(BindSymbolicName) and becomes FacetAccessType(ImplWitnessAccess) through a specific.This comes with a test that crashed in eval before this change.