diff --git a/README.md b/README.md index ccb1592..89d12d4 100644 --- a/README.md +++ b/README.md @@ -89,6 +89,37 @@ func (m MyStruct) Equals(other MyStruct) bool { } ``` +### Struct Field Comparisons with `==` + +The analyzer detects when struct-typed fields are compared using `==` instead of delegating to their `Equals()` method. This is important because using `==` for struct comparison ignores any `+noKrtEquals` markers on the nested struct's fields, potentially comparing fields that should be excluded. + +```go +type InnerType struct { + Value int + // +noKrtEquals reason: internal cache + Cache map[string]string +} + +type OuterType struct { + Name string + Inner InnerType +} + +// Bad: Using == compares ALL fields of InnerType, including Cache +func (o OuterType) Equals(other OuterType) bool { + return o.Name == other.Name && o.Inner == other.Inner // Flagged! +} + +// Good: Delegate to InnerType.Equals() which respects +noKrtEquals markers +func (i InnerType) Equals(other InnerType) bool { + return i.Value == other.Value // Cache is intentionally not compared +} + +func (o OuterType) Equals(other OuterType) bool { + return o.Name == other.Name && o.Inner.Equals(other.Inner) +} +``` + ## Markers ### `+noKrtEquals` diff --git a/pkg/analyzer/analyzer.go b/pkg/analyzer/analyzer.go index a68e04e..a66bbd5 100644 --- a/pkg/analyzer/analyzer.go +++ b/pkg/analyzer/analyzer.go @@ -99,6 +99,9 @@ func (a *analyzerImpl) Run(pass *analysis.Pass) (any, error) { usedFields := collectUsedFieldsInEquals(fd.Body, recvIdent, paramNames) + // Check for struct comparisons that may bypass +noKrtEquals markers + checkStructComparisons(pass, fd, recvIdent, paramNames, structs) + sinfo, ok := structs[recvTypeName] if !ok { return @@ -169,6 +172,101 @@ func collectStructs(ins *inspector.Inspector) map[string]*structInfo { return structs } +// checkStructComparisons detects when struct-typed fields are compared using == or != +// instead of delegating to an Equals() method. This is problematic because Go's default +// struct comparison compares ALL fields, potentially comparing fields that have +// +noKrtEquals or +krtEqualsTodo markers that should be excluded. +func checkStructComparisons(pass *analysis.Pass, fd *ast.FuncDecl, recvIdent string, paramIdents []string, structs map[string]*structInfo) { + if pass.TypesInfo == nil { + return + } + + paramSet := make(map[string]struct{}, len(paramIdents)) + for _, p := range paramIdents { + paramSet[p] = struct{}{} + } + + ast.Inspect(fd.Body, func(n ast.Node) bool { + binExpr, ok := n.(*ast.BinaryExpr) + if !ok { + return true + } + + // Check for == or != comparisons + if binExpr.Op != token.EQL && binExpr.Op != token.NEQ { + return true + } + + // Check if left side is a field access from receiver or param + leftSel, leftOk := binExpr.X.(*ast.SelectorExpr) + if !leftOk { + return true + } + + leftIdent, leftIdentOk := leftSel.X.(*ast.Ident) + if !leftIdentOk { + return true + } + + // Check if it's from receiver or parameter + isRecv := leftIdent.Name == recvIdent + _, isParam := paramSet[leftIdent.Name] + if !isRecv && !isParam { + return true + } + + // Get the type of the field being compared + leftType := pass.TypesInfo.TypeOf(leftSel) + if leftType == nil { + return true + } + + // Check if it's a named struct type + namedType, ok := leftType.(*types.Named) + if !ok { + // Try unwrapping pointer + if ptrType, isPtrType := leftType.(*types.Pointer); isPtrType { + namedType, ok = ptrType.Elem().(*types.Named) + } + if !ok { + return true + } + } + + _, ok = namedType.Underlying().(*types.Struct) + if !ok { + return true + } + + // Check if the struct type has any ignored fields + typeName := namedType.Obj().Name() + structInfo, ok := structs[typeName] + if !ok { + return true + } + + // Check if any fields have +noKrtEquals markers + hasIgnoredFields := false + for _, field := range structInfo.fields { + if field.ignore || field.todo { + hasIgnoredFields = true + break + } + } + + if hasIgnoredFields { + opStr := "==" + if binExpr.Op == token.NEQ { + opStr = "!=" + } + pass.Reportf(binExpr.Pos(), "field %q of struct type %q is compared using %s which ignores +noKrtEquals/+krtEqualsTodo markers; use .Equals() method instead", + leftSel.Sel.Name, typeName, opStr) + } + + return true + }) +} + func checkReflectDeepEqual(pass *analysis.Pass, fd *ast.FuncDecl) { if pass.TypesInfo == nil { return diff --git a/pkg/analyzer/analyzer_test.go b/pkg/analyzer/analyzer_test.go index c4d3bfb..f29a1c8 100644 --- a/pkg/analyzer/analyzer_test.go +++ b/pkg/analyzer/analyzer_test.go @@ -53,3 +53,21 @@ func TestUnexportedCheckDisabled(t *testing.T) { a := NewAnalyzer(&Config{CheckUnexported: false}) analysistest.Run(t, testdata, a, "unexportedoff") } + +func TestNestedFields(t *testing.T) { + testdata := analysistest.TestData() + a := NewAnalyzer(&Config{}) + analysistest.Run(t, testdata, a, "nested") +} + +func TestNestedFieldsWithUnexported(t *testing.T) { + testdata := analysistest.TestData() + a := NewAnalyzer(&Config{CheckUnexported: true}) + analysistest.Run(t, testdata, a, "nestedunexported") +} + +func TestStructComparisons(t *testing.T) { + testdata := analysistest.TestData() + a := NewAnalyzer(&Config{}) + analysistest.Run(t, testdata, a, "problem") +} diff --git a/pkg/analyzer/testdata/src/nested/nested.go b/pkg/analyzer/testdata/src/nested/nested.go new file mode 100644 index 0000000..7ca1931 --- /dev/null +++ b/pkg/analyzer/testdata/src/nested/nested.go @@ -0,0 +1,52 @@ +package nested + +import ( + "time" +) + +// Inner is a nested struct that should have its own Equals method +type Inner struct { + Value1 int + Value2 string +} + +// NestedStruct has fields of a custom struct type. +// The linter should check that these nested fields are compared properly. +type NestedStruct struct { + Name string + Config Inner // want "field \"Config\" in type \"NestedStruct\" is not used in Equals" + // +noKrtEquals reason: internal bookkeeping + Cache map[string]string +} + +// Missing comparison - should fail because Config is not compared +func (n NestedStruct) Equals(other NestedStruct) bool { + return n.Name == other.Name +} + +// ListenerPolicyIR mimics the structure from the issue +type ListenerPolicyIR struct { + ct time.Time + defaultPolicy listenerPolicy + perPortPolicy map[uint32]listenerPolicy + // +noKrtEquals reason: When set to true, suppress source reporting metadata from + // ListenerPolicy specific fields that are irrelevant to the (now deprecated) HTTPListenerPolicy. Remove when HTTPListenerPolicy is removed. + NoOrigin bool +} + +type listenerPolicy struct { + proxyProtocol *int + perConnectionBufferLimitBytes *uint32 + http *HttpListenerPolicyIr +} + +type HttpListenerPolicyIr struct { + Value string +} + +// Equals for ListenerPolicyIR that doesn't compare unexported fields +// The linter should NOT flag this with default config (CheckUnexported: false) +func (l ListenerPolicyIR) Equals(other ListenerPolicyIR) bool { + // NoOrigin is marked with +noKrtEquals so it shouldn't be required + return true +} diff --git a/pkg/analyzer/testdata/src/nestedmissing/nested.go b/pkg/analyzer/testdata/src/nestedmissing/nested.go new file mode 100644 index 0000000..4cbefd3 --- /dev/null +++ b/pkg/analyzer/testdata/src/nestedmissing/nested.go @@ -0,0 +1,40 @@ +package nestedmissing + +// NestedType is a struct without an Equals method +type NestedType struct { + Value1 int + Value2 string +} + +// ParentStruct has a field of NestedType +// If ParentStruct.Equals uses == comparison on NestedType, +// it will use Go's default struct comparison +type ParentStruct struct { + Name string + Nested NestedType +} + +// This Equals uses default Go struct comparison for Nested field +// The linter should ideally warn that NestedType doesn't have its own Equals +func (p ParentStruct) Equals(other ParentStruct) bool { + return p.Name == other.Name && p.Nested == other.Nested +} + +// ParentWithDelegation properly delegates to nested Equals +type ParentWithDelegation struct { + Name string + Nested NestedWithEquals +} + +type NestedWithEquals struct { + Value1 int + Value2 string +} + +func (n NestedWithEquals) Equals(other NestedWithEquals) bool { + return n.Value1 == other.Value1 && n.Value2 == other.Value2 +} + +func (p ParentWithDelegation) Equals(other ParentWithDelegation) bool { + return p.Name == other.Name && p.Nested.Equals(other.Nested) +} diff --git a/pkg/analyzer/testdata/src/nestedunexported/nested.go b/pkg/analyzer/testdata/src/nestedunexported/nested.go new file mode 100644 index 0000000..624e676 --- /dev/null +++ b/pkg/analyzer/testdata/src/nestedunexported/nested.go @@ -0,0 +1,33 @@ +package nestedunexported + +import ( + "time" +) + +// ListenerPolicyIR mimics the structure from the issue +// When CheckUnexported: true, unexported fields should be flagged +type ListenerPolicyIR struct { + ct time.Time // want "field \"ct\" in type \"ListenerPolicyIR\" is not used in Equals" + defaultPolicy listenerPolicy // want "field \"defaultPolicy\" in type \"ListenerPolicyIR\" is not used in Equals" + perPortPolicy map[uint32]listenerPolicy // want "field \"perPortPolicy\" in type \"ListenerPolicyIR\" is not used in Equals" + // +noKrtEquals reason: When set to true, suppress source reporting metadata from + // ListenerPolicy specific fields that are irrelevant to the (now deprecated) HTTPListenerPolicy. Remove when HTTPListenerPolicy is removed. + NoOrigin bool +} + +type listenerPolicy struct { + proxyProtocol *int + perConnectionBufferLimitBytes *uint32 + http *HttpListenerPolicyIr +} + +type HttpListenerPolicyIr struct { + Value string +} + +// Equals for ListenerPolicyIR that doesn't compare unexported fields +// With CheckUnexported: true, this should flag the unexported fields +func (l ListenerPolicyIR) Equals(other ListenerPolicyIR) bool { + // NoOrigin is marked with +noKrtEquals so it shouldn't be required + return true +} diff --git a/pkg/analyzer/testdata/src/problem/problem.go b/pkg/analyzer/testdata/src/problem/problem.go new file mode 100644 index 0000000..abd7f51 --- /dev/null +++ b/pkg/analyzer/testdata/src/problem/problem.go @@ -0,0 +1,71 @@ +package problem + +import "time" + +// InnerType has fields with markers +type InnerType struct { + Used int + // +noKrtEquals reason: should not be compared + Ignored string +} + +// OuterType has a field of InnerType +type OuterType struct { + Name string + Inner InnerType +} + +// This Equals uses == which ignores the +noKrtEquals marker in InnerType +// This is the problem: the linter sees that Inner is used, +// but doesn't check that InnerType.Ignored has a +noKrtEquals marker +func (o OuterType) Equals(other OuterType) bool { + return o.Name == other.Name && o.Inner == other.Inner // want "field \"Inner\" of struct type \"InnerType\" is compared using == which ignores \\+noKrtEquals/\\+krtEqualsTodo markers" +} + +// ListenerPolicyIR mimics the exact structure from the GitHub issue +type ListenerPolicyIR struct { + ct time.Time + defaultPolicy listenerPolicy + perPortPolicy map[uint32]listenerPolicy + // +noKrtEquals reason: When set to true, suppress source reporting metadata + NoOrigin bool +} + +type listenerPolicy struct { + proxyProtocol *int + perConnectionBufferLimitBytes *uint32 + // +noKrtEquals reason: should not be compared + http *HttpListenerPolicyIr +} + +type HttpListenerPolicyIr struct { + Value string +} + +// This Equals incorrectly uses == for defaultPolicy comparison +// which bypasses the +noKrtEquals marker on listenerPolicy.http +func (l ListenerPolicyIR) Equals(other ListenerPolicyIR) bool { + return l.ct == other.ct && l.defaultPolicy == other.defaultPolicy // want "field \"defaultPolicy\" of struct type \"listenerPolicy\" is compared using == which ignores \\+noKrtEquals/\\+krtEqualsTodo markers" +} + +// CorrectComparison shows the correct way to compare struct fields +type CorrectComparison struct { + Name string + Config InnerTypeWithEquals +} + +type InnerTypeWithEquals struct { + Value int + // +noKrtEquals reason: should not be compared + Internal string +} + +func (i InnerTypeWithEquals) Equals(other InnerTypeWithEquals) bool { + return i.Value == other.Value + // Internal is intentionally not compared due to +noKrtEquals marker +} + +// This correctly uses .Equals() to delegate comparison +func (c CorrectComparison) Equals(other CorrectComparison) bool { + return c.Name == other.Name && c.Config.Equals(other.Config) +} diff --git a/pkg/analyzer/testdata/src/unexportedon/unexported.go b/pkg/analyzer/testdata/src/unexportedon/unexported.go index 51dde9e..f2f4fae 100644 --- a/pkg/analyzer/testdata/src/unexportedon/unexported.go +++ b/pkg/analyzer/testdata/src/unexportedon/unexported.go @@ -52,4 +52,3 @@ func (a *AllFieldsUsed) Equals(other *AllFieldsUsed) bool { } return a.name == other.name && a.value == other.value && a.extra == other.extra } -