Skip to content
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.

Sign up for GitHub

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

Jump to bottom

eval: matchN unexpectedly passes validation without evalv3 #3418

Closed
rogpeppe opened this issue Sep 2, 2024 · 0 comments
Closed

eval: matchN unexpectedly passes validation without evalv3 #3418

rogpeppe opened this issue Sep 2, 2024 · 0 comments
Labels
encoding evalv3-win Issues resolved by switching from evalv2 to evalv3 jsonschema / openapi NeedsFix

Comments

@rogpeppe
Copy link
Member

rogpeppe commented Sep 2, 2024

What version of CUE are you using (cue version)?

$ cue version
1440b9eae7fa9ea40133418df0e5bacdcca29cb4

Does this issue reproduce with the latest stable release?

N/A

What did you do?

env CUE_EXPERIMENT=evalv3
! exec cue vet x.cue

env CUE_EXPERIMENT=''
! exec cue vet x.cue

-- x.cue --
"foo" & matchN(1, [1&2]) & matchN(1, [_])

What did you expect to see?

Passing test. The CUE in x.cue should fail validation in both the old and new evaluators.

What did you see instead?

> env CUE_EXPERIMENT=evalv3
> ! exec cue vet x.cue
[stderr]
0: conflicting values 2 and 1:
    ./x.cue:1:20
    ./x.cue:1:22
[exit status 1]
> env CUE_EXPERIMENT=''
> ! exec cue vet x.cue
FAIL: /tmp/y.txtar:5: unexpected command success

Validation unexpectedly succeeds under the old evaluator.
cueckoo pushed a commit that referenced this issue Sep 3, 2024
@rogpeppe
encoding/jsonschema: encode oneOf etc using matchN
bfce150 
This CL updates JSON Schema generation to use the newly added
`matchN` primitive for `allOf`, `anyOf` and `oneOf`.

This has a much closer correlation with the JSON Schema primitives
than the current approach of using CUE's `&` and `|` operators.
Specifically:
- the schema arguments should not affect the final result other
than to validate it, but both `&` and `|` can affect the result.
- the result could become non-concrete due to `|` ambiguity in `anyOf`.

Although this does fix a bunch of issues, there are some regressions.
See issues #3418, #3420, #3422 for details.

Comparative test stats (pre-matchN before post-matchN):

```
v2:
	schema extract (pass / total): 971 / 1637 = 59.3%
	schema extract (pass / total): 975 / 1637 = 59.6%
	tests (pass / total): 3081 / 7175 = 42.9%
	tests (pass / total): 3140 / 7175 = 43.8%
	tests on extracted schemas (pass / total): 3081 / 3542 = 87.0%
	tests on extracted schemas (pass / total): 3140 / 3546 = 88.6%

v3:
	schema extract (pass / total): 971 / 1637 = 59.3%
	schema extract (pass / total): 967 / 1637 = 59.1%
	tests (pass / total): 3063 / 7175 = 42.7%
	tests (pass / total): 3074 / 7175 = 42.8%
	tests on extracted schemas (pass / total): 3063 / 3542 = 86.5%
	tests on extracted schemas (pass / total): 3074 / 3538 = 86.9%
```

This change also requires that we update the CI generated code
and remove the workaround for the previous `oneOf` limitation.

Also add a `brokenInV2` tag to the encoding/jsonschema tests to work
around the fact that some V2 tests are now broken.

For #3380
For #3165

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I2630a6d2b1614b2479802e788c16249d2cf4aa6b
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1200526
Reviewed-by: Daniel Martí <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
@rogpeppe rogpeppe changed the title eval: matchN unexpectedly passes validation under evalv3 eval: matchN unexpectedly passes validation without evalv3 Sep 5, 2024
cueckoo pushed a commit that referenced this issue Sep 5, 2024
@mpvl
internal/core/adt: add tests for validator dedupping
2662ebe 
Issue #3418

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: Id499a84ed80f18eb5ec8efdc0b2cda1a54c07188
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1200736
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Roger Peppe <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
@cueckoo cueckoo closed this as completed in 86fdd97 Oct 8, 2024
@mvdan mvdan mentioned this issue Oct 9, 2024
Closed
vanhtuan0409 pushed a commit to anduintransaction/cue that referenced this issue Oct 15, 2024
@mpvl @vanhtuan0409
internal/core/adt: fix simplifcation of validators
eea7e07 
Now Validators can be NonConcrete, arguments are
not necessarily fully evaluated. This is now addressed.

The also prevents other issues in an upcoming CL.

Issue cue-lang#3165
Fixes cue-lang#3418

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: Ie103c9710eb9a89ee796a2486a9f4d365760f404
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202211
Reviewed-by: Matthew Sackman <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
encoding evalv3-win Issues resolved by switching from evalv2 to evalv3 jsonschema / openapi NeedsFix
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@rogpeppe