-
Notifications
You must be signed in to change notification settings - Fork 312
evalv3: stackoverflow when using the function pattern #3731
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
Comments
Reduced a bit:
As of 0e42b0b:
|
Two further observations regarding the reduction in #3731 (comment):
|
This turns out to be quite an interesting one. The problem can be reduced to something not using the function pattern:
|
cueckoo
pushed a commit
that referenced
this issue
Feb 27, 2025
We want to do the same processing for Evaluator as for a Resolver, if the former resolves to a Vertex. We refactor the code in a separate CL to make the diff smaller later. Issue #3731 Issue #3634 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: I33e300b9c873ebf10d4e40bbe8f507c74f3bc426 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1209384 TryBot-Result: CUEcueckoo <[email protected]> Reviewed-by: Matthew Sackman <[email protected]>
cueckoo
pushed a commit
that referenced
this issue
Feb 27, 2025
With the structural cycle detection rework, this no longer seems necessary. As it is safer to be non-permissive, we do not set this flag. We leave it in, however, to quickly be able to set it if an issue bisects to this change. Issue #3731 Issue #3634 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: Ie411bf9eca647e675e5a74b12245175e7aa0909c Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1209385 Reviewed-by: Matthew Sackman <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>
cueckoo
pushed a commit
that referenced
this issue
Feb 27, 2025
By caching inline expressions, we can treat them more like regular fields, which will, in turn, allow simplifying the structural cycle handling. Note that some of the dependency analysis results have changed. In let2 the ones dropped are for non-existent references, which seems fine. For selfref.txtar, it seems like this should be fixed, although V2 isn't grand either. Issue #3731 Issue #3634 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: I0e30f5dec08c8e120e5d24ad3115fce26f565fec Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1209386 Reviewed-by: Matthew Sackman <[email protected]> Unity-Result: CUE porcuepine <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>
cueckoo
pushed a commit
that referenced
this issue
Feb 27, 2025
…sharing In some cases, an ancestor node may be shared in a child node. This bypasses the usual structure sharing checks. This adds the check to catch this edge case. We should probably find something more principled down the line. This problem was discovered as an issue when debugging Issue #3731 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: Ib5349a113088f82dfcb0c982a9ff0957c8af545f Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1209390 Reviewed-by: Matthew Sackman <[email protected]> TryBot-Result: CUEcueckoo <[email protected]> Unity-Result: CUE porcuepine <[email protected]>
Confirming that as of c479844 (which is now in tip) this still passes as expected, but also that we do not need to set
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
What version of CUE are you using (
cue version
)?Does this issue reproduce with the latest stable release?
Yes
What did you do?
Ran the following test script:
What did you expect to see?
The same result as the one generated by the old evaluator:
Passing test.
What did you see instead?
A panic log from the Go runtime:
If instead of using the function pattern the code inside
#lookupSiblings.out
is put at the call site, then there's no stack overflow panic and the new evaluator correctly evaluatestree
.cc @gotwarlost
The text was updated successfully, but these errors were encountered: