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

Jump to bottom

x/exp/apidiff: has a missing case in check whether a struct implements a interface #55963

Open
jjkklling opened this issue Sep 30, 2022 · 1 comment
Open

x/exp/apidiff: has a missing case in check whether a struct implements a interface #55963

jjkklling opened this issue Sep 30, 2022 · 1 comment
Assignees
@jba
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@jjkklling
Copy link

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

$ go version
go1.18

Does this issue reproduce with the latest release?

yes

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

$ go env
GOHOSTARCH="amd64"
GOOS="linux"

What did you do?

First, I create two packages named old and new in different folders, and create a file in each package.
The file content is here.

// the file in old package
package demo

type Iface interface{
	Func()
}

type S struct {}

func (s *S) Func() {}

// the file in new package
package demo

type Iface interface{
	Func()
}

type S struct {}

// func (s *S) Func() {}

And I use apidiff to diff these two packages

func ImplementTest() {
	// get pkgs in old package
	oldDir := "/the/old/package/path"
	oldCfg := &packages.Config{
		Mode:  packages.NeedTypes | packages.NeedName | packages.NeedImports | packages.NeedDeps,
		Tests: false,
		Dir:   oldDir,
	}
	oldPkgs, err := packages.Load(oldCfg, "./...")
	if err != nil {
		fmt.Println("packages.Load() error: ", err.Error())
	}
	// then do same action in new package
	// ...
	
	// then use apidiff the diff them
	for _, op := range oldPkgs {
		for _, np := range newPkgs {
			if op.Name == np.Name {
				fmt.Println(apidiff.Changes(op.Types, np.Types))
			}
		}
	}
}

What did you expect to see?

Incompatible changes:
- (*S).Func: removed
- (*S): no longer implements Iface

What did you see instead?

Incompatible changes:
- (*S).Func: removed

The problem I found

Problem location

// in file apidiff.go
// line 109 ~ 116
for otn2, nt2 := range d.correspondMap {
	if otn1 == otn2 {
		continue
	}
	if types.Implements(otn2.Type(), oIface) && !types.Implements(nt2, nIface) {
		d.incompatible(otn2, "", "no longer implements %s", objectString(otn1))
	}
}

This code can only check whether S implements Iface, but cannot check whether *S implments Iface, which is the reason why the results I get are not as expected.
Whether this code should be changed to this?

// in file apidiff.go
// line 109 ~ 116
for otn2, nt2 := range d.correspondMap {
	if otn1 == otn2 {
		continue
	}
	if types.Implements(otn2.Type(), oIface) && !types.Implements(nt2, nIface) {
		d.incompatible(otn2, "", "no longer implements %s", objectString(otn1))
	}
	// to check whether *S implements interface
	if types.Implements(types.NewPointer(otn2.Type()), oIface) && !types.Implements(types.NewPointer(nt2), nIface) {
		d.incompatible(otn2, "", "no longer implements %s", objectString(otn1))
	}
}
@dmitshur dmitshur changed the title golang.org/x/exp/apidiff: has a missing case in check whether a struct implements a interface x/exp/apidiff: has a missing case in check whether a struct implements a interface Sep 30, 2022
@gopherbot gopherbot added this to the Unreleased milestone Sep 30, 2022
@dmitshur
Copy link
Contributor

CC @jba.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 30, 2022
@jba jba self-assigned this Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jba jba

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@dmitshur @gopherbot @jba @jjkklling