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

cmd/vet: False positive on stdmethods check #52445

Open
fishy opened this issue Apr 19, 2022 · 12 comments
Open

cmd/vet: False positive on stdmethods check #52445

fishy opened this issue Apr 19, 2022 · 12 comments
Labels
Analysis NeedsInvestigation

Comments

@fishy
Copy link

@fishy fishy commented Apr 19, 2022

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

$ go version
go version go1.18.1 linux/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/fishy/.cache/go-build"
GOENV="/home/fishy/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/fishy/.gopath/pkg/mod"
GONOPROXY="redacted"
GONOSUMDB="redacted"
GOOS="linux"
GOPATH="/home/fishy/.gopath"
GOPRIVATE="redacted"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go-1.18"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go-1.18/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.1"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/fishy/work/thrift/go.mod"
GOWORK=""
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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2627081732=/tmp/go-build -gno-record-gcc-switches"

What did you do?

In Apache Thrift go library, the interface TProtocol has
ReadByte and WriteByte that's different from the expectation of stdmethods:

type TProtocol interface {
  ...
  WriteByte(ctx context.Context, value int8) error
  ...
  ReadByte(ctx context.Context) (value int8, err error)
  ...
}

This causes go vet to complain about the implementations, and we have to do extra work to disable stdmethods check for packages containing the implementation, for example: https://github.com/reddit/baseplate.go/blob/205ac6852ad82ad5508106a6cb7156e2e73d3b3a/scripts/linters.sh#L19-L31

IMHO the stdmethods check should auto skip "violations" that explicitly implement a different interface (e.g. this line of var _ thrift.TProtocol = (*tDuplicateToProtocol)(nil) should be enough to silent stdmethods on (*tDuplicateToProtocol).ReadByte and (*tDuplicateToProtocol).WriteByte), as an explicit interface with different function signature should be enough signal that the difference is intentional, otherwise the interface should embed io.ByteReader/io.ByteWriter instead.

What did you expect to see?

What did you see instead?

$ go vet .
# github.com/apache/thrift/lib/go/thrift
./binary_protocol.go:215:27: method WriteByte(ctx context.Context, value int8) error should have signature WriteByte(byte) error
./binary_protocol.go:423:27: method ReadByte(ctx context.Context) (int8, error) should have signature ReadByte() (byte, error)
...
@timothy-king timothy-king changed the title cmd/vet: False negative/exception in stdmethods check cmd/vet: False positive on ReadByte/WriteByte in stdmethods Apr 19, 2022
@timothy-king timothy-king added Analysis NeedsDecision labels Apr 19, 2022
@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Apr 19, 2022

See also #36970. This is roughly the same but a different function.

An alternative is to rename the methods. This is not ideal, but it does work.

IMHO the stdmethods check should auto skip "violations" that explicitly implement a different interface (e.g. this line of var _ thrift.TProtocol = (*tDuplicateToProtocol)(nil) should be enough to silent stdmethods on (*tDuplicateToProtocol).ReadByte and (*tDuplicateToProtocol).WriteByte), as an explicit interface with different function signature should be enough signal that the difference is intentional

This is a reasonable mechanism for suppression. There would be a requirement that the assignment appears in the declaring package. There would also need to be a suppression mechanism for the warning on the interface definition. A possible mechanism could again be an assignment:

var _ interface{WriteByte(ctx context.Context, value int8) error} = TProtocol(nil)

IMO changing the interface of stdmethods to allow for suppression [mechanism aside] goes against my intuition of 'if it passes cmd/vet [stdmethods], then if I see name a method named M, M has signature S [and implements standard interface I]'. I think this is a sufficiently large interface change to cmd/vet to require a proposal. Instructions on the proposal process: https://github.com/golang/proposal#the-proposal-process.

otherwise the interface should embed io.ByteReader/io.ByteWriter instead.

Would this effectively require that interfaces embed standard interfaces for interface definitions to be checked by stdmethods? That would be turning off checking for pre-existing interfaces if they contain the same function but do not embed the interface. This would also be true going forward. (Is this different than just type-checking at this point?)

@timothy-king timothy-king added NeedsInvestigation and removed NeedsDecision labels Apr 19, 2022
@fishy
Copy link
Author

@fishy fishy commented Apr 19, 2022

An alternative is to rename the methods. This is not ideal, but it does work.

thrift.TProtocol dated back to 0.7.0/2011 (the ctx args were added in 0.14.0/2021). Renaming the methods will be a big breaking change to all users. Also all other thrift language libraries call the methods readByte/writeByte (see: java, c++, py, etc.), so renaming is really infeasible in this case.

Would this effectively require that interfaces embed standard interfaces for interface definitions to be checked by stdmethods?

No. What I'm suggesting is by default it still checks everything (the current behavior), just allow explicit suppression.

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Apr 20, 2022

Thank you for the clarification. If I now understand correctly, this can be read as 'If not suppressed, keep the pre-existing check'?

@fishy
Copy link
Author

@fishy fishy commented Apr 20, 2022

Thank you for the clarification. If I now understand correctly, this can be read as 'If not suppressed, keep the pre-existing check'?

correct

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Apr 20, 2022

Thinking out loud about yet more options:

  1. An option is to post-process the vet output. (Probably as annoying as excluding the package in the shell script.)
  2. An option is to add a flag that allows for suppression. Some precedent for this, but precedent is not flexible enough to turn this off for some packages or some types.
  3. An option is to remove these [edit: ReadByte/WriteByte] from stdmethods. (IMO this would also need a proposal).

@robpike
Copy link
Contributor

@robpike robpike commented Apr 20, 2022

You can run vet by hand with -stdmethods=false. Run go tool vet help for more information.

@fishy
Copy link
Author

@fishy fishy commented Apr 20, 2022

We are already using -stdmethods=false, the problem is that this also disables stdmethods check for things we do not want to suppress (for example, if there's a bug in Read, the bug will not be caught by go vet -stdmethods=false).

I also disagree with the title change. This is a false positive with any stdmethods functions, not just ReadByte/WriteByte. It's just that I have this example that's ReadByte/WriteByte.

@timothy-king timothy-king changed the title cmd/vet: False positive on ReadByte/WriteByte in stdmethods cmd/vet: False positive on stdmethods check Apr 20, 2022
@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Apr 20, 2022

I have partially reverted the title.

If none of the suggested paths forward is appealing, I think the next step is to put forward a proposal for a suppression mechanism for this checker. A community contribution would be welcomed here.

@guodongli-google
Copy link

@guodongli-google guodongli-google commented Apr 25, 2022

As @timothy-king mentioned, the suggested mechanism may be insufficient since the interface declaration alone

type TProtocol interface {
	WriteByte(ctx context.Context, value int8) error
	ReadByte(ctx context.Context) (value int8, err error)
}

will trigger the warnings:

./t1.go:11:2: method WriteByte(ctx context.Context, value int8) error should have signature WriteByte(byte) error
./t1.go:12:2: method ReadByte(ctx context.Context) (value int8, err error) should have signature ReadByte() (byte, error)

Basically checker stdmethods disallows reusing these method names with different signatures. On one hand, it is too strong since it uses only the method names to do matching. On the other hand, it is guessing that people are using these methods in a standard manner, and it is right in most cases.

Instead of introducing a complicated suppression mechanism, I would suggest a simple one that allows people to define the subset of method names that should be checked or skipped checking, as checker printf does.

@fishy
Copy link
Author

@fishy fishy commented Apr 25, 2022

I think the anonymous interface way as proposed by @timothy-king should work? e.g.

var _ interface{WriteByte(ctx context.Context, value int8) error} = TProtocol(nil)

but adding a flag to stdmethods as similar to the printf one allowing removing some of the names from the check would also be good enough for our case. I do prefer it to be a denylist rather than allowlist (as the printf case) though, as this way if we have other names added to stdmethods in the future we don't need to add that to the overridden list.

@zpavlinovic
Copy link
Contributor

@zpavlinovic zpavlinovic commented May 13, 2022

I think the anonymous interface way as proposed by @timothy-king should work? e.g.

var _ interface{WriteByte(ctx context.Context, value int8) error} = TProtocol(nil)

I guess @guodongli-google point is that the current implementation of the checker would still complain as there is a definition of interface TProtocol and the checker analyzes such definitions too.

but adding a flag to stdmethods as similar to the printf one allowing removing some of the names from the check would also be good enough for our case. I do prefer it to be a denylist rather than allowlist (as the printf case) though, as this way if we have other names added to stdmethods in the future we don't need to add that to the overridden list.

I agree a deny list seems a better fit here as the checker already has a predefined set of methods+signatures it looks at. Either way, this will likely require a proposal IMO.

@fishy
Copy link
Author

@fishy fishy commented May 13, 2022

yea I will write a proposal when I get time 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

5 participants