Skip to content

Commit

Permalink
Prepare linter for golangci-lint (#5)
Browse files Browse the repository at this point in the history
# golangci/golangci-lint#4196
1. TestdataDir() -> analysistest.TestData()
2. Described False-Negative cases as testcases and in README.md
3. Fixed link for type_nested.go.skip in README.md
4. Added testcase with generic
5. Added testcase with alias
6. Added testcase with map, chan, array, func
# golangci/golangci-lint#4196
7. Added glob syntax for pkgs
8. Renamed options
9. Extended unexpected code message
10. Added unimplemented testcases
  • Loading branch information
maranqz authored Nov 19, 2023
1 parent ce8cbf4 commit 5f8864e
Show file tree
Hide file tree
Showing 36 changed files with 665 additions and 150 deletions.
26 changes: 23 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ Installation

### Options

- `-b`, `--blockedPkgs` - list of packages, where the structures should be created by factories. By default, all structures in all packages should be created by factories, [tests](testdata/src/factory/blockedPkgs).
- `-ob`, `onlyBlockedPkgs` - only blocked packages should use factory to initiate struct, [tests](testdata/src/factory/onlyBlockedPkgs).
- `--packageGlobs` - list of glob packages, which can create structures without factories inside the glob package. By default, all structures from another package should be created by factories, [tests](testdata/src/factory/packageGlobs).
- `onlyPackageGlobs` - use a factory to initiate a structure for glob packages only, [tests](testdata/src/factory/onlyPackageGlobs).

## Example

Expand Down Expand Up @@ -120,8 +120,28 @@ func nextID() int64 {
</td></tr>
</tbody></table>

## False Negative

Linter doesn't catch some cases.

1. Buffered channel. You can initialize struct in line `v, ok := <-bufCh` [example](testdata/src/factory/unimplemented/chan.go).
2. Local initialization, [example](testdata/src/factory/unimplemented/local/).
3. Named return. If you want to block that case, you can use [nonamedreturns](https://github.com/firefart/nonamedreturns) linter, [example](testdata/src/factory/unimplemented/named_return.go).
4. var declaration, `var initilized nested.Struct` gives structure without factory, [example](testdata/src/factory/unimplemented/var.go).
5. Casting to nested struct, [example](testdata/src/factory/unimplemented/casting/).

## TODO

### Possible Features

1. Catch nested struct in the same package, [example](testdata/src/factory/unimplemented/local/nested_struct.go).
```go
return Struct{
Other: OtherStruct{}, // want `Use factory for nested.Struct`
}
```
2. Resolve false negative issue with `var declaration`.

### Features that are difficult to implement and unplanned

1. Type assertion, type declaration and type underlying, [tests](testdata/src/factory/default/type_nested.go.skip).
1. Type assertion, type declaration and type underlying, [tests](testdata/src/factory/simple/type_nested.go.skip).
212 changes: 113 additions & 99 deletions factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@ import (
"fmt"
"go/ast"
"go/types"
"log/slog"
"strings"

"github.com/gobwas/glob"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/inspect"
)

type config struct {
blockedPkgs stringsFlag
onlyBlockedPkgs bool
pkgGlobs stringsFlag
onlyPkgGlobs bool
}

type stringsFlag []string
Expand All @@ -28,21 +30,18 @@ func (s *stringsFlag) Set(value string) error {
}

func (s stringsFlag) Value() []string {
blockedPkgs := make([]string, 0, len(s))
res := make([]string, 0, len(s))

for _, pgk := range s {
pgk = strings.TrimSpace(pgk)
pgk = strings.TrimSuffix(pgk, "/") + "/"

blockedPkgs = append(blockedPkgs, pgk)
for _, str := range s {
res = append(res, strings.TrimSpace(str))
}

return blockedPkgs
return res
}

const (
blockedPkgsDesc = "List of packages, which should use factory to initiate struct."
onlyBlockedPkgsDesc = "Only blocked packages should use factory to initiate struct."
packageGlobsDesc = "List of glob packages, which can create structures without factories inside the glob package"
onlyPkgGlobsDesc = "Use a factory to initiate a structure for glob packages only."
)

func NewAnalyzer() *analysis.Analyzer {
Expand All @@ -54,11 +53,9 @@ func NewAnalyzer() *analysis.Analyzer {

cfg := config{}

analyzer.Flags.Var(&cfg.blockedPkgs, "b", blockedPkgsDesc)
analyzer.Flags.Var(&cfg.blockedPkgs, "blockedPkgs", blockedPkgsDesc)
analyzer.Flags.Var(&cfg.pkgGlobs, "packageGlobs", packageGlobsDesc)

analyzer.Flags.BoolVar(&cfg.onlyBlockedPkgs, "ob", false, onlyBlockedPkgsDesc)
analyzer.Flags.BoolVar(&cfg.onlyBlockedPkgs, "onlyBlockedPkgs", false, onlyBlockedPkgsDesc)
analyzer.Flags.BoolVar(&cfg.onlyPkgGlobs, "onlyPackageGlobs", false, onlyPkgGlobsDesc)

analyzer.Run = run(&cfg)

Expand All @@ -68,14 +65,19 @@ func NewAnalyzer() *analysis.Analyzer {
func run(cfg *config) func(pass *analysis.Pass) (interface{}, error) {
return func(pass *analysis.Pass) (interface{}, error) {
var blockedStrategy blockedStrategy = newAnotherPkg()
if len(cfg.blockedPkgs) > 0 {
if len(cfg.pkgGlobs) > 0 {
defaultStrategy := blockedStrategy
if cfg.onlyBlockedPkgs {
if cfg.onlyPkgGlobs {
defaultStrategy = newNilPkg()
}

pkgGlobs, err := compileGlobs(cfg.pkgGlobs.Value())
if err != nil {
return nil, err
}

blockedStrategy = newBlockedPkgs(
cfg.blockedPkgs.Value(),
pkgGlobs,
defaultStrategy,
)
}
Expand Down Expand Up @@ -109,134 +111,146 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor {
return v
}

var selExpr *ast.SelectorExpr
compLitType := compLit.Type

// check []*Struct{{},&Struct}
arr, isArr := compLit.Type.(*ast.ArrayType)
if isArr && len(compLit.Elts) > 0 {
v.checkSlice(arr, compLit)
slice, isMap := compLitType.(*ast.ArrayType)
if isMap && len(compLit.Elts) > 0 {
v.checkSlice(slice, compLit)

return v
}

ident, ok := compLit.Type.(*ast.Ident)
if !ok {
selExpr, ok = compLit.Type.(*ast.SelectorExpr)
if !ok {
return v
}
// check map[Struct]Struct{{}:{}}
mp, isMap := compLitType.(*ast.MapType)
if isMap {
v.checkMap(mp, compLit)

ident = selExpr.Sel
return v
}

// check Struct{}
ident := v.getIdent(compLitType)
identObj := v.pass.TypesInfo.ObjectOf(ident)

if identObj == nil {
return v
}

if v.blockedStrategy.IsBlocked(v.pass.Pkg, identObj) {
v.report(node, identObj)
v.report(ident, identObj)
}

return v
}

func (v *visitor) checkSlice(arr *ast.ArrayType, compLit *ast.CompositeLit) {
arrElt := arr.Elt
if starExpr, ok := arr.Elt.(*ast.StarExpr); ok {
arrElt = starExpr.X
func (v *visitor) getIdent(expr ast.Expr) *ast.Ident {
// pointer *Struct{}
if starExpr, ok := expr.(*ast.StarExpr); ok {
expr = starExpr.X
}

selExpr, ok := arrElt.(*ast.SelectorExpr)
if !ok {
return
// generic Struct[any]{}
indexExpr, ok := expr.(*ast.IndexExpr)
if ok {
expr = indexExpr.X
}

identObj := v.pass.TypesInfo.ObjectOf(selExpr.Sel)
if identObj != nil {
for _, elt := range compLit.Elts {
eltCompLit, ok := elt.(*ast.CompositeLit)
if ok && eltCompLit.Type == nil {
if v.blockedStrategy.IsBlocked(v.pass.Pkg, identObj) {
v.report(elt, identObj)
}
}
}
selExpr, ok := expr.(*ast.SelectorExpr)
if !ok {
return nil
}
}

func (v *visitor) report(node ast.Node, obj types.Object) {
v.pass.Reportf(
node.Pos(),
fmt.Sprintf(`Use factory for %s.%s`, obj.Pkg().Name(), obj.Name()),
)
return selExpr.Sel
}

type blockedStrategy interface {
IsBlocked(currentPkg *types.Package, identObj types.Object) bool
}
func (v *visitor) checkSlice(arr *ast.ArrayType, compLit *ast.CompositeLit) {
ident := v.getIdent(arr.Elt)
identObj := v.pass.TypesInfo.ObjectOf(ident)

type nilPkg struct{}
if identObj == nil {
return
}

func newNilPkg() nilPkg {
return nilPkg{}
for _, elt := range compLit.Elts {
v.checkBrackets(elt, identObj)
}
}

func (nilPkg) IsBlocked(_ *types.Package, _ types.Object) bool {
return false
}
func (v *visitor) checkMap(mp *ast.MapType, compLit *ast.CompositeLit) {
keyIdent := v.getIdent(mp.Key)
keyIdentObj := v.pass.TypesInfo.ObjectOf(keyIdent)

type anotherPkg struct{}
valueIdent := v.getIdent(mp.Value)
valueIdentObj := v.pass.TypesInfo.ObjectOf(valueIdent)

func newAnotherPkg() anotherPkg {
return anotherPkg{}
}
if keyIdentObj == nil && valueIdentObj == nil {
return
}

func (anotherPkg) IsBlocked(
currentPkg *types.Package,
identObj types.Object,
) bool {
return currentPkg.Path() != identObj.Pkg().Path()
}
for _, elt := range compLit.Elts {
keyValueExpr, ok := elt.(*ast.KeyValueExpr)
if !ok {
v.unexpectedCode(elt)

type blockedPkgs struct {
blockedPkgs []string
defaultStrategy blockedStrategy
}
continue
}

func newBlockedPkgs(
pkgs []string,
defaultStrategy blockedStrategy,
) blockedPkgs {
return blockedPkgs{
blockedPkgs: pkgs,
defaultStrategy: defaultStrategy,
v.checkBrackets(keyValueExpr.Key, keyIdentObj)
v.checkBrackets(keyValueExpr.Value, valueIdentObj)
}
}

func (b blockedPkgs) IsBlocked(
currentPkg *types.Package,
identObj types.Object,
) bool {
identPkgPath := identObj.Pkg().Path() + "/"
currentPkgPath := currentPkg.Path() + "/"
// checkBrackets check {} in array, slice, map.
func (v *visitor) checkBrackets(expr ast.Expr, identObj types.Object) {
compLit, ok := expr.(*ast.CompositeLit)
if ok && compLit.Type == nil && identObj != nil {
if v.blockedStrategy.IsBlocked(v.pass.Pkg, identObj) {
v.report(compLit, identObj)
}
}
}

for _, blockedPkg := range b.blockedPkgs {
isBlocked := strings.HasPrefix(identPkgPath, blockedPkg)
isIncludedInBlocked := strings.HasPrefix(currentPkgPath, blockedPkg)
func (v *visitor) report(node ast.Node, obj types.Object) {
v.pass.Reportf(
node.Pos(),
fmt.Sprintf(`Use factory for %s.%s`, obj.Pkg().Name(), obj.Name()),
)
}

if isIncludedInBlocked {
continue
}
func (v *visitor) unexpectedCode(node ast.Node) {
fset := v.pass.Fset
pos := fset.Position(node.Pos())
slog.Error(
fmt.Sprintf("Unexpected code in %s:%d:%d, please report to the developer with example.",
fset.File(node.Pos()).Name(),
pos.Line,
pos.Column,
),
)
}

if isBlocked {
func containsMatchGlob(globs []glob.Glob, el string) bool {
for _, g := range globs {
if g.Match(el) {
return true
}
}

if b.defaultStrategy.IsBlocked(currentPkg, identObj) {
return true
return false
}

func compileGlobs(globs []string) ([]glob.Glob, error) {
compiledGlobs := make([]glob.Glob, len(globs))

for idx, globString := range globs {
glob, err := glob.Compile(globString)
if err != nil {
return nil, fmt.Errorf("unable to compile globs %s: %w", glob, err)
}

compiledGlobs[idx] = glob
}

return false
return compiledGlobs, nil
}
5 changes: 4 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ module github.com/maranqz/go-factory-lint

go 1.20

require golang.org/x/tools v0.15.0
require (
github.com/gobwas/glob v0.2.3
golang.org/x/tools v0.15.0
)

require (
golang.org/x/mod v0.14.0 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y=
github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8=
golang.org/x/mod v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0=
golang.org/x/mod v0.14.0/go.mod h1:hTbmBsO62+eylJbnUtE2MGJUyE7QWk4xUqPFrRgJ+7c=
golang.org/x/sync v0.5.0 h1:60k92dhOjHxJkrqnwsfl8KuaHbn/5dl0lUPUklKo3qE=
Expand Down
Loading

0 comments on commit 5f8864e

Please sign in to comment.