From 7a38de6dceb4b79ea284a11dbb7c2f1931d13b04 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 12 Jan 2026 18:48:48 +0000 Subject: [PATCH 1/6] Initial plan From a3c0d43777e21be25a6d7da7c780e7c309f7afa8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 12 Jan 2026 18:52:34 +0000 Subject: [PATCH 2/6] Add test cases for nested struct fields Co-authored-by: timflannagan <9899409+timflannagan@users.noreply.github.com> --- pkg/analyzer/analyzer_test.go | 12 +++++ pkg/analyzer/testdata/src/nested/nested.go | 53 +++++++++++++++++++ .../testdata/src/nestedunexported/nested.go | 33 ++++++++++++ 3 files changed, 98 insertions(+) create mode 100644 pkg/analyzer/testdata/src/nested/nested.go create mode 100644 pkg/analyzer/testdata/src/nestedunexported/nested.go diff --git a/pkg/analyzer/analyzer_test.go b/pkg/analyzer/analyzer_test.go index c4d3bfb..d5bcc51 100644 --- a/pkg/analyzer/analyzer_test.go +++ b/pkg/analyzer/analyzer_test.go @@ -53,3 +53,15 @@ 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") +} diff --git a/pkg/analyzer/testdata/src/nested/nested.go b/pkg/analyzer/testdata/src/nested/nested.go new file mode 100644 index 0000000..7cd5490 --- /dev/null +++ b/pkg/analyzer/testdata/src/nested/nested.go @@ -0,0 +1,53 @@ +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/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 +} From bdb98eb111550e7205415f5bf3988c8d9aff529c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 12 Jan 2026 18:58:08 +0000 Subject: [PATCH 3/6] Fix linter gap: detect struct comparisons that bypass +noKrtEquals markers Co-authored-by: timflannagan <9899409+timflannagan@users.noreply.github.com> --- README.md | 31 +++++++ pkg/analyzer/analyzer.go | 92 +++++++++++++++++++ pkg/analyzer/analyzer_test.go | 6 ++ pkg/analyzer/testdata/src/nested/nested.go | 1 - .../testdata/src/nestedmissing/nested.go | 40 ++++++++ pkg/analyzer/testdata/src/problem/problem.go | 71 ++++++++++++++ .../testdata/src/unexportedon/unexported.go | 1 - 7 files changed, 240 insertions(+), 2 deletions(-) create mode 100644 pkg/analyzer/testdata/src/nestedmissing/nested.go create mode 100644 pkg/analyzer/testdata/src/problem/problem.go 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..185e64d 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,95 @@ func collectStructs(ins *inspector.Inspector) map[string]*structInfo { return structs } +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 + } + } else { + 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 { + pass.Reportf(binExpr.Pos(), "field %q of struct type %q is compared using %s which ignores +noKrtEquals markers in nested fields; use .Equals() method instead", + leftSel.Sel.Name, typeName, binExpr.Op) + } + + 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 d5bcc51..f29a1c8 100644 --- a/pkg/analyzer/analyzer_test.go +++ b/pkg/analyzer/analyzer_test.go @@ -65,3 +65,9 @@ func TestNestedFieldsWithUnexported(t *testing.T) { 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 index 7cd5490..7ca1931 100644 --- a/pkg/analyzer/testdata/src/nested/nested.go +++ b/pkg/analyzer/testdata/src/nested/nested.go @@ -50,4 +50,3 @@ 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/problem/problem.go b/pkg/analyzer/testdata/src/problem/problem.go new file mode 100644 index 0000000..65d6e3e --- /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 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 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 } - From e43230de9889869e4e53e4627c5d81ad20b2908f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 12 Jan 2026 19:00:42 +0000 Subject: [PATCH 4/6] Fix error message and test expectations based on code review feedback Co-authored-by: timflannagan <9899409+timflannagan@users.noreply.github.com> --- pkg/analyzer/analyzer.go | 2 +- pkg/analyzer/testdata/src/problem/problem.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/analyzer/analyzer.go b/pkg/analyzer/analyzer.go index 185e64d..0f3511d 100644 --- a/pkg/analyzer/analyzer.go +++ b/pkg/analyzer/analyzer.go @@ -253,7 +253,7 @@ func checkStructComparisons(pass *analysis.Pass, fd *ast.FuncDecl, recvIdent str } if hasIgnoredFields { - pass.Reportf(binExpr.Pos(), "field %q of struct type %q is compared using %s which ignores +noKrtEquals markers in nested fields; use .Equals() method instead", + pass.Reportf(binExpr.Pos(), "field %q of struct type %q is compared using %s which ignores +noKrtEquals markers; use .Equals() method instead", leftSel.Sel.Name, typeName, binExpr.Op) } diff --git a/pkg/analyzer/testdata/src/problem/problem.go b/pkg/analyzer/testdata/src/problem/problem.go index 65d6e3e..82243c8 100644 --- a/pkg/analyzer/testdata/src/problem/problem.go +++ b/pkg/analyzer/testdata/src/problem/problem.go @@ -19,7 +19,7 @@ type OuterType struct { // 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 markers" + return o.Name == other.Name && o.Inner == other.Inner // want "field \"Inner\" of struct type \"InnerType\" is compared using == which ignores \\+noKrtEquals markers" } // ListenerPolicyIR mimics the exact structure from the GitHub issue @@ -45,7 +45,7 @@ type HttpListenerPolicyIr struct { // 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 markers" + return l.ct == other.ct && l.defaultPolicy == other.defaultPolicy // want "field \"defaultPolicy\" of struct type \"listenerPolicy\" is compared using == which ignores \\+noKrtEquals markers" } // CorrectComparison shows the correct way to compare struct fields From 8c39974ae5957bda728dd5ab66711437c5c3ee57 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 12 Jan 2026 19:01:53 +0000 Subject: [PATCH 5/6] Improve error message readability and simplify pointer unwrapping logic Co-authored-by: timflannagan <9899409+timflannagan@users.noreply.github.com> --- pkg/analyzer/analyzer.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/analyzer/analyzer.go b/pkg/analyzer/analyzer.go index 0f3511d..f83d817 100644 --- a/pkg/analyzer/analyzer.go +++ b/pkg/analyzer/analyzer.go @@ -223,10 +223,8 @@ func checkStructComparisons(pass *analysis.Pass, fd *ast.FuncDecl, recvIdent str // Try unwrapping pointer if ptrType, isPtrType := leftType.(*types.Pointer); isPtrType { namedType, ok = ptrType.Elem().(*types.Named) - if !ok { - return true - } - } else { + } + if !ok { return true } } @@ -253,8 +251,12 @@ func checkStructComparisons(pass *analysis.Pass, fd *ast.FuncDecl, recvIdent str } 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 markers; use .Equals() method instead", - leftSel.Sel.Name, typeName, binExpr.Op) + leftSel.Sel.Name, typeName, opStr) } return true From e1b5c3c4d97fcdc97fe24670b16e17eaadc0dbaf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 12 Jan 2026 19:03:13 +0000 Subject: [PATCH 6/6] Add function documentation and improve error message clarity Co-authored-by: timflannagan <9899409+timflannagan@users.noreply.github.com> --- pkg/analyzer/analyzer.go | 6 +++++- pkg/analyzer/testdata/src/problem/problem.go | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/analyzer/analyzer.go b/pkg/analyzer/analyzer.go index f83d817..a66bbd5 100644 --- a/pkg/analyzer/analyzer.go +++ b/pkg/analyzer/analyzer.go @@ -172,6 +172,10 @@ 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 @@ -255,7 +259,7 @@ func checkStructComparisons(pass *analysis.Pass, fd *ast.FuncDecl, recvIdent str if binExpr.Op == token.NEQ { opStr = "!=" } - pass.Reportf(binExpr.Pos(), "field %q of struct type %q is compared using %s which ignores +noKrtEquals markers; use .Equals() method instead", + 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) } diff --git a/pkg/analyzer/testdata/src/problem/problem.go b/pkg/analyzer/testdata/src/problem/problem.go index 82243c8..abd7f51 100644 --- a/pkg/analyzer/testdata/src/problem/problem.go +++ b/pkg/analyzer/testdata/src/problem/problem.go @@ -19,7 +19,7 @@ type OuterType struct { // 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 markers" + 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 @@ -45,7 +45,7 @@ type HttpListenerPolicyIr struct { // 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 markers" + 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