cuelang / cue

CUE has moved to https://github.com/cue-lang/cue
https://cuelang.org
Apache License 2.0
3.09k stars 171 forks source link

cmd/cue: reference error in value imported from a package lost during export #854

Closed uhthomas closed 3 years ago

uhthomas commented 3 years ago

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

$ cue version
cue version v0.3.0-beta.7 linux/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

! exec cue export
cmp stderr stderr.golden

-- a.cue --
package a

import "mod.com/b"

theb: b.name
-- b/b.cue --
package b

import "mod.com/c"

b: c.c & {
    other: "name"
}

name: b.other
-- c/c.cue --
package c
-- cue.mod/module.cue --
module: "mod.com"
-- stderr.golden --
b: undefined field c:
    ./b/b.cue:5:6

With beta.3 this test passes; with beta.4 (or indeed any commit including bedb047a988217bab040c29eeb8a51988ce23b76) fails with:

> ! exec cue export
[stdout]
{
    "theb": "name"
}
FAIL: /tmp/testscript235319619/repro.txt/script.txt:1: unexpected command success

What did you expect to see?

A passing test.

What did you see instead?

Per above; a failing test, because cue export succeeds where it should not.

Edit: updated description to the contents of https://github.com/cuelang/cue/issues/854#issuecomment-808683933 for ease of reference.

uhthomas commented 3 years ago

The following snippet works as expected.

import networkingv1 "k8s.io/api/networking/v1"

network_policy: [...networkingv1.#NetworkPolicy]

network_policy: [{
        this_field: "should not work"
}]

Output

network_policy.0: field "this_field" not allowed in closed struct:
    ./test.cue:6:21
verdverm commented 3 years ago

@uhthomas Can you try with CUE v0.3.0-beta.7?

There have been significant changes since v0.2.x and we are nearing a v0.3.0 release

Could you also show a more complete example (the NetworkPolicyList is missing above`)

Preferably, if you could create a reproducer with beta.7 using the guide found at this link, that would be great.

https://github.com/cuelang/cue/wiki/Creating-test-or-performance-reproducers

uhthomas commented 3 years ago

Yes, sorry.

I was actually using v0.3.0-beta.5. Have confirmed the same behavior with v0.3.0-beta.7.

The NetworkPolicyList is gettable via cue get go k8s.io/api/networking/v1.

For convenience however, please refer to automata to see the generated definition.

verdverm commented 3 years ago

is the network_policy: networkingv1.NetworkPolicyList & { (without the # for a definition a typo?)

verdverm commented 3 years ago

This appears to have your desired output, both cases fail. I believe this represents the situation

cue eval
-- cue.mod/module.cue --
module: ""
-- cue.mod/pkg/foo.bar/lib/main.cue --
package lib

#A: {
        foo: string | *"bar"
}

#L: {
        name: string
        items: [...#A]
}
-- main.cue --
package repro

import (
        "foo.bar/lib"
)

ex1: lib.#L & {
        name: "ex1"
}

ex1: items: [{
        this_field: "should not work"
}]

ex2: [...lib.#A]
ex2: [{
        this_field: "should not work"
}]

ex1.items.0: field this_field not allowed:
    ./cue.mod/pkg/foo.bar/lib/main.cue:3:5
    ./cue.mod/pkg/foo.bar/lib/main.cue:9:13
    ./main.cue:7:6
    ./main.cue:12:2
ex2.0: field this_field not allowed:
    ./cue.mod/pkg/foo.bar/lib/main.cue:3:5
    ./main.cue:15:10
    ./main.cue:17:2

for completeness, I also tried moving the cue.mod/pkg/ to cue.mod/gen/ and had the same results

uhthomas commented 3 years ago

You're correct, I was missing the #. 😬

Thank you.

Although this was definitely a confusing typo, I still imagine CUE probably shouldn't compile under these conditions.

Both the following do not compile:

a: [...A]
#A: {}

a: [...A]

As such, I would have expected my example earlier to also not compile.

uhthomas commented 3 years ago

Although interestingly, when packages get involved, it does...

cue eval
-- cue.mod/module.cue --
module: ""
-- cue.mod/pkg/foo.bar/lib/main.cue --
import "foo.bar/lib/other"

[...other.A]
-- cue.mod/pkg/foo.bar/lib/other/other.cue --
package other
verdverm commented 3 years ago

cue export will likely fail, cue eval prints an incomplete object. This is because imported packages are open, there's a comment from @mpvl about considering changing this, but that is why an error is not occuring unless you ask for a concrete value.


removing the #'s from my example:

cue eval

import "foo.bar/lib"

ex1: lib.L & {
    name: "ex1"
} & {
    items: [{
        this_field: "should not work"
    }]
}
ex2: [{
    this_field: "should not work"
} & lib.A]

cue export

ex1: undefined field L:
    ./main.cue:7:10
ex2.0: undefined field A:
    ./main.cue:15:14
uhthomas commented 3 years ago

Have just tested and cue export doesn't fail, which is consistent with what I was seeing earlier.

I assume it should?

verdverm commented 3 years ago

I'd guess there is another typo?

I cannot repro given the examples above which are producing the expected results

uhthomas commented 3 years ago

I didn't populate it was any data, so it looks like it didn't fully evaluate in my example.

Pass:

import networkingv1 "k8s.io/api/networking/v1"

a: [...networkingv1.NetworkPolicyList]

Fail:

import networkingv1 "k8s.io/api/networking/v1"

a: [...networkingv1.NetworkPolicyList]

a: [{}]

Not sure if that is a bug or expected behavior.

uhthomas commented 3 years ago

I definitely see that it was exported incorrectly, as kubectl complains about it.

For context, I'm using Bazel and rules_cue which seems to use cue def, so maybe it gets skipped?

verdverm commented 3 years ago

cue def does not enforce concreteness, maybe that should be a cue export

uhthomas commented 3 years ago

It looks like it ends up collecting all those defs and then exports them. See here. I wonder if something in the chain breaks it.

uhthomas commented 3 years ago

Yeah, so cue export does fail as expected when the # is removed, but exporting with Bazel works okay.

I am inclined to say this probably isn't a fault with rules_cue simply because it's using the standard cue tool under the hood.

Should cue def have better enforcement of correctness?

verdverm commented 3 years ago

I'm not familiar with Bazel

In https://github.com/uhthomas/rules_cue/blob/master/cue/cue.bzl

with the dep / merge stuff

is that

  1. for each dep, cue def, zip
  2. for each dep, zip... later cue def the entire tree with the root?

cue def is not meant to enforce concreteness by default (or at all?). You might look at cue vet -c as a check

uhthomas commented 3 years ago

Okay, have done some playing and it looks like cue def behaves really weirdly.

Given the CUE file

import networkingv1 "k8s.io/api/networking/v1"

a: networkingv1.NetworkPolicyList & {}

Fail

$ cue def test.cue -o d.json
a: undefined field NetworkPolicyList:
    ./test.cue:3:17

Pass (incorrect)

cue def test.cue > d.json

d.json then contains an invalid definition.

verdverm commented 3 years ago

in the first case, you are asking CUE to output JSON, which requires a concrete value

in the second case, you are asking the shell to put CUE's output into a file called d.json .There is no type information here and cue def defaults to outputting CUE code

verdverm commented 3 years ago

Note, there are inconsistencies in CUE's commands which have a work item to clean them up and make them consistent. I'm not finding a specific issue, but it has been raised in comments somewhere

mpvl commented 3 years ago

@uhthomas what commands where your running when you say "compile"?

Yes, it should fail. The faulty v1.NetworkingPolicyList results in an incomplete error because v1 exists and is not closed (arguably imports should be implicitly closed, as they are fully resolved at the time they are used, but that is another topic). This means it does not generate an error as it could theoretically still be resolved later.

However, at some point it should change that temporary error into a permanent error, which seems to fail to happen here.

The def behavior is explainable: for generating CUE using def incomplete errors are ignored, as they could still be satisfied later by making a configuration more concrete. In this case, of course, that would require making a package more concrete, which is maybe being too optimistic.

Using cue def -o x.json, however, exporting it to a json file, effectively turns cue def into an export. CUE is aware of the filetype you're exporting to here, and knows. Note that for cue def > x.json CUE is not aware of the file type: in this case it is a file redirect, which is a shell feature. In this case CUE outputs to CUE, and since it is cue def, permitting incomplete errors.

There should be an open issue related to this, but cannot find it. @myitcv?

uhthomas commented 3 years ago

Okay, it took a little bit of playing but I have got a working example of cue export misbehaving.

Please refer to https://github.com/uhthomas/cue854 which explains in detail.

myitcv commented 3 years ago

@uhthomas - thanks for reproducing this issue.

@mpvl the related issue that comes to mind is https://github.com/cuelang/cue/issues/629, although that's a slightly different context because it's talking about disjunctions masking issues/errors.

In this case I think there is actually a genuine error here. The regression happened as part of bedb047a988217bab040c29eeb8a51988ce23b76, so post-beta.3:

! exec cue export
cmp stderr stderr.golden

-- a.cue --
package a

import "mod.com/b"

theb: b.name
-- b/b.cue --
package b

import "mod.com/c"

b: c.c & {
    other: "name"
}

name: b.other
-- c/c.cue --
package c
-- cue.mod/module.cue --
module: "mod.com"
-- stderr.golden --
b: undefined field c:
    ./b/b.cue:5:6

With beta.3 this passes; with beta.4 (or indeed any commit including bedb047a988217bab040c29eeb8a51988ce23b76) fails with:

> ! exec cue export
[stdout]
{
    "theb": "name"
}
FAIL: /tmp/testscript235319619/repro.txt/script.txt:1: unexpected command success

I'll update the description with this repro to save others have to read through the entire thread to get to the punchline.

cueckoo commented 3 years ago

This issue has been migrated to https://github.com/cue-lang/cue/issues/854.

For more details about CUE's migration to a new home, please see https://github.com/cue-lang/cue/issues/1078.