Don't keep insts in eval block from failed deduce #5601
Conversation
danakj
changed the title
|
This is based on #5597, in order to place the tests together. The changes are in related code but it's not strictly dependent on the former PR. |
| if (stack_) { | ||
| stack_->dependent_inst_stack_.PopArray(); | ||
| stack_->pending_eval_block_stack_.PopArray(); | ||
| stack_->ongoing_commit_ = false; | ||
| stack_ = nullptr; | ||
| } |
There was a problem hiding this comment.
I think we'll need to do something with the ConstantsInGenericMap too -- either removing the entries from there corresponding to the things we removed from the eval block, or re-adding the items in that map back into the eval block if they're referenced again.
Specifically, if a generic references constant X during a transaction that gets reverted, and then later references the constant X again, if it's still in the ConstantsInGenericMap then I think we won't re-add it to the eval block, but we presumably need to.
There was a problem hiding this comment.
This is much more tricky, and the immediate crash is solved, so I am going to mark this WIP to come back to.
Deduction does
SubstConstant()to apply enclosing specifics, and deduced arguments, to the arguments. This process creates new instructions with the applied substitutions, which may includeLookupImplWitness. If the conversion fails, those instructions will not be used, but they have been added to the enclosing generic when there is one. This leaves behind aLookupImplWitnessinstruction with a runtime constant value as a dependent instruction and in the eval block. When the generic's eval block is re-evaluated later, theLookupImplWitnessinstruction fails to produce a constant value (again), which violates its contract and we crash.To avoid this, we scope the SubstConstant and Convert steps in deduce with a "commit" for the generic stack. If the conversion fails, we drop all instructions created by SubstConstant and Convert from the enclosing generic context. Such instructions must not be referred to again, but they will not be as they are not stored anywhere or returned if Convert failed.
This fixes the crash demonstrated in https://carbon.godbolt.org/z/1EjMroT6e