-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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/compile: inconsistent quoting of program text #67685
Comments
We use quotes when a program identifier appears in an error message and the identifier might change the meaning of the error message. We don't use quotes when the identifier stands alone after a colon. We also don't use quotes for anything that's not an identifier (and thus cannot be mistaken as a word of the error message). For instance, in this case, if the identifier were say This was a deliberate choice. |
See #65790 for background. |
Okay but it's still inconsistent. We don't print
It is very confusing as a user to see output like what I quoted above where identifiers are quoted sometimes and not quoted other times. |
The change was an attempt to address the issue of (sometimes) confusing error messages. It's easy to change (the quotes happen in one place), but we should come to an understanding of what the best approach is. |
I understand the confusion being addressed, and I see there are many errors affected, not just the one I ran into. It still seems like we need a clearer rule that should be applied consistently. Here are two that aren't quoted, for example: https://go.dev/play/p/pCMkUtPE1Vi. There is also the question of which quotes to use. I believe clang uses ‘ ’ (Unicode U+2018 and U+2019) instead of the ASCII ones. That would at least avoid the pedantic ambiguity of using quotation marks that already have a meaning in Go programs. In general I think our errors are clear enough not to need quoting when they say "noun X" for some name/variable/expression X. For example the quotes seem entirely noise in
Everyone should understand what "built-in max" means without the quotes. The quotes are just adding line noise (and they render weird in fonts like the one on GitHub that doesn't tilt the quotes the same amount; using Unicode would fix that at least). It may be that for Go 1.23 the best answer is to change quote to be a no-op. But if we have time to take them case by case, this is what I see:
"built-in" is enough lead-in; no quote.
s/for/for built-in/
s/%s/built-in %s/
s/to/to built-in/
s/to/to built-in/
s/to %s/to built-in %s/
s/%s/name %s/
s/expected %s/expected package %s/
name foo is clear; drop quote
label foo is clear; drop quotes
change to "declared and not used: %s"
"result parameter Foo" is clear. drop quotes
method foo is clear; drop quotes
s/%s/method %s/
s/%s/method %s/
package foo is clear; drop quotes
type alias foo is clear; drop quotes I see that #65790 was originally about a syntax error. That one seems like it could change to
That is
|
Change https://go.dev/cl/589118 mentions this issue: |
Change https://go.dev/cl/589455 mentions this issue: |
CL 589118 is changing the format of go/types errors. Update tests and the unusedvariable analyzer to be tolerant of this new format. For golang/go#67685 Change-Id: Ic1d3e663973edac3dcc6d0d6cc512fffd595eeb2 Reviewed-on: https://go-review.googlesource.com/c/tools/+/589455 LUCI-TryBot-Result: Go LUCI <[email protected]> Auto-Submit: Robert Findley <[email protected]> Reviewed-by: Robert Griesemer <[email protected]>
The second error should not use quotation marks. None of the others do.
That is, we don't print:
We also don't print:
We should similarly not print quotes in
It should be
or if that's considered ambiguous:
/cc @rfindley @griesemer
The text was updated successfully, but these errors were encountered: