x/text: potential Ineffective Assignments in a few packages

[davewichers]

Background: I'm a Software Security consultant, but not a Go expert. Just learning Go.

What version of Go are you using (go version)?

$ go version
 go version go1.13.1 darwin/amd64 (i.e., MacOS)

Does this issue reproduce with the latest release?

I believe I'm using the latest version of Go.

What did you do?

I ran a free Go tool called ineffassign (from: https://github.com/gordonklaus/ineffassign) and noticed it pointed out some issues in the go libraries themselves. I asked Gordon Klaus to review them and his response was: "Some of those are harmless (if sloppy), but others look like they might be real bugs. I think they're worth reporting at golang.org/issues."

What did you expect to see?

No issues pointed out in Go libraries by this tool.

What did you see instead?

I ran ineffassign on a Go app I'm doing a security review for and noticed these results as part of the output:

mpapp/vendor/golang.org/x/text/collate/build/colelem.go:205:5: ineffectual assignment to p mpapp/vendor/golang.org/x/text/collate/maketables.go:449:4: ineffectual assignment to d mpapp/vendor/golang.org/x/text/transform/transform.go:146:5: ineffectual assignment to err mpapp/vendor/golang.org/x/text/unicode/bidi/core.go:489:5: ineffectual assignment to preceedingCharacterType mpapp/vendor/golang.org/x/text/unicode/bidi/core.go:565:4: ineffectual assignment to i mpapp/vendor/golang.org/x/text/unicode/bidi/core.go:644:4: ineffectual assignment to i mpapp/vendor/golang.org/x/text/unicode/cldr/resolve.go:486:7: ineffectual assignment to err

These look like part of Go itself, so I find them interesting. Do you think these are real issues that need to be reported to the maintainers of Go? Or false positives? (Gordon said report them, so here they are.)

[bradfitz]

/cc @mpvl

[filewalkwithme]

Hi, if it's possible, I would like to work on this.

I had run ineffassign on x/text again an found more results. See the the updated list below.

Almost all issues can be easily fixed by simply removing or refactoring the ineffectual assignments clauses.

However, there are two entries that may actually need a deeper look:

• ~/go/src/github.com/golang/text/unicode/bidi/core.go:575:4: ineffectual assignment to i
• ~/go/src/github.com/golang/text/unicode/bidi/core.go:654:4: ineffectual assignment to i

These lines are part of a port of the reference implementation of the Bidi Parentheses Algorithm (found here: https://www.unicode.org/Public/PROGRAMS/BidiReferenceJava/BidiReference.java).

The original code uses basic FOR loops to iterate over an array, sometimes modifying the index inside the loop. At the other hand, the ported code from x/text uses a range FOR loop, and this may cause the algorithm to behave differently than expected. Example:

// Original Java Implementation:
for (int i = 0; i < length; ++i) {
    byte t = types[i];
    ...
    // skip over run of (former) neutrals
    i = runlimit;
}

// x/text port, using range instead basic loop:
for i, t := range s.types {
    ...
    // skip over run of (former) neutrals
    i = runlimit;   <------ INEFFECTUAL.
}

If it's ok, I can send a PR right away, fixing all issues.

Here is the updated list of entries generated by inefassign

~/go/src/github.com/golang/text/transform/transform.go:146:5: ineffectual assignment to err
~/go/src/github.com/golang/text/internal/number/decimal.go:381:3: ineffectual assignment to mult
~/go/src/github.com/golang/text/feature/plural/message.go:147:2: ineffectual assignment to form
~/go/src/github.com/golang/text/unicode/cldr/resolve.go:486:7: ineffectual assignment to err
~/go/src/github.com/golang/text/message/pipeline/pipeline.go:415:2: ineffectual assignment to args
~/go/src/github.com/golang/text/cmd/gotext/common.go:42:2: ineffectual assignment to args
~/go/src/github.com/golang/text/collate/build/colelem.go:205:5: ineffectual assignment to p
~/go/src/github.com/golang/text/unicode/bidi/core.go:499:5: ineffectual assignment to precedingCharacterType
~/go/src/github.com/golang/text/unicode/bidi/core.go:575:4: ineffectual assignment to i
~/go/src/github.com/golang/text/unicode/bidi/core.go:654:4: ineffectual assignment to i
~/go/src/github.com/golang/text/language/display/display.go:397:10: ineffectual assignment to reg
~/go/src/github.com/golang/text/encoding/encoding_test.go:56:3: ineffectual assignment to enc
~/go/src/github.com/golang/text/message/pipeline/pipeline_test.go:150:8: ineffectual assignment to err
~/go/src/github.com/golang/text/unicode/norm/normalize_test.go:943:2: ineffectual assignment to out 

[davewichers]

@filewalkwithme - thanks for volunteering to work on this ..... 18 month old issue ...... :-)

[gopherbot]

Change https://go.dev/cl/727080 mentions this issue: net: fix some ineffectual assignments reported by ineffassign

[gopherbot]

Change https://go.dev/cl/728860 mentions this issue: internal: fix unused errors reported by ineffassign

[gopherbot]

Change https://go.dev/cl/731040 mentions this issue: cmd: fix unused errors reported by ineffassign