go vet says Errorf calls are all wrong, "constant 1 not a string in call to Errorf" about grpc-go

[tsuna]

It's kinda ridiculous that go vet won't get fixed and you guys don't wanna change your usage that fools go vet either... It's a big pain in the ass for those of us who use go vet, which should be considered best practice.

If anyone else runs into this, here's a workaround:

    errf := grpc.Errorf // Confuse `go vet' to not check this `Errorf' call. :(
    // See https://github.com/grpc/grpc-go/issues/90
    return errf(codes.Unimplemented, "RPC not yet implemented")

[dmitshur]

For posterity, this issue has been resolved in go vet in golang/go#12294 and will be available in Go 1.7.

[dsymonds]

I don't think there's much that can be done about it.

[tv42]

I guess the only thing grpc can do is avoid that name.. for a while I misread the message as being about the Error vs Errorf confusion, but it's not even that. Maybe go vet should learn to look for the first format string parameter, and start its logic from there. Of course, that doesn't belong in grpc.

[mikeatlas]

Thanks for the workaround @tsuna, we'll use that as part of the existing sed calls we run on the grpc output to clean up other things as well.

[AdamSroka]

The format string should be the first argument. This has a been a convention since the beginning of time. https://en.wikipedia.org/wiki/Printf_format_string

[mikeatlas]

@AdamSroka sure but grpc.Errorf is not Printf; I personally think a custom package's Errorf implementation can require the first argument(s) to be package-specific, which, in this case, they make reasonable sense:

https://godoc.org/google.golang.org/grpc#Errorf
func Errorf(c codes.Code, format string, a ...interface{}) error

Errorf returns an error containing an error code and a description; Errorf returns nil if c is OK.

The only real issue here is that go vet was naïvely matching the method name "Errorf" regardless of whether it was the core fmt.Errorf function or not. go vet had no business warning other packages that they can't make methods named Errorf with a different signature than the fmt package.

[dmitshur]

@AdamSroka, what about fmt.Fprintf?

[AdamSroka]

@mikeatlas yeah, fair enough. I think making go vet less brittle is a good thing, but I would also prefer if familiar sounding methods had the signature that their names imply. I would do something like grpc.Code(codes.Code).Errorf(fmt string, a ...interface{}) and have Code return an interface that knows what to do with that.

[puellanivis]

I would like to support the idea of making the Errorf a receiver method of
codes.Code. It does make sense since the Errorf is semantically domained by
the codes.Code you're passing in.

And, codes.InvalidArgument.Errof("gave a bad XY: %v", thing) looks better
in a syntax parallels semantics way.

Adam Sroka [email protected] schrieb am Fr., 24. Juni 2016 um
23:17 Uhr:

@mikeatlas https://github.com/mikeatlas yeah, fair enough. I think
making go vet less brittle is a good thing, but I would also prefer if
familiar sounding methods had the signature that their names imply. I would
do something like grpc.Code(codes.Code).Errorf(fmt string, a
...interface{}) and have Code return an interface that knows what to do
with that.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#90 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AKVkVuqmhWdKMjX75wBFgMpas9ymb9Tqks5qPElvgaJpZM4Do-gl
.

[mikeatlas]

Bleh. grpc.Code(codes.Code).Errorf(...) is not an intuitive way to build a package-level error in my opinion. Any Error-builder/emitter should look like a factory method/function, and take in all required parameters in one shot. Why would I instantiate a Code first so that I can create an Error? What if the code were for something like "Code.UnknownCode"? Now I'm making an Error from an unknown code? It's just more seems more like natural logic to create an Error and give it an ErrorCode. The semantics just seem far more obvious. From an OO mindset, Code is an attribute of an Error, not something from which Error inherits (loosely speaking here). I'd raise a fuss in a code review if you designed it like that!

[tamird]

This is already fixed in 1.7 golang/go#12294

[mikeatlas]

@tamird, @shurcooL posted above. OP can close ;)

[puellanivis]

Why would I instantiate a Code first so that I can create an Error?

You shouldn't be instantiating codes at all.

import "github.com/grpc/grpc-go/codes"

func fn() error {
return codes.PermissionDenied.Errorf("policy determined that user %s is restricted", "myUsername")
}

Again, why would you ever “instantiate” your own code? It is semantically an enum, not an object.

In fact, the only difference between codes.PermissionDenied.Errorf("policy determined that user %s is restricted", "myUsername") and grpc.Errorf(codes.PermissionDenied, "policy determined that user %s is restricted", "myUsername") is syntax: one is a receiver and the other is not.

They both have the same call-stack regardless.

Creating a new error with a Errorf function for a specific code, is a domain being encapsulated semantically by that code itself. Every Errorf(code, format, args...) is bound semantically with the code.Code in the first place. So, why not express that as a receiver function?

“One things you can do with a codes.Code is generate a new error with arbitrary text using its Errorf function, which works exactly as one would expect such an Errorf function to work”

[puellanivis]

https://github.com/puellanivis/grpc-go/blob/master/codes/codes.go shows how the Errorf would work.

Then StreamError in transport/transport.go can be just:

type StreamError struct { codes.RPCError }

With a custom Error to wrap the codes.RCPError.Error by adding in a "stream error : " in front. Accessing err.Code and err.Desc however, still work just the same as before.

Oh, and rpc_util.go kind of has a redundant rpcError type after this. Well, unless it's specifically desired that the error type and fields be unexported.

Now, in server.go, we can do: codes.Internal.Errorf("io.ErrUnexpectedEOF")

And later in transport/handler_server.go we can use: code.Errorf("%s", se.Error()) (where code of type transport.StreamError)