gordonklaus / ineffassign

Detect ineffectual assignments in Go code.
MIT License
394 stars 22 forks source link

it would be nice if ineffassign detected cases within a for loop. #17

Closed james-lawrence closed 7 years ago

james-lawrence commented 7 years ago

noticed this the other day, and was surprised the linter didn't pick it up:

type X struct {
    A int
}

xs := []X{{A:1},{A:2},{A:3}}

for _, x := range xs {
  // would expect to see a warning here.
  x.A = 5
}
log.Println(xs)
gordonklaus commented 7 years ago

It would be nice to catch this case but I don't think it will happen any time soon. I'll add a note to the README regarding such limitations.

The main reason this ineffectual assignment is not detected is not because of the for loop but simply because the tool does not track struct fields. It does not track struct fields because doing so is complicated by the lack of type information, of which none is considered.

The tool assumes, conservatively, that the x in a selector expression x.A might be a pointer. Therefore, assignments to x.A might have an effect elsewhere in the program – where the value pointed at by x might be reachable in another context (e.g. another variable). To determine whether the value is used in another context would involve tracing the flow of data and considering type information.

As a concrete example, change your provided code so that xs is a []*X. Suddenly the assignment x.A = 5 becomes effectual.

Type- and pointer-analysis may be beyond the desired scope of this tool. They could impose a significant run-time cost while providing only a small benefit.

A more complete solution to the problem of ineffectual assignments should use the go/pointer package, which is, notably, quite expensive to run.

james-lawrence commented 7 years ago

I'm aware of the pointer value case. you wouldn't need to track fields just the type of X in the selector expession, not sure how expensive that'd be for function scoping, I could see it getting expensive for globals...

I may take a peek at what is going on later.