gordonklaus / ineffassign

Detect ineffectual assignments in Go code.
MIT License
398 stars 23 forks source link

nil-assigned value does not always need to be used #10

Closed gyuho closed 8 years ago

gyuho commented 8 years ago
package main

import "fmt"

func main() {
    var kvs [][]string
    fmt.Println(kvs)
    kvs = nil
}

ineffassign ineffassign.go
/home/gyuho/ineffassign.go:8:2: kvs assigned and not used

We assign nil to kvs, which is already used in previous line. And the tool assumes it's a new assignment. Could you fix this?

Thanks.

gordonklaus commented 8 years ago

Your assignment of kvs to nil is pointless because kvs is not used after the assignment.

Perhaps you think that this assignment to nil has some effect on your program, such as allowing garbage to be collected. It does not. As soon as kvs goes out of scope, its value becomes eligible for collection. There is no need to assign to nil here.

mark-kubacki commented 5 years ago

I feel I have an instance of this.

The intention here is indeed to make the rather large map available for the garbage collector. This time end-of-scope is not the saviour, unless perhaps Go tracks lifetime within a function.

type T struct{} // but indeed something large

func() {
  kvs := make(map[int64]T{})
  // kvs is being filled and grows

  lst := make([]T{}, 0, len(kvs))
  for … := range kvs {…} // map to list
  // forgo kvs -- this triggers ineffassign
  kvs = nil

  // save lst to disk (takes a long time)
  wg := new(sync.WaitGroup)
  wg.Add(2)
  filestore.SaveAsXML(wg, lst, …)
  filestore.SaveAsJSON(wg, list, …)
  wg.Wait() // best effort, we don't care for errors

  return
}
gordonklaus commented 5 years ago

@wmark I believe Go does track variable lifetime within a function. But I'm not certain of that, you should measure. In any case, it should. You can also introduce a scope (just wrap some lines in a {} block) to make the lifetime explicit.

mark-kubacki commented 5 years ago

Thanks for looking into this.

I do indeed am aware of and use custom scoping. Which comes at a cost you don't always want to pay, and not always is a non-overlapping non-awkward scoping possible.

As per the spec variable lifetime of above example ends when the function body ends; which makes kvs = nil a sensible operation and not ineffective.

gordonklaus commented 5 years ago

The spec talks about scopes, not lifetimes. Scopes restrict where identifiers can be used, whereas lifetimes determine when memory becomes garbage. Since the spec says nothing about lifetimes, it is up to the compiler writers to decide exactly how that works. I seem to recall them solving this exact issue some years ago, but I am unable to find it now.

I agree that assigning nil is the easiest way to explicitly say "I don't need this memory any more". But, as I said, it should be unnecessary.

In fact, I've now run some tests and verified my suspicion that the garbage collector does indeed mark memory as garbage as soon as possible, without unnecessarily waiting until the end of a scope. I urge you to do your own measurements if this matters to you.

mark-kubacki commented 5 years ago

I can confirm lifetime tracking within scopes is in in Go 1.11 (implemented in Go, not the GCC flavour).

Thanks, and apologies.