Add unitless duration lint check.

Fixes #130.

Signed-off-by: David Symonds <dsymonds@golang.org>

lint.go

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:

testdata/time.go

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/
+}