-
Notifications
You must be signed in to change notification settings - Fork 301
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
"incomplete value" regression in evalv3 which appears to be order dependent #3638
Comments
Reduced from a failure in Holos as reported by @jeffmccune. Note that #3637 was a spin-off from the example above, but a different bug entirely. |
There appear to be two issues going on: somewhere a closedness value is erased, allowing the structs in a disjunction to be unified that otherwise fails. However, for the location where the error occurs, V2 actually suffers from the same problem. One could argue that the embedding, in that case, should actually allow it and that it is not an issue. Either way, in one other location V3 does suffer from this issue. The other issue is that the default marker is incorrectly erased. A reduction that focusses on that issue is:
In this case, out.foo will have a default marker for V2, but not V3, causing the value to be incomplete. The closedness issue is exposed by:
This issue only occurs when DebugDebs is disabled!, which is a good hint as to where to start debugging. |
The default issue can be analyzed as follows. Comprehensions are treated as embeddings. Each Let's focus on the former. Embedding So far so good. This is the same for both V2 and V3. Where they differ is where
which is
which is V2 returns V3 currently returns I think the correct answer should be The motivation for this is that unification is idempotent, but embedding is not. So to get the correct result, one should distribute the unifications over disjunctions before embedding them. Note that if we replace Taking this stance would mean that V3 will differ from V2, although it would no longer result in an incomplete value. Judging from the particular meaning here, I reckon that will be acceptable. Note that V2 is inconsistent with itself: if we unify a pattern constraint with a single disjunction it is most eager. |
The main question that arises for me here looking at this example:
is "what was the intent of the author?" I ask that in light of my observations in #3296 (comment). i.e. the CUE written by the author does not necessarily capture their intent, for whatever reason:
Through one lens, the expected result might be, in rather more colloquial terms: " I ask this question about the original intent because without properly understanding that intent I can't be sure that the user intended the following implications:
These implications are not obvious to my mind, as demonstrated by the level of analysis required in this issue. I ask this question and raise these points because I wonder whether we can actually perform a neat side-step here. Because if the user actually intended something that doesn't require that we answer what the result of the presented code should be, then is there a simpler solution? Indeed can we leave this behaviour unspecified (or an error in case we can detect certain edge cases)? |
In that case, my suggested correct semantics would indeed get closest to that.
That is not how closedness works. The closedness is temporarily not enforced, a small but important distinction.
Well, there is a factor of correctness. I've identified the problem that causes V3 to treat After more analysis it is quite clear that this is the correct result. |
For more info see https://cuelang.org/cl/1207410/2. |
Note that the added explanation explains the diff for after the upcoming fix. Issue #3638 Signed-off-by: Marcel van Lohuizen <[email protected]> Change-Id: Iccc5d04f2fb59560a1d715d254a21dfde1c87f2c Reviewed-on: https://gerrithub.io/c/cue-lang/cue/+/1207409 Unity-Result: CUE porcuepine <[email protected]> Reviewed-by: Daniel Martí <[email protected]> TryBot-Result: CUEcueckoo <[email protected]>
For posterity, here is a runtime visualization of just before the disjunction is computed of
and
The graph with the pattern constraint case is clearly broken (if you know what to look for). Most notably, the ARC link is missing. This causes a whole path (red) to be ignored in the disjunction. The ARC was missing because it erroneously got deduped. Once that was fixed it behaved exactly as the regular field case, as one would expect. |
input: k1: [string]: a: 1
input: k1: foo: #x
out: {
for x in input { x }
for x in input { x }
}
#x: { a: int } | *{ b: 2 } Let me see if I can clarify. At the highest level, I want to manage a Kubernetes The schema was obtained from The schema Note in my own httproutes.cue I don't use defaults, so I'm unclear where those are coming from in the example you provided. There are quite a few places in the imported schema where they're used, searching for All this to say: If the issue can be attributed to the imported schema and not my own use of that schema, I'd love to understand because I'm able to replace the schema with a different one that avoids the problem. Let me know if this helps clarify the intent enough. If not I can try again and break it down further. Versions:
|
Also, the double embed isn't really intentional, it was more simply for readability:
The two get combined together here: https://github.com/holos-run/kargo-demo/blob/v0.0.12/stacks/network/components/httproutes/httproutes.cue#L181 This is perhaps more accidental than intentional, but I do want the act of composing into |
Hrm, let me try again point by point:
Any double embedding is accidental, not my intent. Assuming
Did not intend this as per above, intent is to vet against v1.#HTTPRoute
I wasn't aware.
I've been assuming embedding is idempotent. |
@mvdan was going to follow up with you, but these hugely detailed replies give us plenty to work through. Thank you very much! |
He did, thanks... I'm concerned they're counter-productive so please let me know if that's the case are and I'll edit them down or delete them entirely. |
They are absolutely not counter productive. Your responses are exactly what we need to hear (above and beyond fixing the core evaluator, and that's the parallel conversation with Marcel's points in this issue). So to reiterate I'm genuinely very grateful for you spending the time to add this detail. In case jumping onto a call is quicker/easier from your perspective please say. Whilst writing is often a good way to order thoughts etc, for my part I'm conscious that I don't unduly impinge on your time! |
Note that the current fix for V3 makes the double embedding essentially idempotent in this case. In general, with this change if a field has a disambiguated disjunction, it will not get "disambiguated" by embedding. We are thinking of a new way to do defaults uncoupled from disjunctions to simply things a bit. But that is a bit down the line. These cases help shape our thoughts there. |
Indeed, and that is what I was trying to convey in "in the declaration of
with 053f47b gives:
Absolutely. My observation here was certainly not to question or stop that analysis. Because it's critical. My observation was that, like #3296, it's not always clear (to me at least) that the original intent is necessarily captured by the written CUE. So in parallel to us fixing/etc the evaluator, I was looking to dig a bit deeper on the goal/intent front. @jeffmccune has kindly provided more details that I'm going to work through. |
As of
46fc54aa9caf95393dcd3fe0ac066c8112da986d
:See the comments; removing one repetitive definition embedding, or moving the last two fields to the top of the file, both make the error go away. So it seems like there's some rather nasty bug in evalv3 that causes order-dependent or inconsistent evaluation.
The text was updated successfully, but these errors were encountered: