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 14 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 @@ -665,6 +665,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 @@ -2358,6 +2367,7 @@ linters:
- godot
- godox
- goerr113
- gofactory
- gofmt
- gofumpt
- goheader
Expand Down Expand Up @@ -2478,6 +2488,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.2
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 @@ -200,6 +200,7 @@ type LintersSettings struct {
Gocyclo GoCycloSettings
Godot GodotSettings
Godox GodoxSettings
Gofactory GoFactorySettings
Gofmt GoFmtSettings
Gofumpt GofumptSettings
Goheader GoHeaderSettings
Expand Down Expand Up @@ -480,6 +481,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 @@ -174,6 +175,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 @@ -488,6 +490,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
1 change: 1 addition & 0 deletions test/linters_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func TestSourcesFromTestdataSubDir(t *testing.T) {
"ginkgolinter",
"zerologlint",
"protogetter",
"gofactory",
}

for _, dir := range subDirs {
Expand Down
8 changes: 6 additions & 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,11 @@ 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.

WithArgs(
"--enable-all",
"-D",
"gofactory",
).
WithTargetPath(testdataDir, "unsafe").
Runner().
Install().
Expand Down
46 changes: 46 additions & 0 deletions test/testdata/gofactory/blocked.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 gofactory

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

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

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

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

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

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

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

uPtr, _ := url.Parse("")
u := *uPtr
_ = URL(u)
}
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/default.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//golangcitest:args -Egofactory
package gofactory

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`
}
4 changes: 4 additions & 0 deletions test/testdata/gofactory/go.mod
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
module gofactory

go 1.18