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/compile: inconsistent quoting of program text #67685

Closed
rsc opened this issue May 29, 2024 · 7 comments
Closed

cmd/compile: inconsistent quoting of program text #67685

rsc opened this issue May 29, 2024 · 7 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rsc
Copy link
Contributor

rsc commented May 29, 2024

% go run gemini.go
./gemini.go:34:25: cannot use 1 (constant of type int) as io.Reader value in argument to json.NewDecoder: int does not implement io.Reader (missing method Read)
./gemini.go:62:6: `request' declared and not used
./gemini.go:67:2: undefined: req
./gemini.go:68:27: undefined: req
% 

The second error should not use quotation marks. None of the others do.

That is, we don't print:

./gemini.go:67:2: undefined: `req'

We also don't print:

./gemini.go:34:25: cannot use `1' (constant of type `int') as `io.Reader' value in argument to `json.NewCoder': `int' does not implement `io.Reader' (missing method `Read')

We should similarly not print quotes in

./gemini.go:62:6: `request' declared and not used

It should be

./gemini.go:62:6: request declared and not used

or if that's considered ambiguous:

./gemini.go:62:6: declared and not used: request

/cc @rfindley @griesemer

@rsc rsc added the NeedsFix The path to resolution is known, but the work has not been done. label May 29, 2024
@rsc rsc added this to the Go1.23 milestone May 29, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 29, 2024
@griesemer
Copy link
Contributor

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 not, the error message without quotes would say not declared and not used, which is confusing. There are more convincing examples with other error messages.

This was a deliberate choice.

@griesemer
Copy link
Contributor

See #65790 for background.

@rsc
Copy link
Contributor Author

rsc commented May 29, 2024

Okay but it's still inconsistent. We don't print

./gemini.go:34:25: cannot use `1' (constant of type `int') as `io.Reader' value in argument to `json.NewCoder': `int' does not implement `io.Reader' (missing method `Read')

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.

@griesemer
Copy link
Contributor

griesemer commented May 29, 2024

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.
See cmd/compile/internal/types2/format.go:42.

@griesemer griesemer self-assigned this May 29, 2024
@rsc
Copy link
Contributor Author

rsc commented May 29, 2024

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

./prog.go:6:2: invalid operation: invalid use of ... with built-in `max'

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:

builtins.go:28: invalidOp+"invalid use of ... with built-in %s", quote(bin.name))

"built-in" is enough lead-in; no quote.

builtins.go:213: check.errorf(x, code, invalidArg+"%s for %s", x, quote(bin.name))

s/for/for built-in/

builtins.go:536: check.verifyVersionf(call.Fun, go1_21, "%s", quote(bin.name))

s/%s/built-in %s/

builtins.go:579: check.assignment(x, &emptyInterface, "argument to "+quote(bin.name))

s/to/to built-in/

builtins.go:644: check.assignment(a, nil, "argument to "+quote(predeclaredFuncs[id].name))

s/to/to built-in/

builtins.go:995: check.softErrorf(x, code, "%s not supported as argument to %s for go1.18 (see go.dev/issue/50937)", x, quote(predeclaredFuncs[id].name))

s/to %s/to built-in %s/

call.go:722: check.errorf(e.Sel, UnexportedName, "%s not exported by package %s", quote(sel), quote(pkg.name))

s/%s/name %s/

check.go:313: check.errorf(file, MismatchedPkgName, "package %s; expected %s", quote(name), quote(pkg.name))

s/expected %s/expected package %s/

decl.go:746: err.addf(alt, "field and method with the same name %s", quote(fld.name))

name foo is clear; drop quote

labels.go:35: name = quote(name)
labels.go:44: check.softErrorf(lbl.pos, UnusedLabel, "label %s declared and not used", quote(lbl.name))
labels.go:140: err.addf(lbl.pos, "label %s already declared", quote(name))
labels.go:196: check.errorf(s.Label, MisplacedLabel, "invalid break label %s", quote(name))
labels.go:211: check.errorf(s.Label, MisplacedLabel, "invalid continue label %s", quote(name))

label foo is clear; drop quotes

stmt.go:67: check.softErrorf(v.pos, UnusedVar, "%s declared and not used", quote(v.name))

change to "declared and not used: %s"

stmt.go:499: err.addf(s, "result parameter %s not in scope at return", quote(obj.name))

"result parameter Foo" is clear. drop quotes

typeset.go:229: err.addf(atPos(pos), "duplicate method %s", quote(m.name))

method foo is clear; drop quotes

typeset.go:230: err.addf(atPos(mpos[other.(*Func)]), "other declaration of %s", quote(m.name))

s/%s/method %s/

typeset.go:243: err.addf(atPos(pos), "duplicate method %s", quote(m.name))
typeset.go:244: err.addf(atPos(mpos[other.(*Func)]), "other declaration of %s", quote(m.name))

s/%s/method %s/

typexpr.go:98: check.errorf(e, InvalidPkgUse, "use of package %s not in selector", quote(obj.name))

package foo is clear; drop quotes

typexpr.go:120: check.errorf(e, InvalidDeclCycle, "invalid use of type alias %s in recursive type (see go.dev/issue/50729)", quote(obj.name))

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

unexpected name status at end of statement

That is

case _Name:
- 	tok = "`" + p.lit + "'"
+ 	tok = "name " + p.lit

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/589118 mentions this issue: cmd/compile: remove quoting in favor of clearer prose in error messages

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/589455 mentions this issue: gopls: make tests tolerant of new go/types error format

gopherbot pushed a commit to golang/tools that referenced this issue May 30, 2024
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 <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Reviewed-by: Robert Griesemer <gri@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done.
Projects
Development

No branches or pull requests

3 participants