diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..fd29aee --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,144 @@ +# CLAUDE.md + +This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. + +## Overview + +positionless is a Go static analyzer that detects positional struct literal initialization and suggests converting them to named field initialization. It's built using the `golang.org/x/tools/go/analysis` framework. + +## Commands + + +```bash +# Build +go build -v ./... + +# Run tests +go test -v ./... + +# Install locally +go install . + +# Use as standalone tool +positionless ./... +positionless -fix ./... # Auto-fix issues +positionless -generated ./... # Include generated files +positionless -unexported ./... # Include structs with unexported fields +positionless -internal ./... # Auto-allow unexported in internal/ packages +positionless -ignore=Pattern ./... # Skip matching struct types +positionless -output=json ./... # JSON output (golangci-lint format, to stderr) + +# Use with go vet +go vet -vettool=$(which positionless) ./... +go vet -vettool=$(which positionless) -fix ./... +``` + + +## Architecture + + +**Single-file analyzer** (`main.go`) - The entire analyzer logic is in one file: + +1. **Entry point**: `singlechecker.Main(Analyzer)` - standard analysis framework pattern +2. **Analyzer instance**: Defines name, doc, run function, and `-generated` flag +3. **Analysis flow**: + - `run()` → iterates AST files, skips generated files by default + - `isGeneratedFile()` → checks comments for "code generated", "do not edit", "autogenerated" + - `analyzeFile()` → uses `ast.Inspect` to find all `*ast.CompositeLit` nodes + - `checkCompositeLit()` → validates struct, checks ignore patterns, reports diagnostics + - `isPositionalStruct()` → returns true if no `*ast.KeyValueExpr` elements exist + - `getStructType()` → extracts `*types.Struct` from type info, handles pointers + - `createNamedFieldsFix()` → builds replacement text with named fields + - `isInternalPackage()` → checks if file path contains `/internal/` + - `shouldAllowUnexported()` → determines if unexported fields should be processed + - `matchesIgnorePattern()` → checks struct type against ignore patterns + + + +**Detection logic** (`isPositionalStruct`): A composite literal is positional if it has elements but none are `KeyValueExpr`. + +**Fix generation** (`createNamedFieldsFix`): Reads source file once, extracts original element text by offset, reconstructs with field names. Returns both fix and unexported field status. + +**Type resolution** (`getStructType`): Uses `pass.TypesInfo.Types` to get struct type, unwraps pointers via `typ.(*types.Pointer).Elem()`. + +**Internal detection** (`isInternalPackage`): Checks if file path contains `/internal/` for auto-allowing unexported fields. + +**Ignore patterns** (`matchesIgnorePattern`): Supports glob patterns and substring matching for struct type names. + + +## Test Structure + + +Tests use `golang.org/x/tools/go/analysis/analysistest` framework: + +```bash +testdata/src/ +├── basic/ # Simple positional structs +├── nested/ # Nested struct initialization +├── pointer/ # Pointer receivers, recursive structs +├── edge/ # Empty structs, unexported fields, embedded types +├── generated/ # Generated file detection +├── internal/config # Internal package detection tests +├── unexported/ # Tests for -unexported flag +└── ignore/ # Tests for -ignore flag +``` + +Test annotations use `// want "message"` comments to assert expected diagnostics: +```go +p := Person{"John", 30} // want "positional struct literal initialization is fragile" +``` + +Run specific test packages by editing `analysistest.Run()` call in `main_test.go`. + + +## Limitations + + +- **Unexported fields**: By default, reports but can't fix structs with unexported fields. Use `-unexported` or `-internal` to enable fixes. +- **Cross-file elements**: Skips if element spans multiple files +- **Field count mismatch**: Aborts if element count exceeds struct field count +- **Generated file patterns**: Only detects specific keywords in comments - custom patterns may be missed +- **Exit code 3**: Used for "findings exist" status in CI - not a failure code +- **JSON output**: Use `-output=json` for JSON lines to stderr. Normal text output still goes to stdout. + + +## Verification + + +To verify implementation changes: + +```bash +# 1. Run full test suite +go test -v ./... + +# 2. Test on real code +positionless ./... # Check current project +positionless -fix /tmp/testdir/... # Test fix generation + +# 3. Verify generated file handling +positionless testdata/src/generated/ # Should find nothing +positionless -generated testdata/src/generated/ # Should find issues + +# 4. Test unexported field handling +positionless testdata/src/edge/ # Reports MixedExport without fix +positionless -unexported testdata/src/edge/ # Reports MixedExport WITH fix + +# 5. Test internal package detection +positionless -internal testdata/src/internal/... # Should fix unexported in internal/ + +# 6. Test ignore patterns +positionless -ignore="MixedExport" testdata/src/edge/ # Should skip MixedExport + +# 7. Test JSON output +positionless -output=json testdata/src/basic/ 2>&1 # JSON to stderr + +# 8. Check fix output manually +positionless -fix ./testdata/src/basic/ 2>&1 # Inspect applied fixes +git diff # Review changes +git checkout -- testdata/ # Reset test files +``` + + +## GitHub Action + +The project provides a GitHub Action (`action.yml`) that downloads the binary and runs analysis. Exit code 3 signals findings to fail CI pipelines. diff --git a/README.md b/README.md index 4b83d42..c80e934 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,57 @@ fieldalignment -fix ./... Most Go editors support running custom analyzers. Configure your editor to run this analyzer for real-time feedback. +### With golangci-lint v2 + +`positionless` supports [golangci-lint v2 module plugins](https://golangci-lint.run/docs/plugins/module-plugins/). + +**Step 1:** Create `.custom-gcl.yml` in your project root: + +```yaml +version: v2.1.2 +plugins: + - module: 'github.com/flaticols/positionless' + import: 'github.com/flaticols/positionless' + version: v2.0.0 +``` + +**Step 2:** Build custom golangci-lint: + +```bash +# Build custom binary with positionless (requires golangci-lint v2 installed) +golangci-lint custom +``` + +**Step 3:** Configure `.golangci.yml`: + +```yaml +version: "2" +linters: + enable: + - positionless + settings: + custom: + positionless: + type: "module" + description: Detect positional struct literals + # Pass flags via settings (optional) + settings: + generated: false + unexported: false + internal: true + ignore: "" + output: "text" +``` + +**Step 4:** Run: + +```bash +./custom-gcl run ./... +``` + +> [!NOTE] +> Module plugins are the recommended approach for golangci-lint v2. No toolchain version matching required. + ### As a GitHub Action You can use `positionless` in your GitHub workflows to automatically check for positional struct literals: @@ -236,8 +287,21 @@ The analyzer: 3. Suggests fixes that convert to named field initialization 4. Can automatically apply fixes with the `-fix` flag 5. Preserves your original values and formatting -6. Only processes exported fields (respects Go's visibility rules) -7. Skips generated files by default (use `-generated` to include them) +6. Skips generated files by default (use `-generated` to include them) + +### Flags + +| Flag | Description | +|------|-------------| +| `-fix` | Apply suggested fixes automatically | +| `-generated` | Include generated files in analysis | +| `-unexported` | Include structs with unexported fields in fixes | +| `-internal` | Auto-allow unexported fields in `internal/` packages | +| `-ignore=PATTERN` | Skip structs matching pattern (comma-separated) | +| `-output=FORMAT` | Output format: `text` (default) or `json` | + +> [!TIP] +> Use `-internal` when analyzing your own internal packages where you control all the code ## Example diff --git a/main.go b/main.go index 5181a8e..64f9962 100644 --- a/main.go +++ b/main.go @@ -1,41 +1,139 @@ package main import ( + "encoding/json" "fmt" "go/ast" "go/types" "os" + "path/filepath" "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/singlechecker" ) -var includeGenerated bool +const ( + outputText = "text" + outputJSON = "json" +) + +// Global flags - set by init() and read during analysis. +// Note: These are safe for single-threaded use with singlechecker.Main(). +// For concurrent analysis (e.g., golangci-lint), each run should be isolated. +var ( + includeGenerated bool + includeUnexported bool + detectInternal bool + ignorePatterns string + outputFormat string +) + +// JSONIssue represents a single diagnostic in golangci-lint compatible format +type JSONIssue struct { + FromLinter string `json:"FromLinter"` + Text string `json:"Text"` + Severity string `json:"Severity"` + Pos JSONPos `json:"Pos"` + Fixable bool `json:"Fixable"` +} + +type JSONPos struct { + Filename string `json:"Filename"` + Line int `json:"Line"` + Column int `json:"Column"` +} var Analyzer = &analysis.Analyzer{ Name: "positionless", - Doc: "reports positional struct literal initialization", - Run: run, + Doc: `Detects positional struct literal initialization and suggests named fields. + +DESCRIPTION + +positionless finds struct literals that use positional arguments instead of +named fields. Positional initialization is fragile because it breaks when +struct fields are reordered or new fields are added. + +Example of problematic code: + + // BAD: positional - breaks if Person fields change order + p := Person{"John", 30, "john@example.com"} + + // GOOD: named fields - safe and self-documenting + p := Person{ + Name: "John", + Age: 30, + Email: "john@example.com", + } + +USAGE + + positionless ./... # analyze all packages + positionless -fix ./... # auto-fix issues + positionless -internal ./... # include internal packages + positionless -output=json ./... 2>&1 # JSON output to stderr + +FLAGS + + -fix Apply suggested fixes automatically + -generated Include generated files (default: excluded) + -unexported Fix structs with unexported fields + -internal Auto-allow unexported in internal/ packages + -ignore=PATTERN Skip struct types matching pattern (comma-separated) + -output=FORMAT Output format: text (default) or json + +INTEGRATION + + Standalone: positionless ./... + go vet: go vet -vettool=$(which positionless) ./... + golangci-lint: Use module plugin (see README) + +EXIT CODES + + 0 No issues found + 1 Error occurred + 3 Issues found (use in CI to fail builds) + +For golangci-lint v2 integration, this analyzer exports a New() function +compatible with the module plugin system.`, + Run: run, +} + +// New returns the analyzer for golangci-lint module plugin integration. +// The conf parameter is currently unused - configuration is provided via +// Analyzer.Flags which golangci-lint populates from linter settings. +// See: https://golangci-lint.run/docs/plugins/module-plugins/ +func New(conf any) ([]*analysis.Analyzer, error) { + _ = conf // unused, configuration via Analyzer.Flags + return []*analysis.Analyzer{Analyzer}, nil } func init() { Analyzer.Flags.BoolVar(&includeGenerated, "generated", false, "include generated files in analysis") + Analyzer.Flags.BoolVar(&includeUnexported, "unexported", false, + "include structs with unexported fields in fixes") + Analyzer.Flags.BoolVar(&detectInternal, "internal", false, + "auto-allow unexported fields in internal/ packages") + Analyzer.Flags.StringVar(&ignorePatterns, "ignore", "", + "comma-separated patterns to ignore (e.g., 'ConfigTest,internal/legacy/*')") + Analyzer.Flags.StringVar(&outputFormat, "output", outputText, + "output format: 'text' (default) or 'json' (golangci-lint compatible)") } func run(pass *analysis.Pass) (any, error) { - if !includeGenerated { - for _, file := range pass.Files { - if isGeneratedFile(file) { - continue - } - analyzeFile(pass, file) - } - } else { - for _, file := range pass.Files { - analyzeFile(pass, file) + // Validate output format + if outputFormat != outputText && outputFormat != outputJSON { + fmt.Fprintf(os.Stderr, "warning: invalid output format %q, using %q\n", outputFormat, outputText) + outputFormat = outputText + } + + for _, file := range pass.Files { + if !includeGenerated && isGeneratedFile(file) { + continue } + filePath := pass.Fset.Position(file.Pos()).Filename + analyzeFile(pass, file, filePath) } return nil, nil @@ -55,20 +153,86 @@ func isGeneratedFile(file *ast.File) bool { return false } -func analyzeFile(pass *analysis.Pass, file *ast.File) { +func analyzeFile(pass *analysis.Pass, file *ast.File, filePath string) { ast.Inspect(file, func(n ast.Node) bool { if cl, ok := n.(*ast.CompositeLit); ok { - checkCompositeLit(pass, cl) + checkCompositeLit(pass, cl, filePath) } return true }) } -func checkCompositeLit(pass *analysis.Pass, cl *ast.CompositeLit) { +// isInternalPackage checks if the file path contains /internal/. +// Uses forward slashes after normalization via filepath.ToSlash, +// so works correctly on both Unix and Windows systems. +func isInternalPackage(filePath string) bool { + // Normalize path separators + normalized := filepath.ToSlash(filePath) + return strings.Contains(normalized, "/internal/") +} + +// shouldAllowUnexported returns true if unexported fields should be processed +func shouldAllowUnexported(filePath string) bool { + return includeUnexported || (detectInternal && isInternalPackage(filePath)) +} + +// matchesIgnorePattern checks if the struct type name matches any ignore pattern. +// Supports both glob patterns (e.g., "*Test") and substring matching. +func matchesIgnorePattern(typeName string) bool { + if ignorePatterns == "" || typeName == "" { + return false + } + patterns := strings.Split(ignorePatterns, ",") + for _, pattern := range patterns { + pattern = strings.TrimSpace(pattern) + if pattern == "" { + continue + } + // Try glob pattern match first + if matched, err := filepath.Match(pattern, typeName); err == nil && matched { + return true + } + // Fall back to substring match for simple patterns + if strings.Contains(typeName, pattern) { + return true + } + } + return false +} + +// getTypeName extracts the type name from a composite literal +func getTypeName(cl *ast.CompositeLit) string { + switch t := cl.Type.(type) { + case *ast.Ident: + return t.Name + case *ast.SelectorExpr: + if ident, ok := t.X.(*ast.Ident); ok { + return ident.Name + "." + t.Sel.Name + } + return t.Sel.Name + case *ast.StarExpr: + // Handle pointer types like &Person{} + if ident, ok := t.X.(*ast.Ident); ok { + return ident.Name + } + if sel, ok := t.X.(*ast.SelectorExpr); ok { + return sel.Sel.Name + } + } + return "" +} + +func checkCompositeLit(pass *analysis.Pass, cl *ast.CompositeLit, filePath string) { if !isPositionalStruct(cl) { return } + // Check ignore patterns + typeName := getTypeName(cl) + if matchesIgnorePattern(typeName) { + return + } + structType := getStructType(pass, cl) if structType == nil { return @@ -78,17 +242,45 @@ func checkCompositeLit(pass *analysis.Pass, cl *ast.CompositeLit) { return } - fix := createNamedFieldsFix(pass, cl, structType) - if fix == nil { + allowUnexported := shouldAllowUnexported(filePath) + fix, hasUnexported := createNamedFieldsFix(pass, cl, structType, allowUnexported) + + // Build diagnostic + pos := pass.Fset.Position(cl.Pos()) + message := "positional struct literal initialization is fragile" + + if hasUnexported && !allowUnexported { + message += " (cannot auto-fix: contains unexported fields)" + } + + diagnostic := analysis.Diagnostic{ + Pos: cl.Pos(), + End: cl.End(), + Message: message, + } + + if fix != nil { + diagnostic.SuggestedFixes = []analysis.SuggestedFix{*fix} + } + + // Output based on format + if outputFormat == outputJSON { + // JSON mode: output to stderr only, skip pass.Report() to avoid duplicate output + printJSONIssue(JSONIssue{ + FromLinter: "positionless", + Text: message, + Severity: "warning", + Pos: JSONPos{ + Filename: pos.Filename, + Line: pos.Line, + Column: pos.Column, + }, + Fixable: fix != nil, + }) return } - pass.Report(analysis.Diagnostic{ - Pos: cl.Pos(), - End: cl.End(), - Message: "positional struct literal initialization is fragile", - SuggestedFixes: []analysis.SuggestedFix{*fix}, - }) + pass.Report(diagnostic) } func isPositionalStruct(cl *ast.CompositeLit) bool { @@ -124,38 +316,48 @@ func getStructType(pass *analysis.Pass, cl *ast.CompositeLit) *types.Struct { return structType } +// createNamedFieldsFix generates a fix for positional struct literals. +// Returns the fix (or nil if not possible) and whether unexported fields were encountered. func createNamedFieldsFix(pass *analysis.Pass, cl *ast.CompositeLit, - structType *types.Struct) *analysis.SuggestedFix { + structType *types.Struct, allowUnexported bool) (*analysis.SuggestedFix, bool) { - var newText strings.Builder + if len(cl.Elts) == 0 { + return nil, false + } + + // Read source file once (all elements are in the same file) + firstElt := pass.Fset.Position(cl.Elts[0].Pos()) + src, err := os.ReadFile(firstElt.Filename) + if err != nil { + return nil, false + } - _ = pass.Fset.Position(cl.Lbrace) + var newText strings.Builder + hasUnexported := false newText.WriteString("{\n") for i, elt := range cl.Elts { if i >= structType.NumFields() { - return nil + return nil, false } field := structType.Field(i) if !field.Exported() { - return nil + hasUnexported = true + if !allowUnexported { + // Can't fix, but signal that unexported fields exist + return nil, true + } } eltStart := pass.Fset.Position(elt.Pos()) eltEnd := pass.Fset.Position(elt.End()) - if eltStart.Filename != eltEnd.Filename { - return nil - } - - src, err := os.ReadFile(eltStart.Filename) - if err != nil { - return nil + if eltStart.Filename != firstElt.Filename { + return nil, hasUnexported } eltText := string(src[eltStart.Offset:eltEnd.Offset]) - newText.WriteString(fmt.Sprintf("\t%s: %s,\n", field.Name(), eltText)) } @@ -170,9 +372,23 @@ func createNamedFieldsFix(pass *analysis.Pass, cl *ast.CompositeLit, NewText: []byte(newText.String()), }, }, - } + }, hasUnexported } func main() { singlechecker.Main(Analyzer) } + +// outputJSON prints all collected issues in golangci-lint compatible format. +// This is called during analysis when jsonOutput is enabled, issues are +// still reported normally but also collected for JSON output. +// Note: The JSON is output incrementally as issues are found since +// singlechecker.Main() calls os.Exit() and deferred functions don't run. +func printJSONIssue(issue JSONIssue) { + data, err := json.Marshal(issue) + if err != nil { + fmt.Fprintf(os.Stderr, "positionless: failed to marshal JSON: %v\n", err) + return + } + fmt.Fprintln(os.Stderr, string(data)) +} diff --git a/main_test.go b/main_test.go index 114b569..c438ded 100644 --- a/main_test.go +++ b/main_test.go @@ -6,15 +6,65 @@ import ( "golang.org/x/tools/go/analysis/analysistest" ) +// Test package names +const ( + pkgBasic = "basic" + pkgNested = "nested" + pkgPointer = "pointer" + pkgEdge = "edge" + pkgGenerated = "generated" + pkgUnexported = "unexported" + pkgInternal = "internal/config" + pkgIgnore = "ignore" + pkgJSONTest = "jsontest" +) + +// Test ignore patterns +const testIgnorePatterns = "MixedExport,Anonymous" + func TestPositionless(t *testing.T) { testdata := analysistest.TestData() - analysistest.Run(t, testdata, Analyzer, "basic", "nested", "pointer", "edge") + analysistest.Run(t, testdata, Analyzer, pkgBasic, pkgNested, pkgPointer, pkgEdge) } func TestPositionlessWithGenerated(t *testing.T) { includeGenerated = true defer func() { includeGenerated = false }() - + + testdata := analysistest.TestData() + analysistest.Run(t, testdata, Analyzer, pkgGenerated) +} + +func TestPositionlessWithUnexported(t *testing.T) { + includeUnexported = true + defer func() { includeUnexported = false }() + + testdata := analysistest.TestData() + analysistest.Run(t, testdata, Analyzer, pkgUnexported) +} + +func TestPositionlessWithInternal(t *testing.T) { + detectInternal = true + defer func() { detectInternal = false }() + + testdata := analysistest.TestData() + analysistest.Run(t, testdata, Analyzer, pkgInternal) +} + +func TestPositionlessWithIgnore(t *testing.T) { + ignorePatterns = testIgnorePatterns + defer func() { ignorePatterns = "" }() + + testdata := analysistest.TestData() + analysistest.Run(t, testdata, Analyzer, pkgIgnore) +} + +func TestPositionlessJSONOutput(t *testing.T) { + outputFormat = outputJSON + defer func() { outputFormat = outputText }() + testdata := analysistest.TestData() - analysistest.Run(t, testdata, Analyzer, "generated") + // JSON mode outputs to stderr and skips pass.Report() + // Use jsontest package which has no // want annotations + analysistest.Run(t, testdata, Analyzer, pkgJSONTest) } \ No newline at end of file diff --git a/testdata/src/edge/edge.go b/testdata/src/edge/edge.go index a862671..6aa89b1 100644 --- a/testdata/src/edge/edge.go +++ b/testdata/src/edge/edge.go @@ -31,8 +31,8 @@ func main() { // Should trigger: single field struct o := OneField{42} // want "positional struct literal initialization is fragile" - // Should NOT trigger: struct with unexported fields (can't fix) - m := MixedExport{"public", 123, "another"} + // Should trigger but without fix: struct with unexported fields + m := MixedExport{"public", 123, "another"} // want "positional struct literal initialization is fragile \\(cannot auto-fix: contains unexported fields\\)" // Edge case: more values than fields (should NOT trigger - unsafe) // This would be a compile error anyway @@ -41,8 +41,9 @@ func main() { // Should trigger: struct with embedded type emb := Embedded{nil, 100} // want "positional struct literal initialization is fragile" - // Should NOT trigger: struct with anonymous fields (can't reference by name) - anon := Anonymous{"text", 42} + // Should trigger but without fix: anonymous (embedded) fields like `string` and `int` + // use the type name as the field name, making them unexported + anon := Anonymous{"text", 42} // want "positional struct literal initialization is fragile \\(cannot auto-fix: contains unexported fields\\)" _ = e _ = o diff --git a/testdata/src/ignore/ignore.go b/testdata/src/ignore/ignore.go new file mode 100644 index 0000000..b7a4feb --- /dev/null +++ b/testdata/src/ignore/ignore.go @@ -0,0 +1,32 @@ +package ignore + +// MixedExport - should be ignored when pattern matches +type MixedExport struct { + Public string + private int +} + +// Anonymous - should be ignored when pattern matches +type Anonymous struct { + string + int +} + +// Person - should NOT be ignored +type Person struct { + Name string + Age int +} + +func main() { + // MixedExport and Anonymous should be ignored due to -ignore flag + m := MixedExport{"public", 123} + anon := Anonymous{"text", 42} + + // Person should still trigger + p := Person{"John", 30} // want "positional struct literal initialization is fragile" + + _ = m + _ = anon + _ = p +} diff --git a/testdata/src/internal/config/config.go b/testdata/src/internal/config/config.go new file mode 100644 index 0000000..c6ddf84 --- /dev/null +++ b/testdata/src/internal/config/config.go @@ -0,0 +1,23 @@ +package config + +// Config has unexported fields - should be fixed with -internal flag +type Config struct { + host string + port int + Timeout int +} + +// Service has mixed fields +type Service struct { + Name string + enabled bool +} + +func main() { + // With -internal flag, these should be fixable + cfg := Config{"localhost", 8080, 30} // want "positional struct literal initialization is fragile" + svc := Service{"api", true} // want "positional struct literal initialization is fragile" + + _ = cfg + _ = svc +} diff --git a/testdata/src/jsontest/jsontest.go b/testdata/src/jsontest/jsontest.go new file mode 100644 index 0000000..c68598e --- /dev/null +++ b/testdata/src/jsontest/jsontest.go @@ -0,0 +1,13 @@ +package jsontest + +// Simple struct for JSON output testing. +type Person struct { + Name string + Age int +} + +func example() { + // This triggers in JSON mode - no diagnostic expected since JSON skips pass.Report + p := Person{"John", 30} + _ = p +} diff --git a/testdata/src/unexported/unexported.go b/testdata/src/unexported/unexported.go new file mode 100644 index 0000000..93b9208 --- /dev/null +++ b/testdata/src/unexported/unexported.go @@ -0,0 +1,23 @@ +package unexported + +// MixedExport has both exported and unexported fields +type MixedExport struct { + Public string + private int + Another string +} + +// AllUnexported has only unexported fields +type AllUnexported struct { + name string + age int +} + +func main() { + // With -unexported flag, these should be fixable + m := MixedExport{"public", 123, "another"} // want "positional struct literal initialization is fragile" + a := AllUnexported{"john", 30} // want "positional struct literal initialization is fragile" + + _ = m + _ = a +}