cue get go does not work consistently

[verdverm]

ceu get go does not load its own interfaces when a go vendor directory is present and CUE is not in the dependencies.

The error is in initInterfaces() (https://github.com/cuelang/cue/blob/master/cmd/cue/cmd/get_go.go#L287)

packages.Load does not return an error, but there are errors in p[0].Errors

We should be checking for these errors where this function is used.

---

repro:

mkdir intstr && cd intstr
go mod init foo.bar/intstr
cue mod init foo.bar/intstr
cat << EOF > repro.go
package intstr
import (
        "k8s.io/apimachinery/pkg/util/intstr"
)
_ = intstr.IntOrString
EOF
go mod vendor
cue get go k8s.io/apimachinery/pkg/util/intstr

---

possible resolutions:

1. check for the errors and return better messages
2. modify the Config passed to packages.Load
3. embed and load the toTop and toText interfaces

(1) I'd generally prefer to have a solution that does not require the user to have Cue as a dependency. This seems like unpleasant DX, essentially telling them they need to have an artificial dependency via overly complex instructions.

(2) could be an easy solution. However, I'm leery of modifying the Config so that Cue code is fetched automatically. This might make some SecOps people unhappy / break in corp intranets.

(3) Config has an Overlay we could use to embed these files as strings. This seems like the best way to go.

We will also want to catch the errors in the other locations and provide an error message there.

What do others think?

[NicolasRouquette]

After trying a few variations, I came to the conclusion that for reproducible behavior,
it is really helpful to use 'go mod vendor'; however, in that case, we are limited to
go 1.13 to avoid this problem.

[verdverm]

I have a fix here: https://cue-review.googlesource.com/c/cue/+/8021

Not pretty or ideal... ended up embedding and temp writing the files to disk...

Maybe some of the other Go compiler API could remove this requirement? Not sure

[mpvl]

Interesting. Thanks for identifying the issue!

[myitcv]

@verdverm - just so that I'm clear, can you explain from the repro above what you believe to be not working?

What version of Go are you using?

[NicolasRouquette]

With go 1.13, following the instructions above, you get the correct behavior; that is, cue.mod/gen/k8s.io/apimachinery/pkg/util/intstr/intstr_go_gen.cue will have:

// IntOrString is a type that can hold an int32 or a string.  When used in
// JSON or YAML marshalling and unmarshalling, it produces or consumes the
// inner type.  This allows you to have, for example, a JSON field that can
// accept a name or number.
// TODO: Rename to Int32OrString
//
// +protobuf=true
// +protobuf.options.(gogoproto.goproto_stringer)=false
// +k8s:openapi-gen=true
#IntOrString: _

Use go > 1.13, e.g., 1.15.6, then we get instead:

// IntOrString is a type that can hold an int32 or a string.  When used in
// JSON or YAML marshalling and unmarshalling, it produces or consumes the
// inner type.  This allows you to have, for example, a JSON field that can
// accept a name or number.
// TODO: Rename to Int32OrString
//
// +protobuf=true
// +protobuf.options.(gogoproto.goproto_stringer)=false
// +k8s:openapi-gen=true
#IntOrString: {
        Type:   #Type  @protobuf(1,varint,opt,name=type,casttype=Type)
        IntVal: int32  @protobuf(2,varint,opt,name=intVal)
        StrVal: string @protobuf(3,bytes,opt,name=strVal)
}

This happens with cue 0.3.0-alpha6, 0.3.0-beta1 and 0.3.0-beta2
I haven't tried other versions of 0.3.0

[myitcv]

@NicolasRouquette - and just to confirm, the secondfirst of these outputs is a) correct and b) what you are expecting?

Edit: critical edit

[myitcv]

With go 1.13

We probably need to state clearly somewhere what versions of Go are supported. Is there a reason you are using Go 1.13?

(asking in the context of https://golang.org/doc/devel/release.html#policy)

[myitcv]

Ok, here's my now complete analysis.

Example 1

For completeness, here is a testscript-style test that asserts the correct result:

exec go mod init foo.bar/intstr
exec cue mod init foo.bar/intstr
exec cue get go k8s.io/apimachinery/pkg/util/intstr
cmp cue.mod/gen/k8s.io/apimachinery/pkg/util/intstr/intstr_go_gen.cue intstr_go_gen.cue.golden

-- repro.go --
package intstr
import (
        "k8s.io/apimachinery/pkg/util/intstr"
)
_ = intstr.IntOrString
-- intstr_go_gen.cue.golden --
// Code generated by cue get go. DO NOT EDIT.

//cue:generate cue get go k8s.io/apimachinery/pkg/util/intstr

package intstr

// IntOrString is a type that can hold an int32 or a string.  When used in
// JSON or YAML marshalling and unmarshalling, it produces or consumes the
// inner type.  This allows you to have, for example, a JSON field that can
// accept a name or number.
// TODO: Rename to Int32OrString
//
// +protobuf=true
// +protobuf.options.(gogoproto.goproto_stringer)=false
// +k8s:openapi-gen=true
#IntOrString: _

// Type represents the stored type of IntOrString.
#Type: int64 // #enumType

#enumType:
	#Int |
	#String

#Int:    #Type & 0
#String: #Type & 1

Note this test does not include a vendor step.

Analysis based on the above:

• Example 1 works with Go Version <= 1.15X by virtue of cmd/go build operations (and this includes the cmd/go list that happens as part of a go/packages.Load) automatically updating go.mod as required. That is to say, when cue get go attempts to load type information for the cuelang.org/go/cmd/cue/cmd/interfaces package, it will add a dependency to cuelang.org/go to the go.mod as required
• Example 1 fails with Go 1.16. This is because in 1.16, by default build commands do not update go.{mod,sum} - they behave as if -mod=readonly was specified. See cmd/go: default to & improve -mod=readonly golang/go#40728

Example 2

Now consider:

exec go mod init foo.bar/intstr
exec cue mod init foo.bar/intstr
exec go mod vendor
exec cue get go k8s.io/apimachinery/pkg/util/intstr
cmp cue.mod/gen/k8s.io/apimachinery/pkg/util/intstr/intstr_go_gen.cue intstr_go_gen.cue.golden

-- repro.go --
package intstr
import (
        "k8s.io/apimachinery/pkg/util/intstr"
)
_ = intstr.IntOrString
-- intstr_go_gen.cue.golden --
// Code generated by cue get go. DO NOT EDIT.

//cue:generate cue get go k8s.io/apimachinery/pkg/util/intstr

package intstr

// IntOrString is a type that can hold an int32 or a string.  When used in
// JSON or YAML marshalling and unmarshalling, it produces or consumes the
// inner type.  This allows you to have, for example, a JSON field that can
// accept a name or number.
// TODO: Rename to Int32OrString
//
// +protobuf=true
// +protobuf.options.(gogoproto.goproto_stringer)=false
// +k8s:openapi-gen=true
#IntOrString: _

// Type represents the stored type of IntOrString.
#Type: int64 // #enumType

#enumType:
	#Int |
	#String

#Int:    #Type & 0
#String: #Type & 1

i.e. we add a go mod vendor step.

• Example 2 works with Go <= 1.13X
• Example 2 fails with Go >= 1.14, because 1.14 introduced the concept of honouring a vendor directory if it exists (cmd/go: automatically check and use vendored packages golang/go#33848 (comment)). Furthermore, a vendor is not modified/updated if a dependency is missing, unlike the pre Go 1.16 behaviour without vendor

Conclusion

• The version of cuelang.org/go/cmd/cue/cmd/interfaces loaded should correspond to the version of cmd/cue being used, not a version loaded via a go.mod requirement on cuelang.org/go
• As such, we should embed cuelang.org/go/cmd/cue/cmd/interfaces (@verdverm I will comment over in https://cue-review.googlesource.com/c/cue/+/8021 if you are still interested in pursuing that CL?). This change will include proper surfacing of errors of any packages loaded, including cuelang.org/go/cmd/cue/cmd/interfaces
• In https://cue-review.googlesource.com/c/cue/+/8021 we should add a cue get go test that exercises the loading of types that follow these rules:

cue/cmd/cue/cmd/get_go.go

Lines 99 to 105 in f0c025c

	- a type that implements MarshalJSON, UnmarshalJSON, MarshalYAML, or
	UnmarshalYAML is translated to top (_) to indicate it may be any
	value. For some Go core types for which the implementation of these
	methods is known, like time.Time, the type may be more specific.
	- a type implementing MarshalText or UnmarshalText is represented as
	the CUE type string

• Given the failure of Example 1 with Go 1.16, we need to make some sort of change/fix. I think this also gives us the opportunity to decide whether we want cue get go to be a wrapper around go get or not. The "fix" for Go 1.16 will be to add a cmd/go get step as part of cue get go. However, I think then forces us to have cue get go mirror cmd/go get in terms of docs/behaviour. This feels a bit unnecessary to my mind. Perhaps better would be to not try and wrap cmd/go get at all, but instead simply fail if the package(s) passed to cue get go fail to load (which would require the user to do something via cmd/go {get,mod tidy}). If we go down this path (which seems to make sense) then I'm not sure that cue get is necessarily the right/best name - but it's not totally clear cut one way or the other

[myitcv]

Noting an offline conversation with @mpvl. Next steps:

• fix the loading of the cuelang.org/go/cmd/cue/cmd/interfaces package by embedding, via https://cue-review.googlesource.com/c/cue/+/8021 or equivalent. Add tests that exercise the various rules in

  cue/cmd/cue/cmd/get_go.go

  Lines 99 to 105 in f0c025c

  	- a type that implements MarshalJSON, UnmarshalJSON, MarshalYAML, or
  	UnmarshalYAML is translated to top (_) to indicate it may be any
  	value. For some Go core types for which the implementation of these
  	methods is known, like time.Time, the type may be more specific.
  	- a type implementing MarshalText or UnmarshalText is represented as
  	the CUE type string

• decide which errors for loading cuelang.org/go/cmd/cue/cmd/interfaces or arguments to cue get go should be surfaced. If we are unable to resolve a package, that is a fatal error. However a package not type checking is arguably not fatal: cue get go could be considered a best-effort if code does not fully type check
• change cue get go to error in case any package arguments it is passed cannot be loaded (note this does not include cuelang.org/go/cmd/cue/cmd/interfaces). This will mean the user has to ensure that a package argument to cue get go can be resolved via go list -mod=readonly pkgs...
• update all docs to add additional go get steps as necessary (although it appears the Kubernetes example is already clear on this: https://github.com/cuelang/cue/blame/f0c025c1a4cb7602e497445a58578212730c3698/doc/tutorial/kubernetes/README.md#L1062)
• deprecate cue get go in favour of cue import go (cmd/cue: rename 'get go' to 'import go' #646) - this naming better reflect that cmd/cue is not doing any sort of dependency management a la go get. Must include updating all docs
• in a later release, remove cue get go

[myitcv]

One further aspect we will need to consider, that isn't purely a function of the changes in Go 1.16 but is exacerbated by it.

Consider the following:

# Initial run
exec go get k8s.io/api/core/v1@v0.20.2
exec cue get go k8s.io/api/core/v1
exec cue def -e '#Service' k8s.io/api/core/v1

# Tidy then re-run
exec go mod tidy
exec cue get go k8s.io/api/core/v1

-- go.mod --
module blah.com

-- blah.go --
package blah

-- cue.mod/module.cue --
module: "blah.com"

With Go < 1.16 this will "work" because, despite the go mod tidy, the go/packages.Load call in the subsequent cue get go will re-add the module providing k8s.io/api/core/v1 to the go.mod.

That is to say, the side effects of Go < 1.16 were hiding a bigger issue, namely that the dependencies used as input to cue get go are not captured anywhere.

There is a precedent however for keeping track of such dependencies: tools. The current best practice for keeping track of tool dependencies is as follows (per golang/go#25922 (comment)): create a build-ignored .go file that declares the dependencies as underscore-imports. Rewriting the example above:

# Initial run
exec go get k8s.io/api/core/v1@v0.20.2
exec cue get go k8s.io/api/core/v1
exec cue def -e '#Service' k8s.io/api/core/v1

# Tidy then re-run
exec go mod tidy
exec cue get go k8s.io/api/core/v1

-- go.mod --
module blah.com

-- blah.go --
package blah

-- cue.mod/module.cue --
module: "blah.com"

-- cuegetgo.go --
// +build cuegetgo

package cuegetgo

import (
	_ "k8s.io/api/core/v1"
)

This advice works for all Go versions, and should probably have been the advice from the beginning (otherwise you have an unstable go.{mod,sum} files).

This is a point to consider for the docs/Go 1.16 docs for the cue get go/cue import go command.

[NicolasRouquette]

@myitcv Thanks for improving our earlier analysis; LGTM.

I also want to note that during my analysis, I found a potentially confounding problem.
See: https://cuelang.slack.com/archives/CR7BJ142F/p1609288836062200?thread_ts=1609225131.058200&cid=CR7BJ142F

It seems to me that there's at least one bug, calls to packages.Load(...) from "golang.org/x/tools/go/packages" currently only check for errors in the return.
However, it seems that the callers should also check whether the returned packages have errors according to the function's doc:
// Load loads and returns the Go packages named by the given patterns.
//
// Config specifies loading options;
// nil behaves the same as an empty Config.
//
// Load returns an error if any of the patterns was invalid
// as defined by the underlying build system.
// It may return an empty list of packages without an error,
// for instance for an empty expansion of a valid wildcard.
// Errors associated with a particular package are recorded in the
// corresponding Package's Errors list, and do not cause Load to
// return an error. Clients may need to handle such errors before
// proceeding with further analysis. The PrintErrors function is
// provided for convenient display of all errors.
func Load(cfg *Config, patterns ...string) ([]*Package, error) {

[myitcv]

Thanks, @NicolasRouquette - yes, that point is covered in #621 (comment):

• decide which errors for loading cuelang.org/go/cmd/cue/cmd/interfaces or arguments to cue get go should be surfaced. If we are unable to resolve a package, that is a fatal error. However a package not type checking is arguably not fatal: cue get go could be considered a best-effort if code does not fully type check

With the loading of the embedded cuelang.org/go/cmd/cue/cmd/interfaces we should tolerate no errors whatsoever.

With the loading of the arguments to cue get go we should probably tolerate parse and type errors, but no others. I've made comments to this effect over in https://cue-review.googlesource.com/c/cue/+/8021 (we should continue that dialogue there if people disagree)

[NicolasRouquette]

FYI: This bug is still present in cue-0.3.0-beta3.

[myitcv]

That's expected - the CL that fixes this is in progress. This issue will close automatically when the CL is merged.

[cueckoo]

This issue has been migrated to cue-lang/cue#621.

For more details about CUE's migration to a new home, please see cue-lang/cue#1078.