diff --git a/.golangci.yml b/.golangci.yml index 7d69de7ea2..21ff49c7ab 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -33,11 +33,11 @@ linters: - '-QF1001' # Apply De Morgan's law - '-QF1003' # Tagged switch instead of if/else blocks - '-QF1006' # Lift break into loop condition - - '-QF1008' # Remove embeded fields + - '-QF1008' # Remove embedded fields - '-ST1005' # Errors shouldn't end in punctuation or start with capitals - '-ST1012' # Errors should be named errFoo or ErrFoo - '-ST1003' # Variable naming rules (e.g. underscores in packages, and abbreviations) - - '-ST1016' # Methods on the same type should ahve the same reciever name + - '-ST1016' # Methods on the same type should have the same receiver name exclusions: generated: lax presets: diff --git a/linters/jsonneverempty/json_never_empty.go b/linters/jsonneverempty/json_never_empty.go new file mode 100644 index 0000000000..7bbbb96e1e --- /dev/null +++ b/linters/jsonneverempty/json_never_empty.go @@ -0,0 +1,95 @@ +// Copyright 2025, Offchain Labs, Inc. +// For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md +// +// Based on https://github.com/andydotdev/omitlint + +package jsonneverempty + +import ( + "fmt" + "go/ast" + "go/types" + "reflect" + "strings" + + "golang.org/x/tools/go/analysis" + "golang.org/x/tools/go/analysis/passes/inspect" + "golang.org/x/tools/go/ast/inspector" +) + +var Analyzer = &analysis.Analyzer{ + Name: "jsonneverempty", + Doc: "check if the `omitempty` tag is used for fields that cannot be empty", + Run: run, + Requires: []*analysis.Analyzer{inspect.Analyzer}, +} + +func run(pass *analysis.Pass) (interface{}, error) { + pkgAst, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) + if !ok { + return nil, fmt.Errorf("`inspect.Analyzer` hasn't been run or didn't return AST for the package `%v`", pass.Pkg.Name()) + } + pkgAst.Preorder([]ast.Node{(*ast.StructType)(nil)}, func(node ast.Node) { + structType, isStructType := node.(*ast.StructType) + if !isStructType { + panic("node type filtering doesn't work correctly") + } + validateStruct(pass, structType) + }) + return nil, nil +} + +func validateStruct(pass *analysis.Pass, structType *ast.StructType) { + info, infoAvailable := pass.TypesInfo.Types[structType] + if !infoAvailable { + fmt.Printf("[WARNING] type info not available for a struct") + return + } + + typeInfo, isStructInfo := info.Type.(*types.Struct) + if !isStructInfo { + fmt.Printf("[WARNING] type info not a struct") + return + } + + for fieldIndex := range typeInfo.NumFields() { + field := typeInfo.Field(fieldIndex) + if !field.Exported() { + continue // ignore unexported fields + } + if !taggedWithOmitempty(typeInfo.Tag(fieldIndex)) { + continue // ignore fields not tagged with "omitempty" + } + if !typeCanBeEmpty(field) { + pass.Report(analysis.Diagnostic{ + Pos: field.Pos(), + Message: fmt.Sprintf("field '%v' is marked 'omitempty', but it can never be empty; consider making it a pointer", field.Name()), + }) + } + } +} + +func taggedWithOmitempty(rawTag string) bool { + tag := reflect.StructTag(rawTag) + if jsonTag, isJsonTagged := tag.Lookup("json"); isJsonTagged { + return strings.Contains(jsonTag, "omitempty") + } + return false +} + +func typeCanBeEmpty(field *types.Var) bool { + switch typ := field.Type().Underlying().(type) { + case *types.Basic, + *types.Slice, + *types.Pointer, + *types.Map, + *types.Chan, + *types.Signature, + *types.Interface: + return true + case *types.Array: + return typ.Len() == 0 + default: + return false + } +} diff --git a/linters/jsonneverempty/json_never_empty_test.go b/linters/jsonneverempty/json_never_empty_test.go new file mode 100644 index 0000000000..9e199e4125 --- /dev/null +++ b/linters/jsonneverempty/json_never_empty_test.go @@ -0,0 +1,36 @@ +// Copyright 2025, Offchain Labs, Inc. +// For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md +// +// Based on https://github.com/andydotdev/omitlint + +package jsonneverempty + +import ( + "bytes" + "os/exec" + "strings" + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +const aPackagePath = "github.com/offchainlabs/nitro/linters/testdata/src/jsonneverempty/a" + +func TestOmitemptyTagValidity(t *testing.T) { + analysistest.Run(t, getModuleRoot(t), Analyzer, aPackagePath) +} + +func getModuleRoot(t *testing.T) string { + t.Helper() + + var out bytes.Buffer + cmd := exec.Command("go", "list", "-m", "-f", "{{.Dir}}") + cmd.Stdout = &out + + err := cmd.Run() + if err != nil { + t.Fatalf("Failed to get module root directoryy: %v", err) + } + parts := strings.Split(out.String(), "\n") + return strings.TrimSpace(parts[0]) +} diff --git a/linters/linters.go b/linters/linters.go index 3caf42aca3..6c8859fe91 100644 --- a/linters/linters.go +++ b/linters/linters.go @@ -6,6 +6,7 @@ package main import ( "golang.org/x/tools/go/analysis/multichecker" + "github.com/offchainlabs/nitro/linters/jsonneverempty" "github.com/offchainlabs/nitro/linters/koanf" "github.com/offchainlabs/nitro/linters/namedfieldsinit" "github.com/offchainlabs/nitro/linters/pointercheck" @@ -20,5 +21,6 @@ func main() { pointercheck.Analyzer, rightshift.Analyzer, structinit.Analyzer, + jsonneverempty.Analyzer, ) } diff --git a/linters/testdata/src/jsonneverempty/a/a.go b/linters/testdata/src/jsonneverempty/a/a.go new file mode 100644 index 0000000000..2118c97d40 --- /dev/null +++ b/linters/testdata/src/jsonneverempty/a/a.go @@ -0,0 +1,59 @@ +// Copyright 2025, Offchain Labs, Inc. +// For license information, see https://github.com/OffchainLabs/nitro/blob/master/LICENSE.md +// +// Based on https://github.com/andydotdev/omitlint + +package a + +import ( + "github.com/offchainlabs/nitro/linters/testdata/src/jsonneverempty/b" +) + +type ( + aliasBasic = int + underlyingBasic int + aliasUnderlyingBasic = underlyingBasic + + aliasStruct = struct{} + structType struct{} + aliasUnderlyingStruct = structType + + aliasExternalStruct = b.StructType + underlyingExternalStruct b.StructType + aliasUnderlyingExternalStruct = underlyingExternalStruct +) + +type Struct struct { + // Basic types + Bool bool `json:"bool,omitempty"` + Int int `json:"int,omitempty"` + Float32 float32 `json:"float32,omitempty"` + String string `json:"string,omitempty"` + + // Other types that can be empty + Slice []string `json:"slice,omitempty"` + Pointer *string `json:"pointer,omitempty"` + Map map[any]structType `json:"map,omitempty"` + Channel chan structType `json:"channel,omitempty"` + Func func() `json:"func,omitempty"` + Interface interface{} `json:"interface,omitempty"` + EmptyArray [0]structType `json:"empty-array,omitempty"` + + // Aliases of types that can be empty + AliasBasic aliasBasic `json:"aliasbasic,omitempty"` + UnderlyingBasic underlyingBasic `json:"underlyingbasic,omitempty"` + AliasUnderlyingBasic aliasUnderlyingBasic `json:"aliasunderlyingbasic,omitempty"` + + // Types that can never be empty + Array [2]bool `json:"array,omitempty"` // want `field 'Array' is marked 'omitempty', but it can never be empty; consider making it a pointer` + Struct structType `json:"struct,omitempty"` // want `field 'Struct' is marked 'omitempty', but it can never be empty; consider making it a pointer` + AliasStruct aliasStruct `json:"aliasstruct,omitempty"` // want `field 'AliasStruct' is marked 'omitempty', but it can never be empty; consider making it a pointer` + AliasUnderlyingStruct aliasUnderlyingStruct `json:"aliasunderlyingstruct,omitempty"` // want `field 'AliasUnderlyingStruct' is marked 'omitempty', but it can never be empty; consider making it a pointer` + ExternalStruct b.StructType `json:"externalstruct,omitempty"` // want `field 'ExternalStruct' is marked 'omitempty', but it can never be empty; consider making it a pointer` + AliasExternalStruct aliasExternalStruct `json:"aliasexternalstruct,omitempty"` // want `field 'AliasExternalStruct' is marked 'omitempty', but it can never be empty; consider making it a pointer` + UnderlyingExternalStruct underlyingExternalStruct `json:"underlyingexternalstruct,omitempty"` // want `field 'UnderlyingExternalStruct' is marked 'omitempty', but it can never be empty; consider making it a pointer` + AliasUnderlyingExternalStruct aliasUnderlyingExternalStruct `json:"aliasunderlyingexternalstruct,omitempty"` // want `field 'AliasUnderlyingExternalStruct' is marked 'omitempty', but it can never be empty; consider making it a pointer` + + // We ignore unexported fields, even if they have the incorrect `omitempty` tag + unexported structType `json:"unexported,omitempty"` +} diff --git a/linters/testdata/src/jsonneverempty/b/b.go b/linters/testdata/src/jsonneverempty/b/b.go new file mode 100644 index 0000000000..dc321df173 --- /dev/null +++ b/linters/testdata/src/jsonneverempty/b/b.go @@ -0,0 +1,3 @@ +package b + +type StructType struct{}