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/tools/refactor/satisfy: LHS reports a named Type, not an interface #66037

Closed
thesilentg opened this issue Feb 29, 2024 · 3 comments
Closed
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@thesilentg
Copy link

Go version

go version go1.22.0 darwin/amd64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='amd64'
GOBIN='/Users/aggnolek/go/bin'
GOCACHE='/Users/aggnolek/Library/Caches/go-build'
GOENV='/Users/aggnolek/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/aggnolek/go/pkg/mod'
GONOPROXY='*'
GONOSUMDB='*'
GOOS='darwin'
GOPATH='/Users/aggnolek/go'
GOPRIVATE='*'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/aggnolek/sdk/go1.22.0'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/aggnolek/sdk/go1.22.0/pkg/tool/darwin_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Volumes/workplace/TwitchMako/src/TwitchMako/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/x7/2f8ynt3954s4y78yt_54v9fr0000gs/T/go-build3901012278=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

go.mod

module scratchpad

go 1.21

require (
	github.com/stretchr/testify v1.8.4
)

example2/main.go

// Code generated by mockery v0.0.0-dev. DO NOT EDIT.
package main

import (
	context "context"

	mock "github.com/stretchr/testify/mock"
)

type ExampleType struct {
	mock.Mock
}

func (_m *ExampleType) ExampleFunction(ctx context.Context, param1 string) bool {
	ret := _m.Called(ctx, param1)

	var r0 bool
	if rf, ok := ret.Get(0).(func(context.Context, string) bool); ok {
		r0 = rf(ctx, param1)
	} else {
		r0 = ret.Get(0).(bool)
	}

	return r0
}

func NewExampleTypeChecker(t interface {
	mock.TestingT
	Cleanup(func())
}) *ExampleType {
	mock := &ExampleType{}
	mock.Mock.Test(t)

	t.Cleanup(func() { mock.AssertExpectations(t) })

	return mock
}

func main() {

}

I ran the refactor/satisfy tool against this example2 package in the scratchpad module. The code looks roughly as below:

	cfg := &packages.Config{
		// omitted for brevity 
		Mode:  packages.NeedModule | ...,
		Tests: true,
		Dir:   ".",
	}
	pkgs, err := packages.Load(cfg, packagesToCheck...)
	if err != nil {
		return nil, err
	}
	if packages.PrintErrors(pkgs) > 0 {
		return nil, fmt.Errorf("packages contain errors")
	}

	f := satisfy.Finder{}
	for _, pkg := range pkgs {
		f.Find(pkg.TypesInfo, pkg.Syntax)
	}

What did you see happen?

From the refactor/satisfy docs:

Package satisfy inspects the type-checked ASTs of Go packages and reports the set of discovered type constraints of the form (lhs, rhs Type) where lhs is a non-trivial interface, rhs satisfies this interface, and this fact is necessary for the package to be well-typed.

A Constraint records the fact that the RHS type does and must satisfy the LHS type, which is an interface. The names are suggestive of an assignment statement LHS = RHS.

However, one of the constraints returned is actually the opposite (LHS = *types.Named, RHS = *types.Interface) as compared to the expected (LHS = *types.Interface, RHS = *types.Named).

Printing this returns:

{github.com/stretchr/testify/mock.TestingT interface{Cleanup(func()); github.com/stretchr/testify/mock.TestingT}}

The LHS is: *types.Named

github.com/stretchr/testify/mock.TestingT

The RHS is: *types.Interface

methods: Cleanup
embeddeds.obj.object:
    name: TestingT
    pkg: github.com/stretchr/testify/mock 

What did you expect to see?

Honestly, I'm not sure. mock.TestingT is actually an interface (and you can see this via a call to Underlying() on the named type).

type TestingT interface {
	Logf(format string, args ...interface{})
	Errorf(format string, args ...interface{})
	FailNow()
}

I see two options:

  1. My assumption based on the existing documentation that LHS be a *types.Interface was correct, and the code for refactor/satisfy should be updated to ensure that this holds.
  2. My assumption that LHS be a *types.Interface was unfounded, and I should update my code to account for potentially receiving a types.Named for LHS and then accessing the underlying interface.
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Feb 29, 2024
@gopherbot gopherbot added this to the Unreleased milestone Feb 29, 2024
@mknyszek
Copy link
Contributor

mknyszek commented Mar 4, 2024

CC @golang/tools-team

@mknyszek mknyszek added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 4, 2024
@adonovan
Copy link
Member

adonovan commented Mar 4, 2024

In this case, the LHS type, mock.Testing.T, is a named interface type, so I think the code is working as intended. (It would be very surprising if it were not, as the only place in the code that inserts an entry into the Results set is Finder.assign, and it is dominated by an if isInterface(lhs) check; the isInterface predicate strips off types.Named and types.Alias constructors as needed.)

When we describe something as "an interface type", we usually intend that to include aliases and named types whose underlying type is an interface type, though it sometimes depends on the context, and the difference is often significant. In this instance, the fact that Constraint.LHS has type Type, not *Interface, is a tiny hint that it may be a named or alias type too. Preserving the Named/Alias constructors leads to much better error messages in tool such as gopls' Rename operation.

I hope that makes sense.

@adonovan adonovan closed this as completed Mar 4, 2024
@thesilentg
Copy link
Author

Thanks for the context and clarification, that makes sense.

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. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

4 participants