From 9f1c4e518705a322e5b3416e825da1eabd7d8fde Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Sun, 30 Jul 2023 15:45:40 -0400 Subject: [PATCH 01/11] Resolve #69: Support struct literal comment directive --- README.md | 47 +++- analyzer/analyzer.go | 265 ++++++++++++------ analyzer/analyzer_benchmark_test.go | 17 ++ analyzer/analyzer_test.go | 19 +- .../testdata/src/directives/directives.go | 69 +++++ cmd/exhaustruct/main.go | 2 +- 6 files changed, 333 insertions(+), 86 deletions(-) create mode 100644 analyzer/testdata/src/directives/directives.go diff --git a/README.md b/README.md index 626277b5..9c20b86e 100644 --- a/README.md +++ b/README.md @@ -39,11 +39,35 @@ Flags: 4ex: github.com/GaijinEntertainment/go-exhaustruct/v3/analyzer\. github.com/GaijinEntertainment/go-exhaustruct/v3/analyzer\.TypeInfo + + -use-directives value + Boolean that determines whether to scan for comment directives in the form of + '//exhaustruct:enforce' and '//exhaustruct:ignore' for enforcement decisions. Comment + directives on struct literals have higher precedence than command-level inclusion and + exclusion flags. + + Default: false ``` ### Example +Invocation + +```shell +exhaustruct -e 'mymodule/excluded.*' -i 'mymodule/.*' -use-directives 'true' +``` + +Code + ```go +// Package excluded.go +package excluded + +type Point struct { + X int + Y int +} + // Package a.go package a @@ -68,10 +92,18 @@ var b Shape = Shape{ Width: 3, } +// valid due to ignore directive in spite of missing `volume` (when -use-directives=true) +//exhaustruct:ignore +var c Shape = Shape{ + Length: 5, + Width: 3, +} + // Package b.go package b import "a" +import "excluded" // valid var b Shape = a.Shape{ @@ -80,7 +112,18 @@ var b Shape = a.Shape{ } // invalid, `Width` is missing -var b Shape = a.Shape{ +var c Shape = a.Shape{ Length: 5, } -``` \ No newline at end of file + +// valid due to exclusion on this type +var d = excluded.Point{ + X: 1, +} + +// invalid, `Y` is missing, due to enforce directive (when -use-directives=true) +//exhastruct:enforce +var e = excluded.Point{ + X: 1, +} +``` diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index d0cd2d5b..e3d3284c 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -6,6 +6,7 @@ import ( "go/ast" "go/token" "go/types" + "strings" "sync" "golang.org/x/tools/go/analysis" @@ -17,20 +18,28 @@ import ( ) type analyzer struct { - include pattern.List `exhaustruct:"optional"` - exclude pattern.List `exhaustruct:"optional"` + include pattern.List `exhaustruct:"optional"` + exclude pattern.List `exhaustruct:"optional"` + useDirectives bool fieldsCache map[types.Type]fields.StructFields fieldsCacheMu sync.RWMutex `exhaustruct:"optional"` typeProcessingNeed map[string]bool typeProcessingNeedMu sync.RWMutex `exhaustruct:"optional"` + + commentMapCache map[*ast.File]ast.CommentMap + commentMapCacheMu sync.RWMutex `exhaustruct:"optional"` } -func NewAnalyzer(include, exclude []string) (*analysis.Analyzer, error) { +func NewAnalyzer(include, exclude []string, useDirectives bool) (*analysis.Analyzer, error) { a := analyzer{ fieldsCache: make(map[types.Type]fields.StructFields), typeProcessingNeed: make(map[string]bool), + useDirectives: useDirectives, + } + if useDirectives { + a.commentMapCache = make(map[*ast.File]ast.CommentMap) } var err error @@ -67,6 +76,9 @@ Anonymous structs can be matched by '' alias. 4ex: github.com/GaijinEntertainment/go-exhaustruct/v3/analyzer\. github.com/GaijinEntertainment/go-exhaustruct/v3/analyzer\.TypeInfo`) + fs.BoolVar(&a.useDirectives, "use-directives", a.useDirectives, + `Use directives to enforce or ignore analysis on a per struct literal basis, overriding +any include/exclude patterns. Default: false.`) return *fs } @@ -74,58 +86,134 @@ Anonymous structs can be matched by '' alias. func (a *analyzer) run(pass *analysis.Pass) (any, error) { insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) //nolint:forcetypeassert + visitor := &Visitor{ + analyzer: a, + pass: pass, + } insp.WithStack( []ast.Node{ (*ast.CompositeLit)(nil), }, - a.newVisitor(pass), + visitor.Visit, ) return nil, nil //nolint:nilnil } -// newVisitor returns visitor that only expects [ast.CompositeLit] nodes. -func (a *analyzer) newVisitor(pass *analysis.Pass) func(n ast.Node, push bool, stack []ast.Node) bool { - return func(n ast.Node, push bool, stack []ast.Node) bool { - if !push { - return true - } +// visitor that only expects [ast.CompositeLit] nodes. +type Visitor struct { + analyzer *analyzer + pass *analysis.Pass +} - lit, ok := n.(*ast.CompositeLit) - if !ok { - // this should never happen, but better be prepared - return true - } +// Implements inspector.Visitor interface. +func (v *Visitor) Visit(n ast.Node, push bool, stack []ast.Node) bool { + if !push { + return true + } - structTyp, typeInfo, ok := getStructType(pass, lit) - if !ok { - return true - } + lit, ok := n.(*ast.CompositeLit) + if !ok { + // this should never happen, but better be prepared + return true + } - if len(lit.Elts) == 0 { - if ret, ok := stackParentIsReturn(stack); ok { - if returnContainsNonNilError(pass, ret) { - // it is okay to return uninitialized structure in case struct's direct parent is - // a return statement containing non-nil error - // - // we're unable to check if returned error is custom, but at least we're able to - // cover str [error] type. - return true - } + structTyp, typeInfo, ok := v.getStructType(lit) + if !ok { + return true + } + + if len(lit.Elts) == 0 { + if ret, ok := stackParentIsReturn(stack); ok { + if v.returnContainsNonNilError(ret) { + // it is okay to return uninitialized structure in case struct's direct parent is + // a return statement containing non-nil error + // + // we're unable to check if returned error is custom, but at least we're able to + // cover str [error] type. + return true } } + } + + var enforcement EnforcementDirective + if v.analyzer.useDirectives { + enforcement = v.decideEnforcementDirective(lit, stack) + } + + pos, msg := v.processStruct(lit, structTyp, typeInfo, enforcement) + if pos != nil { + v.pass.Reportf(*pos, msg) + } + + return true +} + +//revive:disable-next-line:unused-receiver +func (a *analyzer) litSkippedFields( + lit *ast.CompositeLit, + typ *types.Struct, + onlyExported bool, +) fields.StructFields { + a.fieldsCacheMu.RLock() + f, ok := a.fieldsCache[typ] + a.fieldsCacheMu.RUnlock() + + if !ok { + a.fieldsCacheMu.Lock() + f = fields.NewStructFields(typ) + a.fieldsCache[typ] = f + a.fieldsCacheMu.Unlock() + } + + return f.SkippedFields(lit, onlyExported) +} + +func (a *analyzer) getFileCommentMap(fileSet *token.FileSet, file *ast.File) ast.CommentMap { + a.commentMapCacheMu.RLock() + commentMap, exists := a.commentMapCache[file] + a.commentMapCacheMu.RUnlock() + + if !exists { + // TODO: consider avoiding risk of double-computation by using per-file mutex + commentMap = ast.NewCommentMap(fileSet, file, file.Comments) + + a.commentMapCacheMu.Lock() + a.commentMapCache[file] = commentMap + a.commentMapCacheMu.Unlock() + } + + return commentMap +} - pos, msg := a.processStruct(pass, lit, structTyp, typeInfo) - if pos != nil { - pass.Reportf(*pos, msg) +func (a *analyzer) isTypeProcessingNeeded(info *TypeInfo) bool { + name := info.String() + + a.typeProcessingNeedMu.RLock() + res, ok := a.typeProcessingNeed[name] + a.typeProcessingNeedMu.RUnlock() + + if !ok { + a.typeProcessingNeedMu.Lock() + res = true + + if a.include != nil && !a.include.MatchFullString(name) { + res = false } - return true + if res && a.exclude != nil && a.exclude.MatchFullString(name) { + res = false + } + + a.typeProcessingNeed[name] = res + a.typeProcessingNeedMu.Unlock() } + + return res } -func getStructType(pass *analysis.Pass, lit *ast.CompositeLit) (*types.Struct, *TypeInfo, bool) { - switch typ := pass.TypesInfo.TypeOf(lit).(type) { +func (v *Visitor) getStructType(lit *ast.CompositeLit) (*types.Struct, *TypeInfo, bool) { + switch typ := v.pass.TypesInfo.TypeOf(lit).(type) { case *types.Named: // named type if structTyp, ok := typ.Underlying().(*types.Struct); ok { pkg := typ.Obj().Pkg() @@ -143,8 +231,8 @@ func getStructType(pass *analysis.Pass, lit *ast.CompositeLit) (*types.Struct, * case *types.Struct: // anonymous struct ti := TypeInfo{ Name: "", - PackageName: pass.Pkg.Name(), - PackagePath: pass.Pkg.Path(), + PackageName: v.pass.Pkg.Name(), + PackagePath: v.pass.Pkg.Path(), } return typ, &ti, true @@ -162,11 +250,11 @@ func stackParentIsReturn(stack []ast.Node) (*ast.ReturnStmt, bool) { return ret, ok } -func returnContainsNonNilError(pass *analysis.Pass, ret *ast.ReturnStmt) bool { +func (v *Visitor) returnContainsNonNilError(ret *ast.ReturnStmt) bool { // errors are mostly located at the end of return statement, so we're starting // from the end. for i := len(ret.Results) - 1; i >= 0; i-- { - if pass.TypesInfo.TypeOf(ret.Results[i]).String() == "error" { + if v.pass.TypesInfo.TypeOf(ret.Results[i]).String() == "error" { return true } } @@ -174,21 +262,21 @@ func returnContainsNonNilError(pass *analysis.Pass, ret *ast.ReturnStmt) bool { return false } -func (a *analyzer) processStruct( - pass *analysis.Pass, +func (v *Visitor) processStruct( lit *ast.CompositeLit, structTyp *types.Struct, info *TypeInfo, + enforcement EnforcementDirective, ) (*token.Pos, string) { - if !a.shouldProcessType(info) { + if !v.shouldProcessLit(lit, info, enforcement) { return nil, "" } // unnamed structures are only defined in same package, along with types that has // prefix identical to current package name. - isSamePackage := info.PackagePath == pass.Pkg.Path() + isSamePackage := info.PackagePath == v.pass.Pkg.Path() - if f := a.litSkippedFields(lit, structTyp, !isSamePackage); len(f) > 0 { + if f := v.analyzer.litSkippedFields(lit, structTyp, !isSamePackage); len(f) > 0 { pos := lit.Pos() if len(f) == 1 { @@ -201,58 +289,77 @@ func (a *analyzer) processStruct( return nil, "" } -// shouldProcessType returns true if type should be processed basing off include -// and exclude patterns, defined though constructor and\or flags. -func (a *analyzer) shouldProcessType(info *TypeInfo) bool { +// shouldProcessLit returns true if type should be processed basing off include +// and exclude patterns, defined though constructor and\or flags, as well as off +// comment directives. +func (v *Visitor) shouldProcessLit(lit *ast.CompositeLit, info *TypeInfo, enforcement EnforcementDirective) bool { + a := v.analyzer + + // enforcement directives always have highest precedence if present + switch enforcement { + case Enforce: + return true + + case Ignore: + return false + } + if len(a.include) == 0 && len(a.exclude) == 0 { return true } - name := info.String() + return v.analyzer.isTypeProcessingNeeded(info) +} - a.typeProcessingNeedMu.RLock() - res, ok := a.typeProcessingNeed[name] - a.typeProcessingNeedMu.RUnlock() +func (v *Visitor) decideEnforcementDirective(lit *ast.CompositeLit, stack []ast.Node) EnforcementDirective { + if !v.analyzer.useDirectives { + return EnforcementUnspecified + } - if !ok { - a.typeProcessingNeedMu.Lock() - res = true + file, _ := stack[0].(*ast.File) + commentMap := v.analyzer.getFileCommentMap(v.pass.Fset, file) - if a.include != nil && !a.include.MatchFullString(name) { - res = false - } - - if res && a.exclude != nil && a.exclude.MatchFullString(name) { - res = false - } + if enforcement := v.readEnforcement(commentMap[lit]); enforcement != EnforcementUnspecified { + return enforcement + } - a.typeProcessingNeed[name] = res - a.typeProcessingNeedMu.Unlock() + parent := stack[len(stack)-2] + // allow directives to appear in parent nodes except other composite literals + if _, parentIsCompLit := parent.(*ast.CompositeLit); parentIsCompLit { + return EnforcementUnspecified } - return res + return v.readEnforcement(commentMap[parent]) } -//revive:disable-next-line:unused-receiver -func (a *analyzer) litSkippedFields( - lit *ast.CompositeLit, - typ *types.Struct, - onlyExported bool, -) fields.StructFields { - a.fieldsCacheMu.RLock() - f, ok := a.fieldsCache[typ] - a.fieldsCacheMu.RUnlock() +func (v *Visitor) readEnforcement(commentGroups []*ast.CommentGroup) EnforcementDirective { + // go from the end to the beginning + for i := len(commentGroups) - 1; i >= 0; i-- { + for j := len(commentGroups[i].List) - 1; j >= 0; j-- { + c := commentGroups[i].List[j] - if !ok { - a.fieldsCacheMu.Lock() - f = fields.NewStructFields(typ) - a.fieldsCache[typ] = f - a.fieldsCacheMu.Unlock() + normalized := strings.TrimSpace(c.Text) + switch normalized { + case "//exhaustruct:enforce": + return Enforce + + case "//exhaustruct:ignore": + return Ignore + } + } } - return f.SkippedFields(lit, onlyExported) + return EnforcementUnspecified } +type EnforcementDirective int + +const ( + EnforcementUnspecified EnforcementDirective = iota + Enforce + Ignore +) + type TypeInfo struct { Name string PackageName string diff --git a/analyzer/analyzer_benchmark_test.go b/analyzer/analyzer_benchmark_test.go index 9b66823f..c10312b0 100644 --- a/analyzer/analyzer_benchmark_test.go +++ b/analyzer/analyzer_benchmark_test.go @@ -13,6 +13,23 @@ func BenchmarkAnalyzer(b *testing.B) { a, err := analyzer.NewAnalyzer( []string{`.*[Tt]est.*`, `.*External`, `.*Embedded`, `.*\.`}, []string{`.*Excluded$`, `e\.`}, + false, + ) + require.NoError(b, err) + + b.ResetTimer() + b.ReportAllocs() + + for i := 0; i < b.N; i++ { + _ = analysistest.Run(b, testdataPath, a, "i") + } +} + +func BenchmarkAnalyzer_WithDirectives(b *testing.B) { + a, err := analyzer.NewAnalyzer( + []string{`.*[Tt]est.*`, `.*External`, `.*Embedded`, `.*\.`}, + []string{`.*Excluded$`, `e\.`}, + true, ) require.NoError(b, err) diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index e56366f7..55d9c490 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -16,27 +16,38 @@ var testdataPath, _ = filepath.Abs("./testdata/") //nolint:gochecknoglobals func TestAnalyzer(t *testing.T) { t.Parallel() - a, err := analyzer.NewAnalyzer([]string{""}, nil) + a, err := analyzer.NewAnalyzer([]string{""}, nil, true) assert.Nil(t, a) assert.Error(t, err) - a, err = analyzer.NewAnalyzer([]string{"["}, nil) + a, err = analyzer.NewAnalyzer([]string{"["}, nil, true) assert.Nil(t, a) assert.Error(t, err) - a, err = analyzer.NewAnalyzer(nil, []string{""}) + a, err = analyzer.NewAnalyzer(nil, []string{""}, true) assert.Nil(t, a) assert.Error(t, err) - a, err = analyzer.NewAnalyzer(nil, []string{"["}) + a, err = analyzer.NewAnalyzer(nil, []string{"["}, true) assert.Nil(t, a) assert.Error(t, err) a, err = analyzer.NewAnalyzer( []string{`.*[Tt]est.*`, `.*External`, `.*Embedded`, `.*\.`}, []string{`.*Excluded$`, `e\.`}, + false, ) require.NoError(t, err) analysistest.Run(t, testdataPath, a, "i", "e") + + a, err = analyzer.NewAnalyzer( + []string{`.*[Tt]est.*`, `.*External`, `.*Embedded`, `.*\.`}, + []string{`.*Excluded$`, `e\.`}, + true, + ) + require.NoError(t, err) + + analysistest.Run(t, testdataPath, a, "i", "e", "directives") + } diff --git a/analyzer/testdata/src/directives/directives.go b/analyzer/testdata/src/directives/directives.go new file mode 100644 index 00000000..8bd01d43 --- /dev/null +++ b/analyzer/testdata/src/directives/directives.go @@ -0,0 +1,69 @@ +package directives + +import ( + "i" +) + +func excludedConsumer(e i.TestExcluded) string { + return e.A +} + +func shouldFailEnforcementDirective() { + //exhaustruct:enforce + _ = i.TestExcluded{ // want "i.TestExcluded is missing field A" + B: 0, + } + + _ = i.TestExcluded{ // want "i.TestExcluded is missing field A" + B: 0, + } //exhaustruct:enforce + + _ = excludedConsumer( + //exhaustruct:enforce + i.TestExcluded{ // want "i.TestExcluded is missing field A" + B: 0, + }, + ) + + _ = + //exhaustruct:enforce + i.Test{ // want "i.Test is missing field A" + B: 0, + C: 0.0, + D: false, + E: "", + } +} + +func shouldSucceedIgnoreDirective() { + //exhaustruct:ignore + _ = i.Test3{ + B: 0, + } + + _ = + //exhaustruct:ignore + i.Test3{ + B: 0, + } + + //exhaustruct:ignore + _ = i.Test2{ + //exhaustruct:ignore + Embedded: i.Embedded{}, + } +} + +func misappliedDirectives() { + // associated with wrong parent node + //exhaustruct:enforce + _ = excludedConsumer(i.TestExcluded{ + B: 0, + }) + + // wrong directive name + //exhaustive:enforce + _ = i.TestExcluded{ + B: 0, + } +} diff --git a/cmd/exhaustruct/main.go b/cmd/exhaustruct/main.go index b2e20435..8f66c3a2 100644 --- a/cmd/exhaustruct/main.go +++ b/cmd/exhaustruct/main.go @@ -11,7 +11,7 @@ import ( func main() { flag.Bool("unsafeptr", false, "") - a, err := analyzer.NewAnalyzer(nil, nil) + a, err := analyzer.NewAnalyzer(nil, nil, false) if err != nil { panic(err) } From ebf71fcf8ffb9c18329f0750dbdcfb249f46b0be Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Sun, 30 Jul 2023 16:04:54 -0400 Subject: [PATCH 02/11] Fix lint issues --- analyzer/analyzer.go | 27 +++++++++++++++------------ analyzer/analyzer_test.go | 1 - 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index e3d3284c..ea043a06 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -28,8 +28,8 @@ type analyzer struct { typeProcessingNeed map[string]bool typeProcessingNeedMu sync.RWMutex `exhaustruct:"optional"` - commentMapCache map[*ast.File]ast.CommentMap - commentMapCacheMu sync.RWMutex `exhaustruct:"optional"` + commentMapCache map[*ast.File]ast.CommentMap `exhaustruct:"optional"` + commentMapCacheMu sync.RWMutex `exhaustruct:"optional"` } func NewAnalyzer(include, exclude []string, useDirectives bool) (*analysis.Analyzer, error) { @@ -97,7 +97,7 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { visitor.Visit, ) - return nil, nil //nolint:nilnil + return nil, nil } // visitor that only expects [ast.CompositeLit] nodes. @@ -268,7 +268,7 @@ func (v *Visitor) processStruct( info *TypeInfo, enforcement EnforcementDirective, ) (*token.Pos, string) { - if !v.shouldProcessLit(lit, info, enforcement) { + if !v.shouldProcessLit(info, enforcement) { return nil, "" } @@ -292,9 +292,9 @@ func (v *Visitor) processStruct( // shouldProcessLit returns true if type should be processed basing off include // and exclude patterns, defined though constructor and\or flags, as well as off // comment directives. -func (v *Visitor) shouldProcessLit(lit *ast.CompositeLit, info *TypeInfo, enforcement EnforcementDirective) bool { - a := v.analyzer - +func (v *Visitor) shouldProcessLit( + info *TypeInfo, enforcement EnforcementDirective, +) bool { // enforcement directives always have highest precedence if present switch enforcement { case Enforce: @@ -302,13 +302,16 @@ func (v *Visitor) shouldProcessLit(lit *ast.CompositeLit, info *TypeInfo, enforc case Ignore: return false + + case EnforcementUnspecified: } - if len(a.include) == 0 && len(a.exclude) == 0 { + analyzer := v.analyzer + if len(analyzer.include) == 0 && len(analyzer.exclude) == 0 { return true } - return v.analyzer.isTypeProcessingNeeded(info) + return analyzer.isTypeProcessingNeeded(info) } func (v *Visitor) decideEnforcementDirective(lit *ast.CompositeLit, stack []ast.Node) EnforcementDirective { @@ -319,7 +322,7 @@ func (v *Visitor) decideEnforcementDirective(lit *ast.CompositeLit, stack []ast. file, _ := stack[0].(*ast.File) commentMap := v.analyzer.getFileCommentMap(v.pass.Fset, file) - if enforcement := v.readEnforcement(commentMap[lit]); enforcement != EnforcementUnspecified { + if enforcement := parseEnforcement(commentMap[lit]); enforcement != EnforcementUnspecified { return enforcement } @@ -329,10 +332,10 @@ func (v *Visitor) decideEnforcementDirective(lit *ast.CompositeLit, stack []ast. return EnforcementUnspecified } - return v.readEnforcement(commentMap[parent]) + return parseEnforcement(commentMap[parent]) } -func (v *Visitor) readEnforcement(commentGroups []*ast.CommentGroup) EnforcementDirective { +func parseEnforcement(commentGroups []*ast.CommentGroup) EnforcementDirective { // go from the end to the beginning for i := len(commentGroups) - 1; i >= 0; i-- { for j := len(commentGroups[i].List) - 1; j >= 0; j-- { diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 55d9c490..4eb1f8c1 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -49,5 +49,4 @@ func TestAnalyzer(t *testing.T) { require.NoError(t, err) analysistest.Run(t, testdataPath, a, "i", "e", "directives") - } From 68214fd6ef3b507456a398970e8005ae678f41c3 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Sun, 30 Jul 2023 16:09:04 -0400 Subject: [PATCH 03/11] Fix README typo --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 9c20b86e..531902c6 100644 --- a/README.md +++ b/README.md @@ -122,7 +122,7 @@ var d = excluded.Point{ } // invalid, `Y` is missing, due to enforce directive (when -use-directives=true) -//exhastruct:enforce +//exhaustruct:enforce var e = excluded.Point{ X: 1, } From c055a4fdebb95c8ca3feba85fd843678ffc5a296 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Sun, 30 Jul 2023 16:24:27 -0400 Subject: [PATCH 04/11] Add comments to make benchmarks fairer --- analyzer/testdata/src/i/i.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/analyzer/testdata/src/i/i.go b/analyzer/testdata/src/i/i.go index a723b36c..0c18c38a 100644 --- a/analyzer/testdata/src/i/i.go +++ b/analyzer/testdata/src/i/i.go @@ -7,6 +7,7 @@ import ( "e" ) +// Embedded is a test struct that is subject to enforcement by inclusion flags type Embedded struct { E string F string @@ -14,6 +15,8 @@ type Embedded struct { H string } +// Test is a test struct that is subject to enforcement by inclusion flags but +// contains an optional field type Test struct { A string B int @@ -22,11 +25,13 @@ type Test struct { E string `exhaustruct:"optional"` } +// Test2 is a test struct that is subject to enforcement by inclusion flags type Test2 struct { Embedded External e.External } +// The struct literal is fully filled out and should pass func shouldPassFullyDefined() { _ = Test{ A: "", @@ -37,6 +42,7 @@ func shouldPassFullyDefined() { } } +// The struct pointer literal is fully filled out and should pass func shouldPassPointer() { _ = &Test{ A: "", @@ -47,6 +53,7 @@ func shouldPassPointer() { } } +// The struct pointer literal is fully filled out aside from optional fields and should pass func shouldPassOnlyOptionalOmitted() { _ = Test{ A: "", @@ -56,6 +63,7 @@ func shouldPassOnlyOptionalOmitted() { } } +// The struct pointer literal is missing non-optional fields and should fail func shouldFailRequiredOmitted() { _ = Test{ // want "i.Test is missing field D" A: "", @@ -64,22 +72,27 @@ func shouldFailRequiredOmitted() { } } +// Returning an empty struct literal with a non-nil error should pass func shouldPassEmptyStructWithNonNilErr() (Test, error) { return Test{}, errors.New("some error") } +// Returning an empty struct literal with a nil error should fail func shouldFailEmptyStructWithNilErr() (Test, error) { return Test{}, nil // want "i.Test is missing fields A, B, C, D" } +// Returning an slice of empty struct literals with a nil error should fail func shouldFailEmptyNestedStructWithNonNilErr() ([]Test, error) { return []Test{{}}, nil // want "i.Test is missing fields A, B, C, D" } +// The struct is fully filled out using a list assignment and should pass func shouldPassUnnamed() { _ = []Test{{"", 0, 0.0, false, ""}} } +// The struct and its inner structs are fully filled out and should pass func shouldPassEmbedded() { _ = Test2{ External: e.External{ @@ -95,6 +108,7 @@ func shouldPassEmbedded() { } } +// The embedded inner struct is missing a field and should fail func shouldFailEmbedded() { _ = Test2{ External: e.External{ @@ -109,6 +123,7 @@ func shouldFailEmbedded() { } } +// The embedded inner struct is not specified and should fail func shouldFailEmbeddedCompletelyMissing() { _ = Test2{ // want "i.Test2 is missing field Embedded" External: e.External{ // want "e.External is missing field B" @@ -117,11 +132,13 @@ func shouldFailEmbeddedCompletelyMissing() { } } +// Struct with type parameters type testGenericStruct[T any] struct { A T B string } +// The type-parameterized struct is fully filled out and should pass func shouldPassGeneric() { _ = testGenericStruct[int]{ A: 42, @@ -129,6 +146,7 @@ func shouldPassGeneric() { } } +// The type-parameterized struct is missing a field and should fail func shouldFailGeneric() { _ = testGenericStruct[int]{} // want "i.testGenericStruct is missing fields A, B" _ = testGenericStruct[int]{ // want "i.testGenericStruct is missing field B" @@ -136,29 +154,35 @@ func shouldFailGeneric() { } } +// TestExcluded is a test struct that is subject to exclusion by exclusion flags type TestExcluded struct { A string B int } +// The struct is excluded and should pass func shouldPassExcluded() { _ = TestExcluded{} } +// NotIncluded is a test struct that is not included by inclusion flags type NotIncluded struct { A string B int } +// The struct is not excluded and should pass func shouldPassNotIncluded() { _ = NotIncluded{} } +// Test3 is a test struct that is subject to enforcement by inclusion flags and has an optional field type Test3 struct { A string B int `exhaustruct:"optional"` } +// All structs in the slice are fully filled out and should pass func shouldPassSlicesOfStructs() { _ = []Test3{ {"a", 1}, @@ -167,6 +191,7 @@ func shouldPassSlicesOfStructs() { } } +// All structs in the slice are missing some fields and should fail func shouldFailSlicesOfStructs() { _ = []Test3{ {}, // want "i.Test3 is missing field A" @@ -174,6 +199,7 @@ func shouldFailSlicesOfStructs() { } } +// All structs in the map are fully filled out and should pass func shouldPassMapOfStructs() { _ = map[string]Test3{ "a": {"a", 1}, @@ -182,6 +208,7 @@ func shouldPassMapOfStructs() { } } +// All structs in the map are missing some fields and should fail func shouldFailMapOfStructs() { _ = map[string]Test3{ "a": {}, // want "i.Test3 is missing field A" @@ -189,10 +216,12 @@ func shouldFailMapOfStructs() { } } +// Slices of strings are not subject to enforcement and should pass func shouldPassSlice() { _ = []string{"a", "b"} } +// All anonymous structs are fully filled out and should pass func shouldPassAnonymousStruct() { _ = struct { A string @@ -203,6 +232,7 @@ func shouldPassAnonymousStruct() { } } +// All anonymous structs are subject to enforcement and missing some fields and should fail func shouldFailAnonymousStructUnfilled() { _ = struct { // want "i. is missing field A" A string From ab250d174bac1094865e2a05aaa163b39426a1ac Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Wed, 2 Aug 2023 15:50:49 -0400 Subject: [PATCH 05/11] Unconditionally initialize map --- analyzer/analyzer.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index ea043a06..6ca477fd 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -36,11 +36,9 @@ func NewAnalyzer(include, exclude []string, useDirectives bool) (*analysis.Analy a := analyzer{ fieldsCache: make(map[types.Type]fields.StructFields), typeProcessingNeed: make(map[string]bool), + commentMapeCache: make(map[*ast.File]ast.CommentMap), useDirectives: useDirectives, } - if useDirectives { - a.commentMapCache = make(map[*ast.File]ast.CommentMap) - } var err error From 22540a038076837b2374853edbf9460ea240a0c3 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Wed, 2 Aug 2023 16:03:03 -0400 Subject: [PATCH 06/11] Fix typo --- analyzer/analyzer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 6ca477fd..8251deed 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -36,7 +36,7 @@ func NewAnalyzer(include, exclude []string, useDirectives bool) (*analysis.Analy a := analyzer{ fieldsCache: make(map[types.Type]fields.StructFields), typeProcessingNeed: make(map[string]bool), - commentMapeCache: make(map[*ast.File]ast.CommentMap), + commentMapCache: make(map[*ast.File]ast.CommentMap), useDirectives: useDirectives, } From ba3bc337be804ba938b5874cdc4d341c16d9d8fa Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Tue, 21 Nov 2023 21:38:43 -0500 Subject: [PATCH 07/11] Remove Visitor --- analyzer/analyzer.go | 110 +++++++++++++++++++++---------------------- 1 file changed, 55 insertions(+), 55 deletions(-) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 8251deed..3fd4bdd7 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -36,7 +36,7 @@ func NewAnalyzer(include, exclude []string, useDirectives bool) (*analysis.Analy a := analyzer{ fieldsCache: make(map[types.Type]fields.StructFields), typeProcessingNeed: make(map[string]bool), - commentMapCache: make(map[*ast.File]ast.CommentMap), + commentMapCache: make(map[*ast.File]ast.CommentMap), useDirectives: useDirectives, } @@ -84,15 +84,11 @@ any include/exclude patterns. Default: false.`) func (a *analyzer) run(pass *analysis.Pass) (any, error) { insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) //nolint:forcetypeassert - visitor := &Visitor{ - analyzer: a, - pass: pass, - } insp.WithStack( []ast.Node{ (*ast.CompositeLit)(nil), }, - visitor.Visit, + a.newVisitor(pass), ) return nil, nil @@ -105,46 +101,48 @@ type Visitor struct { } // Implements inspector.Visitor interface. -func (v *Visitor) Visit(n ast.Node, push bool, stack []ast.Node) bool { - if !push { - return true - } +func (a *analyzer) newVisitor(pass *analysis.Pass) func(n ast.Node, push bool, stack []ast.Node) bool { + return func(n ast.Node, push bool, stack []ast.Node) bool { + if !push { + return true + } - lit, ok := n.(*ast.CompositeLit) - if !ok { - // this should never happen, but better be prepared - return true - } + lit, ok := n.(*ast.CompositeLit) + if !ok { + // this should never happen, but better be prepared + return true + } - structTyp, typeInfo, ok := v.getStructType(lit) - if !ok { - return true - } + structTyp, typeInfo, ok := getStructType(pass, lit) + if !ok { + return true + } - if len(lit.Elts) == 0 { - if ret, ok := stackParentIsReturn(stack); ok { - if v.returnContainsNonNilError(ret) { - // it is okay to return uninitialized structure in case struct's direct parent is - // a return statement containing non-nil error - // - // we're unable to check if returned error is custom, but at least we're able to - // cover str [error] type. - return true + if len(lit.Elts) == 0 { + if ret, ok := stackParentIsReturn(stack); ok { + if returnContainsNonNilError(pass, ret) { + // it is okay to return uninitialized structure in case struct's direct parent is + // a return statement containing non-nil error + // + // we're unable to check if returned error is custom, but at least we're able to + // cover str [error] type. + return true + } } } - } - var enforcement EnforcementDirective - if v.analyzer.useDirectives { - enforcement = v.decideEnforcementDirective(lit, stack) - } + var enforcement EnforcementDirective + if a.useDirectives { + enforcement = a.decideEnforcementDirective(pass, lit, stack) + } - pos, msg := v.processStruct(lit, structTyp, typeInfo, enforcement) - if pos != nil { - v.pass.Reportf(*pos, msg) - } + pos, msg := a.processStruct(pass, lit, structTyp, typeInfo, enforcement) + if pos != nil { + pass.Reportf(*pos, msg) + } - return true + return true + } } //revive:disable-next-line:unused-receiver @@ -210,8 +208,8 @@ func (a *analyzer) isTypeProcessingNeeded(info *TypeInfo) bool { return res } -func (v *Visitor) getStructType(lit *ast.CompositeLit) (*types.Struct, *TypeInfo, bool) { - switch typ := v.pass.TypesInfo.TypeOf(lit).(type) { +func getStructType(pass *analysis.Pass, lit *ast.CompositeLit) (*types.Struct, *TypeInfo, bool) { + switch typ := pass.TypesInfo.TypeOf(lit).(type) { case *types.Named: // named type if structTyp, ok := typ.Underlying().(*types.Struct); ok { pkg := typ.Obj().Pkg() @@ -229,8 +227,8 @@ func (v *Visitor) getStructType(lit *ast.CompositeLit) (*types.Struct, *TypeInfo case *types.Struct: // anonymous struct ti := TypeInfo{ Name: "", - PackageName: v.pass.Pkg.Name(), - PackagePath: v.pass.Pkg.Path(), + PackageName: pass.Pkg.Name(), + PackagePath: pass.Pkg.Path(), } return typ, &ti, true @@ -248,11 +246,11 @@ func stackParentIsReturn(stack []ast.Node) (*ast.ReturnStmt, bool) { return ret, ok } -func (v *Visitor) returnContainsNonNilError(ret *ast.ReturnStmt) bool { +func returnContainsNonNilError(pass *analysis.Pass, ret *ast.ReturnStmt) bool { // errors are mostly located at the end of return statement, so we're starting // from the end. for i := len(ret.Results) - 1; i >= 0; i-- { - if v.pass.TypesInfo.TypeOf(ret.Results[i]).String() == "error" { + if pass.TypesInfo.TypeOf(ret.Results[i]).String() == "error" { return true } } @@ -260,21 +258,22 @@ func (v *Visitor) returnContainsNonNilError(ret *ast.ReturnStmt) bool { return false } -func (v *Visitor) processStruct( +func (a *analyzer) processStruct( + pass *analysis.Pass, lit *ast.CompositeLit, structTyp *types.Struct, info *TypeInfo, enforcement EnforcementDirective, ) (*token.Pos, string) { - if !v.shouldProcessLit(info, enforcement) { + if !a.shouldProcessLit(info, enforcement) { return nil, "" } // unnamed structures are only defined in same package, along with types that has // prefix identical to current package name. - isSamePackage := info.PackagePath == v.pass.Pkg.Path() + isSamePackage := info.PackagePath == pass.Pkg.Path() - if f := v.analyzer.litSkippedFields(lit, structTyp, !isSamePackage); len(f) > 0 { + if f := a.litSkippedFields(lit, structTyp, !isSamePackage); len(f) > 0 { pos := lit.Pos() if len(f) == 1 { @@ -290,7 +289,7 @@ func (v *Visitor) processStruct( // shouldProcessLit returns true if type should be processed basing off include // and exclude patterns, defined though constructor and\or flags, as well as off // comment directives. -func (v *Visitor) shouldProcessLit( +func (a *analyzer) shouldProcessLit( info *TypeInfo, enforcement EnforcementDirective, ) bool { // enforcement directives always have highest precedence if present @@ -304,21 +303,22 @@ func (v *Visitor) shouldProcessLit( case EnforcementUnspecified: } - analyzer := v.analyzer - if len(analyzer.include) == 0 && len(analyzer.exclude) == 0 { + if len(a.include) == 0 && len(a.exclude) == 0 { return true } - return analyzer.isTypeProcessingNeeded(info) + return a.isTypeProcessingNeeded(info) } -func (v *Visitor) decideEnforcementDirective(lit *ast.CompositeLit, stack []ast.Node) EnforcementDirective { - if !v.analyzer.useDirectives { +func (a *analyzer) decideEnforcementDirective( + pass *analysis.Pass, lit *ast.CompositeLit, stack []ast.Node, +) EnforcementDirective { + if !a.useDirectives { return EnforcementUnspecified } file, _ := stack[0].(*ast.File) - commentMap := v.analyzer.getFileCommentMap(v.pass.Fset, file) + commentMap := a.getFileCommentMap(pass.Fset, file) if enforcement := parseEnforcement(commentMap[lit]); enforcement != EnforcementUnspecified { return enforcement From f09e1e01cf851bd3518a76456fdbb210ebb8e985 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Tue, 21 Nov 2023 21:44:27 -0500 Subject: [PATCH 08/11] Remove more and restore original ordering --- analyzer/analyzer.go | 134 +++++++++++++++++++++---------------------- 1 file changed, 64 insertions(+), 70 deletions(-) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 3fd4bdd7..cb271a32 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -91,13 +91,7 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { a.newVisitor(pass), ) - return nil, nil -} - -// visitor that only expects [ast.CompositeLit] nodes. -type Visitor struct { - analyzer *analyzer - pass *analysis.Pass + return nil, nil //nolint:nilnil } // Implements inspector.Visitor interface. @@ -145,69 +139,6 @@ func (a *analyzer) newVisitor(pass *analysis.Pass) func(n ast.Node, push bool, s } } -//revive:disable-next-line:unused-receiver -func (a *analyzer) litSkippedFields( - lit *ast.CompositeLit, - typ *types.Struct, - onlyExported bool, -) fields.StructFields { - a.fieldsCacheMu.RLock() - f, ok := a.fieldsCache[typ] - a.fieldsCacheMu.RUnlock() - - if !ok { - a.fieldsCacheMu.Lock() - f = fields.NewStructFields(typ) - a.fieldsCache[typ] = f - a.fieldsCacheMu.Unlock() - } - - return f.SkippedFields(lit, onlyExported) -} - -func (a *analyzer) getFileCommentMap(fileSet *token.FileSet, file *ast.File) ast.CommentMap { - a.commentMapCacheMu.RLock() - commentMap, exists := a.commentMapCache[file] - a.commentMapCacheMu.RUnlock() - - if !exists { - // TODO: consider avoiding risk of double-computation by using per-file mutex - commentMap = ast.NewCommentMap(fileSet, file, file.Comments) - - a.commentMapCacheMu.Lock() - a.commentMapCache[file] = commentMap - a.commentMapCacheMu.Unlock() - } - - return commentMap -} - -func (a *analyzer) isTypeProcessingNeeded(info *TypeInfo) bool { - name := info.String() - - a.typeProcessingNeedMu.RLock() - res, ok := a.typeProcessingNeed[name] - a.typeProcessingNeedMu.RUnlock() - - if !ok { - a.typeProcessingNeedMu.Lock() - res = true - - if a.include != nil && !a.include.MatchFullString(name) { - res = false - } - - if res && a.exclude != nil && a.exclude.MatchFullString(name) { - res = false - } - - a.typeProcessingNeed[name] = res - a.typeProcessingNeedMu.Unlock() - } - - return res -} - func getStructType(pass *analysis.Pass, lit *ast.CompositeLit) (*types.Struct, *TypeInfo, bool) { switch typ := pass.TypesInfo.TypeOf(lit).(type) { case *types.Named: // named type @@ -333,6 +264,69 @@ func (a *analyzer) decideEnforcementDirective( return parseEnforcement(commentMap[parent]) } +//revive:disable-next-line:unused-receiver +func (a *analyzer) litSkippedFields( + lit *ast.CompositeLit, + typ *types.Struct, + onlyExported bool, +) fields.StructFields { + a.fieldsCacheMu.RLock() + f, ok := a.fieldsCache[typ] + a.fieldsCacheMu.RUnlock() + + if !ok { + a.fieldsCacheMu.Lock() + f = fields.NewStructFields(typ) + a.fieldsCache[typ] = f + a.fieldsCacheMu.Unlock() + } + + return f.SkippedFields(lit, onlyExported) +} + +func (a *analyzer) getFileCommentMap(fileSet *token.FileSet, file *ast.File) ast.CommentMap { + a.commentMapCacheMu.RLock() + commentMap, exists := a.commentMapCache[file] + a.commentMapCacheMu.RUnlock() + + if !exists { + // TODO: consider avoiding risk of double-computation by using per-file mutex + commentMap = ast.NewCommentMap(fileSet, file, file.Comments) + + a.commentMapCacheMu.Lock() + a.commentMapCache[file] = commentMap + a.commentMapCacheMu.Unlock() + } + + return commentMap +} + +func (a *analyzer) isTypeProcessingNeeded(info *TypeInfo) bool { + name := info.String() + + a.typeProcessingNeedMu.RLock() + res, ok := a.typeProcessingNeed[name] + a.typeProcessingNeedMu.RUnlock() + + if !ok { + a.typeProcessingNeedMu.Lock() + res = true + + if a.include != nil && !a.include.MatchFullString(name) { + res = false + } + + if res && a.exclude != nil && a.exclude.MatchFullString(name) { + res = false + } + + a.typeProcessingNeed[name] = res + a.typeProcessingNeedMu.Unlock() + } + + return res +} + func parseEnforcement(commentGroups []*ast.CommentGroup) EnforcementDirective { // go from the end to the beginning for i := len(commentGroups) - 1; i >= 0; i-- { From 3bddbac94b2aa0264b16022b82449aa7b3626b25 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Tue, 21 Nov 2023 21:46:08 -0500 Subject: [PATCH 09/11] Slight reorder --- analyzer/analyzer.go | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index cb271a32..81d53def 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -241,6 +241,26 @@ func (a *analyzer) shouldProcessLit( return a.isTypeProcessingNeeded(info) } +//revive:disable-next-line:unused-receiver +func (a *analyzer) litSkippedFields( + lit *ast.CompositeLit, + typ *types.Struct, + onlyExported bool, +) fields.StructFields { + a.fieldsCacheMu.RLock() + f, ok := a.fieldsCache[typ] + a.fieldsCacheMu.RUnlock() + + if !ok { + a.fieldsCacheMu.Lock() + f = fields.NewStructFields(typ) + a.fieldsCache[typ] = f + a.fieldsCacheMu.Unlock() + } + + return f.SkippedFields(lit, onlyExported) +} + func (a *analyzer) decideEnforcementDirective( pass *analysis.Pass, lit *ast.CompositeLit, stack []ast.Node, ) EnforcementDirective { @@ -264,26 +284,6 @@ func (a *analyzer) decideEnforcementDirective( return parseEnforcement(commentMap[parent]) } -//revive:disable-next-line:unused-receiver -func (a *analyzer) litSkippedFields( - lit *ast.CompositeLit, - typ *types.Struct, - onlyExported bool, -) fields.StructFields { - a.fieldsCacheMu.RLock() - f, ok := a.fieldsCache[typ] - a.fieldsCacheMu.RUnlock() - - if !ok { - a.fieldsCacheMu.Lock() - f = fields.NewStructFields(typ) - a.fieldsCache[typ] = f - a.fieldsCacheMu.Unlock() - } - - return f.SkippedFields(lit, onlyExported) -} - func (a *analyzer) getFileCommentMap(fileSet *token.FileSet, file *ast.File) ast.CommentMap { a.commentMapCacheMu.RLock() commentMap, exists := a.commentMapCache[file] From fbcf3dc0d59189d26ee49e1b903764eb7a8408d8 Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Tue, 21 Nov 2023 21:47:58 -0500 Subject: [PATCH 10/11] Revert comment change --- analyzer/analyzer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 81d53def..94f7bfa5 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -94,7 +94,7 @@ func (a *analyzer) run(pass *analysis.Pass) (any, error) { return nil, nil //nolint:nilnil } -// Implements inspector.Visitor interface. +// newVisitor returns visitor that only expects [ast.CompositeLit] nodes. func (a *analyzer) newVisitor(pass *analysis.Pass) func(n ast.Node, push bool, stack []ast.Node) bool { return func(n ast.Node, push bool, stack []ast.Node) bool { if !push { From 4b9e8ebb614773727527fde480f4ad3904ac072b Mon Sep 17 00:00:00 2001 From: Navneeth Jayendran Date: Tue, 21 Nov 2023 21:52:11 -0500 Subject: [PATCH 11/11] Fix lint error --- analyzer/analyzer.go | 1 + 1 file changed, 1 insertion(+) diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 94f7bfa5..b97cb1c5 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -268,6 +268,7 @@ func (a *analyzer) decideEnforcementDirective( return EnforcementUnspecified } + //revive:disable-next-line:unchecked-type-assertion file, _ := stack[0].(*ast.File) commentMap := a.getFileCommentMap(pass.Fset, file)