cue: ResolveReferences option not work in method "Syntax"

[cueckoo]

Originally opened by @leejanee in cuelang/cue#867

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

$ cue version
v0.3.0-beta.8

What did you do?

The code is as follows

       var src =`
t: {name: string}
output: [...t]
`
	var r cue.Runtime
	inst,_:=r.Compile("-",src)
	ct,_:=format.Node(inst.Value().Lookup("output").Syntax(cue.ResolveReferences(true)))
	fmt.Println(string(ct))

After running the code,I get

[...t]

But import the v0.2.2 version, The result is as follows

[...{
        name: string
}]

and, This is what i expected

[cueckoo]

Original reply by @myitcv in cuelang/cue#867 (comment)

@leejanee - thanks for raising this.

Just as an FYI, there are some tips on how to create clean reproducers in https://github.com/cuelang/cue/wiki/Creating-test-or-performance-reproducers. The txtar format is particularly good when it comes to multi-file repros, as is the case here.

For example, here is my reproducer based on the above:

exec go mod tidy
exec go run main.go
cmp stdout stdout.golden

-- go.mod --
module mod.com

go 1.16

require cuelang.org/go v0.3.0-beta.8
-- main.go --
package main

import (
 "fmt"

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

func main() {
 var src = `
t: {name: string}
output: [...t]
`
 var r cue.Runtime
 inst, _ := r.Compile("-", src)
 ct, _ := format.Node(inst.Value().Lookup("output").Syntax(cue.ResolveReferences(true)))
 fmt.Println(string(ct))
}
-- stdout.golden --
[...{
	name: string
}]

What did you expect to see?

The test pass.

What did you see instead?

> exec go run main.go
[stdout]
[...t]
> cmp stdout stdout.golden
[diff -stdout +stdout.golden]
-[...t]
+[...{
+       name: string
+}]

Creating such repros makes it much easier to confirm the issue, and indeed prepare a fix (because we have code/an example that can largely be copy-pasted).

That said, this does appear to be a regression compared to v0.2.2 (confirmed a problem at tip, 792da39), bisected to 845df05. Therefore I suspect this will be fixed after the planned v0.3.0 release.

[cueckoo]

Original reply by @wonderflow in cuelang/cue#867 (comment)

@mpvl Hi, any progress about this issue, we're really want to upgrade to 0.3 while this issue blocks our needs.

[cueckoo]

Original reply by @myitcv in cuelang/cue#867 (comment)

@wonderflow @leejanee - just to check, this remains a priority for you?

[cueckoo]

Original reply by @wonderflow in cuelang/cue#867 (comment)

@myitcv yes, our project KubeVela highly relies on this feature, and we love to use CUE very much.

[cueckoo]

Original reply by @wonderflow in cuelang/cue#867 (comment)

Now we are still using version 0.2.2 because of this block https://github.com/oam-dev/kubevela/blob/master/go.mod#L6

And our users love to use the feature sets in CUE >= 0.3, so we also very hope to upgrade. But one of our feature relies on it.

[cueckoo]

Original reply by @myitcv in cuelang/cue#867 (comment)

Ok, thanks for the update @wonderflow

[cueckoo]

Original reply by @myitcv in cuelang/cue#867 (comment)

@wonderflow I've tentatively marked this for v0.5.0.

On a related subject, I would be keen to understand if/how we can add KubeVela to our regression testing setup, unity. Would you be happy for me to email you offline to discuss further?

[cueckoo]

Original reply by @wonderflow in cuelang/cue#867 (comment)

Sure, very glad ! My email: [email protected]

[cueckoo]

Original reply by @myitcv in cuelang/cue#867 (comment)

@wonderflow @leejanee - when we come to look at this it would be good to understand exactly what semantics you expect from cue.ResolveReferences. My understanding from the example above is that given a CUE Instance/Value, you want to resolve all references to their literal equivalents, whether from within the package or across a package boundary. Does that match your understanding?

[cueckoo]

Original reply by @leejanee in cuelang/cue#867 (comment)

@wonderflow @leejanee - when we come to look at this it would be good to understand exactly what semantics you expect from cue.ResolveReferences. My understanding from the example above is that given a CUE Instance/Value, you want to resolve all references to their literal equivalents, whether from within the package or across a package boundary. Does that match your understanding?

Yes it is, I expect that the result returned by the Syntax(cue.All(),cue.ResolveReferences(true)) method can be recompiled

[hprotzek]

Morning, can we expect this fix in the next version? or is this blocked somehow? A new version of culang would improve the writing of templates in Kubevela very much :-)

[myitcv]

@hprotzek thanks for the reminder. We have this issue on our radar for an upcoming release, as discussed with @wonderflow. As we discussed recently in the first CUE community town hall we are in the process of getting back to the project in earnest. So hope to share details of upcoming releases very soon.

[mpvl]

One problem with arbitrarily expanding references is that it can lead to a combinatorial explosion.

@wonderflow, @leejanee: It would therefore be good to understand what the objective is. Can you share some more detail on the use case?

For instance, if the objective is to create a self-contained snippet of CUE, then a different solution would be better.

In either case, to fully fix any such solution we need to have a different internal representation for let expressions. We are working towards this.

If there are simple use cases you would like to see covered in the mean time, please let us know.

[leejanee]

We use cuelang to render kubernetes resources in kubevela. This process is divided into two stages:

1. First, perform user-side rendering, the result is transmitted to the server in ast.Node format.
2. Server-side rendering.

For example:

User 1 and User 2 cooperate to complete resource definition. User 1 define the resource type and default values, and user 2 focus on replicas

First of all, the template on the user side has a consistent format, for example as follows:

parameter: {
    ...
}

resource: {
   ...
}

the parameter is user input, and the resource is the desired resource definition.

1. The template and input of user 1 is as follows:

----- template.file --------
import "k8s.io/api/core/v1"
parameter: {
    image: string
    replicas: int
}

resource: v1.#Deployment{
    spec: {
       replicas: parameter.replicas
       template: spec: containers: [{ 
                image: parameter.image
                name: "main"
                envs: [...]
        }]
    }
}

----- input.file --------
// set parameter
   image: *"myimage" | string
   // limit replicas scope 
   replicas: *2 ｜ >=1 & <5

2. The template and input of user 2（focus on replicas） is as follows:

----- template.file --------
parameter: {
    replicas: int
}

// not care about specific resource types here.
resource: {
   spec: replicas: parameter.replicas
}

----- input.file --------
// set parameter
replicas: 3

3. on the server-side, process like as follows:

resource:  v1.#Deployment & { // user 1
     spec: replicas: *2 ｜ >=1 & <5
     ...
} & {  // user 2
     spec: replicas: 3
}

I expect that the cue execution result on the user side can be recompiled on the server side again.

[mpvl]

I see. So what I observe going on here is that the input of the two users is resolved to be self-contained without references (like an OpenAPI equivalent) to be recombined later.

What I believe would suffice here, though, is to be able to produce a self-contained CUE file, which still may maintain references within the file. IOW, remove the imports.

Is this correct?

IOW, would it suffice to have a def command that just expands all imports, but not necessarily all references?

Or would do you still need all references to be expanded?

Note that references may act as constraints themselves.

Right now, evaluating references may result in an exponential blowup, analogous to the "billion laughs" in YAML. So if the goal here is to put the onus of this on the user, this is understandable. Once we introduce structure sharing, though, such blowup can be avoided if the depth of the evaluation is limited.

[verdverm]

I could imagine import inlining could have unexpected blowup in size. Even with some tree shaking, the file could get quite large with k8s. We'd probably have to account for the cue.mod/{gen,pkg,usr} folders together. Maybe this has already been handled at the point of rendering?

For cue def it would seem like the less often used output. So would this be enabled with a flag?

[leejanee]

1. @mpvl Yes, it is correct that expand v1.#Deployment and remove imports on user 1 side.
   In other words, process on the server-side should be as follows:

resource:  { // user 1
     ...
     metadata: kind: "Deployment"
     spec: replicas: *2 ｜ >=1 & <5
     ...
} & {  // user 2
     spec: replicas: 3
}

2. But, I sincerely hope that all references to be expanded (though ,the depth of the evaluation is limited). And If the referenced object cannot be found, it can be kept. as follows:

---raw----
x: {}
t: {name: string}
output: [ ... {t & x.value}]

---lookUp(output)---
[... {{name: string} & x.value}]

Although evaluating references may result in an exponential blowup, But if don‘t try to expand the reference, it mean that the reference definition will be lost

[mpvl]

@leejanee: CL https://review.gerrithub.io/c/cue-lang/cue/+/528214 hints at a solution where we could do an expansion where we include definitions outside of scope (e.g. from imports) as top-level let expression in the evaluation until all references are closed. This would create a self-contained file that is guaranteed to not have cycles.

An alternative would be to expand until we have a structural cycle.

I guess either of these would work for you. Will give it some more thought, but it seems that one of these, as least, is doable.

[mpvl]

And just to confirm: it is not sufficient for you to use, for instance, the cue.Final() option. This would also simplify defaults and close lists.

[wonderflow]

Hi @myitcv @mpvl We really appreciate to see the progress. Our team is trying to upgrade and there're some more compatible issues:

• cue: optional fields cannot be referenced in for loops of optional fields #1815 (This affect the most cause all our CUE files need to change, they're almost break change for our users.)
• syntax: syntax converted from value lost map struct #1813 (Maybe this one is related with this issue)
• encoding/gocode/gocodec: some functions use deprecated types #1806 (This not affect our upgrade for now, we can silence the go lint check)

[myitcv]

Our team is trying to upgrade and there're some more compatible issues

Thanks. Will take a look through these today.

[wonderflow]

#1816 one more case, maybe it's also related with the selfcontained and can be resolved together.

[mpvl]

#1816 one more case, maybe it's also related with the selfcontained and can be resolved together.

It indeed solve it, to some extent, but please see my answer there. It may not solve it how you want it, but at least in principle it is able to solve it so it will not be hard to iterate towards something workable.

[mpvl]

The changes that address this particular issue have been checked in. Note that the ultimate implementation is a bit different:

• There is no SelfContained option, as it is now simply default behavior (without expanding imports). CUE now detects when Syntax is called on part of the tree and then activates the algorithm.
• To also expand references to imports use InlineImports.

The reason for this approach is that in the old algorithm CUE would generate potentially invalid CUE. So this seems to be strictly better. It will also be better for debugging, as it will be clearer where errors are coming from.

Also, less options is better. :)

In moving from v0.2 to v0.3/v0.4, note that there are some performance issues with disjunctions. We know what it is and are working toward a solution, but just something to be mindful off.

[mpvl]

Going forward, there is certainly room to tweak the algorithm.

In my opinion, one of the current behaviors is incorrect. Consider

a: b: {
    c: a.b.d
    d: string
}

Applying the algorithm to a.b results in

c: d
d: string

In this case, though, since a points "outside" of the original structure, the value should arguably be pegged to the original value, not the self-contained one. This is how references would work within a CUE program anyway. So the result in this case should be

c: string
d: string

If the original had been

a: b: {
    c: d
    d: string
}

the result of calling Syntax for a.b would still be

c: d
d: string

of course.

[mpvl]

Another possible change is how much is inlined. Right now, there are a lot of cases where let is used where arguably it doesn't have to be.

This should be thought through carefully, though, as inlining indiscriminately can lead to polynomial expansion. See for instance https://en.wikipedia.org/wiki/Billion_laughs_attack.

[mpvl]

On final caveat is that the current implementation doesn't handle if a self-contained node points to its root or any of its ancestors (also holds for roots of the "external" values.

We still plan to address that, but the solution depends a bit on some of the design decisions noted above and whether we have value aliases for embeddings. The latter is the reason we now no longer support rewriting them.

[mpvl]

I'm keeping this issue open to allow for initial feedback to not get buried.

[wonderflow]

@myitcv Much appreciate for the work! We're trying but it seems the package is all internal. How can we use it as we're integrating CUE by using the go module?

[myitcv]

Mentioning here for the benefit of others too, you need to use the new cue.InlineImports() option. Here's a small example:

https://gist.github.com/myitcv/78bfb51d5061365f429372b8968087c5

[myitcv]

@wonderflow @FogDong just following up on the issues linked from above:

• cue: optional fields cannot be referenced in for loops of optional fields #1815 (This affect the most cause all our CUE files need to change, they're almost break change for our users.)

Replied in the issue. I'm not clear there is actually an issue here, just a bogus error value being reported. Perhaps you can confirm my understanding in the issue?

• syntax: syntax converted from value lost map struct #1813 (Maybe this one is related with this issue)

Commented in response to @mpvl. I think my conversation with @FogDong helped move this forward, but please let me know if there are any outstanding questions/issues.

• cue: Some functions are still requiring the deprecated struct #1806 (This not affect our upgrade for now, we can silence the go lint check)

Commented. You can work around this for now by casting a value of *cue.Context to *cue.Runtime.

[myitcv]

Per our discussions in various other issues, I think we have identified a better approach via the cue.Value API so I will mark this as closed. Please comment if this needs to remain open with details of outstanding issues. Thanks