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

proposal: spec: disallow taking the addresses of loop variables #20725

Closed
halleknast opened this Issue Jun 19, 2017 · 13 comments

Comments

Projects
None yet
6 participants
@halleknast

halleknast commented Jun 19, 2017

Consider a range loop:

for k, v := range vals {
  // ...
}

Since the variables k and v are not redefined for each iteration of the loop, their addresses are the same in each iteration. These addresses are therefore very unlikely to be useful, and the following more explicit alternatives are IMO clearer and more robust to refactorings anyway:

  • Instead of overwriting the value by passing its address to a helper function, define a new variable instead:

    for _, v := range vals {
      // Alternative to `writeStuffTo(&v)`:
      var v2 typeOfV
      writeStuffTo(&v2)
    }
    
  • In order to reference the object in the current iteration of the loop, copy it or access the iterated slice directly:

    for i, v := range vals {
      // Calling `sendSomewhere(&v)` here would likely be a bug
      v := v
      sendSomewhere(&v)       // Alternative 1: Use copied value
      sendSomewhere(&vals[i]) // Alternative 2: Take address of currently iterated value
    }
    

I would argue that taking the addresses of range variables is so rarely useful that it would be good to require the intent to be made explicit as follows:

var v typeOfV
vp := &v
for _, v = range vals {
	// Use `vp` if you must...
}
@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Jun 19, 2017

Member

This looks like a dup of #16520. The examples are slightly different (go/defer versus &), but I believe the root problem is the same.

Member

mvdan commented Jun 19, 2017

This looks like a dup of #16520. The examples are slightly different (go/defer versus &), but I believe the root problem is the same.

@halleknast

This comment has been minimized.

Show comment
Hide comment
@halleknast

halleknast Jun 19, 2017

@mvdan I actually wrote this as a comment to that issue before submitting this one instead :)

While the problems do overlap, #16520 is about capturing, which is not the issue here. I think the main difference is that there are many legitimate reasons to capture a loop variable inside a function literal (at least with the go statement).

As I try to argue here, taking a range variable address is much more unlikely to be useful - in part because you can capture the original variable. Also, fixing "false positive" cases is trivial using the mentioned replacements. This will not be the case if you generalize the problem.

halleknast commented Jun 19, 2017

@mvdan I actually wrote this as a comment to that issue before submitting this one instead :)

While the problems do overlap, #16520 is about capturing, which is not the issue here. I think the main difference is that there are many legitimate reasons to capture a loop variable inside a function literal (at least with the go statement).

As I try to argue here, taking a range variable address is much more unlikely to be useful - in part because you can capture the original variable. Also, fixing "false positive" cases is trivial using the mentioned replacements. This will not be the case if you generalize the problem.

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Jun 19, 2017

Member

Ah, I think I see what you're after then. I take it that when you say "taking an address" you mean whenever the compiler takes a variable's address, even if it wasn't explicit in the code via & - such as in a go func.... Is this correct?

Member

mvdan commented Jun 19, 2017

Ah, I think I see what you're after then. I take it that when you say "taking an address" you mean whenever the compiler takes a variable's address, even if it wasn't explicit in the code via & - such as in a go func.... Is this correct?

@halleknast

This comment has been minimized.

Show comment
Hide comment
@halleknast

halleknast Jun 19, 2017

As in the examples, I do mean it to be explicit in the code with &. Note that none of the examples use captures, which is why I think this is a different issue. Also, the other issue is much more difficult to solve than this one is (because the amount of legitimate use cases is smaller).

halleknast commented Jun 19, 2017

As in the examples, I do mean it to be explicit in the code with &. Note that none of the examples use captures, which is why I think this is a different issue. Also, the other issue is much more difficult to solve than this one is (because the amount of legitimate use cases is smaller).

@rsc rsc added the Go2 label Jun 19, 2017

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jun 19, 2017

Contributor

There are no warnings but we could think about disallowing this entirely in Go 2. Leaving for that.

Contributor

rsc commented Jun 19, 2017

There are no warnings but we could think about disallowing this entirely in Go 2. Leaving for that.

@rsc rsc changed the title from cmd/vet: warn about taking the addresses of loop variables to proposal: spec: disallow taking the addresses of loop variables Jun 19, 2017

@gopherbot gopherbot added this to the Proposal milestone Jun 19, 2017

@gopherbot gopherbot added the Proposal label Jun 19, 2017

@halleknast

This comment has been minimized.

Show comment
Hide comment
@halleknast

halleknast Jun 19, 2017

@rsc I used the word "warn" because it seemed to me to be conventionally used by other issues on go vet. I did mean "rejected" by go vet.

I have a simple patch that I wanted to try and submit tonight (UTC+1 time) :)

halleknast commented Jun 19, 2017

@rsc I used the word "warn" because it seemed to me to be conventionally used by other issues on go vet. I did mean "rejected" by go vet.

I have a simple patch that I wanted to try and submit tonight (UTC+1 time) :)

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jun 19, 2017

Contributor

It's too invasive to reject in vet. It fails the "precision" test in cmd/vet/README.

Contributor

rsc commented Jun 19, 2017

It's too invasive to reject in vet. It fails the "precision" test in cmd/vet/README.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Jun 19, 2017

Contributor

Not being able to take the address of an addressable thing turns the "addressable" concept into something very weird. Also:

for _, bigValue := range bigValues {
        foo(&bigValue)
}

is completely reasonable code avoiding making another copy of bigValue when foo just reads its argument.

Contributor

cznic commented Jun 19, 2017

Not being able to take the address of an addressable thing turns the "addressable" concept into something very weird. Also:

for _, bigValue := range bigValues {
        foo(&bigValue)
}

is completely reasonable code avoiding making another copy of bigValue when foo just reads its argument.

@halleknast

This comment has been minimized.

Show comment
Hide comment
@halleknast

halleknast Jun 19, 2017

@rsc How is it determined if it fails the test? I've tried to run my patched tool on a bunch of random projects and found mostly cases which are definitely "worth examining" as the precision test states. Maybe the test could be disabled by default if it becomes too noisy in an existing project?

@cznic As bigValue is already a copy of a value in bigValues, I think this is actually a case where he check would have been helpful to make you not copy the value:

for idx, _ := range bigValues {
        foo(&bigValues[idx])
}

Alternatively, you would just have to define bigValue (and a pointer to it) outside of the loop as suggested earlier.

halleknast commented Jun 19, 2017

@rsc How is it determined if it fails the test? I've tried to run my patched tool on a bunch of random projects and found mostly cases which are definitely "worth examining" as the precision test states. Maybe the test could be disabled by default if it becomes too noisy in an existing project?

@cznic As bigValue is already a copy of a value in bigValues, I think this is actually a case where he check would have been helpful to make you not copy the value:

for idx, _ := range bigValues {
        foo(&bigValues[idx])
}

Alternatively, you would just have to define bigValue (and a pointer to it) outside of the loop as suggested earlier.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Jun 19, 2017

Contributor

There is plenty of correct code it would disallow. That's not OK. See @cznic's comment for one example. I am sure there are more, even if not in your own code base. That's why it's really a spec change.

Contributor

rsc commented Jun 19, 2017

There is plenty of correct code it would disallow. That's not OK. See @cznic's comment for one example. I am sure there are more, even if not in your own code base. That's why it's really a spec change.

@cznic

This comment has been minimized.

Show comment
Hide comment
@cznic

cznic Jun 19, 2017

Contributor

foo(&bigValues[idx])

Iff bigValues is a slice/array.

Contributor

cznic commented Jun 19, 2017

foo(&bigValues[idx])

Iff bigValues is a slice/array.

@halleknast

This comment has been minimized.

Show comment
Hide comment
@halleknast

halleknast Jun 19, 2017

The most common false positive I found so far is return &loopVar, which would be easy to allow. I will try to gather some data on the amount of false positives.

halleknast commented Jun 19, 2017

The most common false positive I found so far is return &loopVar, which would be easy to allow. I will try to gather some data on the amount of false positives.

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Feb 20, 2018

Contributor

There is nothing wrong with taking the address of a range variable if you don't save the pointer across loop iterations. And, this restriction doesn't fix the problem of referring to a range variable in a closure, which is if anything more likely to be problematic. Closing.

Contributor

ianlancetaylor commented Feb 20, 2018

There is nothing wrong with taking the address of a range variable if you don't save the pointer across loop iterations. And, this restriction doesn't fix the problem of referring to a range variable in a closure, which is if anything more likely to be problematic. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment