x/tools/go/analysis/passes/fieldalignment/cmd: document exit codes

[switchupcb]

Step 1: https://github.com/golang/proposal#the-proposal-process
The proposal author creates a brief issue describing the proposal.
Note: There is no need for a design document at this point.
Note: A non-proposal issue can be turned into a proposal by simply adding the proposal label.
Note: Language changes should follow a separate template

Context

https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/fieldalignment

Usecase

Using https://github.com/switchupcb/disgo/blob/v10/_gen/bundle/main.go#L80

• I have to do it in this way because running fieldalignment -fix returns an exit code 3.
  • exit code 3 is from go/exec Command Run . fieldalignment Stdout shows that structs were fixed.
• Running fieldalignment -fix again twice more fixes things (fixes https://github.com/switchupcb/disgo/blob/v10/wrapper/json_unmarshal.go#L186)

Sure, I can fix it in the developer version. However, I'm still unaware of what exit code 3 is.

Code

Add the meaning of the exit codes for fieldalignment to fieldalignment -help or somewhere.

[timothy-king]

Exit code 3 most likely comes from printDiagnostics in the analysis package. Documented here:

// It returns the exitcode: in plain mode, 0 for success, 1 for analysis
// errors, and 3 for diagnostics. We avoid 2 since the flag package uses
// it. JSON mode always succeeds at printing errors and diagnostics in a
// structured form to stdout.

Add the meaning of the exit codes for fieldalignment to fieldalignment -help or somewhere.

I think this comes down to whether we think we should document the exit codes within analysisflags.Help.

cc @adonovan

[adonovan]

I suggest you use the -json flag, which cause the tool to exit zero and report its diagnostics and fixes in a machine-readable form. You don't need to read them, but it solves the exit code problem.

xtools$ cat a.go
package tools

type S struct {
	byte
	int
	bool
}
xtools$ go run ./go/analysis/passes/fieldalignment/cmd/fieldalignment -json -fix a.go; echo $?
{
	"command-line-arguments": {
		"fieldalignment": [
			{
				"posn": "/Users/adonovan/w/xtools/a.go:3:8",
				"message": "struct of size 24 could be 16",
				"suggested_fixes": [
					{
						"message": "Rearrange fields",
						"edits": [
							{
								"filename": "/Users/adonovan/w/xtools/a.go",
								"start": 22,
								"end": 49,
								"new": "struct {\n\tint\n\tbyte\n\tbool\n}"
							}
						]
					}
				]
			}
		]
	}
}
0
xtools$ cat a.go
package tools

type S struct {
	int
	byte
	bool
}
xtools$ 

BTW I'm curious why you would want to apply fieldalignment to code you plan to bundle. The fixes applied by fieldalignment should be reviewed, since in some cases they may degrade performance and they can certainly cause tests that rely on field order (via reflection) to fail.

[switchupcb]

@adonovan Thanks for letting me know. That library (disgo) is based on a types library dasgo which defines the types in the manner specified by Discord API. There are many structs so the assumption is that the costs of saving memory (beneficial) over millions of requests will reduce the amount of GC pauses (also beneficial)

I didn't know about the potential for performance degradation (negative). Do you think I should avoid using the fieldalignment tool given that the documentation specifies "rare cases"? Does the performance degradation from both goroutines slowing down outweigh the costs of saving memory?

[adonovan]

I didn't know about the potential for performance degradation (negative). Do you think I should avoid using the fieldalignment tool given that the documentation specifies "rare cases"? Does the performance degradation from both goroutines slowing down outweigh the costs of saving memory?

'It depends" is always the answer to this kind of question; the only way to know for sure is to measure it.

My main concern is that you should "ship the bits you test", which means you should't blindly apply a transformation from any static analysis tool without review and testing.

[switchupcb]

Thanks for letting me know. The bundle is tested and analyzed in the CICD workflow.