Protobuf: protoc-gen-go: remove generated dependency on github.com/golang/protobuf

[kevinburkemeter]

Staticcheck is a popular tool for vetting Go source code. For example, the golang.org/x/tools project includes it in the workflow for lsp.

Attempting to run staticcheck on generated .pb.go files yields the following error:

pb/api.pb.go:10:2: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (SA1019)

This makes it more difficult to use staticcheck as part of a workflow that includes generated proto files. Further, it's not clear that this situation can be resolved by end users; the generated files are going to import golang/protobuf for some time still.

[dsnet]

The only reason the newly generated .pb.go files depend on the deprecated proto package is to enforce a weak dependency on a sufficiently new version of the legacy package. This is necessary because not everyone is using Go modules such that the Go toolchain would enforce this dependency constraint. I wasn't fond of adding it, but I think it's probably necessary to keep at least for a few months.

The staticcheck tool has the ability to disable the "deprecated" check (with the -checks=-SA1019 flag). Is there a reason why that is not sufficient?

[kevinburkemeter]

It is, it's just extra work, I guess, that didn't need to be done previously. It seems like if something is deprecated it should be possible to switch whole cloth to something new but that's not currently possible.

[dsnet]

switch whole cloth to something new but that's not currently possible.

The challenge that's specific to protobufs in Go is the existence of a global registry for all messages linked into a binary. We need to ensure that if someone uses the new proto package, then usages of the legacy proto package must be sufficiently recent so that they both share the same view on the registry. Otherwise there can be disjoint registries leading to subtle and difficult to diagnose bugs.

I'm not sure there's much we can do here other than one of the following:

• Drop the deprecation notice on the legacy proto package, but we do want people to move over to the new proto package, which will be maintained and better polished over time.
• Drop the generated marker that statically enforces a sufficiently new version of the legacy proto package, but that may cause problems for people not building with module support (which is still the case for many users).
• Move the marker constant to a different package in the in the github.com/golang/protobuf module, but that seems pretty heavy-weight just to work around a static analysis warning.

None of the options seem great.

[puellanivis]

I’m kind of confused why staticcheck should be checking generated code. Because they’re not supposed to be edited, so doing checks on them is kind of pointless, because even if it finds an issue, there is not really anything that can be done to fix it. The issue will just be restored the next time the file is generated again.

[kevinburkemeter]

Presumably you can edit the generator to generate code that does not trigger the linter.

[dominikh]

I’m kind of confused why staticcheck should be checking generated code.

Kevin has the right idea here. Just because code has been generated doesn't mean that it is a) free of mistakes b) impossible to change.

I think we can agree that if generated code has a clear bug, then the bug should be flagged. I extend that logic to the use of deprecated code: if something is deprecated, it shouldn't be used anymore. I think most people agree with me on that. That's why we're flagging generated code.¹

The staticcheck tool has the ability to disable the "deprecated" check (with the -checks=-SA1019 flag). Is there a reason why that is not sufficient?

This is a poor choice if the generated code co-exists with other code in the same package, as disabling the check on this scale risks missing genuine offenders. A more appropriate solution might be to use a //lint:ignore (or possibly //lint:file-ignore) directive to ignore the specific import (or deprecated uses in the generated file). However, I can understand why you wouldn't want to emit this directive from your code generator. Not everyone uses staticcheck, and some use it via other runners that don't support those directives, which currently includes gopls, among others.

I'm not sure there's much we can do here other than one of the following:

I am not opposed to handling this in staticcheck itself, requiring no work on your part. Staticcheck can detect specific code generators if they have unique Code generated by strings, and we can special-case the particular import in code generated by protobuf.

¹: Note that there is also a class of checks in staticcheck that do _not_ check generated code. For example, generated code is not expected to abide by most style guidelines.

[dsnet]

we can special-case the particular import in code generated by protobuf.

Is there an existing precedence for special-casing certain popular libraries and whatnot in staticcheck? Personally, I find special-casing to be slippery slope and a maintenance burden long-term, but since you're the maintainer of the project, your best informed to make that decision.

[dominikh]

Is there an existing precedence for special-casing certain popular libraries and whatnot in staticcheck? Personally, I find special-casing to be slippery slope and a maintenance burden long-term, but since you're the maintainer of the project, your best informed to make that decision.

There are a number of special cases, yes. It is somewhat of a burden, but minimizing the amount of unhelpful diagnostics is one of the design goals. I would rather modify staticcheck than argue for a change in someone else's code that isn't objectively better. Overall, the number of instances of "well, this is a unique situation that has no better solution" are few and the slippery slope isn't very steep.

Moving the marker to a different package that isn't deprecated would be easier for me, but if you think that's too heavy-handed, then I'd rather add an exception than argue the case.

[bconway]

Bumping into this now as well, and have disabled SA1019 to work around it. If possible, it would be great to get a workaround on the staticcheck side "at least for a few months" until the dependency is phased out.

[dominikh]

https://github.com/dominikh/go-tools/commit/7e758a3887f90d0ded5f0446112616758183866b should address the issue.

[dsnet]

Thanks @dominikh. I'm going to re-purpose this issue to be about the eventual removal of the generated "weak" dependency on github.com/golang/protobuf. Will probably remove it in 6months.

[josephlr]

I hit this issue today, and I think it's OK to remove this check (or at the very least have a --go_opt argument to avoid emitting it).

Having to have both versions of the protobuf library in go.mod seems very confusing when migrating to the new version of this library.

[dsnet]

In https://github.com/golang/protobuf/issues/1077#issuecomment-617954633, I suggested that we remove this generated hard dependency in 6 months. It's been 7 months since the initial google.golang.org/protobuf module release. It's time to remove this.

[johanbrandhorst]

The generated code still imports github.com/golang/protobuf as best as I can tell; https://github.com/protocolbuffers/protobuf-go/blob/4394568c4bed76e9170dd11c611eb0a4c9c7a10c/cmd/protoc-gen-go/internal_gengo/main.go#L60. Was this issue closed prematurely or am I missing something?

[josephlr]

@johanbrandhorst it doesn't seem to be there on the latest master: https://github.com/protocolbuffers/protobuf-go/blob/master/cmd/protoc-gen-go/internal_gengo/main.go#L82-L91

I think there just has to be a new release of protoc-gen-go.

EDIT: Oh, I see. It looks like a line was missed in the CL that closed this issue.

[dsnet]

Was this issue closed prematurely or am I missing something?

That line is an unused constant in the implementation of protoc-gen-go itself. Thanks for pointing that out, I'll remove it. The logic for actually emitting a generated dependency has been removed. See https://go-review.googlesource.com/c/protobuf/+/259901

[rogers0]

@dsnet could you kindly make a new release since this issue has been resolved? such as 1.26.0
so we can bump up the version in debian. thank you!

[anthonyfok]

@dsnet could you kindly make a new release since this issue has been resolved? such as 1.26.0
so we can bump up the version in debian. thank you!

@dsnet or perhaps v1.25.1 or v1.26.0? :-)