Nested facets #5644
Nested facets #5644
Conversation
danakj
force-pushed
the
nested-facets
branch
2 times, most recently
from
74b1aa8 to
2cb1262
Compare
danakj
force-pushed
the
nested-facets
branch
4 times, most recently
from
f86f5ea to
b66387e
Compare
|
I reworked the PR description to match the changes I made since initial upload. |
| auto lhs_id = | ||
| GetConstantValueIgnoringPeriodSelf(eval_context, rewrite.lhs_id, phase); | ||
| auto rhs_id = | ||
| GetConstantValueIgnoringPeriodSelf(eval_context, rewrite.rhs_id, phase); | ||
| info.rewrite_constraints.push_back({.lhs_id = lhs_id, .rhs_id = rhs_id}); | ||
| rewrite = {.lhs_id = lhs_id, .rhs_id = rhs_id}; |
There was a problem hiding this comment.
I'm trying to puzzle through the assignment, why not:
| auto lhs_id = | |
| GetConstantValueIgnoringPeriodSelf(eval_context, rewrite.lhs_id, phase); | |
| auto rhs_id = | |
| GetConstantValueIgnoringPeriodSelf(eval_context, rewrite.rhs_id, phase); | |
| info.rewrite_constraints.push_back({.lhs_id = lhs_id, .rhs_id = rhs_id}); | |
| rewrite = {.lhs_id = lhs_id, .rhs_id = rhs_id}; | |
| rewrite.lhs_id = | |
| GetConstantValueIgnoringPeriodSelf(eval_context, rewrite.lhs_id, phase); | |
| rewrite.rhs_id = | |
| GetConstantValueIgnoringPeriodSelf(eval_context, rewrite.rhs_id, phase); |
Is this a holdover from the rewrite_constraints.push_back, or is there some issue with constant evaluation ordering (which might be worth a comment)?
There was a problem hiding this comment.
I did this then undid it because the assignment means if we add another field to the RewriteConstraint type (it's not an instruction) then the assignment version fails to build and points out the need to handle it here, whereas the per-field assignments don't.
There was a problem hiding this comment.
if we add another field to the RewriteConstraint type (it's not an instruction) then the assignment version fails to build
Is that a bug, or a feature? What field are you planning on adding to RewriteConstraint that it would be okay to not rewrite (or rewrite)?
There was a problem hiding this comment.
Note, I wasn't intending this to block review. I was more in a comment-approve-merge frame of mind. It's every other unaddressed comment that's the issue.
There was a problem hiding this comment.
Much like an enum in a switch telling you when you need to handle a new case, I think it's a feature to have to think about new fields. This is similar to some other bugs we've seen/addressed by constructing new objects instead of constructing up front and mutating fields - I am thinking of something in the pattern matching that I think you reviewed.
|
Generally LG, but I can't merge due to conflicts. |
Yeah format stopped me, and I have a couple more tests I concocted last night that are broken, so probably need to make a fix in the sorting/deduping code here - or maybe in a followup. We can't merge this until #5647 is reviewed/landed. Then I will rebase back to trunk. |
Test added, and a fix applied in comparison to make it pass. |
danakj
force-pushed
the
nested-facets
branch
2 times, most recently
from
66877ce to
d432257
Compare
…hereExpr and BitAnd (#5647) The `BitAnd` operation combines two `FacetTypeInfo` structures by concatenating their lists, but did not apply the current specific to the instructions in the `FacetTypeInfo` as it forgot to go through `GetContantFacetTypeInfo`. `WhereExpr` handling duplicates a lot of the logic in `GetConstantFacetTypeInfo` by calling `GetConstantValue` on things, instead of calling `GetConstantFacetTypeInfo` on the `FacetTypeInfo` it constructs. This meant it also needed to call `GetConstantFacetTypeInfo` on the base facet type, and on any `impls`-requirement facet types before merging their values together into a single `FacetTypeInfo`. Instead, make `WhereExpr` more like `BitAnd`, and have it concatenate things together as-is to construct a `FacetTypeInfo`. Then call `GetConstantFacetTypeInfo` to canonicalize it and return a constant value referring to it. In `GetConstantFacetTypeInfo` we fix a crasher by propagating errors inserted into the `FacetTypeInfo` out to the `Phase` so that the resulting instruction depending on the `FacetTypeInfo` is not resolved to a constant value with errors inside it. A test is added for this, which was crashing on import of the `FacetType` with an error within from the imported `impl` decl. This refactoring gives us three benefits: * There's now only a single place that does `ResolveRewriteConstraintsAndCanonicalize`, which is inside `GetConstantFacetTypeInfo`. This makes the inputs/behaviour of `ResolveRewriteConstraintsAndCanonicalize` more consistent. * There's now only a single place that updates the instructions in `FacetTypeInfo` constraints with new constant values, so that changes that rely on observing and interacting with that code only need to be written in a single place. This will avoid duplicating logic in #5644. * This will make it easier to move `WhereExpr` handling to a `EvalConstantInst` function, as it no longer directly depends on `GetConstantValue()` from `eval.cpp`.
Given a facet type:
(Z where .X = .Y) where .X =.YThe rewrite constraints in the inner facet type are each an
ImplWitnessAccessinto a witness for the self of typeZ(which is the facet type before thewhere). The rewrite constraints in the outer facet type are each anImplWitnessAccessfor the self of typeZ where .X = .Y, which is a different self facet type.This means when deduping in canonicalization, the first
.Xand the second.Xare different instructions, and different constant values, so they both remain in the rewrite constraints, incorrectly. Then if the outer.Xis allowed to evaluate to a value from its facet type, it finds.Yresulting in.Y = .Ywhich is also incorrect.Because of the failure to dedupe the first facet type, that is also diagnosed as two different assignments to the same
.X. To resolve that, we introduceCompareFacetTypeConstraintValues()compare values in facet type constraints, and treat accesses to the same associated constant in the same facet value asequivalenteven when through different witnesses. This allows us to dedupe the two.X = .Yrules into one in the combined facet type.Given a different facet type:
(Z where .X = ()) where .X = {}. Here we want to diagnose that.Xhas been assigned two different values. To do so, we need to see that the two.Xvalues are the same, and we useCompareFacetTypeConstraintValues()to do this comparison. Then we see two rewrite rules for the same LHS, and we can diagnose that.We enable evaluating
ImplWitnessAccesson.Selfto pull a value from rewrite constraints in a facet type so that we can see that we are not incorrect evaluating the LHS of rewrite constraints and producing cycles. By doing so, also enable generic code to see and use concrete values in associated constants in facet types.