golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
124.02k stars 17.68k forks source link

x/tools/go/analysis/passes/errorsas: support errors.As provided by other packages #44784

Open ShoshinNikita opened 3 years ago

ShoshinNikita commented 3 years ago

go/analysis/passes/errorsas works only with the standard package errors. So, I believe it is useless for many projects which use very popular github.com/pkg/errors. Of course, it's possible to alias errors package (for example stderrors) and use it, but I think it's better to update the analyzer.

I see 2 options here:

I think the best solution is to combine these options: use flags with the default functions errors.As and github.com/pkg/errors.As

dmitshur commented 3 years ago

CC @matloob, @timothy-king.

gopherbot commented 3 years ago

Change https://golang.org/cl/299429 mentions this issue: go/analysis/passes/errorsas: support other packages

timothy-king commented 3 years ago

So, I believe it is useless for many projects which use very popular github.com/pkg/errors.

github.com/pkg/errors has 6.6k github stars and pkg.go.dev reports "75628 Imported by" today. This seems like it probably meets the Frequency requirement for vet https://golang.org/src/cmd/vet/README. I don't have numbers on the usage of the As function.

it's possible to alias errors package (for example stderrors)

This is worth fixing.

pass a list of functions via flags. go/analysis/passes/printf uses the similar approach

This seems like a reasonable improvement, and there is a precedent for this.

hard code github.com/pkg/errors.As. It's the simplest solution but requires an update for every new package

I am not aware of a precedent for supporting modeling [/hard coding knowledge] of libraries outside of the standard library or golang.org/x/... from within golang.org/x/tools/go/analysis/passes. I need to double check this. Supporting libraries outside of these locations is a concern. If we don't accept modeling functions in github.com/pkg/errors, this would lower the impact of this change as users would need to opt in.

An option that is more implementation work but would support github.com/pkg/errors.As without modeling it would be to 1) infer when a function is exactly a wrapper of errors.As, 2) exports this as a Fact, 3) errorsas examines whenever As or a wrapper is called. This would be slower as Facts require analyzing dependencies & transitive dependencies. lostcancel and printf both use Facts and are enabled in vet. Running more fast checkers with Facts when these are already enabled would likely not be too significant of a performance hit.

timothy-king commented 3 years ago

I am not aware of a precedent for supporting modeling [/hard coding knowledge] of libraries outside of the standard library or golang.org/x/... from within golang.org/x/tools/go/analysis/passes. I need to double check this.

Double checked the current analyzers. I found no examples of analysis modeling any types or functions outside of the standard library or golang.org/x/... modules.

Options I can think of:

  1. Get a decision to approve modeling github.com/pkg/errors.As from within golang.org/x/tools/go/analysis/passes.
  2. Drop modeling and submit the change with the flag.
  3. Drop modeling and infer errors.As wrappers using Facts.
  4. Have the checker outside ofgolang.org/x/tools/go/analysis/passes. staticcheck may be interested.