Treat constants with symbolic type as being symbolic. #4082
Conversation
When substituting into a symbolic constant, also substitute into its type.
toolchain/check/eval.cpp
Outdated
| return context.types().GetConstantId(type_id).is_symbolic() ? Phase::Symbolic | ||
| : Phase::Template; |
There was a problem hiding this comment.
Should this CHECK that it's constant? (i.e., that the expectation of either Symbolic or Template is correct)
(also, might consider a brief comment on the function)
There was a problem hiding this comment.
Good call -- it turns out that this was also getting called when type_id is invalid (specifically for StructTypeField instructions that don't have a type_id). I've cleaned this up and added a check for that case.
| @@ -63,7 +63,9 @@ static auto PushOperand(Context& context, Worklist& worklist, | |||
| worklist.Push(static_cast<SemIR::InstId>(arg)); | |||
| break; | |||
| case SemIR::IdKind::For<SemIR::TypeId>: | |||
| worklist.Push(context.types().GetInstId(static_cast<SemIR::TypeId>(arg))); | |||
| if (auto type_id = static_cast<SemIR::TypeId>(arg); type_id.is_valid()) { | |||
There was a problem hiding this comment.
Why do you use static_cast here instead of SemIR::TypeId(arg)? I grant the code was doing this previously, but it seems inconsistent (e.g., in ValueStore, we have IdT id = IdT(values_.size());, not static_cast<IdT>(values_.size())). I'm actually a little surprised this works with the explicit constructors.
I see you add SemIR::TypeId(type_id) on line 157, and static_cast<SemIR::TypeId>(arg) on line 106. I do think we should be consistent on this.
Similar elsewhere in this file -- a quick scan suggests this may be the only file using static_cast for Id types. If you prefer to do cleanup separately, that's fine, though I'm also fine if you were to just remove the static_casts within this PR -- it looks like there's 8 pre-existing (I don't count the static_cast<int>).
There was a problem hiding this comment.
I did this here just to be consistent with the existing code -- which I think was doing that as a probably-unnecessary attempt to avoid the clang-tidy check here. I'll send a separate PR to clean this up.
When constant evaluation produces a known non-symbolic value, treat the result as a symbolic constant anyway if the type of the value is symbolic.
We don't yet have many ways to produce a constant that has a known value but a symbolic type. The added test case is one such way: an array
[T; 0]initialized from()is a symbolic constant only because its type is symbolic -- we know its value is always(). More ways to form such constants will be appearing soon as we start to support generics: for example, a method of a generic class has a symbolic type but a known constant value of{}.When substituting into a symbolic constant, also substitute into its type.