Detect struct comparisons that bypass +noKrtEquals markers

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`

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

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")
+}

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
+}

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)
+}

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
+}

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)
+}

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
 }
-