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.

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

value/unify: unify unfies value in every alternation #1878

Open
FogDong opened this issue Aug 24, 2022 · 9 comments
Open

value/unify: unify unfies value in every alternation #1878

FogDong opened this issue Aug 24, 2022 · 9 comments
Labels

Comments

@FogDong
Copy link

FogDong commented Aug 24, 2022

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

$ cue version
v0.4.4

Does this issue reproduce with the latest release?

yes

What did you do?

The repro is as follows:

exec cue eval test.cue
cmp stdout stdout.golden
-- stdout.golden --
test: {
    value: {
        option1: "test"
    }
}
-- test.cue --
test: {
	value: {
		option1: string
	} | {
		option2: {
			v: string
		}
	}
}

test: value: option1: "test"

The result is:

$ testscript repro.txt

> exec cue eval test.cue
[stdout]
test: {
    value: {
        option1: "test"
    } | {
        option1: "test"
        option2: {
            v: string
        }
    }
}
> cmp stdout stdout.golden
--- stdout
+++ stdout.golden
@@ -1,10 +1,5 @@
 test: {
     value: {
         option1: "test"
-    } | {
-        option1: "test"
-        option2: {
-            v: string
-        }
     }
 }

FAIL: /var/folders/2n/0vszm3355n114dk2r_ln8vrh0000gp/T/testscript4089038542/repro.txt/script.txt:2: stdout and stdout.golden differ
error running repro.txt in /var/folders/2n/0vszm3355n114dk2r_ln8vrh0000gp/T/testscript4089038542/repro.txt

You can also checkout the result in playground: https://cuelang.org/play/?id=ZawmQvcToMn#cue@export@cue

What did you expect to see?

The stdout.golden is also the result in cue v0.2.0:

test: {
    value: {
        option1: "test"
    }
}

What did you see instead?

@FogDong FogDong added NeedsInvestigation Triage Requires triage/attention labels Aug 24, 2022
@myitcv myitcv added the x/user/KubeVela Experimental CUE-user-based labels label Aug 24, 2022
@myitcv
Copy link
Member

myitcv commented Aug 24, 2022

Thanks @FogDong.

v0.4.4

We haven't actually release v0.4.4 yet! What version are you testing again here therefore?

Is the repro you have provided how you are using CUE here, or is this a problem you are seeing via the API?

Assuming it's the latter, please can you provide a repro of what you are seeing and expecting via the API?

@FogDong
Copy link
Author

FogDong commented Aug 24, 2022

Sorry for the misleading, I'm testing it with cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353.

The repro with API is like:

# v0.2.2
go get cuelang.org/go@v0.2.2
go mod tidy -compat=1.17
go run main.go
cmp stdout stdout.golden

# v0.4.4
go get cuelang.org/go@v0.4.4-0.20220729051708-0a46a1624353
go mod tidy -compat=1.17
go run main.go
cmp stdout stdout.golden
-- go.mod --
module test-cue/unify-alter

go 1.17

require cuelang.org/go v0.4.4-0.20220729051708-0a46a1624353

require (
	github.com/cockroachdb/apd/v2 v2.0.1 // indirect
	github.com/emicklei/proto v1.10.0 // indirect
	github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b // indirect
	github.com/google/uuid v1.2.0 // indirect
	github.com/mitchellh/go-wordwrap v1.0.1 // indirect
	github.com/mpvl/unique v0.0.0-20150818121801-cbe035fff7de // indirect
	github.com/pkg/errors v0.8.1 // indirect
	github.com/protocolbuffers/txtpbfmt v0.0.0-20220428173112-74888fd59c2b // indirect
	golang.org/x/net v0.0.0-20220425223048-2871e0cb64e4 // indirect
	golang.org/x/text v0.3.7 // indirect
	gopkg.in/yaml.v3 v3.0.1 // indirect
)
-- main.go --
package main

import (
	"fmt"

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

var unifyAlter = `
test: {
	value: {
		option1: string
	} | {
		option2: {
			v: string
		}
	}
}

test: value: option1: "test"
`

func main() {
	ctx := cuecontext.New()
	v := ctx.CompileString(unifyAlter)

	s, err := format.Node(v.Syntax(cue.Final()))
	if err != nil {
		panic(err)
	}
	fmt.Println(string(s))
}
-- repro.txt --
-- stdout.golden --
{
        test: {
                value: {
                        option1: "test"
                }
        }
}

result:


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

> go mod tidy -compat=1.17
[stderr]
go: downloading github.com/mpvl/unique v0.0.0-20150818121801-cbe035fff7de
go: downloading github.com/cockroachdb/apd/v2 v2.0.1
go: downloading golang.org/x/net v0.0.0-20220425223048-2871e0cb64e4
go: downloading github.com/google/go-cmp v0.5.7
go: downloading github.com/stretchr/testify v1.2.2
go: downloading gopkg.in/yaml.v3 v3.0.1
go: downloading github.com/kr/pretty v0.1.0
go: downloading github.com/davecgh/go-spew v1.1.1
go: downloading github.com/pmezard/go-difflib v1.0.0
go: downloading github.com/pkg/errors v0.8.1
go: downloading github.com/cockroachdb/apd v1.1.0
go: downloading github.com/lib/pq v1.0.0
go: downloading gopkg.in/check.v1 v1.0.0-20180628173108-788fd7840127
go: downloading github.com/kr/text v0.1.0
go: downloading golang.org/x/text v0.3.7
go: finding module for package cuelang.org/go/cue/cuecontext
go: downloading cuelang.org/go v0.4.3
go: found cuelang.org/go/cue/cuecontext in cuelang.org/go v0.4.3
go: downloading github.com/rogpeppe/go-internal v1.8.1
go: downloading github.com/google/uuid v1.2.0
go: downloading gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405
go: downloading github.com/emicklei/proto v1.10.0
go: downloading github.com/protocolbuffers/txtpbfmt v0.0.0-20220428173112-74888fd59c2b
go: downloading github.com/mitchellh/go-wordwrap v1.0.1
go: downloading github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b

> go run main.go
[stdout]
{
        test: {
                value: {
                        option1: "test"
                } | {
                        option1: "test"
                        option2: {
                                v: string
                        }
                }
        }
}

> cmp stdout stdout.golden
--- stdout
+++ stdout.golden
@@ -1,12 +1,7 @@
 {
-       test: {
-               value: {
-                       option1: "test"
-               } | {
-                       option1: "test"
-                       option2: {
-                               v: string
-                       }
-               }
-       }
+        test: {
+                value: {
+                        option1: "test"
+                }
+        }
 }

FAIL: /var/folders/2n/0vszm3355n114dk2r_ln8vrh0000gp/T/testscript1522450007/repro.txt/script.txt:5: stdout and stdout.golden differ
error running repro.txt in /var/folders/2n/0vszm3355n114dk2r_ln8vrh0000gp/T/testscript1522450007/repro.txt

@myitcv
Copy link
Member

myitcv commented Sep 9, 2022

Noting that the repro here suffers from the same problem of unintentionally upgrading beyond v0.2.2 because of the use of the cuecontext package. Here is a fixed repro:

# v0.2.2
go get cuelang.org/go@v0.2.2
go mod tidy
go list -m cuelang.org/go
stdout 'cuelang.org/go v0.2.2'
go run main.go
cmp stdout stdout.golden

# v0.4.4
go get cuelang.org/go@v0.4.4-0.20220905100514-9e9786e93b63
go mod tidy
go list -m cuelang.org/go
stdout 'cuelang.org/go v0.4.4-0.20220905100514-9e9786e93b63'
go run main.go
cmp stdout stdout.golden
-- go.mod --
module test-cue/unify-alter

go 1.18

require cuelang.org/go v0.2.2

-- main.go --
package main

import (
	"fmt"
	"log"

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

var unifyAlter = `
test: {
	value: {
		option1: string
	} | {
		option2: {
			v: string
		}
	}
}

test: value: option1: "test"
`

func main() {
	var r cue.Runtime
	i, err := r.Compile("test.cue", unifyAlter)
	if err != nil {
		log.Fatal(err)
	}
	v := i.Value()

	s, err := format.Node(v.Syntax(cue.Final()))
	if err != nil {
		panic(err)
	}
	fmt.Println(string(s))
}
-- stdout.golden --
{
	test: {
		value: {
			option1: "test"
		}
	}
}

This gives:

# v0.2.2 (0.372s)
# v0.4.4 (0.273s)
> go get cuelang.org/go@v0.4.4-0.20220905100514-9e9786e93b63
[stderr]
go: upgraded cuelang.org/go v0.2.2 => v0.4.4-0.20220905100514-9e9786e93b63
go: upgraded github.com/cockroachdb/apd/v2 v2.0.1 => v2.0.2
go: upgraded golang.org/x/net v0.0.0-20200226121028-0de0cce0169b => v0.0.0-20220722155237-a158d28d115b
go: upgraded golang.org/x/text v0.3.2 => v0.3.7
go: upgraded gopkg.in/yaml.v3 v3.0.0-20200121175148-a6ecf24a6d71 => v3.0.1

> go mod tidy
> go list -m cuelang.org/go
[stdout]
cuelang.org/go v0.4.4-0.20220905100514-9e9786e93b63

> stdout 'cuelang.org/go v0.4.4-0.20220905100514-9e9786e93b63'
> go run main.go
[stdout]
{
        test: {
                value: {
                        option1: "test"
                } | {
                        option1: "test"
                        option2: {
                                v: string
                        }
                }
        }
}

> cmp stdout stdout.golden
--- stdout
+++ stdout.golden
@@ -2,11 +2,6 @@
        test: {
                value: {
                        option1: "test"
-               } | {
-                       option1: "test"
-                       option2: {
-                               v: string
-                       }
                }
        }
 }

FAIL: /tmp/testscript2525458982/repro.txtar/script.txtar:15: stdout and stdout.golden differ

This is actually working as intended.

The release of the v0.3.x series included the new evaluator. This changed the way in which disjunction elements were eliminated. In this example under the new evaluator (which is what we have at tip), we cannot eliminate the disjunction because both struct values are open. Hence, option1 can appear in either element of the disjunct. Hence the output:

test: {
       value: {
               option1: "test"
       } | {
               option1: "test"
               option2: {
                       v: string
               }
       }
}

Let's discuss where you are seeing this and how to deal with it.

@myitcv
Copy link
Member

myitcv commented Sep 9, 2022

cc @mpvl for comment on disjunctions.

@FogDong
Copy link
Author

FogDong commented Sep 26, 2022

If we close every option in value, this should work like: https://cuelang.org/play/?id=cpoiRRyb5hA#cue@export@cue

parameter: {
	value: close({
		option1: string
	}) | close({
		option2: {
			v: string
		}
	})
}

parameter: value: option1: "test"


-------result---------
parameter: {
	value: {
		option1: "test"
	}
}

But if we close the outer field, this will not work like: https://cuelang.org/play/?id=mJeqrcuPbNY#cue@export@cue

parameter: close({
	value: {
		option1: string
	} | {
		option2: {
			v: string
		}
	}
})

parameter: value: option1: "test"

----------result------------
parameter: {
    value: {
        option1: "test"
    } | {
        option1: "test"
        option2: {
            v: string
        }
    }
}

Is this working as intended?

@myitcv
Copy link
Member

myitcv commented Sep 29, 2022

@FogDong close() is only defined with respect to structs. We could change close() so that it is defined to work on disjunctions, but then I think we would also want close() to error more aggressively when it is not operating on struct values. In any case there are a number of open questions about how it could/should behave, some shown below:

close(5)               // this is an error today
close(5 | 4)           // this is not an error today. Should it be?
close({a: int} | 4)    // ok? 

@mpvl mpvl added this to the v0.5.0 comprehension rework milestone Sep 29, 2022
@mpvl
Copy link
Member

mpvl commented Sep 29, 2022

We are thinking about whether we should bump the priority of the implementation of numdefined, which will simplify working with "oneofs" patterns like this a whole lot simpler as well as more powerful.

@myitcv
Copy link
Member

myitcv commented Oct 6, 2022

I've left the "WorkingAsIntended" tag on because the original issue is working as intended.

However, as noted by @mpvl, we are considering repurposing this issue to implement numdefined(), hence I've tentatively marked this as v0.5.0.

@myitcv
Copy link
Member

myitcv commented Nov 29, 2022

We have made significant progress on the new builtins as part of preparation work for v0.6.0. Trying to include this in v0.5.0 was proving to be too much, given the issues that we've needed to shake out from the cycle and comprehension rework. Therefore, moving this issue to v0.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants