Fix incomplete check of record row constraints #1558
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In an attempt to free the allocation of the constraints of a record row unification variable that is being assigned to some record rows, the code checking that row constraints are satisfied was removing the constraints from the global state. Since rows are defined as linked list, if the constraint wasn't violated on the first row, this function would recursively call itself. But then the subsequent recursive calls would try to get the constraints from the state again, to only find an empty set constraints, as it has been removed just before. The function thus wouldn't detect constraint violations happening in the tail of the record rows. This patch defines a subfunction which passes the initial constraints along recursive calls, such that they aren't lost during recursion, instead of trying to get them from the state again at each recursive call.
yannham
force-pushed
the
fix/row-conflict-check
branch
from
c64fbc0
to
459c042
Compare
vkleen
approved these changes
Sep 1, 2023
suimong
pushed a commit
to suimong/nickel
that referenced
this pull request
Sep 17, 2023
In an attempt to free the allocation of the constraints of a record row unification variable that is being assigned to some record rows, the code checking that row constraints are satisfied was removing the constraints from the global state. Since rows are defined as linked list, if the constraint wasn't violated on the first row, this function would recursively call itself. But then the subsequent recursive calls would try to get the constraints from the state again, to only find an empty set constraints, as it has been removed just before. The function thus wouldn't detect constraint violations happening in the tail of the record rows. This patch defines a subfunction which passes the initial constraints along recursive calls, such that they aren't lost during recursion, instead of trying to get them from the state again at each recursive call.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1393
In an attempt to free the allocation of the constraints of a record row unification variable that is being assigned to some other record rows, the code checking that row constraints are satisfied (
constr_unify_rrows) was removing the constraints from the global state. Since rows are defined as linked lists, if the constraint wasn't violated on the first row, this function would recursively call itself. But then the subsequent recursive calls would try to get the constraints from the state again, to only find an empty set constraints, as it has been removed just before. The function thus wouldn't detect constraint violations happening in the tail of the record rows.This explains why #1393 was getting triggered randomly (at first, then the issue became deterministic, maybe after the many refactorings related to introducing type variable levels), as it depends on the order of the row declarations.
This patch defines a subfunction which passes the initial constraints along recursive calls, such that they aren't lost during recursion, instead of trying to get them from the state again at each recursive call.
Additionally,
constr_unify_rrowswasn't properly called in the body ofremove_row, so row constraints weren't propagated properly there. Fixingconstr_unify_rrowsas described before and calling it at the right place fixed #1393.