cue: optional fields cannot be referenced in for loops of optional fields

FogDong commented 2 years ago

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

$ cue version
0.4.3

Does this issue reproduce with the latest release?

yes

What did you do?

parameter: {
    optioanlArr?: [...{
        name:  string
        data?: string
    }]
}

outputs: {
    for v in parameter.optionalArr if v.data != _|_  {
        name: v.name
    }
}

What did you expect to see?

This is also the result in cue 0.2.2:

outputs: {
    for v in parameter.optionalArr if false {
        name: v.name
    }
}

What did you see instead?

outputs: {
    for v in parameter.optionalArr if v.data != _|_ // explicit error (_|_ literal) in source
    {
        name: v.name
    }
}

Nor-arc commented 2 years ago

Not sure exactly what you're trying to accomplish without more context, but would something simaliar to this solve what you're trying to do?

FogDong commented 2 years ago

Not sure exactly what you're trying to accomplish without more context, but would something simaliar to this solve what you're trying to do?

Thanks for the reply.

What I tried to do is to reference an optional field in for loops of optional fields like here

The problem is that it will report an error in v0.4.3 but works fine in v0.2.2, I tried to upgrade our cue version from v0.2.2 to v0.4.3 but this problem is like a breaking change for our users who're using cue.

Similar to this issue: #715

FogDong commented 2 years ago

Currently we're using this workaround to fix the problem: kubevela/cue#2 It's not elegant but it works...😞

mpvl commented 2 years ago

We are close to committing a first version of both the reworked cycle detection algorithm and comprehension rewrite.

I'll keep this on the list of things to validate in rolling this out.

myitcv commented 2 years ago

I'm not quite sure I follow what the issue is here.

Looking at the example in the issue description:

exec cue eval x.cue
cmp stdout stdout.golden

-- x.cue --
parameter: {
    optioanlArr?: [...{
        name:  string
        data?: string
    }]
}

outputs: {
    for v in parameter.optionalArr if v.data != _|_ {
        name: v.name
    }
}
-- stdout.golden --
parameter: {}
outputs: {
    for v in parameter.optionalArr if v.data != _|_ // explicit error (_|_ literal) in source
    {
        name: v.name
    }
}

Note the field name optioanlArr is spelled different to the reference optionalArr. Was this intentional?

For the rest of my analysis, I assume that was not intentional, and so fix the reference.

Just to confirm, that even with that fix, the error from cue eval remains:

exec cue eval x.cue
cmp stdout stdout.golden

-- x.cue --
parameter: {
    optionalArr?: [...{
        name:  string
        data?: string
    }]
}

outputs: {
    for v in parameter.optionalArr if v.data != _|_ {
        name: v.name
    }
}
-- stdout.golden --
parameter: {}
outputs: {
    for v in parameter.optionalArr if v.data != _|_ // explicit error (_|_ literal) in source
    {
        name: v.name
    }
}

For completeness, let's look at the result from cue export:

! exec cue export x.cue
cmp stdout stdout.golden
cmp stderr stderr.golden

-- x.cue --
parameter: {
    optionalArr?: [...{
        name:  string
        data?: string
    }]
}

outputs: {
    for v in parameter.optionalArr if v.data != _|_ {
        name: v.name
    }
}
-- stdout.golden --
-- stderr.golden --
outputs: cannot reference optional field: optionalArr:
    ./x.cue:9:21

This is as expected, because optionalArr is optional.

I don't think we should be showing an error here from cue eval, but as I demonstrate below this is not a fatal error. In the case that a value for parameter.optionalArr is provided, the "error" disappears. Nor is there a problem in case the access to parameter.optionalArr is guarded by a != _|_ check, which it probably should be because parameter.optionalArr is itself optional (as demonstrated by cue export).

Firstly a demonstration that providing an actual value for parameter.optionalArr causes the "problem" to go away:

exec cue eval x.cue y.cue
cmp stdout stdout.golden

-- x.cue --
parameter: {
    optionalArr?: [...{
        name:  string
        data?: string
    }]
}

outputs: {
    for v in parameter.optionalArr if v.data != _|_ {
        name: v.name
    }
}
-- y.cue --
parameter: optionalArr: []

-- stdout.golden --
parameter: {
    optionalArr: []
}
outputs: {}

Instead of providing a value for parameter.optionalArr, let's guard access to the reference:

exec cue eval x.cue
cmp stdout stdout.golden

-- x.cue --
parameter: {
    optionalArr?: [...{
        name:  string
        data?: string
    }]
}

outputs: {
    if parameter.optionalArr != _|_ for v in parameter.optionalArr if v.data != _|_ {
        name: v.name
    }
}
-- stdout.golden --
parameter: {}
outputs: {}

I think this just leaves us to confirm that this kind of comprehension construct behaves as expected in various different use cases. Note how I have guarded access to the optionalArr in each example case here:

exec cue eval x.cue
cmp stdout stdout.golden

-- x.cue --
examples: [string]: {
    optionalArr?: [...{
        name:  string
        data?: string
    }]
}

examples: eg1: {}
examples: eg2: {
    optionalArr: []
}
examples: eg3: {
    optionalArr: []
}

examples: eg4: {
    optionalArr: [{
        name: "name4"
    }]
}

examples: eg5: {
    optionalArr: [{
        name: "name5"
        data: "data5"
    }]
}

outputs: {
    for k, eg in examples {
        (k): {
            if eg.optionalArr != _|_ for v in eg.optionalArr if v.data != _|_ {
                name: v.name
            }
        }
    }
}
-- stdout.golden --
examples: {
    eg1: {}
    eg2: {
        optionalArr: []
    }
    eg3: {
        optionalArr: []
    }
    eg4: {
        optionalArr: [{
            name: "name4"
        }]
    }
    eg5: {
        optionalArr: [{
            name: "name5"
            data: "data5"
        }]
    }
}
outputs: {
    eg1: {}
    eg2: {}
    eg3: {}
    eg4: {}
    eg5: {
        name: "name5"
    }
}

I might have totally missed the point of this issue, in which case please disregard the above and help me understand what I misunderstood!

Otherwise, I think the issue here is that we are reporting a non-fatal error (via cue eval) when really there shouldn't be one. cue eval is tolerant of access to undefined optional fields. But that otherwise this should not affect cmd/cue or the Go API.

myitcv commented 2 years ago

Or perhaps it's the fact that the error is reported that is causing you problems? i.e. this error gets seen by the user, indicating there is a problem when in fact there is not?

FogDong commented 2 years ago

Note the field name optioanlArr is spelled different to the reference optionalArr. Was this intentional?

Sorry that's a typo.

I know that we can add default value for optionalArr or add if optionalArr != _|_ to resolve the problem, the reason I created this issue is that the behaviour is inconsistent in v0.2.2 and v0.4.3.

In v0.4.3, the result is:

outputs: {
    for v in parameter.optionalArr if v.data != _|_ // explicit error (_|_ literal) in source
    {
        name: v.name
    }
}

In v0.2.2, the result is:

outputs: {
    for v in parameter.optionalArr if false {
        name: v.name
    }
}

Our project is using v0.2.2 and trying to upgrade to v0.4.3, in our recognization, we think that in cue, you can reference optional fields in for loops of optional fields because that's allowed in v0.2.2, so the behavior in v0.4.3 is a BREAK CHANGE for our users because the working cue codes will report error after the upgradation.

Yes I think that's probably a problem with cue eval if you think we should not report this error in cue eval. We'll still need to use ResolveReference to get the result in data mode and patch two cue values in data mode, and the error remains. (We'll actually do some modifications in the cue values, like, add some empty struct in cue value ListLit to make the value unifiable, using InlineImports will make the cue struct more complicated so our original code can not work)

I create a example here: https://gist.github.com/FogDong/990c4be6e51e786343e4b3baa052e128

This is a panic case, I try to use a real case in our project to make you understand better, so the cue is a little bit complicated.

myitcv commented 2 years ago

Hi @FogDong

so the behavior in v0.4.3 is a BREAK CHANGE for our users because the working cue codes will report error after the upgradation.

What I'm trying to understand and confirm is how you see this as a breaking change. That will be important to understanding whether this is a genuine regression, or whether what you're seeing is the result of a bug fix for something that should not have worked before. Please can you provide some code that works with v0.2.2, but does not with current tip?

For example, the following shows a change in behaviour between v0.2.2 and current tip:

# v0.2.2
go get cuelang.org/go@v0.2.2
go mod tidy
go run .
stdout 'outputs is not in error'

# current tip
go get cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353
go mod tidy
go run .
stdout 'outputs is not in error'

-- go.mod --
module mod.com

go 1.18

require cuelang.org/go v0.2.2
-- main.cue --
package p

parameter: {
    optionalArr?: [...{
        name:  string
        data?: string
    }]
}

outputs: {
    for v in parameter.optionalArr if v.data != _|_ {
        name: v.name
    }
}
-- main.go --
package main

import (
    "fmt"
    "log"

    "cuelang.org/go/cue"
    "cuelang.org/go/cue/load"
)

func main() {
    bps := load.Instances([]string{"."}, nil)
    v := cue.Build(bps)[0].Value()
    outputs := v.Lookup("outputs")
    if err := outputs.Err(); err != nil {
        log.Fatalf("outputs is in error: %v", err)
    }
    fmt.Printf("outputs is not in error")
}

As written, the test is expected to pass but it does not. Instead it gives:

# v0.2.2 (0.416s)
# current tip (1.814s)
> go get cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353
[stderr]
go: upgraded cuelang.org/go v0.2.2 => v0.4.4-0.20220729051708-0a46a1624353
go: upgraded github.com/emicklei/proto v1.6.15 => v1.10.0
go: upgraded golang.org/x/net v0.0.0-20200226121028-0de0cce0169b => v0.0.0-20220425223048-2871e0cb64e4
go: upgraded golang.org/x/text v0.3.2 => v0.3.7
go: upgraded golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543 => v0.0.0-20200804184101-5ec99f83aff1
go: upgraded gopkg.in/yaml.v3 v3.0.0-20200121175148-a6ecf24a6d71 => v3.0.1

> go mod tidy
> go run .
[stderr]
main.go:16: outputs is in error: outputs: cannot reference optional field: optionalArr (and 1 more errors)
exit status 1

[exit status 1]
FAIL: /tmp/testscript1892563312/repro.txt/script.txt:10: unexpected go command failure
error running repro.txt in /tmp/testscript1892563312/repro.txt

i.e. with current tip (0a46a1624353), we get an error in the outputs field where before we did not.

Please can you provide a similar comparison to show what is breaking for you?

In my example, whilst it appears it is a regression, I believe this is actually a bug fix. i.e. CUE that apparently worked with v0.2.2 should have not, and the behaviour in current tip is correct. Consider:

# current tip
go get cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353
go mod tidy
go run .
stdout 'a_tests.direct: cannot reference optional field'
stdout 'a_tests.comprehension: cannot reference optional field'

# v0.2.2
go get cuelang.org/go@v0.2.2
go mod tidy
go run .
stdout 'a_tests.direct: field "a" is optional'
stdout 'a_tests.comprehension: field "a" is optional'

-- go.mod --
module mod.com

go 1.18

require cuelang.org/go v0.4.4-0.20220729051708-0a46a1624353
-- main.cue --
package p

parameter: {
    a?: int
}

a_tests: {
    comprehension: {
        if parameter.a == 5 {
            5
        }
    }
    direct: parameter.a
}
-- main.go --
package main

import (
    "fmt"
    "strings"

    "cuelang.org/go/cue"
    "cuelang.org/go/cue/load"
)

func main() {
    bps := load.Instances([]string{"."}, nil)
    v := cue.Build(bps)[0].Value()
    for _, elem := range []string{"direct", "comprehension"} {
        path := []string{"a_tests", elem}
        outputs := v.Lookup(path...)
        if err := outputs.Err(); err != nil {
            // path is included in error
            fmt.Printf("%v\n", err)
        } else {
            fmt.Printf("%v is not in error\n", strings.Join(path, "."))
        }
    }
}

Notice how I am now testing current tip first, then v0.2.2. This test fails with:

# current tip (0.507s)
> go get cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353
> go mod tidy
> go run .
[stdout]
a_tests.direct: cannot reference optional field: a
a_tests.comprehension: cannot reference optional field: a (and 2 more errors)

> stdout 'a_tests.direct: cannot reference optional field'
> stdout 'a_tests.comprehension: cannot reference optional field'
# v0.2.2 (0.338s)
> go get cuelang.org/go@v0.2.2
[stderr]
go: downgraded cuelang.org/go v0.4.4-0.20220729051708-0a46a1624353 => v0.2.2
go: added golang.org/x/exp v0.0.0-20200513190911-00229845015e
go: downgraded golang.org/x/mod v0.6.0-dev.0.20220106191415-9b9b3d81d5e3 => v0.2.0
go: downgraded golang.org/x/tools v0.1.10 => v0.0.0-20200612220849-54c614fe050c
go: downgraded golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 => v0.0.0-20191204190536-9bdfabe68543

> go mod tidy
> go run .
[stdout]
a_tests.direct: field "a" is optional
a_tests.comprehension is not in error

> stdout 'a_tests.direct: field "a" is optional'
> stdout 'a_tests.comprehension: field "a" is optional'
FAIL: /tmp/testscript2509366397/repro.txt/script.txt:13: no match for `a_tests.comprehension: field "a" is optional` found in stdout
error running repro.txt in /tmp/testscript2509366397/repro.txt

This shows that current tip is more consistent in reporting errors with respect to referring to optional field access, whereas v0.2.2 masked that error when the access was within a comprehension.

@mpvl will be able to confirm if this indeed a correct bug fix or not as I suspect it is.

Separately in your linked example I see:

// ...

volumesList: pvcVolumesList + configMapVolumesList 

// ...

Note also that since v0.2.2 we have introduce the list builtin for concatenating list values:

https://cuelang.org/play/?id=lE0qm1xysTz#cue@export@cue

In a future release, we will drop support for +, i.e. d will become an error.

We'll still need to use ResolveReference to get the result in data mode and patch two cue values in data mode, and the error remains. (We'll actually do some modifications in the cue values, like, add some empty struct in cue value ListLit to make the value unifiable, using InlineImports will make the cue struct more complicated so our original code can not work)

I think I understand from this that something is not working as you expect with InlineImports()? If so, please can you follow up on that in https://github.com/cue-lang/cue/issues/867?

FogDong commented 2 years ago

Hi Paul @myitcv , thanks for the reply.

I updated my gist: https://gist.github.com/FogDong/990c4be6e51e786343e4b3baa052e128

Sorry for the complicated cue value in the example, in master version of cue, this example will report panic, however, in v0.2.2, this example works fine.

myitcv commented 2 years ago

Thanks, @FogDong. For anyone following along, here is a standalone reproducer:

# v0.2.2
go get cuelang.org/go@v0.2.2
go mod tidy
go run common.go v0.2.2.go

# current tip
go get cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353
go mod tidy
go run common.go tip.go

-- common.go --
package main

import (
    "fmt"

    "cuelang.org/go/cue/ast"
    "cuelang.org/go/cue/token"
)

var storage = `
pvcVolumesList: *[
    for v in parameter.pvc if v.mountPath != _|_{
    {
        name: "pvc-" + v.name
        persistentVolumeClaim: claimName: v.name
    }
},
] | []

configMapVolumesList: *[
        for v in parameter.configMap if v.mountPath != _|_ {
    {
        name: "configmap-" + v.name
        configMap: {
            name:        v.name
        }
    }
},
] | []

volumesList: pvcVolumesList + configMapVolumesList
deDupVolumesArray: [
for val in [
    for i, vi in volumesList {
        for j, vj in volumesList if j < i && vi.name == vj.name {
            ignore: true
        }
        vi
    },
] if val.ignore == _|_ {
    val
},
]

patch: spec: template: spec: {
// +patchKey=name
volumes: deDupVolumesArray
}

parameter: {
// +usage=Declare pvc type storage
pvc?: [...{
    name:       string
    mountPath?: string
}]

// +usage=Declare config map type storage
configMap?: [...{
    name:      string
    mountPath?:  string
}]
}
`

// copy from cuelang.org/go/internal/internal.go
func ToFile(n ast.Node) *ast.File {
    switch x := n.(type) {
    case nil:
        return nil
    case *ast.StructLit:
        return &ast.File{Decls: x.Elts}
    case ast.Expr:
        ast.SetRelPos(x, token.NoSpace)
        return &ast.File{Decls: []ast.Decl{&ast.EmbedDecl{Expr: x}}}
    case *ast.File:
        return x
    default:
        panic(fmt.Sprintf("Unsupported node type %T", x))
    }
}
-- go.mod --
module mod.com

go 1.18

require cuelang.org/go v0.2.2

-- tip.go --
package main

import (
    "fmt"

    "cuelang.org/go/cue"
    "cuelang.org/go/cue/build"
    "cuelang.org/go/cue/cuecontext"
    "cuelang.org/go/cue/format"
    "cuelang.org/go/cue/parser"
)

func main() {
    ctx := cuecontext.New()
    file, err := parser.ParseFile("-", storage)
    if err != nil {
        panic(err)
    }
    builder := &build.Instance{}
    err = builder.AddSyntax(file)
    if err != nil {
        panic(err)
    }
    v := ctx.BuildInstance(builder)

    subv := v.LookupPath(cue.ParsePath("patch"))
    node := subv.Syntax()
    // We'll do some modification to the node here so convert file to node
    f := ToFile(node)
    // panic here
    _ = ctx.BuildFile(f)

    formatting, err := format.Node(node)
    if err != nil {
        panic(err)
    }

    fmt.Println(string(formatting))
}
-- v0.2.2.go --
package main

import (
    "fmt"

    "cuelang.org/go/cue"
    "cuelang.org/go/cue/format"
)

func main() {
    var r cue.Runtime
    cueInst, err := r.Compile("-", storage)
    if err != nil {
        panic(err)
    }
    subv := cueInst.Value().Lookup("patch")
    node := subv.Syntax()
    f := ToFile(node)
    // no panic
    _, _ = r.CompileFile(f)
    formatting, err := format.Node(node)
    if err != nil {
        panic(err)
    }
    fmt.Println(string(formatting))
}

@FogDong's expectation is that this test should pass, but it fails with:

# v0.2.2 (0.684s)
> go get cuelang.org/go@v0.2.2
> go mod tidy
[stderr]
go: finding module for package cuelang.org/go/cue/cuecontext
go: found cuelang.org/go/cue/cuecontext in cuelang.org/go v0.4.3

> go run common.go v0.2.2.go
[stdout]
{
        spec: {
                template: {
                        spec: {
                                volumes: deDupVolumesArray
                        }
                }
        }
}

# current tip (0.418s)
> go get cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353
[stderr]
go: upgraded cuelang.org/go v0.4.3 => v0.4.4-0.20220729051708-0a46a1624353
go: upgraded golang.org/x/net v0.0.0-20200226121028-0de0cce0169b => v0.0.0-20220425223048-2871e0cb64e4
go: upgraded gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b => v3.0.1

> go mod tidy
> go run common.go tip.go
[stderr]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x48 pc=0x1eeec8]

goroutine 1 [running]:
cuelang.org/go/internal/core/adt.(*Environment).evalCached(0x0, 0x4000310240, {0x5d4fd0, 0x4000335830})
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/composite.go:143 +0x58
cuelang.org/go/internal/core/adt.(*LetReference).evaluate(0x0?, 0x5d5190?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/expr.go:898 +0x48
cuelang.org/go/internal/core/adt.(*OpContext).evalState(0x4000310240, {0x5d5190, 0x4000332820}, 0x2)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:653 +0x234
cuelang.org/go/internal/core/adt.(*OpContext).Evaluate(0x4000310240, 0x40003379a0, {0x5d5190, 0x4000332820})
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:557 +0x124
cuelang.org/go/internal/core/adt.(*OpContext).Concrete(0x4000310240, 0x0?, {0x5d5190?, 0x4000332820?}, {0x492320?, 0x888dd0})
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:495 +0x38
cuelang.org/go/internal/core/adt.(*BinaryExpr).evaluate(0x4000335cb0, 0x4000310240)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/expr.go:1214 +0x22c
cuelang.org/go/internal/core/adt.(*OpContext).evalState(0x4000310240, {0x5d4d10, 0x4000335cb0}, 0x2)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:653 +0x234
cuelang.org/go/internal/core/adt.(*Environment).evalCached(0x40003379a0, 0x4000310240, {0x5d4d10, 0x4000335cb0})
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/composite.go:150 +0x178
cuelang.org/go/internal/core/adt.(*LetReference).evaluate(0x0?, 0x5d5190?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/expr.go:898 +0x48
cuelang.org/go/internal/core/adt.(*OpContext).unifyNode(0x4000310240, {0x5d5190, 0x4000332a80}, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:738 +0x2a4
cuelang.org/go/internal/core/adt.(*OpContext).node(0x4000310240, {0x5d27f0?, 0x4000335ce0}, {0x5d5190?, 0x4000332a80?}, 0x1, 0x52?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:919 +0x50
cuelang.org/go/internal/core/adt.(*ForClause).yield(0x4000335ce0, 0x4000310240, 0xffff82e30108?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/expr.go:1702 +0x58
cuelang.org/go/internal/core/adt.(*OpContext).yield(0x4000310240, 0x0, 0x4000337b30, 0x1d358?, 0x0?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:486 +0x1bc
cuelang.org/go/internal/core/adt.(*nodeContext).addLists(0x4000347500)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:2176 +0x1504
cuelang.org/go/internal/core/adt.(*nodeContext).postDisjunct(0x4000347500, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:488 +0x100
cuelang.org/go/internal/core/adt.(*nodeContext).expandDisjuncts(0x4000347500, 0x0?, 0x4000347500, 0x0, 0x0, 0x1)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/disjunct.go:151 +0x320
cuelang.org/go/internal/core/adt.(*OpContext).Unify(0x4000310240, 0x4000342fc0, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:323 +0x878
cuelang.org/go/internal/core/adt.(*ListLit).evaluate(0x4000335800, 0x4000310240)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/expr.go:307 +0x124
cuelang.org/go/internal/core/adt.(*OpContext).unifyNode(0x4000310240, {0x5d51d0, 0x4000335800}, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:738 +0x2a4
cuelang.org/go/internal/core/adt.(*OpContext).node(0x4000310240, {0x5d27f0?, 0x4000335e60}, {0x5d51d0?, 0x4000335800?}, 0x1, 0x0?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:919 +0x50
cuelang.org/go/internal/core/adt.(*ForClause).yield(0x4000335e60, 0x4000310240, 0x4000321500?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/expr.go:1702 +0x58
cuelang.org/go/internal/core/adt.(*OpContext).yield(0x4000310240, 0x0, 0x4000337ae0, 0x23b770?, 0x40000b0630?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/context.go:486 +0x1bc
cuelang.org/go/internal/core/adt.(*nodeContext).addLists(0x4000347180)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:2176 +0x1504
cuelang.org/go/internal/core/adt.(*nodeContext).postDisjunct(0x4000347180, 0x2)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:488 +0x100
cuelang.org/go/internal/core/adt.(*nodeContext).expandDisjuncts(0x4000347180, 0x4?, 0x4000347180, 0x0, 0x0, 0x1)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/disjunct.go:151 +0x320
cuelang.org/go/internal/core/adt.(*OpContext).Unify(0x4000310240, 0x4000342f30, 0x2)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:323 +0x878
cuelang.org/go/internal/core/adt.(*nodeContext).addVertexConjuncts(0x4000346e00, {0x4000337a90, {0x5d27a0, 0x4000333000}, {0x0, 0x0, 0x0, 0x0}}, 0x4000342f30, 0x0)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:1594 +0x454
cuelang.org/go/internal/core/adt.(*nodeContext).evalExpr(0x4000346e00, {0x4000337a90, {0x5d27a0, 0x4000333000}, {0x0, 0x0, 0x0, 0x0}})
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:1450 +0x4bc
cuelang.org/go/internal/core/adt.(*nodeContext).addExprConjunct(0x4000346e00, {0x4000337a90, {0x5d27a0, 0x4000333000}, {0x0, 0x0, 0x0, 0x0}})
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:1410 +0x614
cuelang.org/go/internal/core/adt.(*nodeContext).insertConjuncts(0x4000346e00, 0x60?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:421 +0xe8
cuelang.org/go/internal/core/adt.(*OpContext).Unify(0x4000310240, 0x4000342ea0, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:255 +0xdac
cuelang.org/go/internal/core/adt.(*nodeContext).completeArcs(0x4000346a80, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:732 +0x264
cuelang.org/go/internal/core/adt.(*nodeContext).postDisjunct(0x4000346a80, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:611 +0x694
cuelang.org/go/internal/core/adt.(*nodeContext).expandDisjuncts(0x4000346a80, 0x14?, 0x4000346a80, 0x0, 0x0, 0x1)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/disjunct.go:151 +0x320
cuelang.org/go/internal/core/adt.(*OpContext).Unify(0x4000310240, 0x4000342e10, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:323 +0x878
cuelang.org/go/internal/core/adt.(*nodeContext).completeArcs(0x4000346700, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:732 +0x264
cuelang.org/go/internal/core/adt.(*nodeContext).postDisjunct(0x4000346700, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:611 +0x694
cuelang.org/go/internal/core/adt.(*nodeContext).expandDisjuncts(0x4000346700, 0xf0?, 0x4000346700, 0x0, 0x0, 0x1)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/disjunct.go:151 +0x320
cuelang.org/go/internal/core/adt.(*OpContext).Unify(0x4000310240, 0x4000342d80, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:323 +0x878
cuelang.org/go/internal/core/adt.(*nodeContext).completeArcs(0x4000346380, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:732 +0x264
cuelang.org/go/internal/core/adt.(*nodeContext).postDisjunct(0x4000346380, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:611 +0x694
cuelang.org/go/internal/core/adt.(*nodeContext).expandDisjuncts(0x4000346380, 0x0?, 0x4000346380, 0x0, 0x0, 0x1)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/disjunct.go:151 +0x320
cuelang.org/go/internal/core/adt.(*OpContext).Unify(0x4000310240, 0x4000342cf0, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:323 +0x878
cuelang.org/go/internal/core/adt.(*nodeContext).completeArcs(0x4000346000, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:732 +0x264
cuelang.org/go/internal/core/adt.(*nodeContext).postDisjunct(0x4000346000, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:611 +0x694
cuelang.org/go/internal/core/adt.(*nodeContext).expandDisjuncts(0x4000346000, 0x0?, 0x4000346000, 0x0, 0x0, 0x1)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/disjunct.go:151 +0x320
cuelang.org/go/internal/core/adt.(*OpContext).Unify(0x4000310240, 0x400031fcb0, 0x5)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/eval.go:323 +0x878
cuelang.org/go/internal/core/adt.(*Vertex).Finalize(...)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/internal/core/adt/composite.go:487
cuelang.org/go/cue.newVertexRoot(0x4908e0?, 0x4000310240?, 0x4000011bc0?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/cue/types.go:623 +0x7c
cuelang.org/go/cue.newValueRoot(0x5d5590?, 0x4000021e80?, {0x5d5510?, 0x400031fcb0?})
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/cue/types.go:632 +0x44
cuelang.org/go/cue.(*Context).make(0x4000021e80?, 0x0?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/cue/context.go:248 +0x64
cuelang.org/go/cue.(*Context).compile(0x4000091eb8?, 0x400031de68?, 0x400007e840?)
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/cue/context.go:167 +0x44
cuelang.org/go/cue.(*Context).BuildFile(0x4000021e80, 0x4000336ff0?, {0x0, 0x0, 0x0?})
        /home/myitcv/gostuff/pkg/mod/cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353/cue/context.go:160 +0xd8
main.main()
        $WORK/tip.go:31 +0x2a4
exit status 2

[exit status 1]
FAIL: /tmp/testscript476803494/repro.txt/script.txt:9: unexpected go command failure
error running repro.txt in /tmp/testscript476803494/repro.txt

Looking into this now.

myitcv commented 2 years ago

Ok, that was a tricky one to track down. But I think I've demonstrated in https://github.com/cue-lang/cue/issues/1828 that the panic you are seeing above is not related to the use of optional fields.

So my belief, expressed in https://github.com/cue-lang/cue/issues/1815#issuecomment-1200925221, is still that any error you are seeing relating to the use of an optional field is a valid error, and the change in behaviour to report that error is the result of a bug fix for previously incorrect behaviour where that error was not reported in comprehensions.

However, I will confirm all of this with @mpvl tomorrow.

In the meantime, please can you help us to understand the impact of this bug fix? i.e. what problems does it create for you and/or your users? Talking that through will help us to explore potential solutions, which might include code rewriting like the tricks employed in cue fmt.

FogDong commented 2 years ago

Thanks for the reply, I understand that reporting the error is actually a bug fix, I create another example here using value.MarshalJSON() : https://gist.github.com/FogDong/843c7618678d55b11d344bed30fc8d85

If you write an example like this in master, it will report an error when marshaling the value to json, but marshal successfully in v0.2.2.

So for our users, if they have cue files in which is a deployment, and its volumes is an array like in the example, they can apply the deployment to the cluster without any error in v0.2.2 but not in the latest version of cue. (KubeVela will marshal their output cue values and apply to the Kubernetes cluster.)

myitcv commented 2 years ago

Hi @FogDong

Thanks for the reply, I understand that reporting the error is actually a bug fix, I create another example here using value.MarshalJSON() : https://gist.github.com/FogDong/843c7618678d55b11d344bed30fc8d85

If you write an example like this in master, it will report an error when marshaling the value to json, but marshal successfully in v0.2.2.

For reference, here is a standalone version of your reproducer:

# v0.2.2
go get cuelang.org/go@v0.2.2
go mod tidy
go run .
cmp stdout v0.2.2.stdout.golden

# current tip
go get cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353
go mod tidy
! go run .
stderr 'panic: cue: marshal error: output.spec.template.spec.volumes: cannot reference optional field: pvc'

-- go.mod --
module mod.com

go 1.18

require cuelang.org/go v0.2.2

-- main.go --
package main

import (
    "fmt"

    "cuelang.org/go/cue"
)

var test = `
parameter: {
    pvc?: [...{
        name:       string
        mountPath?: string
    }]
}
pvcVolumesList: *[
    for v in parameter.pvc if v.mountPath != _|_{
    {
        name: "pvc-" + v.name
        persistentVolumeClaim: claimName: v.name
    }
},
] | []
output: {
    apiVersion: "apps/v1"
    kind:       "Deployment"
    spec: template: spec: volumes: pvcVolumesList
}
`

// in 0.2
func main() {
    var r cue.Runtime
    cueInst, err := r.Compile("-", test)
    if err != nil {
        panic(err)
    }
    subv := cueInst.Lookup("output")
    b, err := subv.MarshalJSON()
    if err != nil {
        panic(err)
    }
    fmt.Println(string(b))
}
-- v0.2.2.stdout.golden --
{"apiVersion":"apps/v1","kind":"Deployment","spec":{"template":{"spec":{"volumes":[]}}}}

This, however, is an instance of the same problem. This only "worked successfully" for v0.2.2 because of a bug. The error being reported in tip is therefore the correct behaviour.

The correct fix here is in the CUE:

        }]
 }
 pvcVolumesList: *[
-       for v in parameter.pvc if v.mountPath != _|_{
+       if parameter.pvc != _|_ for v in parameter.pvc if v.mountPath != _|_{
        {
                name: "pvc-" + v.name
                persistentVolumeClaim: claimName: v.name

(where - marks the "current" version, and + marks what it needs to be changed to when referencing an optional field).

So my belief, expressed in #1815 (comment), is still that any error you are seeing relating to the use of an optional field is a valid error, and the change in behaviour to report that error is the result of a bug fix for previously incorrect behaviour where that error was not reported in comprehensions.

However, I will confirm all of this with @mpvl tomorrow.

I spoke with @mpvl and he confirmed my analysis.

This might require you to change the source CUE for lots of your users; please let us know if you'd like to discuss ways to handle this upgrade.

wonderflow commented 2 years ago

Thanks! @myitcv We already found out that changes and able to fix on our own side.

This might require you to change the source CUE for lots of your users; please let us know if you'd like to discuss ways to handle this upgrade.

Yes, unfortunately, this is a break change to our users as all their CUE source files may be broken by this upgrade. Do we have any potential solutions?

myitcv commented 2 years ago

Do we have any potential solutions?

Will discuss with @mpvl and come back to you tomorrow. We might be able to suggest something along the lines of rewrites that happen as part of cue fix (which happens automatically as part of cue fmt).

In the meantime, I'm trying to see if there isn't a different/better way of applying these patches. In the examples linked above, @FogDong includes some CUE that looks like it's part of a patch process. Please can you provide some examples showing the input and output CUE for the patches you apply? Do you have some test cases perhaps that show this in action?

(You previously linked me to https://kubevela.io/docs/next/platform-engineers/traits/patch-trait and https://kubevela.io/docs/next/platform-engineers/cue/patch-strategy, but what I'm after here is specifically the CUE input and expected CUE output of each type of patch you can apply).

Thanks

myitcv commented 2 years ago

@wonderflow

Yes, unfortunately, this is a break change to our users as all their CUE source files may be broken by this upgrade. Do we have any potential solutions?

Please can you provide some samples of the CUE that needs to be rewritten?

We can also discuss how your users would apply this fix, whether via cue fix or some other means.

wonderflow commented 2 years ago

@myitcv Sure, Thanks for following up this so quick!

@FogDong is collecting the real user examples.

But the workflow maybe not that straight-forward. Because KubeVela merge CUE definitions and user input in our own Go code. KubeVela users don't use CUE tools directly, the process is something like:

Now the platform engineers already install the CUE definitions inside the clusters and when KubeVela upgrades it may casue crash.

I hope the solution can be something like:

We need some function to work like "cue fix", the function we can call in our serverside. So the user could use the existing CUE files and our server(some webhooks) can mutate and fix that in the new version automatically.

FogDong commented 2 years ago

Hi @myitcv , we have this official cue file in KubeVela repo called storage: https://github.com/kubevela/kubevela/blob/master/vela-templates/definitions/internal/trait/storage.cue#L186

Users can use this cue to add env, volumes, and volumeMounts in their deployments, and the env, volumes, volumeMounts are all array.

For example, the user can choose a configMap type volume or secret type volume, so configMap and secret should be optional fields, and the volumes are like:

    configMapVolumesList: *[
                for v in parameter.configMap if v.mountPath != _|_ {
            {
                name: "configmap-" + v.name
                configMap: {
                    defaultMode: v.defaultMode
                    name:        v.name
                    if v.items != _|_ {
                        items: v.items
                    }
                }
            }
        },
    ] | []

    secretVolumesList: *[
                for v in parameter.secret if v.mountPath != _|_ {
            {
                name: "secret-" + v.name
                secret: {
                    defaultMode: v.defaultMode
                    secretName:  v.name
                    if v.items != _|_ {
                        items: v.items
                    }
                }
            }
        },
    ] | []

// will use list function instead of + in the future
volumes: configMapVolumesList + secretVolumesList

So as you see, the error reports in the latest version of cue.

FYI, I'm collecting the specific example that we use patch in our code, and will reply to the issue when I'm done.

myitcv commented 2 years ago

So the user could use the existing CUE files and our server(some webhooks) can mutate and fix that in the new version automatically.

In the very short term you might conclude this is your best option, but you will ultimately need/want to write back the "fixed" CUE to the users. Otherwise other CUE tools your users might use (e.g. editor-based tools) will report errors. It will also be a problem where that source CUE is composed into a larger configuration for use in other situations. Better to fix the problem at source.

cue fix based solutions are designed to fix the source CUE as a one-off. Indeed a fix like this in cue fix would likely live on for a few releases, at which point we would remove it.

cue fix wraps existing API for applying fixes via https://pkg.go.dev/cuelang.org/go/tools/fix. However, that package does not currently contain a fix for your situation - that's what we will investigate and report back on.

Do you have some means/tooling of helping the KubeVela users fix their code? That would seem like a useful feature in any case. Because any futures changes/additions to CUE can then be solved in the same way, e.g. the rewriting of list concatenation.

myitcv commented 2 years ago

cue fix wraps existing API for applying fixes via https://pkg.go.dev/cuelang.org/go/tools/fix. However, that package does not currently contain a fix for your situation - that's what we will investigate and report back on.

FYI @rogpeppe is going to help looking at a tools/fix based solution here.

wonderflow commented 2 years ago

you will ultimately need/want to write back the "fixed" CUE to the users.

Yes, I think we need to write the fixed thing back, and the Kubernetes mutate webhook can actually do that. \cc @FogDong But we really need an API/Function Call to help us do that on our own server.

myitcv commented 2 years ago

Yes, I think we need to write the fixed thing back, and the Kubernetes mutate webhook can actually do that

Great.

But we really need an API/Function Call to help us do that on our own server.

https://pkg.go.dev/cuelang.org/go/tools/fix#Instances will likely be that; but will wait for @rogpeppe's analysis to confirm.

myitcv commented 2 years ago

I've just marked this as "NeedsFix" - but to be clear, the "fix" here is making a change in tools/fix (no pun intended) to help KubeVela handle user code which relied on this old bug. The ultimate fix is to write back fixed code to those users, and not rely on fixing "on the fly" because the "fix" in tools/fix will be removed in a later release.

rogpeppe commented 2 years ago

I've been looking into this a bit more. Here's a smaller version of the example which I think represents the problem concisely:

p?: []
a: [for k in p {k}] | null

A cue export on this succeeds OK on v0.2.2 but fails with "cannot reference optional field: p" on v0.4.3.

Note that without the disjunction (just using a: [for k in p {k}]) it fails on both versions, so the key here seems to be the disjunction.

By contrast, this code works fine on both versions of CUE:

p?: {}
a: p | null
b: p.foo | null

so it's clear to me that a reference to an optional value evaluates to bottom, as do expressions containing such a reference, and according to the spec:

The disjunction of a value a with bottom is always a.

AFAICS there should not be a qualitative difference between a comprehension involving a reference to an optional value and any other expression involving an optional value, so at the moment I see this as a straightforward bug in the latest version of CUE.

rogpeppe commented 2 years ago

After further discussion with @myitcv, I think there are 3 distinct issues here:

under v0.4.3, the following CUE fails export (and vet):

p?: []
a: [for k in p {k}] | null

It seems like that should work, so we should be fixing that in the near future.

in v0.2.2, there's discrepancy between cue export and cue vet -c: cue vet -c succeeds but cue export fails on the following code:

p?: {}

a: {
    for k in p  {
        {name: k}
    }
}

in v0.2.2, it's possible to access the field a in the above CUE even though it's not possible to export it.

Here's some code to demonstrate that issue:

package main

import (
    "fmt"
    "log"

    "cuelang.org/go/cue"
)

func main() {
    r := new(cue.Runtime)
    inst, err := r.Compile("foo.cue", `
p?: {}

a: {
    for k in p  {
        {name: k}
    }
}`)
    if err != nil {
        log.Fatal(err)
    }
    a := inst.Lookup("a")
    fmt.Printf("error: %v\n", a.Err())   // Succeeds inappropriately on v0.2.2.
}

Note that a slight variant of the above code does not demonstrate the issue: changing the struct comprehension to a list comprehension causes the code to fail in all cases (as it should):

a: [
    for k in p  {
        {name: k}
    }
]

rogpeppe commented 2 years ago

So, to be a bit clearer for folks following along, fixing the bug in v0.4.3 will fix some but not all of the issues that are being seen. As an example, this code has several list comprehensions that will be fixed, but some struct comprehensions (e.g. this) which won't and will need the CUE to be fixed.

FogDong commented 2 years ago

Thanks for the investigating, Roger @rogpeppe .

I'm a little confused, why for k in p {k} should behave differently in list comprehension and struct comprehension?

mvdan commented 2 years ago

@FogDong I believe Roger is just pointing out that the bug appears in one case but not the other.

myitcv commented 2 years ago

@wonderflow @FogDong I am in the process of writing up notes from our call the other day relating to "why" we believe this bug fix to be more correct than the previous v0.2.2 behaviour.

@FogDong just to confirm some details in @rogpeppe's comment in https://github.com/cue-lang/cue/issues/1815#issuecomment-1205459480.

The first bullet point is highlighting a further bug in current tip, which @rogpeppe has raised as https://github.com/cue-lang/cue/issues/1838. This bug is relatively significant because it somewhat confuses other problems you are seeing. With reference to https://github.com/cue-lang/cue/issues/1838#issuecomment-1208058815, you have shared some CUE examples that are similar to the list-with-default and list-no-default cases. Both of these should successfully decode a value true in that linked comment (which implies no error returned by cue.Value.Decode()). However, list-with-default returns an error from cue.Value.Decode() and list-no-default fails to decode a value. The error returned by list-with-default is, I think, the root cause of a few issues you have reported, because you see an error where you previously did not - and just to be clear, in this case, because you are using a disjunction, you should not seen an error either. That said, in other sample CUE you have shared, you are not using a disjunction to "guard" access to an optional field, so fixing this bug will likely not by itself be a solution.

The second and third bullet points can largely be ignored, because they are demonstrating bugs in v0.2.2. Not totally insignificant, because those bugs can help to explain why you saw certain behaviour before, and therefore changes you might need to adapt to whilst moving to tip. But in the grand scheme of things, they are less significant that bugs at tip because we won't be fixing bugs in v0.2.2

myitcv commented 2 years ago

FYI, I'm collecting the specific example that we use patch in our code, and will reply to the issue when I'm done.

@wonderflow @FogDong - we talked earlier about how we need to understand the scope of a tools/fix-based solution. Regardless of whether we can patch things via the cue.Value API, we will still need to fix your users' code.

To better understand the scope of a tools/fix based solution, we would like to look at a more complete set of the situations we will need to fix. For example, @rogpeppe mentioned we might be able to come up with a simple fix that covers a sufficiently large percentage of users by only considering comprehensions that refer to parameter.X.

You referenced https://github.com/kubevela/kubevela/blob/master/vela-templates/definitions/internal/trait. My understanding is that this is the definition of the patches.

What's the best place to look to see a corpus of the user CUE code we will need to fix?

Is there a schema against which we can then determine which fields are optional, and therefore which parts of the user code need fixing up?

wonderflow commented 2 years ago

@myitcv All the code about the patch logic in KubeVela is here: https://github.com/kubevela/kubevela/blob/c3ca30848963572547e1030c9cc98007e5178729/pkg/cue/model/sets/operation.go#L262

The user input is complicated, I will try to draw a picture to show you.

wonderflow commented 2 years ago

The CUE code in the picture is copied here for convenient:

parameter: {
    image: "nginx"
}

output: {
    apiVersion: "apps/v1"
    kind:       "Deployment"
    spec: {
        template: {
            spec: containers: [{
                image: parameter.image
                hostAlias: [{
                    ip: "10.10.0.1"
                }]
            }]}}
}
parameter: {
    image: string
}

parameter: {
    hostAliases: [{
        ip: "10.10.0.1"
        hostnames: [ "my.example.com"]
    }]
}

patch: {
    // +patchKey=ip
    spec: template: spec: hostAliases: parameter.hostAliases
}
parameter: {
    // +usage=Specify the hostAliases to add
    hostAliases: [...{
        ip: string
        hostnames: [...string]
    }]
}

output: {
    apiVersion: "apps/v1"
    kind:       "Deployment"
    spec: {
        template: {
            spec: containers: [{
                image: "nginx"
                hostAlias: [{
                    ip: "10.10.0.1"
                }]
            }]}}
}

patch: {
    // +patchKey=ip
    spec: template: spec: hostAliases: [{
        ip: "10.10.0.1"
        hostnames: [ "my.example.com"]
    }]
}

wonderflow commented 2 years ago

In the picture above, you can see there’re at least three parts of files to describe the patch process. 1) App.yaml ; 2) Component-Definition.cue ; 3) Trait-Definition.cue .

What input will come from the app.yaml is strictly defined inside the component-def and trait-def as parameter.
The real world component definition is a bit complicated, you can find examples here: https://github.com/kubevela/kubevela/tree/master/vela-templates/definitions/internal/component
The real world trait examples can be found here: https://github.com/kubevela/kubevela/blob/master/vela-templates/definitions/internal/trait.

myitcv commented 2 years ago

@wonderflow thanks for confirming these details.

myitcv commented 1 year ago

@FogDong @wonderflow - based on exchanges between @rogpeppe and @FogDong I think we are all sorted here, so I will close this issue.

But please let us know if that is not the case.

cue-lang / cue