From 012209a3a2c82c481852c1d4b487f516c1573536 Mon Sep 17 00:00:00 2001 From: Manuel Doncel Martos Date: Tue, 13 May 2025 23:22:16 +0200 Subject: [PATCH 1/2] docs(readme): updating README.md with golangci-lint info (#11) --- README.md | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 3fe5abb..cad1041 100644 --- a/README.md +++ b/README.md @@ -36,7 +36,20 @@ type Client struct { ## ⬇️ Getting Started -Install it by running: +### As a golangci-lint linter + +Enable the linter in your golangci-lint configuration file, e.g: + +```yaml +linters: + enable: + - embeddedstructfieldcheck + ... +``` + +### Standalone application + +Install EmbeddedStructFieldCheck by running: ```bash go install github.com/manuelarte/embeddedstructfieldcheck@latest From 8cd3d53cfff1ced87c499a875b2dfb4c461b5d55 Mon Sep 17 00:00:00 2001 From: Manuel Doncel Martos Date: Thu, 15 May 2025 16:46:51 +0200 Subject: [PATCH 2/2] feat: adding forbid-mutex setting (#12) Co-authored-by: ccoVeille <3875889+ccoVeille@users.noreply.github.com> Co-authored-by: Ludovic Fernandez --- README.md | 64 +++++++++++++++++++++++++- analyzer/analyzer.go | 26 +++++++---- analyzer/analyzer_test.go | 20 ++++++++ analyzer/testdata/src/mutex/mutex.go | 27 +++++++++++ analyzer/testdata/src/simple/simple.go | 5 ++ internal/astutils.go | 9 ---- internal/diag.go | 8 ++++ internal/structanalyzer.go | 37 ++++++++++++++- 8 files changed, 174 insertions(+), 22 deletions(-) create mode 100644 analyzer/testdata/src/mutex/mutex.go delete mode 100644 internal/astutils.go diff --git a/README.md b/README.md index cad1041..5b93700 100644 --- a/README.md +++ b/README.md @@ -45,6 +45,12 @@ linters: enable: - embeddedstructfieldcheck ... + + settings: + embeddedstructfieldcheck: + # Checks that sync.Mutex is not used as embedded field. + # Default: false + forbid-mutex: true ``` ### Standalone application @@ -58,10 +64,64 @@ go install github.com/manuelarte/embeddedstructfieldcheck@latest And then use it as: ```bash -embeddedstructfieldcheck [--fix] +embeddedstructfieldcheck [-forbid-mutex] [--fix] +``` + +- `forbid-mutex`: `true|false` (default `false`) + Checks that `sync.Mutex` and `sync.RWMutex` are not used as embedded fields. +- `fix`: `true|false` (default `false`) + Fix the case when there is no space between the embedded fields and the regular fields. + +## Why not using `sync` mutex as embedded field + +You are granting access to your internal synchronization methods out of your struct. + +This should not be delegated out to the callers. It's a source of bugs. + +As an example: + + + + + + + +
❌ Bad✅ Good
+ +```go +type ViewCount struct { + sync.Mutex + + N int +} + +v := ViewCount{} +v.Lock() +v.N++ +v.Unlock() ``` -- `fix`: Fix the case when there is no space between the embedded fields and the regular fields. + + +```go +type ViewCount struct { + mu sync.Mutex + + n int +} + +func (v *ViewCount) Increment() { + v.mu.Lock() + defer v.mu.Unlock() + + v.n++ +} + +v := ViewCount{} +v.Increment() +``` + +
## Resources diff --git a/analyzer/analyzer.go b/analyzer/analyzer.go index 23ddab7..d1b56c4 100644 --- a/analyzer/analyzer.go +++ b/analyzer/analyzer.go @@ -10,23 +10,34 @@ import ( "github.com/manuelarte/embeddedstructfieldcheck/internal" ) +const ForbidMutexName = "forbid-mutex" + func NewAnalyzer() *analysis.Analyzer { + var forbidMutex bool + a := &analysis.Analyzer{ Name: "embeddedstructfieldcheck", Doc: "Embedded types should be at the top of the field list of a struct, " + - "and there must be an empty line separating embedded fields from regular fields. methods, and constructors", - Run: run, + "and there must be an empty line separating embedded fields from regular fields.", + URL: "https://github.com/manuelarte/embeddedstructfieldcheck", + Run: func(pass *analysis.Pass) (any, error) { + run(pass, forbidMutex) + + //nolint:nilnil // impossible case. + return nil, nil + }, Requires: []*analysis.Analyzer{inspect.Analyzer}, } + a.Flags.BoolVar(&forbidMutex, ForbidMutexName, false, "Checks that sync.Mutex is not used as an embedded field.") + return a } -func run(pass *analysis.Pass) (any, error) { +func run(pass *analysis.Pass, forbidMutex bool) { insp, found := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) if !found { - //nolint:nilnil // impossible case. - return nil, nil + return } nodeFilter := []ast.Node{ @@ -39,9 +50,6 @@ func run(pass *analysis.Pass) (any, error) { return } - internal.Analyze(pass, node) + internal.Analyze(pass, node, forbidMutex) }) - - //nolint:nilnil //any, error - return nil, nil } diff --git a/analyzer/analyzer_test.go b/analyzer/analyzer_test.go index 028d190..d20beff 100644 --- a/analyzer/analyzer_test.go +++ b/analyzer/analyzer_test.go @@ -10,14 +10,28 @@ func TestAnalyzer(t *testing.T) { testCases := []struct { desc string patterns string + options map[string]string }{ { desc: "simple", patterns: "simple", + options: map[string]string{ + ForbidMutexName: "false", + }, }, { desc: "comments", patterns: "comments", + options: map[string]string{ + ForbidMutexName: "false", + }, + }, + { + desc: "mutex", + patterns: "mutex", + options: map[string]string{ + ForbidMutexName: "true", + }, }, } @@ -25,6 +39,12 @@ func TestAnalyzer(t *testing.T) { t.Run(test.desc, func(t *testing.T) { a := NewAnalyzer() + for k, v := range test.options { + if err := a.Flags.Set(k, v); err != nil { + t.Fatal(err) + } + } + analysistest.Run(t, analysistest.TestData(), a, test.patterns) }) } diff --git a/analyzer/testdata/src/mutex/mutex.go b/analyzer/testdata/src/mutex/mutex.go new file mode 100644 index 0000000..c98cd0c --- /dev/null +++ b/analyzer/testdata/src/mutex/mutex.go @@ -0,0 +1,27 @@ +package mutex + +import "sync" + +type MutextEmbedded struct { + sync.Mutex // want `sync.Mutex should not be embedded` +} + +type MutextNotEmbedded struct { + mu sync.Mutex +} + +type PointerMutextEmbedded struct { + *sync.Mutex // want `sync.Mutex should not be embedded` +} + +type RWMutextEmbedded struct { + sync.RWMutex // want `sync.RWMutex should not be embedded` +} + +type RWMutextNotEmbedded struct { + mu sync.RWMutex +} + +type PointerRWMutextEmbedded struct { + *sync.RWMutex // want `sync.RWMutex should not be embedded` +} diff --git a/analyzer/testdata/src/simple/simple.go b/analyzer/testdata/src/simple/simple.go index 62847ca..f24572f 100644 --- a/analyzer/testdata/src/simple/simple.go +++ b/analyzer/testdata/src/simple/simple.go @@ -2,6 +2,7 @@ package simple import ( "context" + "sync" "time" ) @@ -49,3 +50,7 @@ type StructWithTagsNoSpace struct { MixedEmbeddedAndNotEmbedded `json:"bar"` // want `there must be an empty line separating embedded fields from regular fields` D string } + +type SyncMutexEmbedded struct { + sync.Mutex +} diff --git a/internal/astutils.go b/internal/astutils.go deleted file mode 100644 index f524153..0000000 --- a/internal/astutils.go +++ /dev/null @@ -1,9 +0,0 @@ -package internal - -import ( - "go/ast" -) - -func IsFieldEmbedded(field *ast.Field) bool { - return len(field.Names) == 0 -} diff --git a/internal/diag.go b/internal/diag.go index b894811..337b0df 100644 --- a/internal/diag.go +++ b/internal/diag.go @@ -1,6 +1,7 @@ package internal import ( + "fmt" "go/ast" "golang.org/x/tools/go/analysis" @@ -38,3 +39,10 @@ func NewMissingSpaceDiag( }, } } + +func NewForbiddenEmbeddedFieldDiag(forbidField *ast.SelectorExpr) analysis.Diagnostic { + return analysis.Diagnostic{ + Pos: forbidField.Pos(), + Message: fmt.Sprintf("%s.%s should not be embedded", forbidField.X, forbidField.Sel.Name), + } +} diff --git a/internal/structanalyzer.go b/internal/structanalyzer.go index 034f926..834da5d 100644 --- a/internal/structanalyzer.go +++ b/internal/structanalyzer.go @@ -6,7 +6,7 @@ import ( "golang.org/x/tools/go/analysis" ) -func Analyze(pass *analysis.Pass, st *ast.StructType) { +func Analyze(pass *analysis.Pass, st *ast.StructType, forbidMutex bool) { var firstEmbeddedField *ast.Field var lastEmbeddedField *ast.Field @@ -14,7 +14,9 @@ func Analyze(pass *analysis.Pass, st *ast.StructType) { var firstNotEmbeddedField *ast.Field for _, field := range st.Fields.List { - if IsFieldEmbedded(field) { + if isFieldEmbedded(field) { + checkForbiddenEmbeddedField(pass, field, forbidMutex) + if firstEmbeddedField == nil { firstEmbeddedField = field } @@ -35,6 +37,22 @@ func Analyze(pass *analysis.Pass, st *ast.StructType) { checkMissingSpace(pass, lastEmbeddedField, firstNotEmbeddedField) } +func checkForbiddenEmbeddedField(pass *analysis.Pass, field *ast.Field, forbidMutex bool) { + if !forbidMutex { + return + } + + switch e := field.Type.(type) { + case *ast.StarExpr: + if se, isSelectorExpr := e.X.(*ast.SelectorExpr); isSelectorExpr { + reportSyncMutex(pass, se) + } + + case *ast.SelectorExpr: + reportSyncMutex(pass, e) + } +} + func checkMissingSpace(pass *analysis.Pass, lastEmbeddedField, firstNotEmbeddedField *ast.Field) { if lastEmbeddedField != nil && firstNotEmbeddedField != nil { // check for missing space @@ -51,3 +69,18 @@ func checkMissingSpace(pass *analysis.Pass, lastEmbeddedField, firstNotEmbeddedF } } } + +func isFieldEmbedded(field *ast.Field) bool { + return len(field.Names) == 0 +} + +func reportSyncMutex(pass *analysis.Pass, se *ast.SelectorExpr) { + packageName, isIdent := se.X.(*ast.Ident) + if !isIdent { + return + } + + if packageName.Name == "sync" && (se.Sel.Name == "Mutex" || se.Sel.Name == "RWMutex") { + pass.Report(NewForbiddenEmbeddedFieldDiag(se)) + } +}