x/tools/refactor/satisfy: LHS reports a named Type, not an interface

[thesilentg]

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.

[mknyszek]

CC @golang/tools-team

[adonovan]

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.

[thesilentg]

Thanks for the context and clarification, that makes sense.