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

cmd/cue: get go panics extracting package for type alias #1607

Closed
c-kruse opened this issue Mar 30, 2022 · 6 comments
Closed

cmd/cue: get go panics extracting package for type alias #1607

c-kruse opened this issue Mar 30, 2022 · 6 comments
Labels
get go issues related to cue get go NeedsFix panic

Comments

@c-kruse
Copy link

c-kruse commented Mar 30, 2022

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

$ cue version
cue version v0.4.2 linux/amd64

Does this issue reproduce with the latest release?

Y

What did you do?

I was experimenting with importing go type definitions instead of the protobuf definitions they are generated from, and hit a panic.

I ran again in a debugger to see what I was doing wrong, and kind of suspect it could be a more general problem when we extract package A, where package A imports package B, then package A uses an aliased type from package C that was declared in package B.

Minimal go project to reproduce this:

exec cue get go --local ./a

-- go.mod --

module github.com/c-kruse/repro

go 1.17
-- a/a.go --

package a

import "github.com/c-kruse/repro/b"

type Foo struct {
	Bar b.Type
}
-- b/b.go --

package b

import "github.com/c-kruse/repro/c"

type Type = c.Type
-- c/c.go --

package c

type Type int64

What did you expect to see?

Passing test.

What did you see instead?

> exec cue get go --local ./a
[stderr]
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x61f760]

goroutine 1 [running]:
cuelang.org/go/cmd/cue/cmd.recoverError(0x4000019e70)
        /home/myitcv/dev/cuelang/cue/cmd/cue/cmd/root.go:229 +0x98
panic({0x6bfc40, 0xcd98f0})
        /home/myitcv/gos/src/runtime/panic.go:838 +0x20c
cuelang.org/go/cmd/cue/cmd.(*extractor).extractPkg(0x400036bdd0, {0x4000036094, 0x2b}, 0x0)
        /home/myitcv/dev/cuelang/cue/cmd/cue/cmd/get_go.go:454 +0x60
cuelang.org/go/cmd/cue/cmd.(*extractor).extractPkg(0x400036bdd0, {0x4000036094, 0x2b}, 0x40001ac3c0)
        /home/myitcv/dev/cuelang/cue/cmd/cue/cmd/get_go.go:570 +0x920
cuelang.org/go/cmd/cue/cmd.extract(0x4000320f60, {0x400031eb80, 0x1, 0x2})
        /home/myitcv/dev/cuelang/cue/cmd/cue/cmd/get_go.go:433 +0x330
cuelang.org/go/cmd/cue/cmd.mkRunE.func1(0x4000331b80?, {0x400031eb80?, 0x2?, 0x2?})
        /home/myitcv/dev/cuelang/cue/cmd/cue/cmd/root.go:48 +0x70
github.com/spf13/cobra.(*Command).execute(0x4000331b80, {0x400031eb60, 0x2, 0x2})
        /home/myitcv/gostuff/pkg/mod/github.com/spf13/[email protected]/command.go:856 +0x4dc
github.com/spf13/cobra.(*Command).ExecuteC(0x4000330500)
        /home/myitcv/gostuff/pkg/mod/github.com/spf13/[email protected]/command.go:974 +0x360
github.com/spf13/cobra.(*Command).Execute(...)
        /home/myitcv/gostuff/pkg/mod/github.com/spf13/[email protected]/command.go:902
cuelang.org/go/cmd/cue/cmd.(*Command).Run(0x4000320f60, {0x4?, 0x4?})
        /home/myitcv/dev/cuelang/cue/cmd/cue/cmd/root.go:214 +0x60
cuelang.org/go/cmd/cue/cmd.mainErr({0x8a4390, 0x400003a0a0}, {0x40000320b0?, 0x40000021a0?, 0x4000073f28?})
        /home/myitcv/dev/cuelang/cue/cmd/cue/cmd/root.go:151 +0x5c
cuelang.org/go/cmd/cue/cmd.Main()
        /home/myitcv/dev/cuelang/cue/cmd/cue/cmd/root.go:133 +0x8c
main.main()
        /home/myitcv/dev/cuelang/cue/cmd/cue/main.go:24 +0x20
[exit status 2]
FAIL: /tmp/testscript2894356260/repro.txt/script.txt:1: unexpected command failure
error running repro.txt in /tmp/testscript2894356260/repro.txt
@c-kruse c-kruse added NeedsInvestigation Triage Requires triage/attention labels Mar 30, 2022
@mpvl mpvl added NeedsFix and removed NeedsInvestigation Triage Requires triage/attention labels Mar 31, 2022
@mpvl
Copy link
Member

mpvl commented Mar 31, 2022

The issue seems to be that the package import is not looked up in the package in which it is important, but rather always the top-level package. This is why this only fails with two layers of imports.

@c-kruse
Copy link
Author

c-kruse commented Mar 31, 2022

More generally, the issue is that cue get go makes an assumption that all of the types referenced in a given package belong to either the current package or a package imported by the current package. This was probably a safe assumption until go added type aliases in go 1.9.

It does handle nested imports just fine. In my example above replacing the alias with a type declaration s/type Type = c.Type/type Type c.Type/ would pass.

Life permitting I will attempt a PR this weekend.

@mpvl
Copy link
Member

mpvl commented Apr 1, 2022

Correct, it indeed only occurs with type aliases. The simplest and arguably best thing to do is to preserve the indirection of type aliases. I looked at the underlying types values that are generated, though, and it looks like the alias information is discarded for field types in the Go types. So this may not be so easy to accomplish. I hope I'm missing something, of course.

@rogpeppe rogpeppe added the panic label Apr 26, 2022
@c-kruse
Copy link
Author

c-kruse commented May 14, 2022

Life permitting I will attempt a PR this weekend.

That weekend really got away from me 😆. Confirmed that this is still an issue in v0.4.3, and would (of course) welcome anyone to contribute a fix.

@myitcv myitcv added the get go issues related to cue get go label May 16, 2022
@myitcv myitcv changed the title cmd/cue go get panics extracting package for type alias cmd/cue: get go panics extracting package for type alias May 16, 2022
@myitcv
Copy link
Member

myitcv commented May 16, 2022

No worries, @c-kruse!

I updated the description with a self-contained repro for ease of checking/fixing this issue. We have a wiki which gives tips on how to easily create such repros.

@tommyknows
Copy link

I just ran into this as well and had a look at how to fix this.

The repro as given above can be fixed by scanning through all imported packages imports, but that actually doesn't work 100% for all cases. After implementing this (very rudimentary), the repro ran through fine, but I tried to cue get go github.com/kong/kubernetes-ingress-controller/v2/pkg/apis/..., which still failed (and panicked).

A solution to this is to instead of going through the package's imported packages, go through all packages that are known.

diff --git a/cmd/cue/cmd/get_go.go b/cmd/cue/cmd/get_go.go
index b67bdf4e..20a62fb9 100644
--- a/cmd/cue/cmd/get_go.go
+++ b/cmd/cue/cmd/get_go.go
@@ -580,7 +580,16 @@ func (e *extractor) extractPkg(root string, p *packages.Package) error {
        for path := range e.usedPkgs {
                if !e.done[path] {
                        e.done[path] = true
-                       p := p.Imports[path]
+                       for _, pkg := range e.pkgs {
+                               if pkg.PkgPath == path {
+                                       p = pkg
+                                       break
+                               }
+                       }
+
+                       if p == nil {
+                               return fmt.Errorf("no package %v found", path)
+                       }
                        if err := e.extractPkg(root, p); err != nil {
                                return err
                        }

@myitcv myitcv added the zGarden label Jun 13, 2023
@mvdan mvdan removed the zGarden label Feb 8, 2024
cueckoo pushed a commit that referenced this issue Mar 17, 2024
Most Go APIs like go/types.Package.Path and go/packages.Package.PkgPath
use full canonical package paths, whereas go/packages.Package.Imports
uses source import paths as they appear in the import declarations.

We mixed the two, using paths from go/types to index into Imports,
resulting in nil packages in the edge cases when they are different.
One such instance is vendored packages, whose full path may be
"vendor/foo/bar" when they are simply imported as "foo/bar".

This also fixes a similar panic with some type aliases,
in particular when an alias points to an indirectly imported package,
as such a package would be missing from the Imports map.

The two added testscripts reproduced both panics without the fix,
and now work as expected with the fix.

Fixes #1607.
Fixes #2046.

Change-Id: I51df3465da76b39f031855982b48552604817f80
Signed-off-by: Thomas Way <[email protected]>
cueckoo pushed a commit that referenced this issue Mar 17, 2024
Most Go APIs like go/types.Package.Path and go/packages.Package.PkgPath
use full canonical package paths, whereas go/packages.Package.Imports
uses source import paths as they appear in the import declarations.

We mixed the two, using paths from go/types to index into Imports,
resulting in nil packages in the edge cases when they are different.
One such instance is vendored packages, whose full path may be
"vendor/foo/bar" when they are simply imported as "foo/bar".

This also fixes a similar panic with some type aliases,
in particular when an alias points to an indirectly imported package,
as such a package would be missing from the Imports map.

The two added testscripts reproduced both panics without the fix,
and now work as expected with the fix.

Fixes #1607.
Fixes #2046.

Change-Id: I51df3465da76b39f031855982b48552604817f80
Signed-off-by: Thomas Way <[email protected]>
Co-authored-by: Daniel Martí <[email protected]>
Signed-off-by: Daniel Martí <[email protected]>
cueckoo pushed a commit that referenced this issue Apr 3, 2024
Most Go APIs like go/types.Package.Path and go/packages.Package.PkgPath
use full canonical package paths, whereas go/packages.Package.Imports
uses source import paths as they appear in the import declarations.

We mixed the two, using paths from go/types to index into Imports,
resulting in nil packages in the edge cases when they are different.
One such instance is vendored packages, whose full path may be
"vendor/foo/bar" when they are simply imported as "foo/bar".

This also fixes a similar panic with some type aliases,
in particular when an alias points to an indirectly imported package,
as such a package would be missing from the Imports map.

The two added testscripts reproduced both panics without the fix,
and now work as expected with the fix.

Fixes #1607.
Fixes #2046.

Change-Id: I51df3465da76b39f031855982b48552604817f80
Signed-off-by: Thomas Way <[email protected]>
Co-authored-by: Daniel Martí <[email protected]>
Signed-off-by: Daniel Martí <[email protected]>
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1173100
Reviewed-by: Paul Jolly <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
(cherry picked from commit b648cf4)
cueckoo pushed a commit that referenced this issue Apr 3, 2024
Most Go APIs like go/types.Package.Path and go/packages.Package.PkgPath
use full canonical package paths, whereas go/packages.Package.Imports
uses source import paths as they appear in the import declarations.

We mixed the two, using paths from go/types to index into Imports,
resulting in nil packages in the edge cases when they are different.
One such instance is vendored packages, whose full path may be
"vendor/foo/bar" when they are simply imported as "foo/bar".

This also fixes a similar panic with some type aliases,
in particular when an alias points to an indirectly imported package,
as such a package would be missing from the Imports map.

The two added testscripts reproduced both panics without the fix,
and now work as expected with the fix.

Fixes #1607.
Fixes #2046.

Change-Id: I51df3465da76b39f031855982b48552604817f80
Signed-off-by: Thomas Way <[email protected]>
Co-authored-by: Daniel Martí <[email protected]>
Signed-off-by: Daniel Martí <[email protected]>
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1173100
Reviewed-by: Paul Jolly <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
(cherry picked from commit b648cf4)
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1190977
TryBot-Result: CUEcueckoo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
get go issues related to cue get go NeedsFix panic
Projects
None yet
Development

No branches or pull requests

6 participants