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

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

Open
ShoshinNikita opened this issue Mar 4, 2021 · 4 comments · May be fixed by golang/tools#283
Open

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

ShoshinNikita opened this issue Mar 4, 2021 · 4 comments · May be fixed by golang/tools#283

Comments

@ShoshinNikita
Copy link

@ShoshinNikita ShoshinNikita commented Mar 4, 2021

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:

  • hard code github.com/pkg/errors.As. It's the simplest solution but requires an update for every new package
  • pass a list of functions via flags. go/analysis/passes/printf uses the similar approach

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 dmitshur changed the title go/analysis/passes/errorsas: support errors.As provided by other packages x/tools/go/analysis/passes/errorsas: support errors.As provided by other packages Mar 5, 2021
@gopherbot gopherbot added the Tools label Mar 5, 2021
@gopherbot gopherbot added this to the Unreleased milestone Mar 5, 2021
@dmitshur
Copy link
Contributor

@dmitshur dmitshur commented Mar 5, 2021

ShoshinNikita added a commit to ShoshinNikita/tools that referenced this issue Mar 6, 2021
errorsas now supports other packages. By default, it checks
github.com/pkg/errors and golang.org/x/xerrors, but additional
packages can be passed via -pkgs flag.

Fixes golang/go#44784
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 6, 2021

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

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Mar 9, 2021

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
Copy link
Contributor

@timothy-king timothy-king commented Mar 9, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants