-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
internal/core/adt: Environment.up nil vertex panic when calling Value.Expr #2911
Comments
After trying out different cue. Below are the observation: Given a simple cue a: string
b: bool | *false
if !b {
c: a
} The above panic error occurs only when there is a In this case @mvdan any workaround till root cause is fixed would be highly appreciated as we are in middle of upgrading our tool's cue library. |
Thank you for the detailed report and the reproducer! I believe that this is the same underlying bug as #2411, which did not have a reproducer but shows an extremely similar call stack, as well as #2619, which does have a reproducer as the OpenAPI encoder calls the same Expr method. I will opt for closing the other two older issues in favor of this one given how you've managed to produce the smallest reproducer so far. Before we mark this bug as "fixed", I will verify that the fix also solves the other two issues, to double check that I didn'g get it wrong in terms of all three bugs having the same root cause. Below is your reproducer in testscript form, after some extra reduction:
|
As part of the comprehension refactor, https://cuelang.org/cl/529517 reintroduced a Conjunct.Expr method, and switched from Elem to it in multiple places including these two methods. The commit message said that in these cases the comprehension values are not appropriate or cannot occur anyway (confusing wording though?), but the added tests in the last commit clearly show they can happen, and when they do, using Expr to reach for their values causes Environment.up panics. Taking a look at the TestExpr example a: string if true { v: a } the node with UpCounts is: { a: string if true { v: 〈1;a〉 } } The Expr method takes v.Lookup("v") as that is how the tests are set up. It grabs the single conjunct for that vertex, the "if" comprehension, and uses its environment along with its Expr to continue. The environment corresponds to where the comprehension sits, meaning it belongs to the root struct and has no parent environment, whereas Expr gave us the underlying FieldReference it produces, which ends up calling Environment.up with an UpCount of 1, since the reference to "a" goes up one level due to the comprehension. It seems like mixing Conjunct.Env with Conjunct.Expr is thus invalid for the cases where the conjunct contains a comprehension. In such cases, Env will contain the comprehension's outer environment, but Expr will contain the comprehension's inner value, so the Environment and UpCount are off-by-one. Pairing Conjunct.Env with Conjunct.Elem instead fixes the UpCount panic, as now they both correspond to the comprehension's outer environment. It's not clear to me whether this is the right fix beyond solving the immediate panic, though. Perhaps for comprehensions we _should_ use Expr to reach for the inner value, but then pair that inner value with the inner Environment somehow. Fixes #2911, possibly. Signed-off-by: Daniel Martí <mvdan@mvdan.cc> Change-Id: Id992f79b1489dc731d8d0172ae5bca159c272587
Not doing so could result in bad dereferences. Closes #2911 Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com> Change-Id: Iff82628f3475c75b35e030569eb87e0363d66ac8
Not doing so could result in bad dereferences. Closes #2911 Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com> Change-Id: Ibecd4738115456d408f6c7eb505fd00c66e9d3f0
Not doing so could result in bad dereferences. CL https://cuelang.org/cl/529517 introduced a way to access the expression associated with a comprehension. In cases where this expression was evaluated within the original Environment of the Comprehension, this resulted in a panic due to an unaligned Environment. The problem was that this value expression needed to be evaluated using an adjusted Environment that accounts for the interstitial scopes introduced by the for and let clauses. In general, these scopes cannot be added ahead of time, as the associated values are not known before evaluation. Like is done in other parts of the code, the new EnvExpr method and function adjusts the Environment by inserting dummy scopes that allow references in the expression that reference values outside of the comprehension to be resolved properly. The old Expr method and function are retained for use cases that only need to refer to the value expression without the need to evaluate or resolve it. Closes #2911 Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com> Change-Id: Ibecd4738115456d408f6c7eb505fd00c66e9d3f0
@sahroshan @kajogo777 @shortwavedave please let us know if you still encounter this panic :) |
Not doing so could result in bad dereferences. CL https://cuelang.org/cl/529517 introduced a way to access the expression associated with a comprehension. In cases where this expression was evaluated within the original Environment of the Comprehension, this resulted in a panic due to an unaligned Environment. The problem was that this value expression needed to be evaluated using an adjusted Environment that accounts for the interstitial scopes introduced by the for and let clauses. In general, these scopes cannot be added ahead of time, as the associated values are not known before evaluation. Like is done in other parts of the code, the new EnvExpr method and function adjusts the Environment by inserting dummy scopes that allow references in the expression that reference values outside of the comprehension to be resolved properly. The old Expr method and function are retained for use cases that only need to refer to the value expression without the need to evaluate or resolve it. Closes #2911 Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com> Change-Id: Ibecd4738115456d408f6c7eb505fd00c66e9d3f0 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1186144 Reviewed-by: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: CUEcueckoo <cueckoo@gmail.com> Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com> (cherry picked from commit 3e6a8be)
Not doing so could result in bad dereferences. CL https://cuelang.org/cl/529517 introduced a way to access the expression associated with a comprehension. In cases where this expression was evaluated within the original Environment of the Comprehension, this resulted in a panic due to an unaligned Environment. The problem was that this value expression needed to be evaluated using an adjusted Environment that accounts for the interstitial scopes introduced by the for and let clauses. In general, these scopes cannot be added ahead of time, as the associated values are not known before evaluation. Like is done in other parts of the code, the new EnvExpr method and function adjusts the Environment by inserting dummy scopes that allow references in the expression that reference values outside of the comprehension to be resolved properly. The old Expr method and function are retained for use cases that only need to refer to the value expression without the need to evaluate or resolve it. Closes #2911 Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com> Change-Id: Ibecd4738115456d408f6c7eb505fd00c66e9d3f0 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1186144 Reviewed-by: Daniel Martí <mvdan@mvdan.cc> TryBot-Result: CUEcueckoo <cueckoo@gmail.com> Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com> (cherry picked from commit 3e6a8be) Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1190979 Reviewed-by: Paul Jolly <paul@myitcv.io> TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
What version of CUE are you using (
cue version
)?$ go version
go version go1.21.0 darwin/arm64
Does this issue reproduce with the latest stable release?
yes
What did you do?
my_cues/stack/stack.cue
main.go
to go through the cue and check for any expression.What did you expect to see?
expected to see
false
when checking the operation.What did you see instead?
panic
below are the combinations i tried which failed
The text was updated successfully, but these errors were encountered: