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: clearer error for invalid const values #69955

Closed
varungandhi-src opened this issue Oct 21, 2024 · 11 comments
Closed

cmd/compile: clearer error for invalid const values #69955

varungandhi-src opened this issue Oct 21, 2024 · 11 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@varungandhi-src
Copy link

Go version

go version go1.23.2 darwin/arm64

Output of go env in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN='/Users/varun/.local/share/mise/installs/go/1.23.2/bin'
GOCACHE='/Users/varun/Library/Caches/go-build'
GOENV='/Users/varun/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/varun/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='darwin'
GOPATH='/Users/varun/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/Users/varun/.local/share/mise/installs/go/1.23.2'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/varun/.local/share/mise/installs/go/1.23.2/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23.2'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/varun/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/Users/varun/Code/sourcegraph/go.mod'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/tr/gylnxkts3bn0gyhhlrv8z9_m0000gn/T/go-build266442858=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

https://go.dev/play/p/entU62NdkPA

type S struct{}

const s = S{}

What did you see happen?

The error message is:

./prog.go:7:11: S{} (value of type S) is not constant

What did you expect to see?

It's not unusual to want to specify struct constants in code, but using struct literals with constant fields is not supported.

The error message could be made clearer to indicate that struct literals are not permitted in constant expressions (because at face value, struct literals with constant fields and no pointers or casting are conceptually constant). It might be helpful to state that only boolean, numeric and string literals are allowed in constant expressions.

Potential ideas for improved error messages:

./prog.go:7:11: struct literal S{} (value of type S) not permitted in constant expression; only boolean, numeric and string literals can be used as constants
./prog.go:7:11: struct literal S{} (value of type S) not permitted in constant expression; use var instead of const
@ianlancetaylor
Copy link
Member

It's not precisely correct to say that "only boolean, numeric and string literals" can appear in constant expressions. For example, len([4]int{}) is a valid constant expression. The exact rules are at https://go.dev/ref/spec#Constant_expressions; I don't know whether it's helpful for compiler error to point directly to the spec.

We could at least "is not a constant expression" which is a better search term.

@seankhliao seankhliao changed the title go/compiler: Improve diagnostic on attempting to use struct literals in constant expressions cmd/compile: clearer error for invalid const values Oct 21, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 21, 2024
@varungandhi-src
Copy link
Author

It's not precisely correct to say that "only boolean, numeric and string literals" can appear in constant expressions. For example, len([4]int{}) is a valid constant expression

Yeah, I got that simple expressions as being allowed from reading the spec earlier, I meant it more like "out of all literals only these literals are allowed" but it's hard to convey that within the limited space of a single error message.

I'm not sure if there are other compiler error messages which are split across multiple lines. If that's acceptable, there's potentially more room to provide helpful information without it being overwhelming.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/621435 mentions this issue: go/types, types2: clearer error for invalid const values

@prattmic
Copy link
Member

cc @golang/compiler

@prattmic prattmic added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 21, 2024
@griesemer
Copy link
Contributor

griesemer commented Oct 21, 2024

I believe the current error message "is not constant" is just as clear as what is proposed. I don't believe anything needs to be done here. This is just churn and quite possibly will break a bunch of tests.

@griesemer griesemer self-assigned this Oct 21, 2024
@griesemer griesemer added this to the Backlog milestone Oct 21, 2024
@griesemer
Copy link
Contributor

Marking as backlog. There may be a better way to guide users in the right direction, but changing from "not a constant" to "not a constant expression" doesn't seem to add much and produces needless churn. For one, a simple search for "golang constant" produces about equally helpful results as "golang constant expression".

@varungandhi-src
Copy link
Author

varungandhi-src commented Oct 22, 2024

but changing from "not a constant" to "not a constant expression" doesn't seem to add much

That's why I provided several alternatives in the issue description, for example, this one:

struct literal S{} (value of type S) not permitted in constant expression; use var instead of const

@adonovan
Copy link
Member

The existing error messages (reasonably) assume the reader knows that only basic types can be constant. One small tweak we could make would be to put the type kind name (pointer, chan, numeric, etc) in the error message if the type is named. For example "value of numeric type S" instead "of value of type S".

The original error would then become:

./prog.go:7:11: S{} (value of struct type S) is not constant

which may help remind the reader that struct types have no constants.

@griesemer
Copy link
Contributor

@varungandhi-src "use var instead of const" may work in this specific case, but the compiler doesn't know if that is in fact the error. Maybe you actually meant to use a constant, and then saying "using var" would be strange. That is why the compiler in general doesn't provide suggestions: it simply cannot know what the right suggestion is.

I like @adonovan's suggestion. I will take care of it.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/621458 mentions this issue: go/types, types2: qualify named types in error messages with type kind

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. 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

7 participants