From a2d55a9b0f66f358de031a32f50ac9d612934227 Mon Sep 17 00:00:00 2001 From: Anton Zinovyev Date: Thu, 4 Jan 2024 16:18:59 +0100 Subject: [PATCH] feat: add support for `ignore` and `enforce` comment directives (#88) --- .github/workflows/ci.yml | 2 +- .golangci.yaml | 2 + analyzer/analyzer.go | 54 ++++++++++++++--- analyzer/testdata/src/i/directives.go | 45 ++++++++++++++ internal/comment/cache.go | 35 +++++++++++ internal/comment/directive.go | 28 +++++++++ internal/comment/directive_test.go | 86 +++++++++++++++++++++++++++ 7 files changed, 243 insertions(+), 9 deletions(-) create mode 100644 analyzer/testdata/src/i/directives.go create mode 100644 internal/comment/cache.go create mode 100644 internal/comment/directive.go create mode 100644 internal/comment/directive_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4801ddd1..314b53d0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,7 +37,7 @@ jobs: name: "Test" strategy: matrix: - go-version: [ "1.21" ] + go-version: [ "1.20", "1.21" ] os: [ "ubuntu-latest", "windows-latest", "macos-latest" ] runs-on: ${{ matrix.os }} steps: diff --git a/.golangci.yaml b/.golangci.yaml index fb3c300d..ed08ef64 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -73,6 +73,8 @@ linters-settings: disabled: false arguments: [ 6 ] # disabled rules + - name: unchecked-type-assertion # forcetypeassert is already used + disabled: true - name: max-public-structs # quite annoying rule disabled: true - name: banned-characters # we don't have banned chars diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index d0cd2d5b..35ce1350 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -12,6 +12,7 @@ import ( "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" + "github.com/GaijinEntertainment/go-exhaustruct/v3/internal/comment" "github.com/GaijinEntertainment/go-exhaustruct/v3/internal/fields" "github.com/GaijinEntertainment/go-exhaustruct/v3/internal/pattern" ) @@ -23,6 +24,8 @@ type analyzer struct { fieldsCache map[types.Type]fields.StructFields fieldsCacheMu sync.RWMutex `exhaustruct:"optional"` + comments comment.Cache + typeProcessingNeed map[string]bool typeProcessingNeedMu sync.RWMutex `exhaustruct:"optional"` } @@ -31,6 +34,7 @@ func NewAnalyzer(include, exclude []string) (*analysis.Analyzer, error) { a := analyzer{ fieldsCache: make(map[types.Type]fields.StructFields), typeProcessingNeed: make(map[string]bool), + comments: comment.Cache{}, } var err error @@ -74,12 +78,7 @@ 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 - insp.WithStack( - []ast.Node{ - (*ast.CompositeLit)(nil), - }, - a.newVisitor(pass), - ) + insp.WithStack([]ast.Node{(*ast.CompositeLit)(nil)}, a.newVisitor(pass)) return nil, nil //nolint:nilnil } @@ -115,7 +114,10 @@ func (a *analyzer) newVisitor(pass *analysis.Pass) func(n ast.Node, push bool, s } } - pos, msg := a.processStruct(pass, lit, structTyp, typeInfo) + file := a.comments.Get(pass.Fset, stack[0].(*ast.File)) //nolint:forcetypeassert + rc := getCompositeLitRelatedComments(stack, file) + pos, msg := a.processStruct(pass, lit, structTyp, typeInfo, rc) + if pos != nil { pass.Reportf(*pos, msg) } @@ -124,6 +126,35 @@ func (a *analyzer) newVisitor(pass *analysis.Pass) func(n ast.Node, push bool, s } } +// getCompositeLitRelatedComments returns all comments that are related to checked node. We +// have to traverse the stack manually as ast do not associate comments with +// [ast.CompositeLit]. +func getCompositeLitRelatedComments(stack []ast.Node, cm ast.CommentMap) []*ast.CommentGroup { + comments := make([]*ast.CommentGroup, 0) + + for i := len(stack) - 1; i >= 0; i-- { + node := stack[i] + + switch node.(type) { + case *ast.CompositeLit, // stack[len(stack)-1] + *ast.ReturnStmt, // return ... + *ast.IndexExpr, // map[enum]...{...}[key] + *ast.CallExpr, // myfunc(map...) + *ast.UnaryExpr, // &map... + *ast.AssignStmt, // variable assignment (without var keyword) + *ast.DeclStmt, // var declaration, parent of *ast.GenDecl + *ast.GenDecl, // var declaration, parent of *ast.ValueSpec + *ast.ValueSpec: // var declaration + comments = append(comments, cm[node]...) + + default: + return comments + } + } + + return comments +} + func getStructType(pass *analysis.Pass, lit *ast.CompositeLit) (*types.Struct, *TypeInfo, bool) { switch typ := pass.TypesInfo.TypeOf(lit).(type) { case *types.Named: // named type @@ -179,8 +210,15 @@ func (a *analyzer) processStruct( lit *ast.CompositeLit, structTyp *types.Struct, info *TypeInfo, + comments []*ast.CommentGroup, ) (*token.Pos, string) { - if !a.shouldProcessType(info) { + shouldProcess := a.shouldProcessType(info) + + if shouldProcess && comment.HasDirective(comments, comment.DirectiveIgnore) { + return nil, "" + } + + if !shouldProcess && !comment.HasDirective(comments, comment.DirectiveEnforce) { return nil, "" } diff --git a/analyzer/testdata/src/i/directives.go b/analyzer/testdata/src/i/directives.go new file mode 100644 index 00000000..b9f8606e --- /dev/null +++ b/analyzer/testdata/src/i/directives.go @@ -0,0 +1,45 @@ +package i + +func excludedConsumer(e TestExcluded) string { + return e.A +} + +func shouldNotFailOnIgnoreDirective() (Test, error) { + // directive on previous line + //exhaustruct:ignore + _ = Test2{} + + // directive at the end of the line + _ = Test{} //exhaustruct:ignore + + // some style weirdness + _ = + //exhaustruct:ignore + Test3{ + B: 0, + } + + // directive after the literal + _ = Test{ + B: 0, + } //exhaustruct:ignore + + //exhaustruct:ignore + return Test{}, nil +} + +func shouldFailOnExcludedButEnforced() { + // directive on previous line associated with different ast leaf + //exhaustruct:enforce + _ = excludedConsumer(TestExcluded{B: 0}) // want "i.TestExcluded is missing field A" + + // initially excluded, but enforced + //exhaustruct:enforce + _ = TestExcluded{} // want "i.TestExcluded is missing fields A, B" +} + +func shouldFailOnMisappliedDirectives() { + // wrong directive name + //exhaustive:enforce + _ = TestExcluded{B: 0} +} diff --git a/internal/comment/cache.go b/internal/comment/cache.go new file mode 100644 index 00000000..88edef63 --- /dev/null +++ b/internal/comment/cache.go @@ -0,0 +1,35 @@ +package comment + +import ( + "go/ast" + "go/token" + "sync" +) + +type Cache struct { + comments map[*ast.File]ast.CommentMap + mu sync.RWMutex +} + +// Get returns a comment map for a given file. In case if a comment map is not +// found, it creates a new one. +func (c *Cache) Get(fset *token.FileSet, f *ast.File) ast.CommentMap { + c.mu.RLock() + if cm, ok := c.comments[f]; ok { + c.mu.RUnlock() + return cm + } + c.mu.RUnlock() + + c.mu.Lock() + defer c.mu.Unlock() + + if c.comments == nil { + c.comments = make(map[*ast.File]ast.CommentMap) + } + + cm := ast.NewCommentMap(fset, f, f.Comments) + c.comments[f] = cm + + return cm +} diff --git a/internal/comment/directive.go b/internal/comment/directive.go new file mode 100644 index 00000000..a39a8076 --- /dev/null +++ b/internal/comment/directive.go @@ -0,0 +1,28 @@ +package comment + +import ( + "go/ast" + "strings" +) + +type Directive string + +const ( + prefix = `//exhaustruct:` + DirectiveIgnore Directive = prefix + `ignore` + DirectiveEnforce Directive = prefix + `enforce` +) + +// HasDirective parses a directive from a given list of comments. +// If no directive is found, the second return value is `false`. +func HasDirective(comments []*ast.CommentGroup, expected Directive) bool { + for _, cg := range comments { + for _, commentLine := range cg.List { + if strings.HasPrefix(commentLine.Text, string(expected)) { + return true + } + } + } + + return false +} diff --git a/internal/comment/directive_test.go b/internal/comment/directive_test.go new file mode 100644 index 00000000..173899f6 --- /dev/null +++ b/internal/comment/directive_test.go @@ -0,0 +1,86 @@ +package comment_test + +import ( + "go/ast" + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/GaijinEntertainment/go-exhaustruct/v3/internal/comment" +) + +func TestParseDirective(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + comments []*ast.CommentGroup + directive comment.Directive + found bool + }{ + { + name: "no directive", + comments: []*ast.CommentGroup{ + { + List: []*ast.Comment{ + { + Text: "// some comment", + }, + }, + }, + }, + directive: comment.DirectiveIgnore, + found: false, + }, + { + name: "directive found", + comments: []*ast.CommentGroup{ + { + List: []*ast.Comment{ + { + Text: "//exhaustruct:ignore", + }, + { + Text: "// some comment", + }, + { + Text: "//exhaustruct:enforce", + }, + }, + }, + }, + directive: comment.DirectiveIgnore, + found: true, + }, + { + name: "directive found (partial line match)", + comments: []*ast.CommentGroup{ + { + List: []*ast.Comment{ + { + Text: "//exhaustruct:ignore", + }, + { + Text: "// some comment", + }, + { + Text: "//exhaustruct:enforce beacuse of some reason", + }, + }, + }, + }, + directive: comment.DirectiveEnforce, + found: true, + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + assert.Equal(t, tt.found, comment.HasDirective(tt.comments, tt.directive)) + }) + } +}