Skip to content

Commit

Permalink
Fix linting of preprocessed files
Browse files Browse the repository at this point in the history
Preprocessed files like .qtpl.go quicktemplate Go files can have
//line directives. They map to a source .qtpl file.
This commit fixes linting of such files:
1. don't fail on AST cache loading
2. output Go filename not .qtpl or similar

Also, here we update golint to the upstream version.

Relates: #316, #466, #467, #468
  • Loading branch information
jirfag committed Apr 20, 2019
1 parent eed661b commit 397ba7f
Show file tree
Hide file tree
Showing 17 changed files with 274 additions and 43 deletions.
4 changes: 4 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@ language: go
go:
- 1.11.x
- 1.12.x

before_script:
- go get github.com/valyala/quicktemplate

script: make check_generated test

after_success:
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ require (
github.com/golangci/gofmt v0.0.0-20181105071733-0b8337e80d98
github.com/golangci/gosec v0.0.0-20180901114220-66fb7fc33547
github.com/golangci/ineffassign v0.0.0-20180808204949-2ee8f2867dde
github.com/golangci/lint-1 v0.0.0-20180610141402-4bf9709227d1
github.com/golangci/lint-1 v0.0.0-20180610141402-ee948d087217
github.com/golangci/maligned v0.0.0-20180506175553-b1d89398deca
github.com/golangci/misspell v0.0.0-20180809174111-950f5d19e770
github.com/golangci/prealloc v0.0.0-20180630174525-215b22d4de21
Expand Down
5 changes: 3 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@ github.com/golangci/gosec v0.0.0-20180901114220-66fb7fc33547 h1:qMomh8bv+kDazm1d
github.com/golangci/gosec v0.0.0-20180901114220-66fb7fc33547/go.mod h1:0qUabqiIQgfmlAmulqxyiGkkyF6/tOGSnY2cnPVwrzU=
github.com/golangci/ineffassign v0.0.0-20180808204949-2ee8f2867dde h1:qEGp3ZF1Qw6TkbWKn6GdJ12Ssu/CpJBaBcJ4hrUjrSo=
github.com/golangci/ineffassign v0.0.0-20180808204949-2ee8f2867dde/go.mod h1:e5tpTHCfVze+7EpLEozzMB3eafxo2KT5veNg1k6byQU=
github.com/golangci/lint-1 v0.0.0-20180610141402-4bf9709227d1 h1:PHK2kIh21Zt4IcG0bBRzQwEDVKF64LnkoSXnm8lfJUk=
github.com/golangci/lint-1 v0.0.0-20180610141402-4bf9709227d1/go.mod h1:/X8TswGSh1pIozq4ZwCfxS0WA5JGXguxk94ar/4c87Y=
github.com/golangci/lint-1 v0.0.0-20180610141402-ee948d087217 h1:r7vyX+SN24x6+5AnpnrRn/bdwBb7U+McZqCHOVtXDuk=
github.com/golangci/lint-1 v0.0.0-20180610141402-ee948d087217/go.mod h1:66R6K6P6VWk9I95jvqGxkqJxVWGFy9XlDwLwVz1RCFg=
github.com/golangci/maligned v0.0.0-20180506175553-b1d89398deca h1:kNY3/svz5T29MYHubXix4aDDuE3RWHkPvopM/EDv/MA=
github.com/golangci/maligned v0.0.0-20180506175553-b1d89398deca/go.mod h1:tvlJhZqDe4LMs4ZHD0oMUlt9G2LWuDGoisJTBzLMV9o=
github.com/golangci/misspell v0.0.0-20180809174111-950f5d19e770 h1:EL/O5HGrF7Jaq0yNhBLucz9hTuRzj2LdwGBOaENgxIk=
Expand Down Expand Up @@ -171,6 +171,7 @@ golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGm
golang.org/x/tools v0.0.0-20181117154741-2ddaf7f79a09/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20181205014116-22934f0fdb62/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20190121143147-24cd39ecf745/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20190311212946-11955173bddd/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
golang.org/x/tools v0.0.0-20190420000508-685fecacd0a0 h1:pa1CyBALPFjblgkNQp7T7gEcFcG/GOG5Ck8IcnSVWGs=
golang.org/x/tools v0.0.0-20190420000508-685fecacd0a0/go.mod h1:LCzVGOaR6xXOjkQ3onu1FJEFr0SW1gC7cKk1uF8kGRs=
gopkg.in/airbrake/gobrake.v2 v2.0.9 h1:7z2uVWwn7oVeeugY1DtlPAy5H+KYgB1KeKTnqjNatLo=
Expand Down
48 changes: 25 additions & 23 deletions pkg/lint/astcache/astcache.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"go/parser"
"go/token"
"path/filepath"
"time"

"golang.org/x/tools/go/packages"

Expand Down Expand Up @@ -107,34 +106,37 @@ func LoadFromPackages(pkgs []*packages.Package, log logutils.Log) (*Cache, error
return c, nil
}

func (c *Cache) loadFromPackage(pkg *packages.Package) {
if len(pkg.Syntax) == 0 || len(pkg.GoFiles) != len(pkg.CompiledGoFiles) {
// len(pkg.Syntax) == 0 if only filenames are loaded
// lengths aren't equal if there are preprocessed files (cgo)
startedAt := time.Now()

// can't use pkg.Fset: it will overwrite offsets by preprocessed files
fset := token.NewFileSet()
for _, f := range pkg.GoFiles {
c.parseFile(f, fset)
}
func (c *Cache) extractFilenamesForAstFile(fset *token.FileSet, f *ast.File) []string {
var ret []string

c.log.Infof("Parsed AST of all pkg.GoFiles: %s for %s", pkg.GoFiles, time.Since(startedAt))
return
// false ignores //line comments: name can be incorrect for generated files with //line directives
// mapping e.g. from .rl to .go files.
pos := fset.PositionFor(f.Pos(), false)
if pos.Filename != "" {
ret = append(ret, pos.Filename)
}

return ret
}

func (c *Cache) loadFromPackage(pkg *packages.Package) {
for _, f := range pkg.Syntax {
pos := pkg.Fset.Position(f.Pos())
if pos.Filename == "" {
continue
for _, filename := range c.extractFilenamesForAstFile(pkg.Fset, f) {
filePath := c.normalizeFilename(filename)
c.m[filePath] = &File{
F: f,
Fset: pkg.Fset,
Name: filePath,
}
}
}

filePath := c.normalizeFilename(pos.Filename)

c.m[filePath] = &File{
F: f,
Fset: pkg.Fset,
Name: filePath,
// some Go files sometimes aren't present in pkg.Syntax
fset := token.NewFileSet() // can't use pkg.Fset: it will overwrite offsets by preprocessed files
for _, filePath := range pkg.GoFiles {
filePath = c.normalizeFilename(filePath)
if c.m[filePath] == nil {
c.parseFile(filePath, fset)
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/lint/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,9 @@ func NewRunner(astCache *astcache.Cache, cfg *config.Config, log logutils.Log, g

return &Runner{
Processors: []processors.Processor{
processors.NewPathPrettifier(), // must be before diff, nolint and exclude autogenerated processor at least
processors.NewCgo(goenv),
processors.NewFilenameUnadjuster(astCache, log.Child("filename_unadjuster")), // must go after Cgo
processors.NewPathPrettifier(), // must be before diff, nolint and exclude autogenerated processor at least
skipFilesProcessor,
skipDirsProcessor, // must be after path prettifier

Expand Down
87 changes: 87 additions & 0 deletions pkg/result/processors/filename_unadjuster.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
package processors

import (
"go/token"
"path/filepath"
"strings"

"github.com/golangci/golangci-lint/pkg/logutils"

"github.com/golangci/golangci-lint/pkg/lint/astcache"

"github.com/golangci/golangci-lint/pkg/result"
)

type posMapper func(pos token.Position) token.Position

// FilenameUnadjuster is needed because a lot of linters use fset.Position(f.Pos())
// to get filename. And they return adjusted filename (e.g. *.qtpl) for an issue. We need
// restore real .go filename to properly output it, parse it, etc.
type FilenameUnadjuster struct {
m map[string]posMapper // map from adjusted filename to position mapper: adjusted -> unadjusted position
log logutils.Log
}

var _ Processor = FilenameUnadjuster{}

func NewFilenameUnadjuster(cache *astcache.Cache, log logutils.Log) *FilenameUnadjuster {
m := map[string]posMapper{}
for _, f := range cache.GetAllValidFiles() {
adjustedFilename := f.Fset.PositionFor(f.F.Pos(), true).Filename
if adjustedFilename == "" {
continue
}
unadjustedFilename := f.Fset.PositionFor(f.F.Pos(), false).Filename
if unadjustedFilename == "" || unadjustedFilename == adjustedFilename {
continue
}
if !strings.HasSuffix(unadjustedFilename, ".go") {
continue // file.go -> /caches/cgo-xxx
}

f := f
m[adjustedFilename] = func(adjustedPos token.Position) token.Position {
tokenFile := f.Fset.File(f.F.Pos())
if tokenFile == nil {
log.Warnf("Failed to get token file for %s", adjustedFilename)
return adjustedPos
}
return f.Fset.PositionFor(tokenFile.Pos(adjustedPos.Offset), false)
}
}

return &FilenameUnadjuster{
m: m,
log: log,
}
}

func (p FilenameUnadjuster) Name() string {
return "filename_unadjuster"
}

func (p FilenameUnadjuster) Process(issues []result.Issue) ([]result.Issue, error) {
return transformIssues(issues, func(i *result.Issue) *result.Issue {
issueFilePath := i.FilePath()
if !filepath.IsAbs(i.FilePath()) {
absPath, err := filepath.Abs(i.FilePath())
if err != nil {
p.log.Warnf("failed to build abs path for %q: %s", i.FilePath(), err)
return i
}
issueFilePath = absPath
}

mapper := p.m[issueFilePath]
if mapper == nil {
return i
}

newI := *i
newI.Pos = mapper(i.Pos)
p.log.Infof("Unadjusted from %v to %v", i.Pos, newI.Pos)
return &newI
}), nil
}

func (FilenameUnadjuster) Finish() {}
10 changes: 8 additions & 2 deletions pkg/result/processors/nolint.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"sort"
"strings"

"github.com/pkg/errors"

"github.com/golangci/golangci-lint/pkg/lint/astcache"
"github.com/golangci/golangci-lint/pkg/lint/lintersdb"
"github.com/golangci/golangci-lint/pkg/logutils"
Expand Down Expand Up @@ -88,8 +90,12 @@ func (p *Nolint) getOrCreateFileData(i *result.Issue) (*fileData, error) {
}

file := p.astCache.Get(i.FilePath())
if file == nil || file.Err != nil {
return nil, fmt.Errorf("can't parse file %s: %v, astcache is %v", i.FilePath(), file, p.astCache.ParsedFilenames())
if file == nil {
return nil, fmt.Errorf("no file %s in ast cache %v",
i.FilePath(), p.astCache.ParsedFilenames())
}
if file.Err != nil {
return nil, errors.Wrapf(file.Err, "can't parse file %s", i.FilePath())
}

fd.ignoredRanges = p.buildIgnoredRangesForFile(file.F, file.Fset, i.FilePath())
Expand Down
25 changes: 25 additions & 0 deletions test/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package test

import (
"path/filepath"
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -67,6 +68,30 @@ func TestGovetCustomFormatter(t *testing.T) {
testshared.NewLintRunner(t).Run(getTestDataDir("govet_custom_formatter")).ExpectNoIssues()
}

func TestLineDirectiveProcessedFilesLiteLoading(t *testing.T) {
r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config",
"--exclude-use-default=false", "-Egolint", getTestDataDir("quicktemplate"))

output := strings.Join([]string{
"testdata/quicktemplate/hello.qtpl.go:26:1: exported function `StreamHello` should have comment or be unexported (golint)",
"testdata/quicktemplate/hello.qtpl.go:50:1: exported function `Hello` should have comment or be unexported (golint)",
"testdata/quicktemplate/hello.qtpl.go:39:1: exported function `WriteHello` should have comment or be unexported (golint)",
}, "\n")
r.ExpectExitCode(exitcodes.IssuesFound).ExpectOutputEq(output + "\n")
}

func TestLineDirectiveProcessedFilesFullLoading(t *testing.T) {
r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config",
"--exclude-use-default=false", "-Egolint,govet", getTestDataDir("quicktemplate"))

output := strings.Join([]string{
"testdata/quicktemplate/hello.qtpl.go:26:1: exported function `StreamHello` should have comment or be unexported (golint)",
"testdata/quicktemplate/hello.qtpl.go:50:1: exported function `Hello` should have comment or be unexported (golint)",
"testdata/quicktemplate/hello.qtpl.go:39:1: exported function `WriteHello` should have comment or be unexported (golint)",
}, "\n")
r.ExpectExitCode(exitcodes.IssuesFound).ExpectOutputEq(output + "\n")
}

func TestSkippedDirsNoMatchArg(t *testing.T) {
dir := getTestDataDir("skipdirs", "skip_me", "nested")
r := testshared.NewLintRunner(t).Run("--print-issued-lines=false", "--no-config", "--skip-dirs", dir, "-Egolint", dir)
Expand Down
7 changes: 7 additions & 0 deletions test/testdata/quicktemplate/hello.qtpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
All text outside function templates is treated as comments,
i.e. it is just ignored by quicktemplate compiler (`qtc`). It is for humans.

Hello is a simple template function.
{% func Hello(name string) %}
Hello, {%s name %}!
{% endfunc %}
62 changes: 62 additions & 0 deletions test/testdata/quicktemplate/hello.qtpl.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// This file is automatically generated by qtc from "hello.qtpl".
// See https://github.com/valyala/quicktemplate for details.

// All text outside function templates is treated as comments,
// i.e. it is just ignored by quicktemplate compiler (`qtc`). It is for humans.
//
// Hello is a simple template function.

//line hello.qtpl:5
package quicktemplate

//line hello.qtpl:5
import (
qtio422016 "io"

qt422016 "github.com/valyala/quicktemplate"
)

//line hello.qtpl:5
var (
_ = qtio422016.Copy
_ = qt422016.AcquireByteBuffer
)

//line hello.qtpl:5
func StreamHello(qw422016 *qt422016.Writer, name string) {
//line hello.qtpl:5
qw422016.N().S(`
Hello, `)
//line hello.qtpl:6
qw422016.E().S(name)
//line hello.qtpl:6
qw422016.N().S(`!
`)
//line hello.qtpl:7
}

//line hello.qtpl:7
func WriteHello(qq422016 qtio422016.Writer, name string) {
//line hello.qtpl:7
qw422016 := qt422016.AcquireWriter(qq422016)
//line hello.qtpl:7
StreamHello(qw422016, name)
//line hello.qtpl:7
qt422016.ReleaseWriter(qw422016)
//line hello.qtpl:7
}

//line hello.qtpl:7
func Hello(name string) string {
//line hello.qtpl:7
qb422016 := qt422016.AcquireByteBuffer()
//line hello.qtpl:7
WriteHello(qb422016, name)
//line hello.qtpl:7
qs422016 := string(qb422016.B)
//line hello.qtpl:7
qt422016.ReleaseByteBuffer(qb422016)
//line hello.qtpl:7
return qs422016
//line hello.qtpl:7
}
7 changes: 4 additions & 3 deletions vendor/github.com/golangci/lint-1/.travis.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/github.com/golangci/lint-1/CONTRIBUTING.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions vendor/github.com/golangci/lint-1/README.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions vendor/github.com/golangci/lint-1/go.mod

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions vendor/github.com/golangci/lint-1/go.sum

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 397ba7f

Please sign in to comment.