diff --git a/.golangci.example.yml b/.golangci.example.yml index dcd760fc7fd2..c0dafcb4b9ed 100644 --- a/.golangci.example.yml +++ b/.golangci.example.yml @@ -6,9 +6,11 @@ run: build-tags: - mytag skip-dirs: - - external_libs + - src/external_libs + - autogenerated_by_my_lib skip-files: - ".*\\.pb\\.go$" + - lib/bad.go output: format: colored-line-number diff --git a/Gopkg.lock b/Gopkg.lock index 556c6f14be9c..d803005e7fbe 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -56,12 +56,6 @@ revision = "1adfc126b41513cc696b209667c8656ea7aac67c" version = "v1.0.0" -[[projects]] - branch = "master" - name = "github.com/golang/lint" - packages = ["."] - revision = "470b6b0bb3005eda157f0275e2e4895055396a81" - [[projects]] branch = "master" name = "github.com/golangci/check" @@ -103,7 +97,7 @@ branch = "master" name = "github.com/golangci/goconst" packages = ["."] - revision = "b67b9035d29a7561902d87eb3757dd490b31c1b1" + revision = "041c5f2b40f3dd334a4a6ee6a3f84ca3fc70680a" [[projects]] branch = "master" @@ -142,6 +136,12 @@ packages = ["."] revision = "e1cc50c0cfa0e058f40ced1b3d54b86014440c91" +[[projects]] + branch = "master" + name = "github.com/golangci/lint-1" + packages = ["."] + revision = "4bf9709227d15e5f14c84a381e315a4695c4a372" + [[projects]] branch = "master" name = "github.com/golangci/maligned" @@ -416,6 +416,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "832e18c919daf77730cda91687bfd2d0b0b70fe6283c38b33e48ed89c2fd2def" + inputs-digest = "4558f81cfd21ec5b1f937b518089a231eb05212cf9545af1fc3df3dd00a7fa22" solver-name = "gps-cdcl" solver-version = 1 diff --git a/Makefile b/Makefile index 351db7042e2a..2ca1b1751f59 100644 --- a/Makefile +++ b/Makefile @@ -3,8 +3,7 @@ test: golangci-lint run -v golangci-lint run --fast --no-config -v golangci-lint run --no-config -v - golangci-lint run --fast --no-config -v ./test/testdata/typecheck.go - go test -v -race ./... + go test -v ./... assets: svg-term --cast=183662 --out docs/demo.svg --window --width 110 --height 30 --from 2000 --to 20000 --profile Dracula --term iterm2 diff --git a/README.md b/README.md index cbb4e04a5b8e..49416b194aec 100644 --- a/README.md +++ b/README.md @@ -235,14 +235,14 @@ Flags: --print-issued-lines Print lines of code with issue (default true) --print-linter-name Print linter name in issue line (default true) --issues-exit-code int Exit code when issues were found (default 1) - --build-tags strings Build tags (not all linters support them) + --build-tags strings Build tags --deadline duration Deadline for total work (default 1m0s) --tests Analyze tests (*_test.go) (default true) --print-resources-usage Print avg and max memory usage of golangci-lint and total time -c, --config PATH Read config from file path PATH --no-config Don't read config - --skip-dirs strings Regexps of directory names to skip - --skip-files strings Regexps of file names to skip + --skip-dirs strings Regexps of directories to skip + --skip-files strings Regexps of files to skip -E, --enable strings Enable specific linter -D, --disable strings Disable specific linter --enable-all Enable all linters @@ -255,7 +255,7 @@ Flags: - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*printf?|os\.(Un)?Setenv). is not checked # golint: Annoying issue about not having a comment. The rare codebase has such comments - - (should have comment|comment on exported method|should have a package comment) + - (comment on exported (method|function)|should have( a package)? comment|comment should be of the form) # golint: False positive when tests are defined in package 'test' - func name will be used as test\.Test.* by other packages, and that stutters; consider calling this diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 32c90694a5b1..328be40ffadd 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -65,14 +65,14 @@ func initFlagSet(fs *pflag.FlagSet, cfg *config.Config) { rc := &cfg.Run fs.IntVar(&rc.ExitCodeIfIssuesFound, "issues-exit-code", 1, wh("Exit code when issues were found")) - fs.StringSliceVar(&rc.BuildTags, "build-tags", nil, wh("Build tags (not all linters support them)")) + fs.StringSliceVar(&rc.BuildTags, "build-tags", nil, wh("Build tags")) fs.DurationVar(&rc.Deadline, "deadline", time.Minute, wh("Deadline for total work")) fs.BoolVar(&rc.AnalyzeTests, "tests", true, wh("Analyze tests (*_test.go)")) fs.BoolVar(&rc.PrintResourcesUsage, "print-resources-usage", false, wh("Print avg and max memory usage of golangci-lint and total time")) fs.StringVarP(&rc.Config, "config", "c", "", wh("Read config from file path `PATH`")) fs.BoolVar(&rc.NoConfig, "no-config", false, wh("Don't read config")) - fs.StringSliceVar(&rc.SkipDirs, "skip-dirs", nil, wh("Regexps of directory names to skip")) - fs.StringSliceVar(&rc.SkipFiles, "skip-files", nil, wh("Regexps of file names to skip")) + fs.StringSliceVar(&rc.SkipDirs, "skip-dirs", nil, wh("Regexps of directories to skip")) + fs.StringSliceVar(&rc.SkipFiles, "skip-files", nil, wh("Regexps of files to skip")) // Linters settings config lsc := &cfg.LintersSettings diff --git a/pkg/config/config.go b/pkg/config/config.go index 30cff3fb688a..888d8b34d4ce 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -28,7 +28,7 @@ var DefaultExcludePatterns = []ExcludePattern{ Why: "Almost all programs ignore errors on these functions and in most cases it's ok", }, { - Pattern: "(should have comment|comment on exported method|should have a package comment)", + Pattern: "(comment on exported (method|function)|should have( a package)? comment|comment should be of the form)", Linter: "golint", Why: "Annoying issue about not having a comment. The rare codebase has such comments", }, diff --git a/pkg/fsutils/fsutils.go b/pkg/fsutils/fsutils.go index 561f9ffd8308..4c3a0db1bc8e 100644 --- a/pkg/fsutils/fsutils.go +++ b/pkg/fsutils/fsutils.go @@ -1,139 +1,9 @@ package fsutils import ( - "context" - "fmt" - "go/build" "os" - "path" - "path/filepath" - "strings" - "time" - - "github.com/sirupsen/logrus" ) -var stdExcludeDirRegexps = []string{ - "^vendor$", "^third_party$", - "^testdata$", "^examples$", - "^Godeps$", - "^builtin$", -} - -func GetProjectRoot() string { - return path.Join(build.Default.GOPATH, "src", "github.com", "golangci", "golangci-worker") -} - -type ProjectPaths struct { - Files []string - Dirs []string - IsDirsRun bool -} - -func (p ProjectPaths) MixedPaths() []string { - if p.IsDirsRun { - return p.Dirs - } - - return p.Files -} - -func (p ProjectPaths) FilesGrouppedByDirs() [][]string { - dirToFiles := map[string][]string{} - for _, f := range p.Files { - dir := filepath.Dir(f) - dirToFiles[dir] = append(dirToFiles[dir], f) - } - - ret := [][]string{} - for _, files := range dirToFiles { - ret = append(ret, files) - } - return ret -} - -func processPaths(root string, paths []string, maxPaths int) ([]string, error) { - if len(paths) > maxPaths { - logrus.Warnf("Gofmt: got too much paths (%d), analyze first %d", len(paths), maxPaths) - paths = paths[:maxPaths] - } - - ret := []string{} - for _, p := range paths { - if !filepath.IsAbs(p) { - ret = append(ret, p) - continue - } - - relPath, err := filepath.Rel(root, p) - if err != nil { - return nil, fmt.Errorf("can't get relative path for path %s and root %s: %s", - p, root, err) - } - ret = append(ret, relPath) - } - - return ret, nil -} - -func processResolvedPaths(paths *PathResolveResult) (*ProjectPaths, error) { - root, err := os.Getwd() - if err != nil { - return nil, fmt.Errorf("can't get working dir: %s", err) - } - - files, err := processPaths(root, paths.Files(), 10000) - if err != nil { - return nil, fmt.Errorf("can't process resolved files: %s", err) - } - - dirs, err := processPaths(root, paths.Dirs(), 1000) - if err != nil { - return nil, fmt.Errorf("can't process resolved dirs: %s", err) - } - - for i := range dirs { - dir := dirs[i] - if dir != "." && !filepath.IsAbs(dir) { - dirs[i] = "./" + dir - } - } - - return &ProjectPaths{ - Files: files, - Dirs: dirs, - IsDirsRun: len(dirs) != 0, - }, nil -} - -func GetPathsForAnalysis(ctx context.Context, inputPaths []string, includeTests bool, skipDirRegexps []string) (ret *ProjectPaths, err error) { - defer func(startedAt time.Time) { - if ret != nil { - logrus.Infof("Found paths for analysis for %s: %s", time.Since(startedAt), ret.MixedPaths()) - } - }(time.Now()) - - for _, path := range inputPaths { - if strings.HasSuffix(path, ".go") && len(inputPaths) != 1 { - return nil, fmt.Errorf("specific files for analysis are allowed only if one file is set") - } - } - - // TODO: don't analyze skipped files also, when be able to do it - excludeDirs := append([]string{}, stdExcludeDirRegexps...) - excludeDirs = append(excludeDirs, skipDirRegexps...) - pr, err := NewPathResolver(excludeDirs, []string{".go"}, includeTests) - if err != nil { - return nil, fmt.Errorf("can't make path resolver: %s", err) - } - paths, err := pr.Resolve(inputPaths...) - if err != nil { - return nil, fmt.Errorf("can't resolve paths %v: %s", inputPaths, err) - } - - return processResolvedPaths(paths) -} - func IsDir(filename string) bool { fi, err := os.Stat(filename) return err == nil && fi.IsDir() diff --git a/pkg/fsutils/path_resolver.go b/pkg/fsutils/path_resolver.go deleted file mode 100644 index 9c77287daf1c..000000000000 --- a/pkg/fsutils/path_resolver.go +++ /dev/null @@ -1,216 +0,0 @@ -package fsutils - -import ( - "fmt" - "os" - "path/filepath" - "regexp" - "sort" - "strings" -) - -type PathResolver struct { - excludeDirs map[string]*regexp.Regexp - allowedFileExtensions map[string]bool - includeTests bool -} - -type pathResolveState struct { - files map[string]bool - dirs map[string]bool -} - -func (s *pathResolveState) addFile(path string) { - s.files[filepath.Clean(path)] = true -} - -func (s *pathResolveState) addDir(path string) { - s.dirs[filepath.Clean(path)] = true -} - -type PathResolveResult struct { - files []string - dirs []string -} - -func (prr PathResolveResult) Files() []string { - return prr.files -} - -func (prr PathResolveResult) Dirs() []string { - return prr.dirs -} - -func (s pathResolveState) toResult() *PathResolveResult { - res := &PathResolveResult{ - files: []string{}, - dirs: []string{}, - } - for f := range s.files { - res.files = append(res.files, f) - } - for d := range s.dirs { - res.dirs = append(res.dirs, d) - } - - sort.Strings(res.files) - sort.Strings(res.dirs) - return res -} - -func NewPathResolver(excludeDirs, allowedFileExtensions []string, includeTests bool) (*PathResolver, error) { - excludeDirsMap := map[string]*regexp.Regexp{} - for _, dir := range excludeDirs { - re, err := regexp.Compile(dir) - if err != nil { - return nil, fmt.Errorf("can't compile regexp %q: %s", dir, err) - } - - excludeDirsMap[dir] = re - } - - allowedFileExtensionsMap := map[string]bool{} - for _, fe := range allowedFileExtensions { - allowedFileExtensionsMap[fe] = true - } - - return &PathResolver{ - excludeDirs: excludeDirsMap, - allowedFileExtensions: allowedFileExtensionsMap, - includeTests: includeTests, - }, nil -} - -func (pr PathResolver) isIgnoredDir(dir string) bool { - dirName := filepath.Base(filepath.Clean(dir)) // ignore dirs on any depth level - - // https://github.com/golang/dep/issues/298 - // https://github.com/tools/godep/issues/140 - if strings.HasPrefix(dirName, ".") && dirName != "." && dirName != ".." { - return true - } - if strings.HasPrefix(dirName, "_") { - return true - } - - for _, dirExludeRe := range pr.excludeDirs { - if dirExludeRe.MatchString(dirName) { - return true - } - } - - return false -} - -func (pr PathResolver) isAllowedFile(path string) bool { - if !pr.includeTests && strings.HasSuffix(path, "_test.go") { - return false - } - - return pr.allowedFileExtensions[filepath.Ext(path)] -} - -func (pr PathResolver) resolveRecursively(root string, state *pathResolveState) error { - walkErr := filepath.Walk(root, func(p string, i os.FileInfo, err error) error { - if err != nil { - return err - } - - if i.IsDir() { - if pr.isIgnoredDir(p) { - return filepath.SkipDir - } - state.addDir(p) - return nil - } - - if pr.isAllowedFile(p) { - state.addFile(p) - } - return nil - }) - - if walkErr != nil { - return fmt.Errorf("can't walk dir %s: %s", root, walkErr) - } - - return nil -} - -func (pr PathResolver) resolveDir(root string, state *pathResolveState) error { - walkErr := filepath.Walk(root, func(p string, i os.FileInfo, err error) error { - if err != nil { - return err - } - - if i.IsDir() { - if filepath.Clean(p) != filepath.Clean(root) { - return filepath.SkipDir - } - state.addDir(p) - return nil - } - - if pr.isAllowedFile(p) { - state.addFile(p) - } - return nil - }) - - if walkErr != nil { - return fmt.Errorf("can't walk dir %s: %s", root, walkErr) - } - - return nil -} - -func (pr PathResolver) Resolve(paths ...string) (*PathResolveResult, error) { - if len(paths) == 0 { - return nil, fmt.Errorf("no paths are set") - } - - state := &pathResolveState{ - files: map[string]bool{}, - dirs: map[string]bool{}, - } - for _, path := range paths { - if strings.HasSuffix(path, "/...") { - if err := pr.resolveRecursively(filepath.Dir(path), state); err != nil { - return nil, fmt.Errorf("can't recursively resolve %s: %s", path, err) - } - continue - } - - fi, err := os.Stat(path) - if err != nil { - return nil, fmt.Errorf("can't find path %s: %s", path, err) - } - - if fi.IsDir() { - if err := pr.resolveDir(path, state); err != nil { - return nil, fmt.Errorf("can't resolve dir %s: %s", path, err) - } - continue - } - - state.addFile(path) - } - - state.excludeDirsWithoutGoFiles() - - return state.toResult(), nil -} - -func (s *pathResolveState) excludeDirsWithoutGoFiles() { - dirToFiles := map[string]bool{} - for f := range s.files { - dir := filepath.Dir(f) - dirToFiles[dir] = true - } - - for dir := range s.dirs { - if !dirToFiles[dir] { // no go files in this dir - delete(s.dirs, dir) - } - } -} diff --git a/pkg/golinters/dupl.go b/pkg/golinters/dupl.go index 2784f3446a4b..0ede2d5037b8 100644 --- a/pkg/golinters/dupl.go +++ b/pkg/golinters/dupl.go @@ -21,7 +21,7 @@ func (Dupl) Desc() string { } func (d Dupl) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issue, error) { - issues, err := duplAPI.Run(lintCtx.Paths.Files, lintCtx.Settings().Dupl.Threshold) + issues, err := duplAPI.Run(lintCtx.PkgProgram.Files(lintCtx.Cfg.Run.AnalyzeTests), lintCtx.Settings().Dupl.Threshold) if err != nil { return nil, err } diff --git a/pkg/golinters/goconst.go b/pkg/golinters/goconst.go index 653561ebb8d2..fdc7c63b39e7 100644 --- a/pkg/golinters/goconst.go +++ b/pkg/golinters/goconst.go @@ -21,12 +21,18 @@ func (Goconst) Desc() string { func (lint Goconst) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issue, error) { var goconstIssues []goconstAPI.Issue - // TODO: make it cross-package: pass package names inside goconst - for _, files := range lintCtx.Paths.FilesGrouppedByDirs() { - issues, err := goconstAPI.Run(files, true, - lintCtx.Settings().Goconst.MinStringLen, - lintCtx.Settings().Goconst.MinOccurrencesCount, - ) + cfg := goconstAPI.Config{ + MatchWithConstants: true, + MinStringLength: lintCtx.Settings().Goconst.MinStringLen, + MinOccurrences: lintCtx.Settings().Goconst.MinOccurrencesCount, + } + for _, pkg := range lintCtx.PkgProgram.Packages() { + files, fset, err := getASTFilesForPkg(lintCtx, &pkg) + if err != nil { + return nil, err + } + + issues, err := goconstAPI.Run(files, fset, &cfg) if err != nil { return nil, err } diff --git a/pkg/golinters/gofmt.go b/pkg/golinters/gofmt.go index 773405dc2057..4c9f60399822 100644 --- a/pkg/golinters/gofmt.go +++ b/pkg/golinters/gofmt.go @@ -105,7 +105,7 @@ func (g Gofmt) extractIssuesFromPatch(patch string) ([]result.Issue, error) { func (g Gofmt) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issue, error) { var issues []result.Issue - for _, f := range lintCtx.Paths.Files { + for _, f := range lintCtx.PkgProgram.Files(lintCtx.Cfg.Run.AnalyzeTests) { var diff []byte var err error if g.UseGoimports { diff --git a/pkg/golinters/golint.go b/pkg/golinters/golint.go index 76ff23cbf449..32cbcfeef6b3 100644 --- a/pkg/golinters/golint.go +++ b/pkg/golinters/golint.go @@ -3,12 +3,13 @@ package golinters import ( "context" "fmt" - "io/ioutil" + "go/ast" + "go/token" - lintAPI "github.com/golang/lint" "github.com/golangci/golangci-lint/pkg/lint/linter" "github.com/golangci/golangci-lint/pkg/logutils" "github.com/golangci/golangci-lint/pkg/result" + lintAPI "github.com/golangci/lint-1" ) type Golint struct{} @@ -24,8 +25,13 @@ func (Golint) Desc() string { func (g Golint) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issue, error) { var issues []result.Issue var lintErr error - for _, pkgFiles := range lintCtx.Paths.FilesGrouppedByDirs() { - i, err := g.lintFiles(lintCtx.Settings().Golint.MinConfidence, pkgFiles...) + for _, pkg := range lintCtx.PkgProgram.Packages() { + files, fset, err := getASTFilesForPkg(lintCtx, &pkg) + if err != nil { + return nil, err + } + + i, err := g.lintPkg(lintCtx.Settings().Golint.MinConfidence, files, fset) if err != nil { lintErr = err continue @@ -39,26 +45,18 @@ func (g Golint) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issu return issues, nil } -func (g Golint) lintFiles(minConfidence float64, filenames ...string) ([]result.Issue, error) { - files := make(map[string][]byte) - for _, filename := range filenames { - src, err := ioutil.ReadFile(filename) - if err != nil { - return nil, fmt.Errorf("can't read file %s: %s", filename, err) - } - files[filename] = src - } - +func (g Golint) lintPkg(minConfidence float64, files []*ast.File, fset *token.FileSet) ([]result.Issue, error) { l := new(lintAPI.Linter) - ps, err := l.LintFiles(files) + ps, err := l.LintASTFiles(files, fset) if err != nil { - return nil, fmt.Errorf("can't lint files %s: %s", filenames, err) + return nil, fmt.Errorf("can't lint %d files: %s", len(files), err) } + if len(ps) == 0 { return nil, nil } - issues := make([]result.Issue, 0, len(ps)) //This is worst case + issues := make([]result.Issue, 0, len(ps)) // This is worst case for _, p := range ps { if p.Confidence >= minConfidence { issues = append(issues, result.Issue{ diff --git a/pkg/golinters/govet.go b/pkg/golinters/govet.go index 5dad22ac3c7e..768138d4753e 100644 --- a/pkg/golinters/govet.go +++ b/pkg/golinters/govet.go @@ -21,8 +21,8 @@ func (Govet) Desc() string { func (g Govet) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issue, error) { // TODO: check .S asm files: govet can do it if pass dirs var govetIssues []govetAPI.Issue - for _, files := range lintCtx.Paths.FilesGrouppedByDirs() { - issues, err := govetAPI.Run(files, lintCtx.Settings().Govet.CheckShadowing) + for _, pkg := range lintCtx.PkgProgram.Packages() { + issues, err := govetAPI.Run(pkg.Files(lintCtx.Cfg.Run.AnalyzeTests), lintCtx.Settings().Govet.CheckShadowing) if err != nil { return nil, err } diff --git a/pkg/golinters/ineffassign.go b/pkg/golinters/ineffassign.go index 3d13ddf351c7..a18e76314dbc 100644 --- a/pkg/golinters/ineffassign.go +++ b/pkg/golinters/ineffassign.go @@ -20,7 +20,7 @@ func (Ineffassign) Desc() string { } func (lint Ineffassign) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issue, error) { - issues := ineffassignAPI.Run(lintCtx.Paths.Files) + issues := ineffassignAPI.Run(lintCtx.PkgProgram.Files(lintCtx.Cfg.Run.AnalyzeTests)) if len(issues) == 0 { return nil, nil } diff --git a/pkg/golinters/megacheck.go b/pkg/golinters/megacheck.go index f853d1b04c2d..69cd53c5dc22 100644 --- a/pkg/golinters/megacheck.go +++ b/pkg/golinters/megacheck.go @@ -8,6 +8,7 @@ import ( megacheckAPI "github.com/golangci/go-tools/cmd/megacheck" "github.com/golangci/golangci-lint/pkg/lint/linter" "github.com/golangci/golangci-lint/pkg/result" + "github.com/sirupsen/logrus" ) type Megacheck struct { @@ -51,6 +52,17 @@ func (m Megacheck) Desc() string { } func (m Megacheck) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Issue, error) { + if len(lintCtx.NotCompilingPackages) != 0 { + var packages []string + for _, p := range lintCtx.NotCompilingPackages { + packages = append(packages, p.String()) + } + logrus.Warnf("Can't run megacheck because of compilation errors in packages "+ + "%s: run `typecheck` linter to see errors", packages) + // megacheck crashes if there are not compiling packages + return nil, nil + } + issues := megacheckAPI.Run(lintCtx.Program, lintCtx.LoaderConfig, lintCtx.SSAProgram, m.StaticcheckEnabled, m.GosimpleEnabled, m.UnusedEnabled) if len(issues) == 0 { diff --git a/pkg/golinters/utils.go b/pkg/golinters/utils.go index 19512cf28130..21dd29bed10f 100644 --- a/pkg/golinters/utils.go +++ b/pkg/golinters/utils.go @@ -2,8 +2,13 @@ package golinters import ( "fmt" + "go/ast" + "go/token" "strings" + "github.com/golangci/golangci-lint/pkg/lint/linter" + "github.com/golangci/golangci-lint/pkg/packages" + "github.com/golangci/golangci-lint/pkg/config" ) @@ -22,3 +27,24 @@ func formatCodeBlock(code string, cfg *config.Config) string { return fmt.Sprintf("```\n%s\n```", code) } + +func getASTFilesForPkg(ctx *linter.Context, pkg *packages.Package) ([]*ast.File, *token.FileSet, error) { + filenames := pkg.Files(ctx.Cfg.Run.AnalyzeTests) + files := make([]*ast.File, 0, len(filenames)) + var fset *token.FileSet + for _, filename := range filenames { + f := ctx.ASTCache.Get(filename) + if f == nil { + return nil, nil, fmt.Errorf("no AST for file %s in cache", filename) + } + + if f.Err != nil { + return nil, nil, fmt.Errorf("can't load AST for file %s: %s", f.Name, f.Err) + } + + files = append(files, f.F) + fset = f.Fset + } + + return files, fset, nil +} diff --git a/pkg/lint/astcache/astcache.go b/pkg/lint/astcache/astcache.go index c9fcd3a6eac9..9415652474bc 100644 --- a/pkg/lint/astcache/astcache.go +++ b/pkg/lint/astcache/astcache.go @@ -16,7 +16,7 @@ type File struct { F *ast.File Fset *token.FileSet Name string - err error + Err error } type Cache struct { @@ -24,6 +24,10 @@ type Cache struct { s []*File } +func (c Cache) Get(filename string) *File { + return c.m[filepath.Clean(filename)] +} + func (c Cache) GetAllValidFiles() []*File { return c.s } @@ -31,7 +35,7 @@ func (c Cache) GetAllValidFiles() []*File { func (c *Cache) prepareValidFiles() { files := make([]*File, 0, len(c.m)) for _, f := range c.m { - if f.err != nil || f.F == nil { + if f.Err != nil || f.F == nil { continue } files = append(files, f) @@ -75,21 +79,24 @@ func LoadFromProgram(prog *loader.Program) (*Cache, error) { return c, nil } -func LoadFromFiles(files []string) *Cache { +func LoadFromFiles(files []string) (*Cache, error) { c := &Cache{ m: map[string]*File{}, } + fset := token.NewFileSet() for _, filePath := range files { + filePath = filepath.Clean(filePath) + f, err := parser.ParseFile(fset, filePath, nil, parser.ParseComments) // comments needed by e.g. golint c.m[filePath] = &File{ F: f, Fset: fset, - err: err, + Err: err, Name: filePath, } } c.prepareValidFiles() - return c + return c, nil } diff --git a/pkg/lint/linter/context.go b/pkg/lint/linter/context.go index 173b25361397..7a9684e48cc2 100644 --- a/pkg/lint/linter/context.go +++ b/pkg/lint/linter/context.go @@ -3,13 +3,13 @@ package linter import ( "github.com/golangci/go-tools/ssa" "github.com/golangci/golangci-lint/pkg/config" - "github.com/golangci/golangci-lint/pkg/fsutils" "github.com/golangci/golangci-lint/pkg/lint/astcache" + "github.com/golangci/golangci-lint/pkg/packages" "golang.org/x/tools/go/loader" ) type Context struct { - Paths *fsutils.ProjectPaths + PkgProgram *packages.Program Cfg *config.Config Program *loader.Program SSAProgram *ssa.Program diff --git a/pkg/lint/load.go b/pkg/lint/load.go index 420f0b44cf4c..a7d54fec708d 100644 --- a/pkg/lint/load.go +++ b/pkg/lint/load.go @@ -4,35 +4,23 @@ import ( "context" "fmt" "go/build" + "go/parser" "os" "os/exec" + "path/filepath" "strings" "time" "github.com/golangci/go-tools/ssa" "github.com/golangci/go-tools/ssa/ssautil" "github.com/golangci/golangci-lint/pkg/config" - "github.com/golangci/golangci-lint/pkg/fsutils" "github.com/golangci/golangci-lint/pkg/lint/astcache" "github.com/golangci/golangci-lint/pkg/lint/linter" + "github.com/golangci/golangci-lint/pkg/packages" "github.com/sirupsen/logrus" "golang.org/x/tools/go/loader" ) -type Context struct { - Paths *fsutils.ProjectPaths - Cfg *config.Config - Program *loader.Program - SSAProgram *ssa.Program - LoaderConfig *loader.Config - ASTCache *astcache.Cache - NotCompilingPackages []*loader.PackageInfo -} - -func (c *Context) Settings() *config.LintersSettings { - return &c.Cfg.LintersSettings -} - func isFullImportNeeded(linters []linter.Config) bool { for _, linter := range linters { if linter.NeedsProgramLoading() { @@ -53,7 +41,30 @@ func isSSAReprNeeded(linters []linter.Config) bool { return false } -func loadWholeAppIfNeeded(ctx context.Context, linters []linter.Config, cfg *config.Run, paths *fsutils.ProjectPaths) (*loader.Program, *loader.Config, error) { +func normalizePaths(paths []string) ([]string, error) { + root, err := os.Getwd() + if err != nil { + return nil, fmt.Errorf("can't get working dir: %s", err) + } + + ret := make([]string, 0, len(paths)) + for _, p := range paths { + if filepath.IsAbs(p) { + relPath, err := filepath.Rel(root, p) + if err != nil { + return nil, fmt.Errorf("can't get relative path for path %s and root %s: %s", + p, root, err) + } + p = relPath + } + + ret = append(ret, "./"+p) + } + + return ret, nil +} + +func loadWholeAppIfNeeded(ctx context.Context, linters []linter.Config, cfg *config.Run, pkgProg *packages.Program) (*loader.Program, *loader.Config, error) { if !isFullImportNeeded(linters) { return nil, nil, nil } @@ -63,13 +74,27 @@ func loadWholeAppIfNeeded(ctx context.Context, linters []linter.Config, cfg *con logrus.Infof("Program loading took %s", time.Since(startedAt)) }() - bctx := build.Default - bctx.BuildTags = append(bctx.BuildTags, cfg.BuildTags...) + bctx := pkgProg.BuildContext() loadcfg := &loader.Config{ - Build: &bctx, - AllowErrors: true, // Try to analyze event partially + Build: bctx, + AllowErrors: true, // Try to analyze partially + ParserMode: parser.ParseComments, // AST will be reused by linters } - rest, err := loadcfg.FromArgs(paths.MixedPaths(), cfg.AnalyzeTests) + + var loaderArgs []string + dirs := pkgProg.Dirs() + if len(dirs) != 0 { + loaderArgs = dirs // dirs run + } else { + loaderArgs = pkgProg.Files(cfg.AnalyzeTests) // files run + } + + nLoaderArgs, err := normalizePaths(loaderArgs) + if err != nil { + return nil, nil, err + } + + rest, err := loadcfg.FromArgs(nLoaderArgs, cfg.AnalyzeTests) if err != nil { return nil, nil, fmt.Errorf("can't parepare load config with paths: %s", err) } @@ -79,7 +104,7 @@ func loadWholeAppIfNeeded(ctx context.Context, linters []linter.Config, cfg *con prog, err := loadcfg.Load() if err != nil { - return nil, nil, fmt.Errorf("can't load program from paths %v: %s", paths.MixedPaths(), err) + return nil, nil, fmt.Errorf("can't load program from paths %v: %s", loaderArgs, err) } return prog, loadcfg, nil @@ -138,6 +163,7 @@ func separateNotCompilingPackages(lintCtx *linter.Context) { } } +//nolint:gocyclo func LoadContext(ctx context.Context, linters []linter.Config, cfg *config.Config) (*linter.Context, error) { // Set GOROOT to have working cross-compilation: cross-compiled binaries // have invalid GOROOT. XXX: can't use runtime.GOROOT(). @@ -147,19 +173,25 @@ func LoadContext(ctx context.Context, linters []linter.Config, cfg *config.Confi } os.Setenv("GOROOT", goroot) build.Default.GOROOT = goroot - logrus.Infof("set GOROOT=%q", goroot) args := cfg.Run.Args if len(args) == 0 { args = []string{"./..."} } - paths, err := fsutils.GetPathsForAnalysis(ctx, args, cfg.Run.AnalyzeTests, cfg.Run.SkipDirs) + skipDirs := append([]string{}, packages.StdExcludeDirRegexps...) + skipDirs = append(skipDirs, cfg.Run.SkipDirs...) + r, err := packages.NewResolver(cfg.Run.BuildTags, skipDirs) if err != nil { return nil, err } - prog, loaderConfig, err := loadWholeAppIfNeeded(ctx, linters, &cfg.Run, paths) + pkgProg, err := r.Resolve(args...) + if err != nil { + return nil, err + } + + prog, loaderConfig, err := loadWholeAppIfNeeded(ctx, linters, &cfg.Run, pkgProg) if err != nil { return nil, err } @@ -172,15 +204,15 @@ func LoadContext(ctx context.Context, linters []linter.Config, cfg *config.Confi var astCache *astcache.Cache if prog != nil { astCache, err = astcache.LoadFromProgram(prog) - if err != nil { - return nil, err - } } else { - astCache = astcache.LoadFromFiles(paths.Files) + astCache, err = astcache.LoadFromFiles(pkgProg.Files(cfg.Run.AnalyzeTests)) + } + if err != nil { + return nil, err } ret := &linter.Context{ - Paths: paths, + PkgProgram: pkgProg, Cfg: cfg, Program: prog, SSAProgram: ssaProg, diff --git a/pkg/lint/load_test.go b/pkg/lint/load_test.go index 099d028abacd..ad231a83b17c 100644 --- a/pkg/lint/load_test.go +++ b/pkg/lint/load_test.go @@ -5,9 +5,9 @@ import ( "testing" "github.com/golangci/golangci-lint/pkg/config" - "github.com/golangci/golangci-lint/pkg/fsutils" "github.com/golangci/golangci-lint/pkg/lint/astcache" "github.com/golangci/golangci-lint/pkg/lint/linter" + "github.com/golangci/golangci-lint/pkg/packages" "github.com/stretchr/testify/assert" ) @@ -19,24 +19,30 @@ func TestASTCacheLoading(t *testing.T) { inputPaths := []string{"./...", "./", "./load.go", "load.go"} for _, inputPath := range inputPaths { - paths, err := fsutils.GetPathsForAnalysis(ctx, []string{inputPath}, true, nil) + r, err := packages.NewResolver(nil, nil) assert.NoError(t, err) - assert.NotEmpty(t, paths.Files) + + pkgProg, err := r.Resolve(inputPath) + assert.NoError(t, err) + + assert.NoError(t, err) + assert.NotEmpty(t, pkgProg.Files(true)) prog, _, err := loadWholeAppIfNeeded(ctx, linters, &config.Run{ AnalyzeTests: true, - }, paths) + }, pkgProg) assert.NoError(t, err) astCacheFromProg, err := astcache.LoadFromProgram(prog) assert.NoError(t, err) - astCacheFromFiles := astcache.LoadFromFiles(paths.Files) + astCacheFromFiles, err := astcache.LoadFromFiles(pkgProg.Files(true)) + assert.NoError(t, err) filesFromProg := astCacheFromProg.GetAllValidFiles() filesFromFiles := astCacheFromFiles.GetAllValidFiles() if len(filesFromProg) != len(filesFromFiles) { - t.Logf("files: %s", paths.Files) + t.Logf("files: %s", pkgProg.Files(true)) t.Logf("from prog:") for _, f := range filesFromProg { t.Logf("%+v", *f) @@ -48,7 +54,7 @@ func TestASTCacheLoading(t *testing.T) { t.Fatalf("lengths differ") } - if len(filesFromProg) != len(paths.Files) { + if len(filesFromProg) != len(pkgProg.Files(true)) { t.Fatalf("filesFromProg differ from path.Files") } } diff --git a/pkg/packages/exclude.go b/pkg/packages/exclude.go new file mode 100644 index 000000000000..be57ebd92fd4 --- /dev/null +++ b/pkg/packages/exclude.go @@ -0,0 +1,8 @@ +package packages + +var StdExcludeDirRegexps = []string{ + "vendor$", "third_party$", + "testdata$", "examples$", + "Godeps$", + "builtin$", +} diff --git a/pkg/packages/package.go b/pkg/packages/package.go new file mode 100644 index 000000000000..8dfae0ff4a6c --- /dev/null +++ b/pkg/packages/package.go @@ -0,0 +1,28 @@ +package packages + +import ( + "go/build" + "path/filepath" +) + +type Package struct { + bp *build.Package + + isFake bool +} + +func (pkg *Package) Files(includeTest bool) []string { + pkgFiles := append([]string{}, pkg.bp.GoFiles...) + + // TODO: add cgo files + if includeTest { + pkgFiles = append(pkgFiles, pkg.bp.TestGoFiles...) + pkgFiles = append(pkgFiles, pkg.bp.XTestGoFiles...) + } + + for i, f := range pkgFiles { + pkgFiles[i] = filepath.Join(pkg.bp.Dir, f) + } + + return pkgFiles +} diff --git a/pkg/packages/program.go b/pkg/packages/program.go new file mode 100644 index 000000000000..d268994446d4 --- /dev/null +++ b/pkg/packages/program.go @@ -0,0 +1,71 @@ +package packages + +import ( + "fmt" + "go/build" +) + +type Program struct { + packages []Package + + bctx build.Context +} + +func (p *Program) String() string { + files := p.Files(true) + if len(files) == 1 { + return files[0] + } + + return fmt.Sprintf("%s", p.Dirs()) +} + +func (p *Program) BuildContext() *build.Context { + return &p.bctx +} + +func (p Program) Packages() []Package { + return p.packages +} + +func (p *Program) addPackage(pkg *Package) { + packagesToAdd := []Package{*pkg} + if len(pkg.bp.XTestGoFiles) != 0 { + // create separate package because xtest files have different package name + xbp := build.Package{ + Dir: pkg.bp.Dir, + ImportPath: pkg.bp.ImportPath + "_test", + XTestGoFiles: pkg.bp.XTestGoFiles, + XTestImportPos: pkg.bp.XTestImportPos, + XTestImports: pkg.bp.XTestImports, + } + packagesToAdd = append(packagesToAdd, Package{ + bp: &xbp, + }) + pkg.bp.XTestGoFiles = nil + pkg.bp.XTestImportPos = nil + pkg.bp.XTestImports = nil + } + + p.packages = append(p.packages, packagesToAdd...) +} + +func (p *Program) Files(includeTest bool) []string { + var ret []string + for _, pkg := range p.packages { + ret = append(ret, pkg.Files(includeTest)...) + } + + return ret +} + +func (p *Program) Dirs() []string { + var ret []string + for _, pkg := range p.packages { + if !pkg.isFake { + ret = append(ret, pkg.bp.Dir) + } + } + + return ret +} diff --git a/pkg/packages/resolver.go b/pkg/packages/resolver.go new file mode 100644 index 000000000000..fcbedd9fcd88 --- /dev/null +++ b/pkg/packages/resolver.go @@ -0,0 +1,206 @@ +package packages + +import ( + "fmt" + "go/build" + "io/ioutil" + "os" + "path/filepath" + "regexp" + "strings" + "time" + + "github.com/sirupsen/logrus" +) + +type Resolver struct { + excludeDirs map[string]*regexp.Regexp + buildTags []string + + skippedDirs []string +} + +func NewResolver(buildTags, excludeDirs []string) (*Resolver, error) { + excludeDirsMap := map[string]*regexp.Regexp{} + for _, dir := range excludeDirs { + re, err := regexp.Compile(dir) + if err != nil { + return nil, fmt.Errorf("can't compile regexp %q: %s", dir, err) + } + + excludeDirsMap[dir] = re + } + + return &Resolver{ + excludeDirs: excludeDirsMap, + buildTags: buildTags, + }, nil +} + +func (r Resolver) isIgnoredDir(dir string) bool { + cleanName := filepath.Clean(dir) + + dirName := filepath.Base(cleanName) + + // https://github.com/golang/dep/issues/298 + // https://github.com/tools/godep/issues/140 + if strings.HasPrefix(dirName, ".") && dirName != "." && dirName != ".." { + return true + } + if strings.HasPrefix(dirName, "_") { + return true + } + + for _, dirExludeRe := range r.excludeDirs { + if dirExludeRe.MatchString(cleanName) { + return true + } + } + + return false +} + +func (r *Resolver) resolveRecursively(root string, prog *Program) error { + // import root + if err := r.resolveDir(root, prog); err != nil { + return err + } + + fis, err := ioutil.ReadDir(root) + if err != nil { + return fmt.Errorf("can't resolve dir %s: %s", root, err) + } + // TODO: pass cached fis to build.Context + + for _, fi := range fis { + if !fi.IsDir() { + // ignore files: they were already imported by resolveDir(root) + continue + } + + subdir := filepath.Join(root, fi.Name()) + + if r.isIgnoredDir(subdir) { + r.skippedDirs = append(r.skippedDirs, subdir) + continue + } + + if err := r.resolveRecursively(subdir, prog); err != nil { + return err + } + } + + return nil +} + +func (r Resolver) resolveDir(dir string, prog *Program) error { + // TODO: fork build.Import to reuse AST parsing + bp, err := prog.bctx.ImportDir(dir, build.ImportComment|build.IgnoreVendor) + if err != nil { + if _, nogo := err.(*build.NoGoError); nogo { + // Don't complain if the failure is due to no Go source files. + return nil + } + + return fmt.Errorf("can't resolve dir %s: %s", dir, err) + } + + pkg := Package{ + bp: bp, + } + prog.addPackage(&pkg) + return nil +} + +func (r Resolver) addFakePackage(filePath string, prog *Program) { + // Don't take build tags, is it test file or not, etc + // into account. If user explicitly wants to analyze this file + // do it. + p := Package{ + bp: &build.Package{ + GoFiles: []string{filePath}, + }, + isFake: true, + } + prog.addPackage(&p) +} + +func (r Resolver) Resolve(paths ...string) (prog *Program, err error) { + startedAt := time.Now() + defer func() { + logrus.Infof("Paths resolving took %s: %s", time.Since(startedAt), prog) + }() + + if len(paths) == 0 { + return nil, fmt.Errorf("no paths are set") + } + + bctx := build.Default + bctx.BuildTags = append(bctx.BuildTags, r.buildTags...) + prog = &Program{ + bctx: bctx, + } + + root, err := os.Getwd() + if err != nil { + return nil, fmt.Errorf("can't get working dir: %s", err) + } + + for _, path := range paths { + if err := r.resolvePath(path, prog, root); err != nil { + return nil, err + } + } + + if len(r.skippedDirs) != 0 { + logrus.Infof("Skipped dirs: %s", r.skippedDirs) + } + + return prog, nil +} + +func (r *Resolver) resolvePath(path string, prog *Program, root string) error { + needRecursive := strings.HasSuffix(path, "/...") + if needRecursive { + path = filepath.Dir(path) + } + + evalPath, err := filepath.EvalSymlinks(path) + if err != nil { + return fmt.Errorf("can't eval symlinks for path %s: %s", path, err) + } + path = evalPath + + if filepath.IsAbs(path) { + var relPath string + relPath, err = filepath.Rel(root, path) + if err != nil { + return fmt.Errorf("can't get relative path for path %s and root %s: %s", + path, root, err) + } + path = relPath + } + + if needRecursive { + if err = r.resolveRecursively(path, prog); err != nil { + return fmt.Errorf("can't recursively resolve %s: %s", path, err) + } + + return nil + } + + fi, err := os.Stat(path) + if err != nil { + return fmt.Errorf("can't find path %s: %s", path, err) + } + + if fi.IsDir() { + if err := r.resolveDir(path, prog); err != nil { + return fmt.Errorf("can't resolve dir %s: %s", path, err) + } + return nil + } + + r.addFakePackage(path, prog) + return nil +} diff --git a/pkg/fsutils/path_resolver_test.go b/pkg/packages/resolver_test.go similarity index 62% rename from pkg/fsutils/path_resolver_test.go rename to pkg/packages/resolver_test.go index ae3d383afd24..f5adf45ddd7d 100644 --- a/pkg/fsutils/path_resolver_test.go +++ b/pkg/packages/resolver_test.go @@ -1,12 +1,14 @@ -package fsutils +package packages_test import ( "io/ioutil" "os" "path/filepath" + "sort" "strings" "testing" + "github.com/golangci/golangci-lint/pkg/packages" "github.com/stretchr/testify/assert" ) @@ -42,7 +44,8 @@ func prepareFS(t *testing.T, paths ...string) *fsPreparer { continue } - err = ioutil.WriteFile(p, []byte("test"), os.ModePerm) + goFile := "package p\n" + err = ioutil.WriteFile(p, []byte(goFile), os.ModePerm) assert.NoError(t, err) } @@ -53,24 +56,19 @@ func prepareFS(t *testing.T, paths ...string) *fsPreparer { } } -func newPR(t *testing.T) *PathResolver { - pr, err := NewPathResolver([]string{}, []string{}, false) +func newTestResolver(t *testing.T, excludeDirs []string) *packages.Resolver { + r, err := packages.NewResolver(nil, excludeDirs) assert.NoError(t, err) - return pr -} - -func TestPathResolverNoPaths(t *testing.T) { - _, err := newPR(t).Resolve() - assert.EqualError(t, err, "no paths are set") + return r } func TestPathResolverNotExistingPath(t *testing.T) { fp := prepareFS(t) defer fp.clean() - _, err := newPR(t).Resolve("a") - assert.EqualError(t, err, "can't find path a: stat a: no such file or directory") + _, err := newTestResolver(t, nil).Resolve("a") + assert.EqualError(t, err, "can't eval symlinks for path a: lstat a: no such file or directory") } func TestPathResolverCommonCases(t *testing.T) { @@ -103,12 +101,31 @@ func TestPathResolverCommonCases(t *testing.T) { resolve: []string{"./..."}, }, { - name: "vendor implicitely resolved", + name: "nested vendor is excluded", + prepare: []string{"d/vendor/a.go"}, + resolve: []string{"./..."}, + }, + { + name: "vendor dir is excluded by regexp, not the exact match", + prepare: []string{"vendors/a.go", "novendor/b.go"}, + resolve: []string{"./..."}, + expDirs: []string{"vendors"}, + expFiles: []string{"vendors/a.go"}, + }, + { + name: "vendor explicitly resolved", prepare: []string{"vendor/a.go"}, resolve: []string{"./vendor"}, expDirs: []string{"vendor"}, expFiles: []string{"vendor/a.go"}, }, + { + name: "nested vendor explicitly resolved", + prepare: []string{"d/vendor/a.go"}, + resolve: []string{"d/vendor"}, + expDirs: []string{"d/vendor"}, + expFiles: []string{"d/vendor/a.go"}, + }, { name: "extensions filter recursively", prepare: []string{"a/b.go", "a/c.txt", "d.go", "e.csv"}, @@ -131,10 +148,10 @@ func TestPathResolverCommonCases(t *testing.T) { expFiles: []string{"a/c.go"}, }, { - name: "implicitely resolved files", + name: "explicitly resolved files", prepare: []string{"a/b/c.go", "a/d.txt"}, resolve: []string{"./a/...", "a/d.txt"}, - expDirs: []string{"a", "a/b"}, + expDirs: []string{"a/b"}, expFiles: []string{"a/b/c.go", "a/d.txt"}, }, { @@ -149,6 +166,32 @@ func TestPathResolverCommonCases(t *testing.T) { expDirs: []string{"ok"}, expFiles: []string{"ok/b.go"}, }, + { + name: "exclude path, not name", + prepare: []string{"ex/clude/me/a.go", "c/d.go"}, + resolve: []string{"./..."}, + expDirs: []string{"c"}, + expFiles: []string{"c/d.go"}, + }, + { + name: "exclude partial path", + prepare: []string{"prefix/ex/clude/me/a.go", "prefix/ex/clude/me/subdir/c.go", "prefix/b.go"}, + resolve: []string{"./..."}, + expDirs: []string{"prefix"}, + expFiles: []string{"prefix/b.go"}, + }, + { + name: "don't exclude file instead of dir", + prepare: []string{"a/exclude.go"}, + resolve: []string{"a"}, + expDirs: []string{"a"}, + expFiles: []string{"a/exclude.go"}, + }, + { + name: "don't exclude file instead of dir: check dir is excluded", + prepare: []string{"a/exclude.go/b.go"}, + resolve: []string{"a/..."}, + }, { name: "ignore _*", prepare: []string{"_any/a.go"}, @@ -191,9 +234,11 @@ func TestPathResolverCommonCases(t *testing.T) { expFiles: []string{"a/c/d.go", "e.go"}, }, { - name: "vendor dir is excluded by regexp, not the exact match", - prepare: []string{"vendors/a.go", "novendor/b.go"}, - resolve: []string{"./..."}, + name: "resolve absolute paths", + prepare: []string{"a/b.go", "a/c.txt", "d.go", "e.csv"}, + resolve: []string{"${CWD}/..."}, + expDirs: []string{".", "a"}, + expFiles: []string{"a/b.go", "d.go"}, }, } @@ -202,22 +247,34 @@ func TestPathResolverCommonCases(t *testing.T) { fp := prepareFS(t, tc.prepare...) defer fp.clean() - pr, err := NewPathResolver([]string{"vendor"}, []string{".go"}, tc.includeTests) - assert.NoError(t, err) + for i, rp := range tc.resolve { + tc.resolve[i] = strings.Replace(rp, "${CWD}", fp.root, -1) + } - res, err := pr.Resolve(tc.resolve...) + r := newTestResolver(t, []string{"vendor$", "ex/clude/me", "exclude"}) + + prog, err := r.Resolve(tc.resolve...) assert.NoError(t, err) + assert.NotNil(t, prog) + + progFiles := prog.Files(tc.includeTests) + sort.StringSlice(progFiles).Sort() + sort.StringSlice(tc.expFiles).Sort() + + progDirs := prog.Dirs() + sort.StringSlice(progDirs).Sort() + sort.StringSlice(tc.expDirs).Sort() if tc.expFiles == nil { - assert.Empty(t, res.files) + assert.Empty(t, progFiles) } else { - assert.Equal(t, tc.expFiles, res.files) + assert.Equal(t, tc.expFiles, progFiles, "files") } if tc.expDirs == nil { - assert.Empty(t, res.dirs) + assert.Empty(t, progDirs) } else { - assert.Equal(t, tc.expDirs, res.dirs) + assert.Equal(t, tc.expDirs, progDirs, "dirs") } }) } diff --git a/pkg/result/processors/skip_files.go b/pkg/result/processors/skip_files.go index 5b9fa652fa37..92fd1a29be33 100644 --- a/pkg/result/processors/skip_files.go +++ b/pkg/result/processors/skip_files.go @@ -2,7 +2,6 @@ package processors import ( "fmt" - "path/filepath" "regexp" "github.com/golangci/golangci-lint/pkg/result" @@ -39,9 +38,8 @@ func (p SkipFiles) Process(issues []result.Issue) ([]result.Issue, error) { } return filterIssues(issues, func(i *result.Issue) bool { - fileName := filepath.Base(i.FilePath()) for _, p := range p.patterns { - if p.MatchString(fileName) { + if p.MatchString(i.FilePath()) { return false } } diff --git a/pkg/result/processors/skip_files_test.go b/pkg/result/processors/skip_files_test.go index b416cab9187b..df557a055fa1 100644 --- a/pkg/result/processors/skip_files_test.go +++ b/pkg/result/processors/skip_files_test.go @@ -23,17 +23,23 @@ func newTestSkipFiles(t *testing.T, patterns ...string) *SkipFiles { } func TestSkipFiles(t *testing.T) { - p := newTestSkipFiles(t) - processAssertSame(t, p, newFileIssue("any.go")) + processAssertSame(t, newTestSkipFiles(t), newFileIssue("any.go")) - p = newTestSkipFiles(t, "file") - processAssertEmpty(t, p, + processAssertEmpty(t, newTestSkipFiles(t, "file"), newFileIssue("file.go"), newFileIssue("file"), newFileIssue("nofile.go")) - p = newTestSkipFiles(t, ".*") - processAssertEmpty(t, p, newFileIssue("any.go")) + processAssertEmpty(t, newTestSkipFiles(t, ".*"), newFileIssue("any.go")) + + processAssertEmpty(t, newTestSkipFiles(t, "a/b/c.go"), newFileIssue("a/b/c.go")) + processAssertSame(t, newTestSkipFiles(t, "a/b/c.go"), newFileIssue("a/b/d.go")) + + processAssertEmpty(t, newTestSkipFiles(t, ".*\\.pb\\.go"), newFileIssue("a/b.pb.go")) + processAssertSame(t, newTestSkipFiles(t, ".*\\.pb\\.go"), newFileIssue("a/b.go")) + + processAssertEmpty(t, newTestSkipFiles(t, ".*\\.pb\\.go$"), newFileIssue("a/b.pb.go")) + processAssertSame(t, newTestSkipFiles(t, ".*\\.pb\\.go$"), newFileIssue("a/b.go")) } func TestSkipFilesInvalidPattern(t *testing.T) { diff --git a/pkg/timeutils/track.go b/pkg/timeutils/track.go new file mode 100644 index 000000000000..279e4e7b4d65 --- /dev/null +++ b/pkg/timeutils/track.go @@ -0,0 +1,12 @@ +package timeutils + +import ( + "fmt" + "time" + + "github.com/sirupsen/logrus" +) + +func Track(from time.Time, format string, args ...interface{}) { + logrus.Infof("%s took %s", fmt.Sprintf(format, args...), time.Since(from)) +} diff --git a/test/linters_test.go b/test/linters_test.go index 3488ac2414cb..6fb8d3bba853 100644 --- a/test/linters_test.go +++ b/test/linters_test.go @@ -2,9 +2,11 @@ package test import ( "bytes" + "io/ioutil" "os/exec" "path/filepath" "runtime" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -21,9 +23,9 @@ func runGoErrchk(c *exec.Cmd, t *testing.T) { const testdataDir = "testdata" const binName = "golangci-lint" -func TestSourcesFromTestdataWithIssuesDir(t *testing.T) { - t.Log(filepath.Join(testdataDir, "*.go")) - sources, err := filepath.Glob(filepath.Join(testdataDir, "*.go")) +func testSourcesFromDir(t *testing.T, dir string) { + t.Log(filepath.Join(dir, "*.go")) + sources, err := filepath.Glob(filepath.Join(dir, "*.go")) assert.NoError(t, err) assert.NotEmpty(t, sources) @@ -38,20 +40,64 @@ func TestSourcesFromTestdataWithIssuesDir(t *testing.T) { } } +func TestSourcesFromTestdataWithIssuesDir(t *testing.T) { + testSourcesFromDir(t, testdataDir) +} + +func TestTypecheck(t *testing.T) { + testSourcesFromDir(t, filepath.Join(testdataDir, "notcompiles")) +} + func testOneSource(t *testing.T, sourcePath string) { goErrchkBin := filepath.Join(runtime.GOROOT(), "test", "errchk") - cmd := exec.Command(goErrchkBin, binName, "run", + args := []string{ + binName, "run", "--no-config", - "--enable-all", - "--dupl.threshold=20", - "--gocyclo.min-complexity=20", + "--disable-all", "--print-issued-lines=false", "--print-linter-name=false", "--out-format=line-number", - "--print-welcome=false", - "--govet.check-shadowing=true", - "--depguard.include-go-root", - "--depguard.packages='log'", - sourcePath) - runGoErrchk(cmd, t) + } + + for _, addArg := range []string{"", "-Etypecheck"} { + caseArgs := append([]string{}, args...) + caseArgs = append(caseArgs, getAdditionalArgs(t, sourcePath)...) + if addArg != "" { + caseArgs = append(caseArgs, addArg) + } + + caseArgs = append(caseArgs, sourcePath) + + cmd := exec.Command(goErrchkBin, caseArgs...) + t.Log(caseArgs) + runGoErrchk(cmd, t) + } +} + +func getAdditionalArgs(t *testing.T, sourcePath string) []string { + data, err := ioutil.ReadFile(sourcePath) + assert.NoError(t, err) + + lines := strings.SplitN(string(data), "\n", 2) + firstLine := lines[0] + + parts := strings.Split(firstLine, "args:") + if len(parts) == 1 { + return nil + } + + return strings.Split(parts[len(parts)-1], " ") +} + +func TestGolintConsumesXTestFiles(t *testing.T) { + dir := filepath.Join(testdataDir, "withxtest") + const expIssue = "if block ends with a return statement, so drop this else and outdent its block" + + out, ec := runGolangciLint(t, "--no-config", "--disable-all", "-Egolint", dir) + assert.Equal(t, 1, ec) + assert.Contains(t, out, expIssue) + + out, ec = runGolangciLint(t, "--no-config", "--disable-all", "-Egolint", filepath.Join(dir, "p_test.go")) + assert.Equal(t, 1, ec) + assert.Contains(t, out, expIssue) } diff --git a/test/run_test.go b/test/run_test.go index b1af7ed8cae6..5f2b7a33979a 100644 --- a/test/run_test.go +++ b/test/run_test.go @@ -37,6 +37,11 @@ func TestCongratsMessageIfNoIssues(t *testing.T) { checkNoIssuesRun(t, out, exitCode) } +func TestSymlinkLoop(t *testing.T) { + out, exitCode := runGolangciLint(t, filepath.Join(testdataDir, "symlink_loop", "...")) + checkNoIssuesRun(t, out, exitCode) +} + func TestDeadline(t *testing.T) { out, exitCode := runGolangciLint(t, "--deadline=1ms", "../...") assert.Equal(t, 4, exitCode) diff --git a/test/testdata/deadcode.go b/test/testdata/deadcode.go index ffa7c1d7052a..a4172e6a0078 100644 --- a/test/testdata/deadcode.go +++ b/test/testdata/deadcode.go @@ -1,13 +1,14 @@ +// args: -Edeadcode package testdata var y int -var unused int // nolint:megacheck // ERROR "`unused` is unused" +var unused int // ERROR "`unused` is unused" func f(x int) { } -func g(x int) { // nolint:megacheck // ERROR "`g` is unused" +func g(x int) { // ERROR "`g` is unused" } func H(x int) { diff --git a/test/testdata/depguard.go b/test/testdata/depguard.go index 9ed1640ab9bb..11efdce7bd82 100644 --- a/test/testdata/depguard.go +++ b/test/testdata/depguard.go @@ -1,3 +1,4 @@ +// args: -Edepguard --depguard.include-go-root --depguard.packages='log' package testdata import ( diff --git a/test/testdata/dupl.go b/test/testdata/dupl.go index 9fe413ee67ad..9e44ede4c150 100644 --- a/test/testdata/dupl.go +++ b/test/testdata/dupl.go @@ -1,3 +1,4 @@ +// args: -Edupl --dupl.threshold=20 package testdata type DuplLogger struct{} @@ -9,7 +10,7 @@ func (DuplLogger) level() int { func (DuplLogger) Debug(args ...interface{}) {} func (DuplLogger) Info(args ...interface{}) {} -func (logger *DuplLogger) First(args ...interface{}) { // ERROR "12-21 lines are duplicate of `testdata/dupl.go:23-32`" +func (logger *DuplLogger) First(args ...interface{}) { // ERROR "13-22 lines are duplicate of `testdata/dupl.go:24-33`" if logger.level() >= 0 { logger.Debug(args...) logger.Debug(args...) @@ -20,7 +21,7 @@ func (logger *DuplLogger) First(args ...interface{}) { // ERROR "12-21 lines are } } -func (logger *DuplLogger) Second(args ...interface{}) { // ERROR "23-32 lines are duplicate of `testdata/dupl.go:12-21`" +func (logger *DuplLogger) Second(args ...interface{}) { // ERROR "24-33 lines are duplicate of `testdata/dupl.go:13-22`" if logger.level() >= 1 { logger.Info(args...) logger.Info(args...) diff --git a/test/testdata/errcheck.go b/test/testdata/errcheck.go index 2adf6a0be211..1411c12b1c1d 100644 --- a/test/testdata/errcheck.go +++ b/test/testdata/errcheck.go @@ -1,3 +1,4 @@ +// args: -Eerrcheck package testdata import ( diff --git a/test/testdata/gas.go b/test/testdata/gas.go index 6ba96de3bd6c..f446fbd52d9b 100644 --- a/test/testdata/gas.go +++ b/test/testdata/gas.go @@ -1,8 +1,9 @@ +// args: -Egas package testdata import ( "crypto/md5" // ERROR "G501: Blacklisted import crypto/md5: weak cryptographic primitive" - "log" // nolint:depguard + "log" ) func Gas() { diff --git a/test/testdata/goconst.go b/test/testdata/goconst.go index 6d881321f543..b022b17f9b70 100644 --- a/test/testdata/goconst.go +++ b/test/testdata/goconst.go @@ -1,8 +1,9 @@ +// args: -Egoconst package testdata import "fmt" -func GoconstA() { // nolint:dupl +func GoconstA() { a := "needconst" // ERROR "string `needconst` has 5 occurrences, make it a constant" fmt.Print(a) b := "needconst" @@ -20,7 +21,7 @@ func GoconstB() { const AlreadyHasConst = "alreadyhasconst" -func GoconstC() { // nolint:dupl +func GoconstC() { a := "alreadyhasconst" // ERROR "string `alreadyhasconst` has 3 occurrences, but such constant `AlreadyHasConst` already exists" fmt.Print(a) b := "alreadyhasconst" diff --git a/test/testdata/gocyclo.go b/test/testdata/gocyclo.go index b1a9983ba73c..0c93dce5fe4a 100644 --- a/test/testdata/gocyclo.go +++ b/test/testdata/gocyclo.go @@ -1,15 +1,16 @@ +// args: -Egocyclo --gocyclo.min-complexity=20 package testdata func GocycloBigComplexity(s string) { // ERROR "cyclomatic complexity .* of func .* is high .*" - if s == "1" || s == "2" || s == "3" || s == "4" || s == "5" || s == "6" || s == "7" { // nolint:dupl + if s == "1" || s == "2" || s == "3" || s == "4" || s == "5" || s == "6" || s == "7" { return } - if s == "1" || s == "2" || s == "3" || s == "4" || s == "5" || s == "6" || s == "7" { // nolint:dupl + if s == "1" || s == "2" || s == "3" || s == "4" || s == "5" || s == "6" || s == "7" { return } - if s == "1" || s == "2" || s == "3" || s == "4" || s == "5" || s == "6" || s == "7" { // nolint:dupl + if s == "1" || s == "2" || s == "3" || s == "4" || s == "5" || s == "6" || s == "7" { return } } diff --git a/test/testdata/gofmt.go b/test/testdata/gofmt.go index c0370875cc9b..382ccc368013 100644 --- a/test/testdata/gofmt.go +++ b/test/testdata/gofmt.go @@ -1,8 +1,9 @@ +// args: -Egofmt package testdata import "fmt" func GofmtNotSimplified() { var x []string - fmt.Print(x[1:len(x)]) // nolint:megacheck // ERROR "File is not gofmt-ed with -s" + fmt.Print(x[1:len(x)]) // ERROR "File is not gofmt-ed with -s" } diff --git a/test/testdata/goimports.go b/test/testdata/goimports.go index c530933f85e3..e4c92eb3a8c1 100644 --- a/test/testdata/goimports.go +++ b/test/testdata/goimports.go @@ -1,3 +1,4 @@ +// args: -Egoimports package testdata import ( diff --git a/test/testdata/golint.go b/test/testdata/golint.go index 0a99f9787fb3..14e0e0f6d0dd 100644 --- a/test/testdata/golint.go +++ b/test/testdata/golint.go @@ -1,3 +1,4 @@ +// args: -Egolint package testdata var Go_lint string // ERROR "don't use underscores in Go names; var Go_lint should be GoLint" @@ -11,7 +12,7 @@ type ExportedStructWithNoComment struct{} type ExportedInterfaceWithNoComment interface{} -// Bad comment // ERROR "comment on exported function ExportedFuncWithBadComment should be of the form .ExportedFuncWithBadComment \.\.\.." +// Bad comment func ExportedFuncWithBadComment() {} type GolintTest struct{} diff --git a/test/testdata/govet.go b/test/testdata/govet.go index 1474f9d2ec78..e8c210e4aa8b 100644 --- a/test/testdata/govet.go +++ b/test/testdata/govet.go @@ -1,3 +1,4 @@ +// args: -Egovet --govet.check-shadowing=true package testdata import ( diff --git a/test/testdata/ineffassign.go b/test/testdata/ineffassign.go index 7b33adc85e6c..96932a3b489d 100644 --- a/test/testdata/ineffassign.go +++ b/test/testdata/ineffassign.go @@ -1,3 +1,4 @@ +// args: -Eineffassign package testdata func _() { diff --git a/test/testdata/interfacer.go b/test/testdata/interfacer.go index 1d232585b00e..e1a1334644cd 100644 --- a/test/testdata/interfacer.go +++ b/test/testdata/interfacer.go @@ -1,3 +1,4 @@ +// args: -Einterfacer package testdata import "io" diff --git a/test/testdata/maligned.go b/test/testdata/maligned.go index d77e706a4525..a6930d12e72a 100644 --- a/test/testdata/maligned.go +++ b/test/testdata/maligned.go @@ -1,3 +1,4 @@ +// args: -Emaligned package testdata type BadAlignedStruct struct { // ERROR "struct of size 24 bytes could be of size 16 bytes" diff --git a/test/testdata/megacheck.go b/test/testdata/megacheck.go index 5bd963ecddb5..b3b5d84feefb 100644 --- a/test/testdata/megacheck.go +++ b/test/testdata/megacheck.go @@ -1,6 +1,7 @@ +// args: -Emegacheck package testdata func Megacheck() { var x int - x = x // nolint:ineffassign // ERROR "self-assignment of x to x" + x = x // ERROR "self-assignment of x to x" } diff --git a/test/testdata/typecheck.go b/test/testdata/notcompiles/typecheck.go similarity index 81% rename from test/testdata/typecheck.go rename to test/testdata/notcompiles/typecheck.go index a31d55387174..6cd5d42b17fb 100644 --- a/test/testdata/typecheck.go +++ b/test/testdata/notcompiles/typecheck.go @@ -1,3 +1,4 @@ +// args: -Etypecheck package testdata fun NotCompiles() { // ERROR "expected declaration, found 'IDENT' fun" diff --git a/test/testdata/typecheck_many_issues.go b/test/testdata/notcompiles/typecheck_many_issues.go similarity index 93% rename from test/testdata/typecheck_many_issues.go rename to test/testdata/notcompiles/typecheck_many_issues.go index 9de48deb0ed8..69d09b3b8170 100644 --- a/test/testdata/typecheck_many_issues.go +++ b/test/testdata/notcompiles/typecheck_many_issues.go @@ -1,3 +1,4 @@ +// args: -Etypecheck package testdata func TypeCheckBadCalls() { diff --git a/test/testdata/structcheck.go b/test/testdata/structcheck.go index 3e1f35ea6630..e169207e4f0d 100644 --- a/test/testdata/structcheck.go +++ b/test/testdata/structcheck.go @@ -1,5 +1,6 @@ +// args: -Estructcheck package testdata -type t struct { // nolint:megacheck // ERROR "`t` is unused" - unusedField int // nolint:megacheck // ERROR "`unusedField` is unused" +type t struct { + unusedField int // ERROR "`unusedField` is unused" } diff --git a/test/testdata/symlink_loop/pkg/subpkg b/test/testdata/symlink_loop/pkg/subpkg new file mode 120000 index 000000000000..b870225aa053 --- /dev/null +++ b/test/testdata/symlink_loop/pkg/subpkg @@ -0,0 +1 @@ +../ \ No newline at end of file diff --git a/test/testdata/symlink_loop/realpkg/p.go b/test/testdata/symlink_loop/realpkg/p.go new file mode 100644 index 000000000000..c89cd18d0fe7 --- /dev/null +++ b/test/testdata/symlink_loop/realpkg/p.go @@ -0,0 +1 @@ +package p diff --git a/test/testdata/unconvert.go b/test/testdata/unconvert.go index 97399673cef3..c5feab2ffad4 100644 --- a/test/testdata/unconvert.go +++ b/test/testdata/unconvert.go @@ -1,6 +1,7 @@ +// args: -Eunconvert package testdata -import "log" // nolint:depguard +import "log" func Unconvert() { a := 1 diff --git a/test/testdata/varcheck.go b/test/testdata/varcheck.go index 914dd7b66378..6aa9aac12c76 100644 --- a/test/testdata/varcheck.go +++ b/test/testdata/varcheck.go @@ -1,3 +1,4 @@ +// args: -Evarcheck package testdata -var v string // nolint:megacheck // ERROR "`v` is unused" +var v string // ERROR "`v` is unused" diff --git a/test/testdata/withxtest/p.go b/test/testdata/withxtest/p.go new file mode 100644 index 000000000000..c89cd18d0fe7 --- /dev/null +++ b/test/testdata/withxtest/p.go @@ -0,0 +1 @@ +package p diff --git a/test/testdata/withxtest/p_test.go b/test/testdata/withxtest/p_test.go new file mode 100644 index 000000000000..94031e8e9148 --- /dev/null +++ b/test/testdata/withxtest/p_test.go @@ -0,0 +1,11 @@ +package p_test + +import "fmt" + +func WithGolintIssues(b bool) { //nolint:megacheck + if b { + return + } else { + fmt.Print("1") + } +} diff --git a/vendor/github.com/golangci/goconst/parser.go b/vendor/github.com/golangci/goconst/parser.go index f1bc4b28e24c..ab5f99a5073d 100644 --- a/vendor/github.com/golangci/goconst/parser.go +++ b/vendor/github.com/golangci/goconst/parser.go @@ -142,26 +142,27 @@ type Issue struct { MatchingConst string } -func Run(files []string, matchWithConstants bool, minStringLength, minOccurrences int) ([]Issue, error) { - p := New("", "", false, matchWithConstants, false, minStringLength) +type Config struct { + MatchWithConstants bool + MinStringLength int + MinOccurrences int +} + +func Run(files []*ast.File, fset *token.FileSet, cfg *Config) ([]Issue, error) { + p := New("", "", false, cfg.MatchWithConstants, false, cfg.MinStringLength) var issues []Issue - fset := token.NewFileSet() - for _, path := range files { - f, err := parser.ParseFile(fset, path, nil, 0) - if err != nil { - return nil, err - } + for _, f := range files { ast.Walk(&treeVisitor{ fileSet: fset, packageName: "", - fileName: path, + fileName: "", p: p, }, f) } for str, item := range p.strs { // Filter out items whose occurrences don't match the min value - if len(item) < minOccurrences { + if len(item) < cfg.MinOccurrences { delete(p.strs, str) } } diff --git a/vendor/github.com/golang/lint/.travis.yml b/vendor/github.com/golangci/lint-1/.travis.yml similarity index 100% rename from vendor/github.com/golang/lint/.travis.yml rename to vendor/github.com/golangci/lint-1/.travis.yml diff --git a/vendor/github.com/golang/lint/CONTRIBUTING.md b/vendor/github.com/golangci/lint-1/CONTRIBUTING.md similarity index 100% rename from vendor/github.com/golang/lint/CONTRIBUTING.md rename to vendor/github.com/golangci/lint-1/CONTRIBUTING.md diff --git a/vendor/github.com/golang/lint/LICENSE b/vendor/github.com/golangci/lint-1/LICENSE similarity index 100% rename from vendor/github.com/golang/lint/LICENSE rename to vendor/github.com/golangci/lint-1/LICENSE diff --git a/vendor/github.com/golang/lint/README.md b/vendor/github.com/golangci/lint-1/README.md similarity index 100% rename from vendor/github.com/golang/lint/README.md rename to vendor/github.com/golangci/lint-1/README.md diff --git a/vendor/github.com/golang/lint/lint.go b/vendor/github.com/golangci/lint-1/lint.go similarity index 97% rename from vendor/github.com/golang/lint/lint.go rename to vendor/github.com/golangci/lint-1/lint.go index 46bd45f4e2c5..0668635f30dc 100644 --- a/vendor/github.com/golang/lint/lint.go +++ b/vendor/github.com/golangci/lint-1/lint.go @@ -16,6 +16,7 @@ import ( "go/printer" "go/token" "go/types" + "io/ioutil" "regexp" "sort" "strconv" @@ -115,6 +116,46 @@ func (l *Linter) LintFiles(files map[string][]byte) ([]Problem, error) { return pkg.lint(), nil } +// LintFiles lints a set of files of a single package. +// The argument is a map of filename to source. +func (l *Linter) LintASTFiles(files []*ast.File, fset *token.FileSet) ([]Problem, error) { + pkg := &pkg{ + fset: fset, + files: make(map[string]*file), + } + var pkgName string + for _, f := range files { + filename := fset.Position(f.Pos()).Filename + if filename == "" { + return nil, fmt.Errorf("no file name for file %+v", f) + } + + if pkgName == "" { + pkgName = f.Name.Name + } else if f.Name.Name != pkgName { + return nil, fmt.Errorf("%s is in package %s, not %s", filename, f.Name.Name, pkgName) + } + + // TODO: reuse golangci-lint lines cache + src, err := ioutil.ReadFile(filename) + if err != nil { + return nil, fmt.Errorf("can't read file %s: %s", filename, err) + } + + pkg.files[filename] = &file{ + pkg: pkg, + f: f, + fset: pkg.fset, + src: src, + filename: filename, + } + } + if len(pkg.files) == 0 { + return nil, nil + } + return pkg.lint(), nil +} + var ( genHdr = []byte("// Code generated ") genFtr = []byte(" DO NOT EDIT.")