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

cmd/go: clarify documentation for DefaultGODEBUG #70245

Open
findleyr opened this issue Nov 7, 2024 · 7 comments
Open

cmd/go: clarify documentation for DefaultGODEBUG #70245

findleyr opened this issue Nov 7, 2024 · 7 comments
Labels
Documentation GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@findleyr
Copy link
Contributor

findleyr commented Nov 7, 2024

The documentation for DefaultGODEBUG at https://go.dev/doc/godebug says "Only differences from the base Go toolchain defaults are reported.", but that's not the case: consider that x/tools/cmd/eg sets gotypesalias=1 in a //go:debug directive. Yet:

  • GOTOOLCHAIN=go1.22.0 go list -f {{.DefaultGODEBUG}} ./cmd/eg returns nothing
  • GOTOOLCHAIN=go1.23.1 go list -f {{.DefaultGODEBUG}} ./cmd/eg reports gotypesalias=1, even though that's the default

I will send a fix to ./cmd/go/internal/load/godebug.go.

CC @rsc @timothy-king @adonovan

@gabyhelp
Copy link

gabyhelp commented Nov 7, 2024

@findleyr findleyr self-assigned this Nov 7, 2024
@findleyr
Copy link
Contributor Author

findleyr commented Nov 7, 2024

Aha, well I see now why this is the case: it's impossible to produce accurate output because nowhere are the current default values for godebug defined: it is up to each library to interpret the empty string.

At this point I don't know how to proceed. I will assert:

  • DefaultGODEBUG is underdocumented.
  • DefaultGODEBUG does not implement the semantics described at https://go.dev/doc/godebug.
  • Given the confusion we've encountered around GODEBUG values, it would be useful if DefaultGODEBUG was accurate.

My recommendation would be to add default values to the table at internal/godebugs/table.go. However, this must have been considered in the past and deemed unnecessary. Certainly, there is a risk that it becomes redundant, or worse inaccurate.

@findleyr findleyr removed their assignment Nov 7, 2024
@findleyr
Copy link
Contributor Author

findleyr commented Nov 7, 2024

GOTOOLCHAIN=go1.22.0 go list -f {{.DefaultGODEBUG}} ./cmd/eg returns nothing

BTW: this tricked me. The reason this returns nothing is that the //go:debug directive is in a file that is also tagged //go:build go1.23. 🤦

@timothy-king
Copy link
Contributor

GOTOOLCHAIN=go1.22.0 go list -f {{.DefaultGODEBUG}} ./cmd/eg returns nothing

BTW: this tricked me. The reason this returns nothing is that the //go:debug directive is in a file that is also tagged //go:build go1.23. 🤦

Yes this is the tricky solution we hit upon to only enable the type aliases GODEBUG when using a toolchain >= 1.23 while in a go.mod for 1.22 to support type parameterized aliases for 1.24.

@findleyr
Copy link
Contributor Author

findleyr commented Nov 7, 2024

Yes this is the tricky solution we hit upon to only enable the type aliases GODEBUG when using a toolchain >= 1.23 while in a go.mod for 1.22 to support type parameterized aliases for 1.24.

Oh yeah, once I realized what was happening I remembered the background. I just wanted to explain the otherwise confusing bullet point in my original issue description.

@cherrymui cherrymui added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go labels Nov 11, 2024
@cherrymui cherrymui added this to the Backlog milestone Nov 11, 2024
@rsc
Copy link
Contributor

rsc commented Nov 19, 2024

Every GODEBUG setting value, like any other self-respecting Go value of type string, defaults to the empty string. As such, the behavior in the report for Go 1.23 is correct: if there is an explicit setting of "1", then it should be reported, even if the library happens to treat "" and "1" as meaning the same thing. (And as was pointed out later, the Go 1.22 behavior is also correct, because the //go:debug was guarded by a build tag.)

I can update /doc/godebug to point out that even though we often talk about GODEBUGs as (say) only having two values 0 and 1, every GODEBUG defaults to an empty string (unset), and it is up to the library defining the GODEBUG to decide how to react to the unset empty string. I agree that /doc/godebug is not clear on this point.

I'm not wild about extending the table to list non-empty default settings, because that list will grow and grow over time and fill with ancient, irrelevant settings. For example, we don't need to see panicnil=0 in all Go binaries for the rest of time. The SNR is much higher if we only report differences from what the toolchain would do with no overrides at all.

@findleyr
Copy link
Contributor Author

Sure, just updating the doc sounds good to me.

The origin of this confusion was https://go.dev/issue/68900, where I was attempting to highlight non-default GODEBUG values when hovering over a package name. Absent a table of effective defaults, I decided that feature would be too confusing, and so decided not to implement.

For example, we don't need to see panicnil=0 in all Go binaries for the rest of time

Just to understand, where would you see this? It wouldn't show up in DefaultGODEBUG, since the setting would match its default value for the go version.

@findleyr findleyr changed the title cmd/go: DefaultGODEBUG appears to mishandle explicit godebug directives cmd/go: clarify documentation for DefaultGODEBUG Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants