"struct with X pointer bytes could be Y" linting errors for (what appears to be) aligned structs

[atc0005]

Summary

The changes from v0.3.19 and GH-287 resulted in numerous CI "failures" across projects that I maintain. Many of the complaints were solved by struct field reordering, but some issues persisted even after hours spent trying to align the fields per various guides. After spending far too long on this, I opened an upstream report with an example to reproduce the problem: golang/go#45541.

I learned that the issue had already been reported and responded to in golang/go#44877 (comment). That issue appears to have been closed without documentation changes, including how to disable that specific linting check.

From what I now understand, the fieldalignment linter bundles two diagnostics:

• struct size
  • due to inefficient struct field alignment
• pointer bytes
  • how many bytes of the object that the garbage collector (GC) has to potentially scan for pointers

For the second, the Go team member who maintained the now deprecated maligned linter indicated that it's not worth reporting to users by default; I've yet to find a way to selectively disable this diagnostic and golangci/golangci-lint#1825 was closed without resolution.

For now, I'm not sure whether it's worth reverting GH-287 or waiting to see what upstream and golangci-lint devs do to help mute the "pointer bytes" warnings. I'm learning towards reverting GH-287 for now.

References

• WARN [runner] The linter 'maligned' is deprecated #287
• x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment incorrect size calculations, false positives golang/go#44877 (comment)
• Deprecate maligned, add govet fieldalignment as replacement golangci/golangci-lint#1765
• govet fieldalignment produces false positives for embedded structures golangci/golangci-lint#1825
• https://medium.com/@sebassegros/golang-dealing-with-maligned-structs-9b77bacf4b97
• https://medium.com/@felipedutratine/how-to-organize-the-go-struct-in-order-to-save-memory-c78afcf59ec2
• https://github.com/Tkanos/optimizing-memory-by-ordering-struct-by-type
• https://go101.org/article/memory-layout.html

[atc0005]

I'm learning towards reverting GH-287 for now.

I re-enabled the deprecated maligned linter for now for the v0.3.20 release.

[atc0005]

There are some changes applied here which look interesting:

Showmax/go-fqdn@5355da1

diff --git a/.golangci.yml b/.golangci.yml
index 5256200..fd02c47 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -11,14 +11,13 @@ linters-settings:
 linters:
   enable:
     - dogsled
+    - exportloopref
     - gochecknoglobals
     - gochecknoinits
     - goconst
     - gomnd
     - goprintffuncname
-    - maligned
     - nakedret
-    - scopelint
     - unconvert
     - unparam
 
@@ -30,3 +29,7 @@ issues:
 
   max-issues-per-linter: 0
   max-same-issues: 0
+
+govet:
+  enable:
+    - fieldalignment

It's been a while since I've looked into this, but if the fieldalignment diagnostic for govet linter is explicitly enabled as shown here, does that leave out the "pointer bytes" diagnostic?

[atc0005]

There are some changes applied here which look interesting:

Turns out that these are the same changes which I applied here in:

• WARN [runner] The linter 'maligned' is deprecated #287
• WARN [runner] The linter 'scopelint' is deprecated #288

and then partially reverted in:

• Re-enable deprecated maligned linter #303

[atc0005]

While working on a new project I finally got past the "just get it working" phase and started the process of cleaning up (adding doc comments, resolving any minor linting errors, etc.).

I encountered several linter errors like this one:

struct of size 272 bytes could be of size 264 bytes

After some work to satisfy the maligned linter (doc comments stripped from our example), this struct:

type SyncPlan struct {
	CreatedAt         StandardAPITime     `json:"created_at"`
	CronExpression    NullString          `json:"cron_expression"`
	Description       NullString          `json:"description"`
	Enabled           bool                `json:"enabled"`
	RecurringLogicID  int                 `json:"foreman_tasks_recurring_logic_id"`
	ID                int                 `json:"id"`
	Interval          string              `json:"interval"`
	Name              string              `json:"name"`
	NextSync          SyncTime            `json:"next_sync"`
	OrganizationID    int                 `json:"organization_id"`
	OrganizationName  string              `json:"-"`
	OrganizationLabel string              `json:"-"`
	OrganizationTitle string              `json:"-"`
	Permissions       SyncPlanPermissions `json:"permissions"`
	Products          []Product           `json:"products"`
	OriginalSyncDate  SyncTime            `json:"sync_date"`
	UpdatedAt         StandardAPITime     `json:"updated_at"`
}

became this struct:

type SyncPlan struct {
	OriginalSyncDate  SyncTime            `json:"sync_date"`
	NextSync          SyncTime            `json:"next_sync"`
	UpdatedAt         StandardAPITime     `json:"updated_at"`
	CreatedAt         StandardAPITime     `json:"created_at"`
	Products          Products            `json:"products"`
	CronExpression    NullString          `json:"cron_expression"`
	Description       NullString          `json:"description"`
	Interval          string              `json:"interval"`
	Name              string              `json:"name"`
	OrganizationName  string              `json:"-"`
	OrganizationLabel string              `json:"-"`
	OrganizationTitle string              `json:"-"`
	RecurringLogicID  int                 `json:"foreman_tasks_recurring_logic_id"`
	ID                int                 `json:"id"`
	OrganizationID    int                 `json:"organization_id"`
	Permissions       SyncPlanPermissions `json:"permissions"`
	Enabled           bool                `json:"enabled"`
}

It now occupies 8 bytes less memory.

If we instead order alphabetically as a human optimization we end up with:

type SyncPlan struct {
	CreatedAt         StandardAPITime     `json:"created_at"`
	CronExpression    NullString          `json:"cron_expression"`
	Description       NullString          `json:"description"`
	Enabled           bool                `json:"enabled"`
	ID                int                 `json:"id"`
	Interval          string              `json:"interval"`
	Name              string              `json:"name"`
	NextSync          SyncTime            `json:"next_sync"`
	OrganizationID    int                 `json:"organization_id"`
	OrganizationLabel string              `json:"-"`
	OrganizationName  string              `json:"-"`
	OrganizationTitle string              `json:"-"`
	OriginalSyncDate  SyncTime            `json:"sync_date"`
	Permissions       SyncPlanPermissions `json:"permissions"`
	Products          []Product           `json:"products"`
	RecurringLogicID  int                 `json:"foreman_tasks_recurring_logic_id"`
	UpdatedAt         StandardAPITime     `json:"updated_at"`
}

Aside from some architectures which specifically require careful manual alignment, I have to ask myself whether the code would benefit more from alphabetizing the fields.

[atc0005]

Said another way, I'm considering dropping the maligned linter from all projects that I maintain as I'm not sure it provides enough value.

[atc0005]

See also:

• Remove maligned inactive linter  #1492