Skip to content

Commit

Permalink
Load AST for fast linters in different way.
Browse files Browse the repository at this point in the history
Use build.Import instead of manual parser.ParseFile and paths traversal. It allows:
1. support build tags for all linters.
2. analyze files only for current GOOS/GOARCH: less false-positives.
3. analyze xtest packages (*_test) by golint: upstream golint and
gometalinter can't do it! And don't break analysis on the first xtest
package like it was before.
4. proper handling of xtest packages for linters like goconst where
package boundary is important: less false-positives is expected.

Also:
1. reuse AST parsing for golint and goconst: minor speedup.
2. allow to specify path (not only name) regexp for --skip-files and
--skip-dirs
3. add more default exclude filters for golint about commits:
`(comment on exported (method|function)|should have( a package)?
    comment|comment should be of the form)`
4. print skipped dir in verbose (-v) mode
5. refactor per-linter tests: declare arguments in comments, run only
one linter and in combination with slow linter
  • Loading branch information
jirfag committed Jun 10, 2018
1 parent f5a9bbb commit 2b587b6
Show file tree
Hide file tree
Showing 59 changed files with 754 additions and 511 deletions.
4 changes: 3 additions & 1 deletion .golangci.example.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 8 additions & 8 deletions Gopkg.lock

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

3 changes: 1 addition & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 4 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
6 changes: 3 additions & 3 deletions pkg/commands/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
Expand Down
130 changes: 0 additions & 130 deletions pkg/fsutils/fsutils.go
Original file line number Diff line number Diff line change
@@ -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()
Expand Down
Loading

0 comments on commit 2b587b6

Please sign in to comment.