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

Add gofactory linter #4196

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .golangci.reference.yml
Original file line number Diff line number Diff line change
Expand Up @@ -686,6 +686,15 @@ linters-settings:
- OPTIMIZE # marks code that should be optimized before merging
- HACK # marks hack-around that should be removed before merging

gofactory:
# List of glob packages, which can create structures without factories inside the glob package.
# Default: []
packageGlobs:
- github.com/author/repository/path/to/package/**
# Use a factory to initiate a structure for glob packages only.
# Default: false
packageGlobsOnly: true

gofmt:
# Simplify code: gofmt with `-s` option.
# Default: true
Expand Down Expand Up @@ -2410,6 +2419,7 @@ linters:
- godot
- godox
- goerr113
- gofactory
- gofmt
- gofumpt
- goheader
Expand Down Expand Up @@ -2531,6 +2541,7 @@ linters:
- godot
- godox
- goerr113
- gofactory
- gofmt
- gofumpt
- goheader
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ require (
github.com/leonklingele/grouper v1.1.1
github.com/lufeee/execinquery v1.2.1
github.com/macabu/inamedparam v0.1.3
github.com/maranqz/gofactory v1.0.0-rc1
github.com/maratori/testableexamples v1.0.0
github.com/maratori/testpackage v1.1.1
github.com/matoous/godox v0.0.0-20230222163458-006bad1f9d26
Expand Down
2 changes: 2 additions & 0 deletions go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions pkg/config/linters_settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ type LintersSettings struct {
Gocyclo GoCycloSettings
Godot GodotSettings
Godox GodoxSettings
Gofactory GoFactorySettings
Gofmt GoFmtSettings
Gofumpt GofumptSettings
Goheader GoHeaderSettings
Expand Down Expand Up @@ -486,6 +487,11 @@ type GodoxSettings struct {
Keywords []string
}

type GoFactorySettings struct {
PackageGlobs []string `mapstructure:"packageGlobs"`
PackageGlobsOnly bool `mapstructure:"packageGlobsOnly"`
}

type GoFmtSettings struct {
Simplify bool
RewriteRules []GoFmtRewriteRule `mapstructure:"rewrite-rules"`
Expand Down
30 changes: 30 additions & 0 deletions pkg/golinters/gofactory.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package golinters

import (
"github.com/maranqz/gofactory"
"golang.org/x/tools/go/analysis"

"github.com/golangci/golangci-lint/pkg/config"
"github.com/golangci/golangci-lint/pkg/golinters/goanalysis"
)

func NewGoFactory(settings *config.GoFactorySettings) *goanalysis.Linter {
analyzer := gofactory.NewAnalyzer()

cfg := make(map[string]map[string]any)
if settings != nil {
cfg[analyzer.Name] = map[string]any{}

if len(settings.PackageGlobs) > 0 {
cfg[analyzer.Name]["packageGlobs"] = settings.PackageGlobs
cfg[analyzer.Name]["packageGlobsOnly"] = settings.PackageGlobsOnly
}
}

return goanalysis.NewLinter(
analyzer.Name,
analyzer.Doc,
[]*analysis.Analyzer{analyzer},
cfg,
).WithLoadMode(goanalysis.LoadModeTypesInfo)
}
8 changes: 8 additions & 0 deletions pkg/lint/lintersdb/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
gocycloCfg *config.GoCycloSettings
godotCfg *config.GodotSettings
godoxCfg *config.GodoxSettings
goFactoryCfg *config.GoFactorySettings
gofmtCfg *config.GoFmtSettings
gofumptCfg *config.GofumptSettings
goheaderCfg *config.GoHeaderSettings
Expand Down Expand Up @@ -176,6 +177,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
gocycloCfg = &m.cfg.LintersSettings.Gocyclo
godotCfg = &m.cfg.LintersSettings.Godot
godoxCfg = &m.cfg.LintersSettings.Godox
goFactoryCfg = &m.cfg.LintersSettings.Gofactory
gofmtCfg = &m.cfg.LintersSettings.Gofmt
gofumptCfg = &m.cfg.LintersSettings.Gofumpt
goheaderCfg = &m.cfg.LintersSettings.Goheader
Expand Down Expand Up @@ -492,6 +494,12 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
WithLoadForGoAnalysis().
WithURL("https://github.com/Djarvur/go-err113"),

linter.NewConfig(golinters.NewGoFactory(goFactoryCfg)).
WithSince("1.56.0").
WithPresets(linter.PresetStyle).
ldez marked this conversation as resolved.
Show resolved Hide resolved
WithLoadForGoAnalysis().
WithURL("https://github.com/maranqz/gofactory"),

linter.NewConfig(golinters.NewGofmt(gofmtCfg)).
WithSince("v1.0.0").
WithPresets(linter.PresetFormatting).
Expand Down
3 changes: 1 addition & 2 deletions test/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func TestCgoOk(t *testing.T) {
"--timeout=3m",
"--enable-all",
"-D",
"nosnakecase,gci",
"nosnakecase,gci,gofactory",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope that is correct solution to pass tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your tests are not in the right places.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean I should move test/testdata/gofactory/blocked.go to test/testdata/gofactory/gofactory_package_globs_only.go?

Copy link
Member

@ldez ldez Dec 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, your tests must be inside test/testdata/ and not a dedicated folder.
and your linter should be removed from test/linters_test.go.

#4196 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to rollback

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added because of linter error Use factory for unsafe.Pointer.

Should I add option to exclude checking of std libraries in gofactory?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add option to exclude checking of std libraries in gofactory?

Nice idea, but I think in case of exclude-std=false we will receive false positive anyway, because a lot of std types could be (or must be) used without factory by design. Like unsafe.Pointer or sync.Mutex.

Zero value is useful 🙂

Look at

$ cd $GOROOT/src
$ gofactory -json ./... | jq '.[] | .[] | .[] .message' | sort | uniq

).
WithTargetPath(testdataDir, "cgo").
Runner().
Expand Down Expand Up @@ -355,7 +355,6 @@ func TestLineDirectiveProcessedFiles(t *testing.T) {
func TestUnsafeOk(t *testing.T) {
testshared.NewRunnerBuilder(t).
WithNoConfig().
WithArgs("--enable-all").
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this change relate to PR?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I return, can I add -D gofactory to fix linter error Use factory for unsafe.Pointer.
Or is solution from comment is better?
#4196 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be reverted.

WithTargetPath(testdataDir, "unsafe").
Runner().
Install().
Expand Down
5 changes: 5 additions & 0 deletions test/testdata/configs/go_factory_package_globs_only.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
linters-settings:
gofactory:
packageGlobs:
- net/url/**
packageGlobsOnly: true
32 changes: 32 additions & 0 deletions test/testdata/gofactory.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//golangcitest:args -Egofactory
package testdata

import (
"net/http"
alias_blocked "net/http"
)

type Struct struct{}

var (
defaultGlobalRequest = http.Request{} // want `Use factory for http.Request`
defaultGlobalRequestPtr = &http.Request{} // want `Use factory for http.Request`
)

func Default() {
_ = http.Request{} // want `Use factory for http.Request`
_ = &http.Request{} // want `Use factory for http.Request`

_ = []http.Request{{}, http.Request{}} // want `Use factory for http.Request`
_ = []*http.Request{{}, &http.Request{}} // want `Use factory for http.Request`

call(http.Request{}) // want `Use factory for http.Request`

_ = []Struct{{}, {}}
}

func call(_ http.Request) {}

func alias() {
_ = alias_blocked.Request{} // want `Use factory for http.Request`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cosmetic question: why not Use factory for alias_blocked.Request?
at fist time I was confused, but cannot decide what option is better 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idk too)
Could you decide how to better based on your experience?

I checked wrapcheck, go-reassign. They both used different printing way for package names.
I don't find another linter which uses package names.

wrapcheck: error returned from external package is unwrapped: sig: func encoding/json.Marshal(v any) ([]byte, error)
go-reassign: reassigning variable EOF in other package alias

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, the option that is better for grep and common understanding – to keep the source code and warnings for it consistent:

_ = neturl.URL{}  // want `Use factory for neturl.URL`
_ = &url.URL{}    // want `Use factory for url.URL`

}
46 changes: 46 additions & 0 deletions test/testdata/gofactory_package_globs_only.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
//golangcitest:args -Egofactory
//golangcitest:config_path configs/go_factory_package_globs_only.yml
package testdata

import (
"net/http"
neturl "net/url"
)

var (
nestedGlobalRequest = http.Request{}
nestedGlobalRequestPtr = &http.Request{}

blockedGlobalURL = neturl.URL{} // want `Use factory for url.URL`
blockedGlobalURLPtr = &neturl.URL{} // want `Use factory for url.URL`
)

func Blocked() {
_ = http.Request{}
_ = &http.Request{}

_ = neturl.URL{} // want `Use factory for url.URL`
_ = &neturl.URL{} // want `Use factory for url.URL`
}

type URL struct {
Scheme string
Opaque string
User *neturl.Userinfo
Host string
Path string
RawPath string
OmitHost bool
ForceQuery bool
RawQuery string
Fragment string
RawFragment string
}

func Casting() {
_ = neturl.URL(URL{}) // want `Use factory for url.URL`

uPtr, _ := neturl.Parse("")
u := *uPtr
_ = URL(u)
}
Loading