cue-lang / cue

The home of the CUE language! Validate and define text-based and dynamic configuration
https://cuelang.org
Apache License 2.0
5.14k stars 294 forks source link

evaluator: field dropped when data transformed into list #2542

Open nxcc opened 1 year ago

nxcc commented 1 year ago

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

$ cue version
cue version v0.6.0

go version go1.20.6
      -buildmode exe
       -compiler gc
       -trimpath true
     CGO_ENABLED 0
          GOARCH amd64
            GOOS linux
         GOAMD64 v1

Does this issue reproduce with the latest stable release?

yes

What did you do?

with test.cue:

package kube

configMap: cm: data: myvalue: "42"

configMap: [ID=_]: this={
    (#metadataName & {id: ID, data: this.data, disable: this._disableNameSuffixHash}).out
}

configMap: [ID=_]: {
    _disableNameSuffixHash: *false | bool
    metadata: labels: {f: "1"}
}

objectSets: [ configMap]
objects: [ for v in objectSets for x in v {x}]

#metadataName: {
    id:      string
    disable: bool
    if !disable {
        out: metadata: name: id + "-my-suffix"
    }
    if disable {
        out: metadata: name: id
    }
}

execute cue export -e 'objects[0].metadata.name' test.cue

What did you expect to see?

"cm-my-suffix"

What did you see instead?

undefined field: name:
    --expression:1:21
myitcv commented 1 year ago

Thanks for the report @nxcc. Definitely looks like a bug, and quite a bad one at that. Rewriting the above into a testscript:

exec cue export -e 'objects[0].metadata.name' x.cue
cmp stdout stdout.golden

-- x.cue --
package kube

configMap: cm: data: myvalue: "42"

configMap: [ID=_]: this={
    (#metadataName & {id: ID, data: this.data, disable: this._disableNameSuffixHash}).out
}

configMap: [ID=_]: {
    _disableNameSuffixHash: *false | bool
    metadata: labels: {f: "1"}
}

objectSets: [ configMap]
objects: [ for v in objectSets for x in v {x}]

#metadataName: {
    id:      string
    disable: bool
    if !disable {
        out: metadata: name: id + "-my-suffix"
    }
    if disable {
        out: metadata: name: id
    }
}
-- stdout.golden --
"cm-my-suffix"

This is a regression compared to v0.5.0, and bisects to https://review.gerrithub.io/c/cue-lang/cue/+/551103

mvdan commented 1 year ago

Slightly smaller repro:

exec cue export -e 'obj2.name' in.cue
cmp stdout stdout.golden

-- in.cue --
obj: this={
    (#makeName & {enable: this._enable}).out
}

obj: _enable: true

obj2: obj

#makeName: {
    enable: bool
    if enable {
        out: name: "enabled"
    }
    if !enable {
        out: name: "disabled"
    }
}
-- stdout.golden --
"enabled"

This passes with v0.5.0, but fails with v0.6.0 and master.

Note that swapping the first two declarations, so that obj: _enable: true goes before obj: this={..., makes the bug go away - so this sounds like an ordering bug, perhaps related to #2555, although that one isn't a recent regression like this one.

Also worth noting that the indirection via obj2 is necessary to reproduce the error, a simpler form of your list comprehension. Directly using obj.name works as expected.

nxcc commented 1 year ago

Execution results being dependent on order of declarations, in this issue as well as #2555, are really scary. I hope this get's fixed soon.

myitcv commented 1 year ago

Execution results being dependent on order of declarations, in this issue as well as #2555, are really scary.

Completely agree. I've marked this as v0.6.x, but just to set expectations lots of fixes are being pushed to v0.7.0 where changes to the evaluator are made much simpler.

Whilst not ideal, if you are able to continue with v0.5.0 that would be the best workaround for now.

nxcc commented 1 month ago

info: works with CUE_EXPERIMENT=evalv3

mvdan commented 1 month ago

@nxcc thanks for following up here! We will add a regression test to mark this issue as closed then.

jstewmon commented 1 week ago

👋 Hi, I think I have a reproducer for this issue, but I wasn't able to confirm that it resolves with evalv3, potentially due to a separate issue. I wanted to share my example.

Given this definition:

import "list"

// replace items in a list using a replacement mapping
#Replace: {
   input: [string, ...]
   // only allow keys that appear in input
   rep: close({for item in input {(item): string | [...string]}})
   // translation mapping
   tr: rep & {for item in input
     {(item): *item | string | [...string]}
   }
   // flatten the translation mapping
   output: list.FlattenN([for item in input {tr[item]}], 1)
}

non-experimental Case 1 PASS:

one: ["foo", "bar", "baz"]
two: (
  #Replace & {
    input: one
    rep: {
      foo: ["fip", "foo"]
      bar: []
      baz: "qux"
      err: ""
    }
  })
--
two.rep.err: field not allowed:
    -:7:15
    -:7:16
    -:7:34
    -:18:3
    -:24:7

non-experimental Case 2 (reference output) FAIL:

one: ["foo", "bar", "baz"]
two: (
  #Replace & {
    input: one
    rep: {
      foo: ["fip", "foo"]
      bar: []
      baz: "qux"
      err: ""
    }
  }).output
--
one:
  - foo
  - bar
  - baz
two:
  - fip
  - foo
  - qux

NB: The expected error is reported when referencing rep or tr - only referencing output suppresses the error.

Trying Case 2 (reference output) with CUE_EXPERIMENT=evalv3, I get the following:

rep.foo: conflicting values string and ["fip","foo"] (mismatched types string and list):
    -:5:43
    -:16:12
rep.foo.0: field not allowed:
    -:16:12

The field not allowed error is reported as expected, but the string | [...string] disjunction from the closed struct causes an unexpected error.

Does my reproducer cover this non-evalv3 issue (data transformed into list) AND some other issue with evalv3?