panic in copy propagation inside comprehensions #1012
Labels
Comments
tsandall
added a commit
to tsandall/opa
that referenced
this issue
Oct 25, 2018
These changes address the panic that was happening during copy propagation when a ref inside of a comprehension was plugged with a composite value (or any value that would lead to an illegal ref). For example: x = [0]; [true | x[0] = 0] In this case, when partial eval ran, the saved expression before copy propagation ran would be the comprehension itself. The problem was that the binding x/[0] would be plugged into the comprehension resulting in: [true | [0][0] = 0] While this could be considered legal, we don't currently support refs like this (in the parser and in the evaluator). As a result, when copy propagation encountered this ref, it panic-ed because it would assume the ref head was a variable. To deal with this issue, we have refactored how partial evaluation handles comprehensions. Instead of simply saving the comprehension and continuing, the implementation will (i) capture bindings that are in scope and copy them into the comprehension body and (ii) namespace all of the variables inside the comprehension. An alternative would be to carefully plug comprehensions (such that ref heads were not replaced) however this would still require namepsacing. It would also require changes to copy propagation to ensure that ref head vars that appear in the containing query are not killed. In the future we can invest in (a) allowing references like [0][0] and (b) extending partial evaluation to deal with comprehensions. Fixes open-policy-agent#1012 Signed-off-by: Torin Sandall <torinsandall@gmail.com>
tsandall
added a commit
that referenced
this issue
Oct 25, 2018
These changes address the panic that was happening during copy propagation when a ref inside of a comprehension was plugged with a composite value (or any value that would lead to an illegal ref). For example: x = [0]; [true | x[0] = 0] In this case, when partial eval ran, the saved expression before copy propagation ran would be the comprehension itself. The problem was that the binding x/[0] would be plugged into the comprehension resulting in: [true | [0][0] = 0] While this could be considered legal, we don't currently support refs like this (in the parser and in the evaluator). As a result, when copy propagation encountered this ref, it panic-ed because it would assume the ref head was a variable. To deal with this issue, we have refactored how partial evaluation handles comprehensions. Instead of simply saving the comprehension and continuing, the implementation will (i) capture bindings that are in scope and copy them into the comprehension body and (ii) namespace all of the variables inside the comprehension. An alternative would be to carefully plug comprehensions (such that ref heads were not replaced) however this would still require namepsacing. It would also require changes to copy propagation to ensure that ref head vars that appear in the containing query are not killed. In the future we can invest in (a) allowing references like [0][0] and (b) extending partial evaluation to deal with comprehensions. Fixes #1012 Signed-off-by: Torin Sandall <torinsandall@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Version:
Reproduction:
The text was updated successfully, but these errors were encountered: