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: new embedded interface behavior (possible regression) #29029

Closed
ccbrown opened this issue Nov 30, 2018 · 6 comments
Closed

go/types: new embedded interface behavior (possible regression) #29029

ccbrown opened this issue Nov 30, 2018 · 6 comments

Comments

@ccbrown
Copy link

@ccbrown ccbrown commented Nov 30, 2018

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

$ go version
go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/chris/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/chris/go"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/x1/md855yzx0lg2yqccxnvnb5lw0000gn/T/go-build287116670=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I type-checked and printed definitions from a package with a test file that embeds a non-test interface into another interface.

What did you expect to see?

The interface method with the correct receiver type:

func (github.com/ccbrown/go-interface-method-type-bug/thepackage.A).Foo()

What did you see instead?

The receiver type was overwritten based the contents of the test file:

func (interface).Foo()

Demo project

Check out this program: https://github.com/ccbrown/go-interface-method-type-bug

Run go run main.go. With Go 1.10, you get the output I would expect. With Go 1.11, you get different output.

This makes tooling such as @dominikh's unused tool break in Go 1.11 for certain cases where tests define interfaces that embed other interfaces.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Nov 30, 2018

Loading

@griesemer griesemer self-assigned this Nov 30, 2018
@griesemer griesemer added this to the Go1.12 milestone Nov 30, 2018
@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 30, 2018

I can reproduce this. It does look like a regression.

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 30, 2018

Stand-alone reproducer (no need to use loader): https://play.golang.org/p/6rledUr3Mhx

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 1, 2018

The loader type-checks the test package by type-checking the _test file incrementally, via a 2nd go/types.Files call. That API is meant to be called like this, but in the process go/types starts with a fresh (internal) interface cache which causes incorrect recomputation of interface methods.

The following diff fixes the problem:

diff --git a/src/go/types/check.go b/src/go/types/check.go
index 91df94dcbc..10e8c500ac 100644
--- a/src/go/types/check.go
+++ b/src/go/types/check.go
@@ -192,7 +192,7 @@ func (check *Checker) initFiles(files []*ast.File) {
 
        check.firstErr = nil
        check.methods = nil
-       check.interfaces = nil
+       // check.interfaces = nil
        check.untyped = nil
        check.delayed = nil

We should definitively do this since adding (test) files to a package cannot change any of the already computed interfaces (even though it may add methods to concrete types). So this is a useful optimization in its own right. That said, it's unclear yet why it's also necessary.

Loading

@gopherbot
Copy link

@gopherbot gopherbot commented Dec 3, 2018

Change https://golang.org/cl/152259 mentions this issue: go/types: fix interface receiver type for incremental type-checking

Loading

@griesemer
Copy link
Contributor

@griesemer griesemer commented Dec 3, 2018

See comments in CL as to why the suggested diff is correct and also necessary.

Loading

@gopherbot gopherbot closed this in 13e40c7 Dec 3, 2018
@golang golang locked and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants