Skip to content
This repository has been archived by the owner on May 9, 2021. It is now read-only.

Check for time.Durations without units #130

Closed
josharian opened this issue Jun 15, 2015 · 11 comments
Closed

Check for time.Durations without units #130

josharian opened this issue Jun 15, 2015 · 11 comments

Comments

@josharian
Copy link
Contributor

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 golang/go#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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@josharian
Copy link
Contributor Author

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
Copy link
Contributor

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.)

josharian added a commit to josharian/lint that referenced this issue Jun 16, 2015
josharian added a commit to josharian/lint that referenced this issue Jun 19, 2015
josharian added a commit to josharian/lint that referenced this issue Jun 22, 2015
@dsymonds
Copy link
Contributor

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

@josharian
Copy link
Contributor Author

Filed golang/go#11455 to ask for the go/types support that would make this feasible.

@josharian
Copy link
Contributor Author

See b550591 for a discussion of the false positives and a very useful comment from Alan Donovan about how to proceed.

@josharian
Copy link
Contributor Author

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

@andybons
Copy link
Member

andybons commented Mar 2, 2018

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.

@andybons andybons closed this as completed Mar 2, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants