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

Jump to bottom

gccgo: "range x" evaluates x even when it shouldn't #22313

Closed
mdempsky opened this issue Oct 17, 2017 · 10 comments
Closed

gccgo: "range x" evaluates x even when it shouldn't #22313

mdempsky opened this issue Oct 17, 2017 · 10 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Gccgo

Comments

@mdempsky
Copy link
Member

The Go spec says (though see also #22258):

The range expression is evaluated once before beginning the loop, with one exception: if the range expression is an array or a pointer to an array and at most one iteration variable is present, only the range expression's length is evaluated; if that length is constant, by definition the range expression itself will not be evaluated.

In the program below *p has a constant length (and even len(*p) is constant) and there's at most one iteration variable present, so *p should not be evaluated:

package main

func main() {
        var p *[10]int
        for range *p {
        }
}

When compiled with cmd/compile, this program runs and exits normally. (Seemingly according to spec.)

When compiled with gccgo, this program panics because of a nil pointer dereference. (Seemingly contrary to the spec.)

/cc @ianlancetaylor @griesemer
@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 17, 2017
@mdempsky mdempsky added this to the Gccgo milestone Oct 17, 2017
@mdempsky
Copy link
Member Author

for _, _ = range *p {} should not panic either, because

If the last iteration variable is the blank identifier, the range clause is equivalent to the same clause without that identifier.

@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2017 •
edited

Just curious: why did we add that caveat to the spec in the first place? It's annoyingly inconsistent with the general “range expression is evaluated once” rule...

@mdempsky
Copy link
Member Author

@bcmills I think so that

for i := range x { ... }

is equivalent to

for i, n := 0, len(x); i < n; i++ { ... }

@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2017

I was going to say that they're already not equivalent, but now I don't even know.
https://play.golang.org/p/P6c1ho-y1M

The current phrasing of the spec suggests that this program should not evaluate the call to panicArray, but it is evaluated nonetheless:

func panicArray() [3]int {
	panic("nope")
}

func main() {
	for i := range panicArray() {
		fmt.Println(i)
	}
}

@griesemer
Copy link
Contributor

len(panicArray()) is not a constant per the spec.

@bcmills
Copy link
Contributor

bcmills commented Oct 18, 2017 •
edited

len(panicArray()) is not a constant per the spec.

That's my point: according to the current phrasing, it seems like for i := range panicArray() should evaluate only the length of the panicArray() expression, but for i, n := 0, len(panicArray()); … should evaluate the expression itself.

“if the range expression is an array or a pointer to an array and at most one iteration variable is present, only the range expression's length is evaluated” is not conditioned upon whether the expression is a constant.

I suppose that depends on fairly subtle details about what it means to “evaluate” the length of an array of constant size returned by a function call.

(Contrast len vs. unsafe.Sizeof: https://play.golang.org/p/1rjtYb3Gdw)

@griesemer
Copy link
Contributor

griesemer commented Oct 18, 2017 •
edited

That's why the new phrasing, suggested via #22258 ( https://go-review.googlesource.com/c/go/+/71431/3/doc/go_spec.html ) makes this clearer.

The range expression x is evaluated (with a single iteration variable) only if len(x) is not a constant. And the spec is clear abut when len(x) is a constant: https://golang.org/ref/spec#Length_and_capacity . It's definitively not a constant if there's a function call in x, which is what we have here.

Note that unsafe.Sizeof are always compile-time constant expressions per the spec.

@mdempsky
Copy link
Member Author

Oh, I just noticed there's even this example in the spec, which demonstrates the same bug:

var testdata *struct {
	a *[7]int
}
for i, _ := range testdata.a {
	// testdata.a is never evaluated; len(testdata.a) is constant
	// i ranges from 0 to 6
	f(i)
}

@gopherbot
Copy link

Change https://golang.org/cl/91555 mentions this issue: compiler: don't incorrectly evaluate range variable

hubot pushed a commit to gcc-mirror/gcc that referenced this issue Feb 2, 2018
compiler: don't incorrectly evaluate range variable
c29f17b 
    
    The language spec says that in `for i = range x`, in which there is no
    second iteration variable, if len(x) is constant, then x is not
    evaluated.  This only matters when x is an expression that panics but
    whose type is an array type; in such a case, we should not evaluate x,
    since len of any array type is a constant.
    
    Fixes golang/go#22313
    
    Reviewed-on: https://go-review.googlesource.com/91555


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@257330 138bc75d-0d04-0410-961f-82ee72b054a4
@gopherbot
Copy link

Change https://golang.org/cl/91715 mentions this issue: compiler: in range, evaluate array if it has receives or calls

gopherbot pushed a commit to golang/gofrontend that referenced this issue Feb 5, 2018
@ianlancetaylor
compiler: in range, evaluate array if it has receives or calls
5031f87 
The last change was incomplete, in that it did not evaluate the array
argument in some cases where it had to be evaluated.  This reuses the
existing code for checking whether len/cap is constant.

Also clean up the use of _ as the second variable in a for/range,
which was previously inconsistent depending on whether the statement
used = or :=.

Updates golang/go#22313

Change-Id: I8fe09a64026aa9f8833197df2454a8581e98f60b
Reviewed-on: https://go-review.googlesource.com/91715
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
hubot pushed a commit to gcc-mirror/gcc that referenced this issue Feb 5, 2018
compiler: in range, evaluate array if it has receives or calls
4714afb 
    
    The last change was incomplete, in that it did not evaluate the array
    argument in some cases where it had to be evaluated.  This reuses the
    existing code for checking whether len/cap is constant.
    
    Also clean up the use of _ as the second variable in a for/range,
    which was previously inconsistent depending on whether the statement
    used = or :=.
    
    Updates golang/go#22313
    
    Reviewed-on: https://go-review.googlesource.com/91715


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@257377 138bc75d-0d04-0410-961f-82ee72b054a4
@golang golang locked and limited conversation to collaborators Feb 3, 2019
asiekierka pushed a commit to WonderfulToolchain/gcc-ia16 that referenced this issue May 16, 2022
compiler: don't incorrectly evaluate range variable
31766e6 
    
    The language spec says that in `for i = range x`, in which there is no
    second iteration variable, if len(x) is constant, then x is not
    evaluated.  This only matters when x is an expression that panics but
    whose type is an array type; in such a case, we should not evaluate x,
    since len of any array type is a constant.
    
    Fixes golang/go#22313
    
    Reviewed-on: https://go-review.googlesource.com/91555

From-SVN: r257330
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Gccgo
Development

No branches or pull requests
5 participants
@mdempsky @ianlancetaylor @bcmills @griesemer @gopherbot