Skip to content

evalv3: structural cycle regression involving a builtin and repeated schemas #3634

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

Closed
mvdan opened this issue Dec 24, 2024 · 2 comments
Closed
Labels
evaluator evalv3 issues affecting only the evaluator version 3 unity-win bugs found thanks to projects added to Unity

Comments

@mvdan
Copy link
Member

mvdan commented Dec 24, 2024

# With the old evaluator.
env CUE_EXPERIMENT=evalv3=0
exec cue export

# With the new evaluator.
env CUE_EXPERIMENT=evalv3=1
exec cue export

-- input.cue --
package p

import "list"

#Schema: {
	required?: [...string] 
	properties?: [string]: null | #Schema
}

out: len([...#Schema] & list.Repeat([{
	#Schema // removing this fixes the bug!
	properties: foo: required: ["bar", "baz"]
}], 3))

As of f2c38f8:

# With the old evaluator. (0.011s)
> env CUE_EXPERIMENT=evalv3=0
> exec cue export
[stdout]
{
    "out": 3
}
# With the new evaluator. (0.033s)
> env CUE_EXPERIMENT=evalv3=1
> exec cue export
[stderr]
0.properties.foo: cannot combine regular field "required" with null:
    ./input.cue:7:25
0.properties.foo: conflicting values null and {required:["bar","baz"]} (mismatched types null and struct):
    ./input.cue:7:25
    ./input.cue:12:19
0.properties.foo: structural cycle:
    ./input.cue:7:32
0.properties.foo: 3 errors in empty disjunction::
    ./input.cue:10:25
[exit status 1]

Note that removing the embedded definition resolves the regression, which is very suspect.

Also note that this example is very similar to #3633.

@mvdan mvdan added Triage Requires triage/attention evaluator evalv3 issues affecting only the evaluator version 3 and removed Triage Requires triage/attention labels Dec 24, 2024
@mpvl
Copy link
Member

mpvl commented Jan 7, 2025

A slightly simplified reproducer:

import "list"

#D: {
	b: int
	a: null | #D
}

out: len(#D & list.Repeat([#D & {a: b: 1}], 1)[0])

@myitcv myitcv added tmp/eval and removed tmp/eval labels Feb 15, 2025
@mpvl
Copy link
Member

mpvl commented Feb 19, 2025

For reference, here is a reproducer that does not use disjunctions or embeddings:

#D: {
	b?: int
	a?: #D
}
out: len(#D & list.Repeat([#D & { a: b: 1 }], 1)[0])

@mvdan mvdan added the unity-win bugs found thanks to projects added to Unity label Feb 25, 2025
cueckoo pushed a commit that referenced this issue Feb 27, 2025
Issue #3634

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I6493a5761af45e218064b88a98c24957b40e5fa8
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1209383
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Matthew Sackman <[email protected]>
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluator evalv3 issues affecting only the evaluator version 3 unity-win bugs found thanks to projects added to Unity
Projects
None yet
Development

No branches or pull requests

3 participants