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

go/types: type-check embedded methods in wrong scope #25008

Closed
nabice opened this issue Apr 23, 2018 · 7 comments
Closed

go/types: type-check embedded methods in wrong scope #25008

nabice opened this issue Apr 23, 2018 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@nabice
Copy link

nabice commented Apr 23, 2018

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version devel +d11df8baa8 Sun Apr 22 22:32:11 2018 +0000 linux/amd64

Does this issue reproduce with the latest release?

no, only recent commit d11df8b

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/nabice/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/nabice/go:/home/nabice/skygo/go"
GORACE=""
GOROOT="/home/nabice/source/go"
GOTMPDIR=""
GOTOOLDIR="/home/nabice/source/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build162693936=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Two go files:
a.go

package main

import (
        "fmt"
        "io"
)

type A interface {
        io.Reader
}

type A1 struct {
}

func (d *A1) Read(p []byte) (n int, err error) {
        fmt.Println("A1")
        return 0, nil
}

func bbb(t A) {
        t.Read(nil)
}

func main() {
        a := new(A1)
        bbb(a)
}

b.go

package main

type B interface {
    A
}

go vet a.go b.go runs OK
go vet b.go a.go failed.

The reason is that, in file src/go/types/interfaces.go:226, We pass wrong context(scope) to lookup for "io". So typecheck cannot found io.Reader.

stack trace:

 0  0x000000000062ab05 in go/types.(*Checker).infoFromTypeLit
    at ./interfaces.go:226
 1  0x000000000062b45a in go/types.(*Checker).infoFromTypeName
    at ./interfaces.go:339
 2  0x000000000062a91a in go/types.(*Checker).infoFromTypeLit
    at ./interfaces.go:224
 3  0x0000000000658272 in go/types.(*Checker).interfaceType
    at ./typexpr.go:545

When we check b.go firstly, go tries to get A's info from b.go's scope

What did you expect to see?

No error found

What did you see instead?

# command-line-arguments

./a.go:21:4: invalid operation: t (variable of type A) has no field or method Read
vet: typecheck failures
@FiloSottile FiloSottile changed the title type-check embedded methods in wrong scope go/types: type-check embedded methods in wrong scope Apr 24, 2018
@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 24, 2018
@FiloSottile FiloSottile added this to the Go1.11 milestone Apr 24, 2018
@FiloSottile
Copy link
Contributor

/cc @griesemer

Looks similar to #23914 and #24140

@griesemer griesemer self-assigned this Apr 24, 2018
@griesemer
Copy link
Contributor

@nabice Thanks for this report and the (correct) analysis! Fix forthcoming.

A shorter reproducer:

  • a.go
package p

import "io"

type A interface {
        io.Reader
}

func f(a A) {
        a.Read(nil)
}
  • b.go
package p

type B interface {
    A
}

@gopherbot
Copy link

Change https://golang.org/cl/109139 mentions this issue: go/types: use correct (file) scopes when computing interface method sets

@myitcv
Copy link
Member

myitcv commented Apr 27, 2018

@griesemer we just had another instance of this via gopherjs/gopherjs#808 - would you consider back-porting this fix to the 1.10 series?

@myitcv
Copy link
Member

myitcv commented Apr 27, 2018

Edit: fixed typo below in second paragraph.

Just to add a bit more colour.

Using 1.10.1, type checking gorgonia.org/gorgonia with go/types succeeds when using importer.For("gc", nil), but fails with importer.For("source", nil).

Using tip it succeeds in both cases (the fix was introduced by https://go-review.googlesource.com/c/go/+/109139).

Here are the repro steps I used:

cd `mktemp -d`
export GOPATH=$PWD
go get gorgonia.org/gorgonia
go install $(go list -f '{{join .Deps " "}}' gorgonia.org/gorgonia )
cat <<EOD > check.go
// +build ignore

package main

import (
	"fmt"
	"go/ast"
	"go/build"
	"go/importer"
	"go/parser"
	"go/token"
	"go/types"
	"os"
	"path/filepath"
)

func main() {
	bpkg, err := build.Import("gorgonia.org/gorgonia", ".", 0)
	if err != nil {
		fatalf("failed to import .: %v", err)
	}

	fset := token.NewFileSet()
	var files []*ast.File

	for _, f := range bpkg.GoFiles {
		fp := filepath.Join(bpkg.Dir, f)
		fi, err := os.Open(fp)
		if err != nil {
			fatalf("failed to open %v: %v", fp, err)
		}

		pf, err := parser.ParseFile(fset, fp, fi, 0)
		if err != nil {
			fatalf("failed to parse %v: %v", fp, err)
		}

		files = append(files, pf)
	}

	{
		conf := types.Config{
			Importer: importer.For("gc", nil),
		}

		_, err = conf.Check("gorgonia.org/gorgonia", fset, files, nil)
		if err != nil {
			fatalf("type checking failed using gc importer: %v", err)
		}
	}

	{
		conf := types.Config{
			Importer: importer.For("source", nil),
		}

		_, err = conf.Check("gorgonia.org/gorgonia", fset, files, nil)
		if err != nil {
			fatalf("type checking failed using source importer: %v", err)
		}
	}
}

func fatalf(format string, args ...interface{}) {
	fmt.Fprintf(os.Stderr, format+"\n", args...)
	os.Exit(1)
}
EOD
go run check.go 

@griesemer
Copy link
Contributor

@myitcv While the CL https://go-review.googlesource.com/c/go/+/109139 fixed this specific issue, the fix is in a (significant) chunk of code that doesn't even exist in the 1.10 version of go/types (https://tip.golang.org/src/go/types/interfaces.go was added, and https://golang.org/src/go/types/ordering.go was removed). So back-porting this fix implies a much larger change than what meets the eye.

You may be able to (temporarily) work around the problem by re-arranging some your source code (relative order of method and interface declarations). It doesn't always work but it's worth a try.

Is there a reason not to move to 1.11 in gorgonia, or perhaps (for now) vendor the latest version of go/types?

@myitcv
Copy link
Member

myitcv commented May 1, 2018

the fix is in a (significant) chunk of code that doesn't even exist in the 1.10 version of go/types

Ah, I hadn't realised this. Thank you for clarifying.

... or perhaps (for now) vendor the latest version of go/types?

Excellent suggestion, thank you. We can do that within GopherJS (which is ultimately where we get bitten by this) until we release GopherJS 1.11 that corresponds to Go 1.11.

myitcv added a commit to myitcv/gopherjs that referenced this issue May 1, 2018
Per golang/go#25008 (comment) the
fix for method set calculation will not be back ported to Go 1.10.
Instead gri suggests we vendor the latest version of go/types. This PR
does just that. It is effectively a temporary sticking plaster until Go
1.11 lands, or more specifically until we release GopherJS for Go 1.11.

In order to successfully vendor go/types, however, we need to move
compiler/vendor to the repo root (because build also uses go/types). And
because golang.org/x/tools/go/gcexportdata and
golang.org/x/tools/go/types/typeutil also reference go/types we need to
vendor them as well.

vendor/vendor.json is updated using govendor, with the one exception
being a manual addition for go/types (govendor doesn't appear to
understand how to vendor a standard library package).

Fixes gopherjs#808.
myitcv added a commit to myitcv/gopherjs that referenced this issue Jul 2, 2018
Per golang/go#25008 (comment) the
fix for method set calculation will not be back ported to Go 1.10.
Instead gri suggests we vendor the latest version of go/types. This PR
does just that. It is effectively a temporary sticking plaster until Go
1.11 lands, or more specifically until we release GopherJS for Go 1.11.

In order to successfully vendor go/types, however, we need to move
compiler/vendor to the repo root (because build also uses go/types). And
because golang.org/x/tools/go/gcexportdata and
golang.org/x/tools/go/types/typeutil also reference go/types we need to
vendor them as well.

vendor/vendor.json is updated using govendor, with the one exception
being a manual addition for go/types (govendor doesn't appear to
understand how to vendor a standard library package).

Fixes gopherjs#808.
myitcv added a commit to myitcv/gopherjs that referenced this issue Jul 2, 2018
Per golang/go#25008 (comment) the
fix for method set calculation will not be back ported to Go 1.10.
Instead gri suggests we vendor the latest version of go/types. This PR
does just that. It is effectively a temporary sticking plaster until Go
1.11 lands, or more specifically until we release GopherJS for Go 1.11.

In order to successfully vendor go/types, however, we need to move
compiler/vendor to the repo root (because build also uses go/types). And
because golang.org/x/tools/go/gcexportdata and
golang.org/x/tools/go/types/typeutil also reference go/types we need to
vendor them as well.

vendor/vendor.json is updated using govendor, with the one exception
being a manual addition for go/types (govendor doesn't appear to
understand how to vendor a standard library package).

Fixes gopherjs#808.
myitcv added a commit to myitcv/gopherjs that referenced this issue Jul 2, 2018
Per golang/go#25008 (comment) the
fix for method set calculation will not be back ported to Go 1.10.
Instead gri suggests we vendor the latest version of go/types. This PR
does just that. It is effectively a temporary sticking plaster until Go
1.11 lands, or more specifically until we release GopherJS for Go 1.11.

In order to successfully vendor go/types, however, we need to move
compiler/vendor to the repo root (because build also uses go/types). And
because golang.org/x/tools/go/gcexportdata and
golang.org/x/tools/go/types/typeutil also reference go/types we need to
vendor them as well.

vendor/vendor.json is updated using govendor, with the one exception
being a manual addition for go/types (govendor doesn't appear to
understand how to vendor a standard library package).

Fixes gopherjs#808.
myitcv added a commit to myitcv/gopherjs that referenced this issue Jul 2, 2018
Per golang/go#25008 (comment) the
fix for method set calculation will not be back ported to Go 1.10.
Instead gri suggests we vendor the latest version of go/types. This PR
does just that. It is effectively a temporary sticking plaster until Go
1.11 lands, or more specifically until we release GopherJS for Go 1.11.

In order to successfully vendor go/types, however, we need to move
compiler/vendor to the repo root (because build also uses go/types). And
because golang.org/x/tools/go/gcexportdata and
golang.org/x/tools/go/types/typeutil also reference go/types we need to
vendor them as well.

vendor/vendor.json is updated using govendor, with the one exception
being a manual addition for go/types (govendor doesn't appear to
understand how to vendor a standard library package).

Fixes gopherjs#808.
@golang golang locked and limited conversation to collaborators May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants