Skip to content

internal/encoding/gotypes: support go import aliasing #4028

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

internal/encoding/gotypes: support go import aliasing #4028

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions cmd/cue/cmd/testdata/script/cmd_exp_gengo_alias.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
# reproduction for https://github.com/cue-lang/cue/issues/3974
exec cue exp gengotypes
cmp cue_types_main_gen.go expect-gengo

-- cue.mod/module.cue --
module: "mod.test/foo"
language: version: "v0.13.0"

-- cue.mod/gen/k8s.io/apimachinery/pkg/apis/meta/v1/types_go_gen.cue --
package v1

#ObjectMeta: {}

-- cue.mod/gen/k8s.io/api/core/v1/types_go_gen.cue --
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you rewrite this test so it doesn't use cue.mod/gen? e.g. make these sub-packages inside the same module.

package v1

#PodSpec: {}

-- main.cue --
package main

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/api/core/v1"
)

#Spec: {
meta: metav1.#ObjectMeta
spec: v1.#PodSpec
}

-- expect-gengo --
// Code generated by "cue exp gengotypes"; DO NOT EDIT.

package main

import (
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

type Spec struct {
Meta metav1.ObjectMeta `json:"meta"`

Spec v1.PodSpec `json:"spec"`
}
59 changes: 55 additions & 4 deletions internal/encoding/gotypes/generate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,15 @@ func Generate(ctx *cue.Context, insts ...*build.Instance) error {
g.pkgRoot = instVal
g.importCuePkgAsGoPkg = make(map[string]string)

// gather the import aliases for the instance.
// NOTE: this implementation means that if an import alias is employed in one file
// it will be used anywhere that package is referenced
importAliases, err := gatherImportAliases(inst)
if err != nil {
return err
}
g.importAliases = importAliases

iter, err := instVal.Fields(cue.Definitions(true))
if err != nil {
return err
Expand Down Expand Up @@ -100,7 +109,7 @@ func Generate(ctx *cue.Context, insts ...*build.Instance) error {
buf = fmt.Appendf(buf, format, args...)
}
printf("// Code generated by \"cue exp gengotypes\"; DO NOT EDIT.\n\n")
goPkgName := goPkgNameForInstance(inst, instVal)
goPkgName := goPkgNameForInstance(inst, instVal, g.importAliases)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is rather crucial - you must NOT use importAliases here, so you should use a nil map instead. this tracks which Go packages (by name) we generate per directory, as we can only generate one package per directory. This is entirely unrelated to what aliases a package is imported under.

if prev, ok := goPkgNamesDoneByDir[inst.Dir]; ok && prev != goPkgName {
return fmt.Errorf("cannot generate two Go packages in one directory; %s and %s", prev, goPkgName)
} else {
Expand All @@ -112,7 +121,12 @@ func Generate(ctx *cue.Context, insts ...*build.Instance) error {
if len(importedGo) > 0 {
printf("import (\n")
for _, path := range importedGo {
printf("\t%q\n", path)
quotedPath := fmt.Sprintf("%q", path)
alias, ok := g.importAliases[path]
if ok {
quotedPath = fmt.Sprintf("%s %s", alias, quotedPath)
}
printf("\t%s\n", quotedPath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd avoid the two-layered formatting, like:

if alias, ok := g.importAliases[path]; ok {
    printf("\t%s %q\n", alias, path)
} else {
    printf("\t%q\n", path)
}

}
printf(")\n")
}
Expand Down Expand Up @@ -217,6 +231,9 @@ type generator struct {

// def tracks the generation state for a single CUE definition.
def *generatedDef

// importAliases maps package names to the alias
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs clarifying - is it to the first alias that we found for each package? I reckon it should be, but then we should document it as such, and update the code to ignore any second or third alias for the same package path.

but also, what happens if file f1.cue does import same "foo.com/bar/p1", and f2.cue does import same "foo.com/bar/p2"? As far as I can see, you'll generate Go code which imports both packages aliased as same.

I think collecting the original aliases is a good idea, because more often than not it will lead to better results - the user will most likely have chosen reasonable aliases. But you'll need some sort of strategy to deal with conflicts, and add a test for that edge case.

I would personally suggest to track which aliases are already taken, and when you encounter an aliased import where the alias is already taken, you can add a number at the end - e.g. the second same above could become same2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also worth noting that you can have duplicates between non-aliased package imports and aliased imports too, so that must also have a test and be checked for duplicates. For example, import "foo.com/bar/same" in f1.cue, and import same "foo.com/bar/p2" in f2.cue.

importAliases map[string]string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that we organize fields on a per-package and per-definition basis. This should be on a per-package basis I assume.

}

type qualifiedPath = string // [build.Instance.ImportPath] + " " + [cue.Path.String]
Expand Down Expand Up @@ -323,6 +340,7 @@ func (g *generator) emitType(val cue.Value, optionalStg optionalStrategy) (typeF
if err != nil {
return facts, fmt.Errorf("cannot parse @go type expression: %w", err)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please avoid whitespace changes elsewhere, as they are more likely to cause git conflicts down the line

for _, pkgPath := range importedByName {
g.importCuePkgAsGoPkg[pkgPath] = pkgPath
}
Expand Down Expand Up @@ -679,11 +697,15 @@ func goValueAttr(val cue.Value) cue.Attribute {

// goPkgNameForInstance determines what to name a Go package generated from a CUE instance.
// By default this is the CUE package name, but it can be overriden by a @go() package attribute.
func goPkgNameForInstance(inst *build.Instance, instVal cue.Value) string {
// Import aliases will also checked at this point and inserted if found.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd reword this slightly, like:

When supplying importAliases, and if no package attribute is found,
the returned package name reflects the alias name that the package is being imported as.

func goPkgNameForInstance(inst *build.Instance, instVal cue.Value, aliasLookup map[string]string) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name the parameter importAliases for consistency.

attr := goValueAttr(instVal)
if s, _ := attr.String(0); s != "" {
return s
}
if alias, ok := aliasLookup[inst.ImportPath]; ok {
return alias
}
return inst.PkgName
}

Expand Down Expand Up @@ -751,7 +773,7 @@ func (g *generator) emitTypeReference(val cue.Value) (bool, typeFacts, error) {
facts.isNillable = true // pointers can be nil
}
if root != g.pkgRoot {
g.def.printf("%s.", goPkgNameForInstance(inst, root))
g.def.printf("%s.", goPkgNameForInstance(inst, root, g.importAliases))
}
g.def.printf("%s", name)
return true, facts, nil
Expand All @@ -772,3 +794,32 @@ func emitDocs(printf func(string, ...any), name string, groups []*ast.CommentGro
}
}
}

// gatherImportAliases collects the aliases from imports across the instance.
// We explicitly ignore "_" aliases because CUE does not support.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CUE source code should never be using underscore-aliased imports, so I would delete this line as well as the check for _ below - it just shouldn't happen

func gatherImportAliases(inst *build.Instance) (map[string]string, error) {
fileAliases := make(map[string]string)
for _, f := range inst.Files {
for _, d := range f.Decls {
impDecl, ok := d.(*ast.ImportDecl)
if !ok {
continue
}
for _, s := range impDecl.Specs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loop directly over f.Imports, which will save you a handful of lines

if s.Path == nil || s.Name == nil {
continue
}
alias := s.Name.Name
if alias == "_" {
continue
}
path, err := strconv.Unquote(s.Path.Value)
if err != nil {
return nil, err
}
fileAliases[path] = alias
}
}
}
return fileAliases, nil
}