Skip to content

Commit

Permalink
Implement deprecation warning
Browse files Browse the repository at this point in the history
  • Loading branch information
stapelberg committed Aug 17, 2017
1 parent c5fb716 commit 6bea9ed
Show file tree
Hide file tree
Showing 9 changed files with 111 additions and 25 deletions.
94 changes: 94 additions & 0 deletions lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"go/printer"
"go/token"
"go/types"
"log"
"regexp"
"sort"
"strconv"
Expand All @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -210,6 +245,7 @@ func (f *file) lint() {
f.lintTimeNames()
f.lintContextKeyTypes()
f.lintContextArgs()
f.lintDeprecated()
}

type link string
Expand Down Expand Up @@ -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 {
Expand Down
9 changes: 0 additions & 9 deletions testdata/broken.go

This file was deleted.

4 changes: 2 additions & 2 deletions testdata/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ 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
func y(s string, ctx context.Context) { // MATCH /context.Context should be the first parameter.*/
}

// 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.*/
}
1 change: 0 additions & 1 deletion testdata/contextkeytypes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
10 changes: 10 additions & 0 deletions testdata/deprecated.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// Test for deprecation warnings.

// Package main ...
package main

import "net/http/httputil"

func main() {
httputil.NewClientConn(nil, nil) // MATCH /deprecated/
}
6 changes: 3 additions & 3 deletions testdata/errorf.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
3 changes: 0 additions & 3 deletions testdata/receiver-names.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
4 changes: 2 additions & 2 deletions testdata/unexp-return.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@
// Package foo ...
package foo

import ()

type hidden struct{}

// Exported returns a hidden type, which is annoying.
Expand All @@ -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.
Expand Down
5 changes: 0 additions & 5 deletions testdata/var-decl.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"io"
"net/http"
"nosuchpkg" // export data unavailable
"os"
)

Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 6bea9ed

Please sign in to comment.