Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
switchupcb opened this issue Dec 5, 2022 · 5 comments
Closed

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

switchupcb opened this issue Dec 5, 2022 · 5 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@switchupcb
Copy link

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

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.

@gopherbot gopherbot added this to the Proposal milestone Dec 5, 2022
@timothy-king timothy-king added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed Proposal labels Dec 5, 2022
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 5, 2022
@timothy-king timothy-king changed the title proposal: x/tools/fieldalignment: add exit code to help x/tools/go/analysis/passes/fieldalignment/cmd: document exit codes Dec 5, 2022
@timothy-king timothy-king added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 5, 2022
@timothy-king
Copy link
Contributor

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
Copy link
Member

adonovan commented Dec 5, 2022

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.

@adonovan adonovan closed this as completed Dec 5, 2022
@switchupcb
Copy link
Author

@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
Copy link
Member

adonovan commented Dec 5, 2022

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
Copy link
Author

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

@golang golang locked and limited conversation to collaborators Dec 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

4 participants