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: untyped float constants representable as int are rejected in range clauses #66561

Closed
zephyrtronium opened this issue Mar 27, 2024 · 9 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

@zephyrtronium
Copy link
Contributor

Go version

go version go1.22.0 linux/amd64

Output of go env in your module/workspace:

~/play/mapsample$ go env
GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/home/branden/.cache/go-build'
GOENV='/home/branden/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/home/branden/go/pkg/mod'
GONOPROXY='*.redacted.net'
GONOSUMDB='*.redacted.net'
GOOS='linux'
GOPATH='/home/branden/go'
GOPRIVATE='*.redacted.net'
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/usr/local/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/usr/local/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='go1.22.0'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
GOMOD='/home/branden/play/mapsample/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 -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build2983741865=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Used a constant representable as int in a range clause.

https://go.dev/play/p/Zkd_wK5-Y20

for range 1e0 {
	fmt.Println("Hello, 世界")
}

(More realistically, I wrote a number of iterations for a test as const N = 2e5, then used N in a range clause.)

What did you see happen?

./prog.go:8:12: cannot range over 1e0 (untyped float constant 1)

What did you expect to see?

Since constant 1e0 is representable as int, I expect it to be implicitly typed as such in a range clause. AFAIK this is the only place the compiler rejects floating-point constants representable as an integer type where an integer expression is expected; e.g., [1e0]struct{}{} is accepted.

Per the spec's section on for loops with range clauses:

The expression on the right in the "range" clause is called the range expression, its core type must be an array, pointer to an array, slice, string, map, channel permitting receive operations, or an integer.
...
If the range expression is a (possibly untyped) integer expression n, n too must be assignable to the iteration variable; if there is no iteration variable, n must be assignable to int.

along with the section on constants:

A constant may be given a type explicitly by a constant declaration or conversion, or implicitly when used in a variable declaration or an assignment statement or as an operand in an expression. It is an error if the constant value cannot be represented as a value of the respective type.

Arguably, the fact that range clauses specify "a (possibly untyped) integer expression" is the justification that floating point constants are always rejected. In particular, that section is the only place where the phrase "integer expression" appears in the spec. However, the section on constants also classifies rune constants separately from integer constants ("Rune, integer, floating-point, and complex constants are collectively called numeric constants"), yet the compiler does not reject a rune constant in a range clause: https://go.dev/play/p/eh307dPM6s5. So, my reading of the spec is that both rune and floating-point constants should be either accepted or rejected.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Mar 27, 2024
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 27, 2024
@thanm
Copy link
Contributor

thanm commented Mar 27, 2024

@gri for spec issues

@mknyszek mknyszek added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 27, 2024
@mknyszek mknyszek modified the milestones: Backlog, Go1.23 Mar 27, 2024
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 27, 2024
@griesemer
Copy link
Contributor

Agreed. The spec should be made consistent here (as needed) and the implementation relaxed accordingly.
I will take care of this.

@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 28, 2024
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 28, 2024
@novitoll
Copy link
Contributor

Few comments:

for range '\x01' {}
for range '1' {}

Both rune literals are valid in devel go1.23-c51f6c6, though the 1st has 1 iteration, the 2nd loop will cycle 49 times (U+0031), not sure if it's intended with rune constant (int32) to be in for-range clause in isInteger(typ) branch.

Regarding floating-point constant in for-loop:

reflect.TypeOf(1e0) == float64 // true

for i, v := range [1e0]struct{}{} {} // ok

for i, v := range [1.0]struct{}{} {} // ok

for i, v := range [1.1]struct{}{} {} // array length 1.1 (untyped float constant) must be integer

So untyped floating-point constants with 0 fractional part 0.0, 1.0 .. are currently valid for the array size, and it seems, that the problem might be in general how we handle such as untyped floating-points.

The last point might relate to this issue as well: #52080

@griesemer
Copy link
Contributor

Rune literals are integer values, there's no issue there.
Untyped floating point values that can be represented without loss of precision should be acceptable as range counts, per this issue.
Ths shift issue (#52080) is more complicated because the type of the shifted values depends on the type of the variable on the left, not just the untyped constant.

@gopherbot
Copy link

Change https://go.dev/cl/580937 mentions this issue: go/types, types2: allow range over untyped constants representable as integers

@gopherbot
Copy link

Change https://go.dev/cl/581295 mentions this issue: go/types, types2: simplify numeric range logic

@griesemer
Copy link
Contributor

It turns out that this is a bit more subtle than anticipated.
I think we want the following rules:

  1. A numeric x in range x must be of integer type or representable as an integer value (the latter relaxation is new).
  2. If x has a (integer) type, the iteration values have that same type. They must be assignable to the iteration variable, if any. This case is straight-forward.
  3. If x is an untyped constant there are additional cases:
    3a) If the iteration variable v is declared outside the loop (v = range x), it must be of integer type, and x must be assignable to it.
    3b) If the iteration variable v is declared by the range clause (v := range x) or is absent (same as _ = range x), the type of v is rune if x is a rune constant, otherwise the type of v is int, and x must be assignable to it.

@gopherbot
Copy link

Change https://go.dev/cl/581256 mentions this issue: spec: fix prose for range over numeric range expression

@griesemer
Copy link
Contributor

After going back and forth on this (including implementing it in full), and discussing internally, I believe we should leave things as they are, for the following reasons:

Contrary to the initial claim, the n in a range n clause is not actually in a situation where we would usually allow any integer type. In those cases (say index expressions), the index argument may be any type or value that is assignable to an int (this excludes for instance the maxUint value). Also, in those cases, we only care about the value of the (index) argument, not its type.

In contrast, in a i = range n or i := range n clause, the type of an untyped n is determined by i or vice versa. That is, the type rules applying to i = range n and i := range n are the type rules that apply to assignments i = n and i := n respectively, with the additional requirement that the type of i (and thus n) must end up being an integer type. Note for instance that range 'a' produces a series of rune (not int) iteration values, and that var i byte; for i = range 256 {} is not permitted because 256 doesn't fit into a byte (playground). This seems all pretty simple and consistent.

We could allow untyped numeric constants of non-integer type as long as they are representable as integers, but that actually adds an additional rule (the assignment equivalence doesn't work directly anymore) and it seems an unnecessary extra complication for a rather hypothetical (or at least rare) case. If one really wants to write range 1e6 one can easily write range int(1e6) and it may in fact be clearer.

In any case, I have updated the spec (CL 581256) to hopefully be clearer, and I have also fixed the implementation where we had a related issue (#67027).

Closing this as working as intended.
If you feel strongly about this, please file a language change proposal. Thanks.

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

6 participants