Check for time.Durations without units

[josharian]

Constant time.Durations ought to have units. 2 * time.Second is far clearer than 2e9 and time.Nanosecond (or 1 * time.Nanosecond) is better than 1. And ever for experienced gophers, it is easy to write time.Sleep(2) and expect a sleep of two seconds.

This was originally filed as https://github.com/golang/go/issues/11137. CL 11002 implements it as a vet check, with some additional restrictions. It is a better fit for lint, where those additional restrictions could be lifted.

[josharian]

I've not written a lint check before, but would be happy to do so if this check is welcome. Many of these could be automatically fixed as well.

[dsymonds]

If the reason why this isn't in vet is that the false positive rate is too high, then it doesn't belong in golint either for the same reason.

This check is framed as a correctness thing, which is vet's bailiwick. I'm confused why it was thought that golint was the better place for it.

[josharian]

IMO, everything that this check found is a stylistic failure. However, most of the code was not buggy. The suggestion of lint as a better home was that vet is about catching bugs whereas lint is about style and readability. For style, the false positive rate is low; for bugs, the false positive rate is high.

[dsymonds]

Can you give a couple of examples of findings that are bad style but not incorrect, outside the flag/time package's tests?

[josharian]

camlistore.googlesource.com/camlistore.git/third_party/labix.org/v2/mgo/cluster.go:536:

        if server == nil {
            // Must have failed the requested tags. Sleep to avoid spinning.
            time.Sleep(1e8)
            continue
        }

100 * time.Millisecond would be better.

github.com/ActiveState/go-dockerclient/testing/server.go:356:

    for {
        time.Sleep(1e6)
        s.cMut.RLock()
        if !container.State.Running {
            s.cMut.RUnlock()
            break
        }
        s.cMut.RUnlock()
    }

time.Millisecond or 1 * time.Millisecond would be better. (Assuming that I've done the math right and that is one millisecond--more evidence that units would be good.)

github.com/armon/go-chord/chord_test.go:127:

func TestCreateShutdown(t *testing.T) {
    // Start the timer thread
    time.After(15)
    conf := fastConf()
    numGo := runtime.NumGoroutine()
    r, err := Create(conf, nil)
    if err != nil {
        t.Fatalf("unexpected err. %s", err)
    }
    r.Shutdown()
    after := runtime.NumGoroutine()
    if after != numGo {
        t.Fatalf("unexpected routines! A:%d B:%d", after, numGo)
    }
}

It is not immediately obvious to a reader whether this is meant to be 15 nanoseconds (I think that it is) or whether it is a bug. 15 * time.Nanosecond would make the intention explicit.

bitbucket.org/telesto/hackurist/hackurist_test.go:40:

func TestWriteHeader(t *testing.T) {

    result := make(chan byte)
    go startServer(1, result)
    time.Sleep(100000)
    c, err := Dial(address, false)
    if err != nil {
        t.Errorf("err: ", err)
    }
    c.writeHeader(1, 3)
    time.Sleep(100000)
    t.Error(ts)
    //          t.Errorf("'%s': wrong value '%t', expected '%t'", tc.name, found, tc.expected)

}

What is 100000, exactly?

github.com/dev-urandom/graft/integration_test.go:132:

func TestA5NodeClusterCanElectLeaderIf2NodesPartitioned(t *testing.T) {
    test := quiz.Test(t)
    c := newCluster(5).withChannelPeers().withTimeouts(2, 9, 9, 9, 9)
    defer c.cleanUp()
    c.startChannelPeers()
    c.partition(4, 5)
    c.startElectionTimers()

    tiktok.Tick(3)

    c.shutdown()

    test.Expect(c.server(1).State).ToEqual(Leader)
}

Are those 2s and 9s nanoseconds or seconds? Ticker of 3? Is that a duration too? (Answer: No. But if other durations came with units, that would be obvious.)

There were 2661 instances flagged from the beginning to github.com/lowercasel, so there are tens of thousands of these, almost all pretty much like the cases above--not bugs, but not particularly clear.

[dsymonds]

Okay, that seems plausible. Go ahead and try to write a lint check; let me know if you get stuck somewhere.

(Also, try to make it fail gracefully if the source code can't be fully typechecked.)

[dsymonds]

For posterity, this was rolled back in 4946cea due to it having too many false positives.

[josharian]

Filed https://github.com/golang/go/issues/11455 to ask for the go/types support that would make this feasible.

[josharian]

See https://github.com/golang/lint/commit/b55059174c9f0cda51e7a12038ba79644d158bf3 for a discussion of the false positives and a very useful comment from Alan Donovan about how to proceed.

[josharian]

I don't seem to be able to re-open this. @dsymonds will you do the honors?

[andybons]

Hey Josh — As part of tending to the repo, we’ve defined a scope for what changes we’ll be considering.

If you like, you can email golang-dev to discuss whether this should be in CodeReviewComments, but since it isn’t right now, I’m going to close this issue.