Skip to content

spec: range over integer expressions underspecified #65137

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

Closed
griesemer opened this issue Jan 17, 2024 · 17 comments
Closed

spec: range over integer expressions underspecified #65137

griesemer opened this issue Jan 17, 2024 · 17 comments
Assignees
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge
Milestone

Comments

@griesemer
Copy link
Contributor

griesemer commented Jan 17, 2024

The following code

var u uint8
for u = range 258 {
   println(u)
}

prints the values 0, 1, 2, ... 255, 0, 1; that is the integer "wraps around". This seems incorrect, but the spec is silent on the subject.

I believe the rules need to be extended such that if:
a) the iteration variable is already declared (u in the example above):

  • the iteration expressions must be assignable to the iteration variable (if the expression is a constant, it must fit into the iteration variable)

b) the iteration variable is being declared (as in for u := range x):

  • if the iteration expression is typed, the iteration variable has the same type
  • if the iteration expression is an untyped constant, it is given the type int, the constant must be representable as an int, and the iteration variable will be of type int

I believe these rules follow more or less from the existing prose (assignment to the iteration variables happens as in an assignment statement), but the fact that the compiler and type checkers get this wrong is an indication that we need more precise prose or better examples.

We need to pin this down before the 1.22 release otherwise we may not be able to make these changes anymore in a backward-compatible way.

cc: @rsc

@atdiar
Copy link

atdiar commented Jan 17, 2024

Related : #61405 (comment)

If it is not problematic and not too late, I would remove the assignment form altogether.
The iteration variable is not really needed and may even be confusing for people.

Then, no need to spec it either.

@cherrymui
Copy link
Member

the iteration expressions must be assignable to the iteration variable (if the expression is a constant, it must fit into the iteration variable)

This would mean

var u uint8
for u = range 256 {
   println(u)
}

would not be allowed, as 256 is not assignable to uint8, although the actual values being assigned, 0, 1, ...., 255, are assignable? (Personally I think that is fine, if that is more consistent with other rules. Just a weird corner case.)

@griesemer
Copy link
Contributor Author

griesemer commented Jan 17, 2024

@cherrymui Correct. I agree it's odd. But it would match the behavior of

var u uint8
for u = 0; u < 256; u++ {}

which is currently also not permitted.

Note that

for u := range 256 {}

will be ok.

@gazerro
Copy link
Contributor

gazerro commented Jan 17, 2024

Given a variable n, for the following code to compile

var u uint8
for u = range n {
   println(u)
}

n must be assignable to u, meaning n must be of type uint8. However, it is not possible to iterate over the entire range [0,255] because n cannot take the value 256. I find this odd as well.

@zephyrtronium
Copy link
Contributor

The workaround is straightforward:

var u uint8
for i := range n {
    u = uint8(i)
    println(u)
}

@atdiar
Copy link

atdiar commented Jan 17, 2024

Better yet

var u uint8
for range n{
    print(u) 
    u++
} 

Now if n is 256, there is still no issue.

@griesemer
Copy link
Contributor Author

@gazerro Note that your example never worked if n is a variable.

As @cherrymui has pointed out, these are really esoteric corner cases. Most of the time there's simply no issue here. If it turns out that we're too restrictive for practical purposes, we can relax the rules in the future, based on experience gained. But if we don't lock down things now, we won't be able to make changes later, once enough code is using behavior that we might consider incorrect.

@gazerro
Copy link
Contributor

gazerro commented Jan 17, 2024

@griesemer my example works if n is a variable (https://go.dev/play/p/kUks7Y5sA2E?v=gotip). Is there perhaps something I'm not understanding?

@timothy-king
Copy link
Contributor

@gazerro The direct translation of #65137 (comment) would be (https://go.dev/play/p/O4ZaNJeuJUG?v=gotip):

var n uint8 = 256
var u uint8
for u = range n {
	println(u)
}

This fails to compile with:

./prog.go:6:16: cannot use 256 (untyped int constant) as uint8 value in variable declaration (overflows)

@atdiar
Copy link

atdiar commented Jan 17, 2024

Another question is, even if we keep the assignment form, why is it even possible for the type of the iteration variable to be uint8?
If I range over a slice of uint8, the index is still of type int?
Edit: I mean that when ranging over a sequence of values, the type of the iteration variable (index) can just be int and not rely on assignment of const int to other numeric types?

I would have it be int every time and then there wouldn't be any wrapping issues (except maybe on some archs, I don't know)

Edit2: oh I think I understand now that it is using an integer number n as a generator of n typed values starting from 0.
If n is typed, those values are the same.
If n is untyped, they are int.
Still not too sure about it because it makes every integer represent a range starting from 0.
These are chosen semantics over integers. In practice it is probably not an issue.
Still it might have been easier to see any integer simply as an instantaneous value that describes the number of loop iterations. Just a number instead of such a specific range.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/556398 mentions this issue: types2: fix range clause checks for constant range expressions

gopherbot pushed a commit that referenced this issue Jan 18, 2024
Add missing checks for the case where the range expression is
a (possibly untyped) constant integer expression.

Add context parameter to assignVar for better error message
where the expression is part of a range clause.

Also, rename s/expr/Expr/ where it denotes an AST expression,
for clarity.

Fixes #65133.
For #65137.

Change-Id: I72962d76741abe79f613e251f7b060e99261d3ae
Reviewed-on: https://go-review.googlesource.com/c/go/+/556398
Run-TryBot: Robert Griesemer <[email protected]>
Auto-Submit: Robert Griesemer <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
@rsc
Copy link
Contributor

rsc commented Jan 18, 2024

New semantics SGTM.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/557596 mentions this issue: spec: clarify iteration variable type for range over integer

@mdempsky
Copy link
Contributor

New semantics SGTM too.

prints the values 0, 1, 2, ... 255, 0, 1

Oof. I suspect this is another consequence of our fuzzy IR. Ideally the compiler should have ICE'd instead.

@griesemer
Copy link
Contributor Author

@gopherbot please consider this for backport to 1.22. It's missing documentation for 1.22.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #65413 (for 1.22).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/559875 mentions this issue: [release-branch.go1.22] spec: clarify iteration variable type for range over integer

gopherbot pushed a commit that referenced this issue Jan 31, 2024
…ge over integer

Also: report language version (plus date) in spec header.

For #65137.

Change-Id: I4f1d220d5922c40a36264df2d0a7bb7cd0756bac
Reviewed-on: https://go-review.googlesource.com/c/go/+/557596
TryBot-Bypass: Robert Griesemer <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
Reviewed-on: https://go-review.googlesource.com/c/go/+/559875
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Add missing checks for the case where the range expression is
a (possibly untyped) constant integer expression.

Add context parameter to assignVar for better error message
where the expression is part of a range clause.

Also, rename s/expr/Expr/ where it denotes an AST expression,
for clarity.

Fixes golang#65133.
For golang#65137.

Change-Id: I72962d76741abe79f613e251f7b060e99261d3ae
Reviewed-on: https://go-review.googlesource.com/c/go/+/556398
Run-TryBot: Robert Griesemer <[email protected]>
Auto-Submit: Robert Griesemer <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Robert Findley <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
Also: report language version (plus date) in spec header.

Fixes golang#65137.

Change-Id: I4f1d220d5922c40a36264df2d0a7bb7cd0756bac
Reviewed-on: https://go-review.googlesource.com/c/go/+/557596
TryBot-Bypass: Robert Griesemer <[email protected]>
Reviewed-by: Matthew Dempsky <[email protected]>
Reviewed-by: Robert Griesemer <[email protected]>
@golang golang locked and limited conversation to collaborators Jan 30, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation Issues describing a change to documentation. FrozenDueToAge
Projects
Status: Done
Development

No branches or pull requests

9 participants