Add unitless duration lint check.
Fixes #130.
Signed-off-by: David Symonds <dsymonds@golang.org>
diff --git a/lint.go b/lint.go
index 7de99bc..11f6ed7 100644
--- a/lint.go
+++ b/lint.go
@@ -21,6 +21,7 @@
"unicode"
"unicode/utf8"
+ "golang.org/x/tools/go/exact"
"golang.org/x/tools/go/gcimporter"
"golang.org/x/tools/go/types"
)
@@ -187,6 +188,7 @@
f.lintErrorReturn()
f.lintUnexportedReturn()
f.lintTimeNames()
+ f.lintDurationUnits()
}
type link string
@@ -1396,6 +1398,70 @@
})
}
+func (f *file) lintDurationUnits() {
+ f.walk(func(node ast.Node) bool {
+ e, ok := node.(ast.Expr)
+ if !ok {
+ return true
+ }
+
+ tv := f.pkg.typesInfo.Types[e]
+ if !f.pkg.isNamedType(tv.Type, "time", "Duration") {
+ return true
+ }
+ if tv.Value == nil {
+ return true
+ }
+ // OK for 0 to be unitless; non-constants are OK.
+ if x, ok := exact.Int64Val(tv.Value); !ok || x == 0 {
+ return true
+ }
+
+ // e has a constant value. What matters now is how it was defined.
+ // Its definition might be spread across many expressions:
+ //
+ // const (
+ // a = iota
+ // b
+ // c = b * time.Second
+ // e = c * 10
+ // )
+ //
+ // Rather than attempt to reconstruct e's origins,
+ // just handle the common cases.
+ switch e.(type) {
+ default:
+ // Assume that any non-literal, non-call expression is ok.
+ // In particular, allow 3 * time.Second, which is an *ast.BinExpr.
+ // This means that we fail to flag cases like
+ // time.Duration(15) + time.Duration(30) and
+ // time.Sleep(x) where 'const x = 10'.
+ return false
+ case *ast.CallExpr:
+ // Descend into call expressions in order to catch time.Duration(15).
+ return true
+ case *ast.BasicLit:
+ // A basic literal cannot possibly use time package constants.
+ }
+
+ f.errorf(e, 0.8, category("time"), `time.Duration expression "%v" should use time package constants`, f.render(e))
+ // TODO: Generate a ReplacementLine with an appropriately sized unit.
+ // For example, 3e9 should become 3 * time.Second. The unit part is easy.
+ // The challenge is figuring out exactly what else is on the line
+ // and handling the case in which there are multiple expressions
+ // to replace on the same line.
+ // The latter opens up API questions. If there are multiple
+ // problems on the same line, should they all generate identical
+ // replacements, or one replacement per problem?
+ // The former would mean that this linter has to maintain state across
+ // expressions.
+ // Also, the test framework does not yet have a means to represent
+ // multiple matches or multiple replacements on a single line.
+
+ return true
+ })
+}
+
func receiverType(fn *ast.FuncDecl) string {
switch e := fn.Recv.List[0].Type.(type) {
case *ast.Ident:
diff --git a/testdata/time.go b/testdata/time.go
index 9271acb..44cda9d 100644
--- a/testdata/time.go
+++ b/testdata/time.go
@@ -11,3 +11,32 @@
var rpcTimeoutMsec = flag.Duration("rpc_timeout", 100*time.Millisecond, "some flag") // MATCH /Msec.*\*time.Duration/
var timeoutSecs = 5 * time.Second // MATCH /Secs.*time.Duration/
+
+var delay = flag.Duration("delay", 5, "wait impatiently") // MATCH /time.*expression.*"5".*constants/
+
+func f(...time.Duration) {}
+
+var (
+ _ = time.Duration(15) // MATCH /time.*expression.*"15".*constants/
+ _ = time.Duration(15) + time.Duration(30) // Not caught, but rare in real code; see comments in lintDurationUnits
+ _ = []time.Duration{
+ 1, // MATCH /time.*expression.*"1".*constants/
+ 10, // MATCH /time.*expression.*"10".*constants/
+ 1 * time.Nanosecond,
+ 10 * 10 * 10 * 10,
+ time.Hour,
+ 0, // 0 is OK
+ }
+ abc = f(17) // MATCH /time.*expression.*"17".*constants/
+)
+
+const x = 10
+
+func g() {
+ time.Sleep(
+ 12, // MATCH /time.*expression.*"12".*constants/
+ )
+ time.Sleep(x) // Not caught; see comments in lintDurationUnits
+ _ = f(1, // MATCH /time.*expression.*"1".*constants/
+ 1e12) // MATCH /time.*expression.*"1e12".*constants/
+}
