cmd/compile: inconsistent quoting of program text

[rsc]

% 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

[griesemer]

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]

See #65790 for background.

[rsc]

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]

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.

[rsc]

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]

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

[gopherbot]

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