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

x/exp/apidiff: interface compatible assumption may be wrong #45420

Open
titanxxh opened this issue Apr 7, 2021 · 2 comments
Open

x/exp/apidiff: interface compatible assumption may be wrong #45420

titanxxh opened this issue Apr 7, 2021 · 2 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@titanxxh
Copy link

titanxxh commented Apr 7, 2021

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

$ go version
go version go1.14.10 windows/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
set GO111MODULE=on
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\x00371158\AppData\Local\go-build
set GOENV=C:\Users\x00371158\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS= -mod=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=D:\goproject
set GOPRIVATE=
set GOPROXY=https://goproxy.io,direct
set GOROOT=D:\go1.14
set GOSUMDB=off
set GOTMPDIR=
set GOTOOLDIR=D:\go1.14\pkg\tool\windows_amd64
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=D:\xuxinhao\gana\go.mod
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\X00371~1\AppData\Local\Temp\go-build498719130=/tmp/go-build -gno-record-gcc-switches

What did you do?

old package

type i interface {
	b()
}

type I interface {
	A()
	i
}

type X struct {

}

func (x X) b() {
}

if we change the I definition to this(adding a B() method), new package:

type i interface {
	b()
}

type I interface {
	A()
	B()
	i
}

type X struct {

}

func (x X) b() {
}

client code

type Impl struct {
	apidiff.X
}

func (i Impl) A() {
}

func TestAssert(t *testing.T)  {
	var a apidiff.I = Impl{}
	fmt.Println(a)
}

What did you expect to see?

Incompatible changes

What did you see instead?

Compatible changes:
- I.B: added

in the readme of this package, here, it says:

Interfaces
A new interface is compatible with an old one if and only if:

  1. The old interface does not have an unexported method, and it corresponds to the new interfaces (i.e. they have the same method set), or
  2. The old interface has an unexported method and the new exported method set is a superset of the old.

Rule 2 is based on the observation that if an interface has an unexported method, the only way a client can implement it is to embed it. Adding a method is compatible in this case, because the embedding struct will continue to implement the interface. Adding a method also cannot break any call sites, since no program that compiles could have any such call sites.

in my example, this assumption is wrong.

@titanxxh
Copy link
Author

titanxxh commented Apr 7, 2021

@jba

@jba
Copy link
Contributor

jba commented Apr 7, 2021

@titanxxh, you are correct: apidiff should have said more.

apidiff assumes that an interface with an unexported method is for use by the package itself, so that all types implementing that interface will continue to do so if methods are added. So the "bug" here is that the author of the package didn't add a B method to X.

I thought apidiff checked for this, by checking that all types that implemented an interface in the old version continue to implement it in the new version. I will look into why you're not seeing that message.

@jba jba self-assigned this Apr 7, 2021
@dmitshur dmitshur changed the title golang/exp/apidiff: interface compatible assumption may be wrong x/exp/apidiff: interface compatible assumption may be wrong Apr 7, 2021
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 7, 2021
@gopherbot gopherbot added this to the Unreleased milestone Apr 7, 2021
@rsc rsc unassigned jba Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants