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

Implement deprecation warning #318

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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