Skip to content

Commit ac3cee1

Browse files
committed
Fix issue where unexported fields where skipped
By default, the linter only checks exported (uppercase) fields in Equals() methods. This leaves structs with private fields completely unchecked. Add a new checkUnexported config option (default: false) that enables linting of unexported fields as well. This is useful for internal types like policy IR structs that use lowercase field names." Signed-off-by: timflannagan <timflannagan@gmail.com>
1 parent 6cf4fa0 commit ac3cee1

5 files changed

Lines changed: 123 additions & 4 deletions

File tree

README.md

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ linters-settings:
3939
type: "module"
4040
description: "Checks Equals() implementations for KRT-style semantic equality issues"
4141
settings:
42-
deepEqual: true # Enable reflect.DeepEqual detection (optional)
42+
deepEqual: true # Enable reflect.DeepEqual detection (optional)
43+
checkUnexported: true # Check unexported (lowercase) fields (optional)
4344
```
4445
4546
### Programmatic Usage
@@ -52,15 +53,18 @@ a := analyzer.Analyzer
5253

5354
// Or create a custom configured analyzer
5455
a := analyzer.NewAnalyzer(&analyzer.Config{
55-
DeepEqual: true,
56+
DeepEqual: true,
57+
CheckUnexported: true,
5658
})
5759
```
5860

5961
## What It Checks
6062

6163
### Missing Field Comparisons
6264

63-
The analyzer ensures that all exported fields of a struct are used in its `Equals()` method. If a field is not compared, it could lead to incorrect equality results.
65+
By default, the analyzer ensures that all exported fields of a struct are used in its `Equals()` method. If a field is not compared, it could lead to incorrect equality results.
66+
67+
When `checkUnexported: true` is enabled, unexported (lowercase) fields are also checked. This is useful for structs with private fields that should still participate in equality comparisons.
6468

6569
```go
6670
type MyStruct struct {
@@ -116,6 +120,7 @@ type MyStruct struct {
116120
| Option | Type | Default | Description |
117121
|--------|------|---------|-------------|
118122
| `deepEqual` | bool | `false` | Enable detection of `reflect.DeepEqual` usage in `Equals()` methods |
123+
| `checkUnexported` | bool | `false` | Check unexported (lowercase) fields in addition to exported fields |
119124

120125
## Project Structure
121126

pkg/analyzer/analyzer.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ type Config struct {
1818
// DeepEqual toggles the rule that flags usage of reflect.DeepEqual inside Equals methods.
1919
// This check is disabled by default for incremental rollout.
2020
DeepEqual bool `json:"deepEqual"`
21+
// CheckUnexported toggles whether unexported (lowercase) fields are checked.
22+
// By default, only exported fields are validated. Enable this to also check
23+
// that unexported fields are used in Equals methods.
24+
CheckUnexported bool `json:"checkUnexported"`
2125
}
2226

2327
// Analyzer is the default analyzer instance with all checks disabled.
@@ -100,7 +104,11 @@ func (a *analyzerImpl) Run(pass *analysis.Pass) (any, error) {
100104
return
101105
}
102106
for _, f := range sinfo.fields {
103-
if !f.exported || f.ignore || f.todo {
107+
if f.ignore || f.todo {
108+
continue
109+
}
110+
// Skip unexported fields unless checkUnexported is enabled
111+
if !f.exported && !a.cfg.CheckUnexported {
104112
continue
105113
}
106114
if !usedFields[f.name] {

pkg/analyzer/analyzer_test.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,3 +41,15 @@ func TestMethodArgs(t *testing.T) {
4141
a := NewAnalyzer(&Config{})
4242
analysistest.Run(t, testdata, a, "methodargs")
4343
}
44+
45+
func TestUnexportedCheckEnabled(t *testing.T) {
46+
testdata := analysistest.TestData()
47+
a := NewAnalyzer(&Config{CheckUnexported: true})
48+
analysistest.Run(t, testdata, a, "unexportedon")
49+
}
50+
51+
func TestUnexportedCheckDisabled(t *testing.T) {
52+
testdata := analysistest.TestData()
53+
a := NewAnalyzer(&Config{CheckUnexported: false})
54+
analysistest.Run(t, testdata, a, "unexportedoff")
55+
}
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
package unexportedoff
2+
3+
// UnexportedFields has only unexported fields.
4+
// When checkUnexported is disabled (default), no errors should be reported.
5+
type UnexportedFields struct {
6+
name string
7+
value int
8+
missing bool
9+
}
10+
11+
func (u *UnexportedFields) Equals(other *UnexportedFields) bool {
12+
if u == nil && other == nil {
13+
return true
14+
}
15+
if u == nil || other == nil {
16+
return false
17+
}
18+
// Only comparing name and value, missing is not used.
19+
// But since checkUnexported is off, no error should be reported.
20+
return u.name == other.name && u.value == other.value
21+
}
22+
23+
// MixedFields has both exported and unexported fields.
24+
type MixedFields struct {
25+
Name string
26+
internal int
27+
Missing string // want "field \"Missing\" in type \"MixedFields\" is not used in Equals"
28+
}
29+
30+
func (m *MixedFields) Equals(other *MixedFields) bool {
31+
if m == nil && other == nil {
32+
return true
33+
}
34+
if m == nil || other == nil {
35+
return false
36+
}
37+
// Only exported Name is checked when checkUnexported is off
38+
return m.Name == other.Name && m.internal == other.internal
39+
}
Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
package unexportedon
2+
3+
// UnexportedFields has only unexported fields.
4+
// When checkUnexported is enabled, missing unexported fields should be flagged.
5+
type UnexportedFields struct {
6+
name string
7+
value int
8+
missing bool // want "field \"missing\" in type \"UnexportedFields\" is not used in Equals"
9+
}
10+
11+
func (u *UnexportedFields) Equals(other *UnexportedFields) bool {
12+
if u == nil && other == nil {
13+
return true
14+
}
15+
if u == nil || other == nil {
16+
return false
17+
}
18+
return u.name == other.name && u.value == other.value
19+
}
20+
21+
// MixedFields has both exported and unexported fields.
22+
type MixedFields struct {
23+
Name string
24+
internal int
25+
missingInternal int // want "field \"missingInternal\" in type \"MixedFields\" is not used in Equals"
26+
Missing string // want "field \"Missing\" in type \"MixedFields\" is not used in Equals"
27+
}
28+
29+
func (m *MixedFields) Equals(other *MixedFields) bool {
30+
if m == nil && other == nil {
31+
return true
32+
}
33+
if m == nil || other == nil {
34+
return false
35+
}
36+
return m.Name == other.Name && m.internal == other.internal
37+
}
38+
39+
// AllFieldsUsed should pass with no errors when all fields are compared.
40+
type AllFieldsUsed struct {
41+
name string
42+
value int
43+
extra bool
44+
}
45+
46+
func (a *AllFieldsUsed) Equals(other *AllFieldsUsed) bool {
47+
if a == nil && other == nil {
48+
return true
49+
}
50+
if a == nil || other == nil {
51+
return false
52+
}
53+
return a.name == other.name && a.value == other.value && a.extra == other.extra
54+
}
55+

0 commit comments

Comments
 (0)