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
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@ShoshinNikita
Copy link

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 This label describes issues relating to any tools in the x/tools repository. label Mar 5, 2021
@gopherbot gopherbot added this to the Unreleased milestone Mar 5, 2021
@dmitshur dmitshur added FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 5, 2021
@dmitshur
Copy link
Contributor

dmitshur commented Mar 5, 2021

CC @matloob, @timothy-king.

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

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

@timothy-king timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Mar 9, 2021
@timothy-king
Copy link
Contributor

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

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.

@timothy-king timothy-king added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analysis Issues related to static analysis (vet, x/tools/go/analysis) FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants