diff --git a/lint.go b/lint.go index fb47da00..1d19a52f 100644 --- a/lint.go +++ b/lint.go @@ -16,6 +16,7 @@ import ( "go/printer" "go/token" "go/types" + "log" "regexp" "sort" "strconv" @@ -24,6 +25,7 @@ import ( "unicode/utf8" "golang.org/x/tools/go/gcexportdata" + "golang.org/x/tools/go/loader" ) const styleGuideBase = "https://golang.org/wiki/CodeReviewComments" @@ -140,6 +142,8 @@ type pkg struct { typesPkg *types.Package typesInfo *types.Info + prog *loader.Program + // sortable is the set of types in the package that implement sort.Interface. sortable map[string]bool // main is whether this is a "main" package. @@ -168,6 +172,37 @@ func (p *pkg) lint() []Problem { */ } + // TODO(stapelberg): skip the p.typeCheck earlier in favor of using what the + // loader gives us. + var files []*ast.File + for _, f := range p.files { + files = append(files, f.f) + } + + conf := loader.Config{ + Fset: p.fset, + ParserMode: parser.ParseComments, // for lintDeprecated + CreatePkgs: []loader.PkgSpec{ + {Files: files}, + }, + TypeChecker: types.Config{ + // Enable FakeImportC so that we can load ad-hoc packages which + // import "C". + FakeImportC: true, + Error: func(err error) {}, // ignore errors + }, + // Skip type-checking the function bodies of dependencies: + TypeCheckFuncBodies: func(path string) bool { + return path == files[0].Name.Name + }, + } + prog, err := conf.Load() + if err != nil { + log.Printf("loading failed: %v", err) + return p.problems + } + p.prog = prog + p.scanSortable() p.main = p.isMain() @@ -210,6 +245,7 @@ func (f *file) lint() { f.lintTimeNames() f.lintContextKeyTypes() f.lintContextArgs() + f.lintDeprecated() } type link string @@ -1466,6 +1502,64 @@ func (f *file) lintContextArgs() { }) } +func docText(n ast.Node) string { + switch n := n.(type) { + case *ast.FuncDecl: + return n.Doc.Text() + + case *ast.Field: + return n.Doc.Text() + + case *ast.ValueSpec: + return n.Doc.Text() + + case *ast.TypeSpec: + return n.Doc.Text() + + default: + return "" + } +} + +// deprecated returns the deprecation message of a doc comment, or "". +func deprecated(doc string) string { + paragraphs := strings.Split(doc, "\n\n") + last := paragraphs[len(paragraphs)-1] + if !strings.HasPrefix(last, "Deprecated: ") { + return "" + } + return strings.Replace(strings.TrimPrefix(last, "Deprecated: "), "\n", " ", -1) +} + +func (f *file) lintDeprecated() { + f.walk(func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok { + return true + } + + obj := f.pkg.prog.Created[0].ObjectOf(sel.Sel) + if obj == nil || obj.Pkg() == nil { + return false + } + // TODO(stapelberg): verify obj is in a different package, add a test + + _, path, _ := f.pkg.prog.PathEnclosingInterval(obj.Pos(), obj.Pos()) + // Expectation: + // path[0] holds an *ast.Ident + // path[1] holds the declaration we are interested in + if len(path) <= 2 { + return false + } + + if dep := deprecated(docText(path[1])); dep != "" { + f.errorf(sel, 1.0, category("deprecation"), "deprecated: %s", dep) + } + + return false + }) +} + // receiverType returns the named type of the method receiver, sans "*", // or "invalid-type" if fn.Recv is ill formed. func receiverType(fn *ast.FuncDecl) string { diff --git a/testdata/broken.go b/testdata/broken.go deleted file mode 100644 index 6543775c..00000000 --- a/testdata/broken.go +++ /dev/null @@ -1,9 +0,0 @@ -// Test of code that is malformed, but accepted by go/parser. -// See https://golang.org/issue/11271 for discussion. -// OK - -// Package pkg ... -package pkg - -// Foo is a method with a missing receiver. -func () Foo() {} diff --git a/testdata/context.go b/testdata/context.go index 501c69b4..5911d192 100644 --- a/testdata/context.go +++ b/testdata/context.go @@ -12,7 +12,7 @@ func x(ctx context.Context) { // ok } // A proper context.Context location -func x(ctx context.Context, s string) { // ok +func x2(ctx context.Context, s string) { // ok } // An invalid context.Context location @@ -20,5 +20,5 @@ func y(s string, ctx context.Context) { // MATCH /context.Context should be the } // An invalid context.Context location with more than 2 args -func y(s string, r int, ctx context.Context, x int) { // MATCH /context.Context should be the first parameter.*/ +func y2(s string, r int, ctx context.Context, x int) { // MATCH /context.Context should be the first parameter.*/ } diff --git a/testdata/contextkeytypes.go b/testdata/contextkeytypes.go index 68dc36a4..6433e625 100644 --- a/testdata/contextkeytypes.go +++ b/testdata/contextkeytypes.go @@ -34,5 +34,4 @@ func contextKeyTypeTests() { context.WithValue(c, complex128(1i), "bar") // MATCH /should not use basic type complex128 as key in context.WithValue/ context.WithValue(c, ctxKey{}, "bar") // ok context.WithValue(c, &ctxKey{}, "bar") // ok - context.WithValue(c, invalid{}, "bar") // ok } diff --git a/testdata/deprecated.go b/testdata/deprecated.go new file mode 100644 index 00000000..4504da0c --- /dev/null +++ b/testdata/deprecated.go @@ -0,0 +1,10 @@ +// Test for deprecation warnings. + +// Package main ... +package main + +import "net/http/httputil" + +func main() { + httputil.NewClientConn(nil, nil) // MATCH /deprecated/ +} diff --git a/testdata/errorf.go b/testdata/errorf.go index 72161f2b..386aab2d 100644 --- a/testdata/errorf.go +++ b/testdata/errorf.go @@ -26,13 +26,13 @@ func f(x int) error { func TestF(t *testing.T) error { x := 1 if x > 10 { - return t.Error(fmt.Sprintf("something %d", x)) // MATCH /should replace.*t\.Error\(fmt\.Sprintf\(\.\.\.\)\).*t\.Errorf\(\.\.\.\)/ + t.Error(fmt.Sprintf("something %d", x)) // MATCH /should replace.*t\.Error\(fmt\.Sprintf\(\.\.\.\)\).*t\.Errorf\(\.\.\.\)/ } if x > 5 { - return t.Error(g("blah")) // ok + t.Error(g("blah")) // ok } if x > 4 { - return t.Error("something else") // ok + t.Error("something else") // ok } return nil } diff --git a/testdata/receiver-names.go b/testdata/receiver-names.go index 6e513279..65b9275a 100644 --- a/testdata/receiver-names.go +++ b/testdata/receiver-names.go @@ -44,6 +44,3 @@ type multiError struct{} func (me multiError) f8() { } - -// Regression test for a panic caused by ill-formed receiver type. -func (recv []*x.y) f() diff --git a/testdata/unexp-return.go b/testdata/unexp-return.go index 7fc9769f..cc570f87 100644 --- a/testdata/unexp-return.go +++ b/testdata/unexp-return.go @@ -3,8 +3,6 @@ // Package foo ... package foo -import () - type hidden struct{} // Exported returns a hidden type, which is annoying. @@ -14,9 +12,11 @@ func Exported() hidden { // MATCH /Exported.*unexported.*hidden/ // ExpErr returns a builtin type. func ExpErr() error { // ok + return nil } func (hidden) ExpOnHidden() hidden { // ok + return hidden{} } // T is another test type. diff --git a/testdata/var-decl.go b/testdata/var-decl.go index 9201d18c..67169272 100644 --- a/testdata/var-decl.go +++ b/testdata/var-decl.go @@ -7,7 +7,6 @@ import ( "fmt" "io" "net/http" - "nosuchpkg" // export data unavailable "os" ) @@ -75,10 +74,6 @@ var out2 io.Writer = newWriter() // MATCH /should omit.*io\.Writer/ func newWriter() io.Writer { return nil } -// We don't figure out the true types of LHS and RHS here, -// so we suppress the check. -var ni nosuchpkg.Interface = nosuchpkg.NewConcrete() - var y string = q(1).String() // MATCH /should.*string/ type q int