Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

whitespace: failed to get line path/parse/yaccpar:329 #3967

Closed
5 tasks done
theory opened this issue Jul 21, 2023 · 15 comments
Closed
5 tasks done

whitespace: failed to get line path/parse/yaccpar:329 #3967

theory opened this issue Jul 21, 2023 · 15 comments
Labels
area: cgo Related to CGO or line directives bug Something isn't working dependencies Relates to an upstream dependency

Comments

@theory
Copy link

theory commented Jul 21, 2023

Welcome

Description of the problem

Similar to #2788, but chokes on files generated by goyacc, it chokes on lines like this:

//line yaccpar:1

Oddly, if I remove lines starting with //line yacc, I get a different error, where it looks like golangci-lint tries to validate the .y grammar file? The error is:

can't run linter goanalysis_metalinter: whitespace: failed to get line goyacc-tutorial/calculator/grammar_03_final.y:462: invalid file line index0 (461) >= len(fc) (59)

Version of golangci-lint

$ golangci-lint --version
golangci-lint has version 1.53.3 built with go1.20.5 from 2dcd82f on 2023-06-14T21:13:17Z

Configuration

linters:
  enable-all: true
  disable:
    - funlen
    - varnamelen
    - cyclop
    - exhaustruct
    - testpackage
    - gomnd # Failed to get ignored-functions to work below
    # Disable deprecated checks
    - exhaustivestruct
    - varcheck
    - maligned
    - structcheck
    - ifshort
    - golint
    - deadcode
    - interfacer
    - scopelint
    - nosnakecase

issues:
  fix: true

Go environment

$ go version && go env
go version go1.20.3 darwin/arm64
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/david/Library/Caches/go-build"
GOENV="/Users/david/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/david/.gvm/pkgsets/go1.20.3/global/pkg/mod"
GONOPROXY="github.com/nytm/*,github.com/NYTimes/*,github.com/nytimes/*"
GONOSUMDB="github.com/nytm/*,github.com/NYTimes/*,github.com/nytimes/*"
GOOS="darwin"
GOPATH="/Users/david/.gvm/pkgsets/go1.20.3/global"
GOPRIVATE="github.com/nytm/*,github.com/NYTimes/*,github.com/nytimes/*"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/david/.gvm/gos/go1.20.3"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/david/.gvm/gos/go1.20.3/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.3"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/david/dev/go/goyacc-tutorial/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/fl/mkyy75ys4mb372g__86mndsr0000gr/T/go-build375712479=/tmp/go-build -gno-record-gcc-switches -fno-common"

Verbose output of running

$ golangci-lint cache clean
$ golangci-lint run -v
INFO [config_reader] Config search paths: [./ /Users/david/dev/go/goyacc-tutorial /Users/david/dev/go /Users/david/dev /Users/david /Users /] 
INFO [config_reader] Used config file .golangci.yaml 
INFO [lintersdb] Active 91 linters: [asasalint asciicheck bidichk bodyclose containedctx contextcheck decorder depguard dogsled dupl dupword durationcheck errcheck errchkjson errname errorlint execinquery exhaustive exportloopref forbidigo forcetypeassert gci ginkgolinter gocheckcompilerdirectives gochecknoglobals gochecknoinits gocognit goconst gocritic gocyclo godot godox goerr113 gofmt gofumpt goheader goimports gomoddirectives gomodguard goprintffuncname gosec gosimple gosmopolitan govet grouper importas ineffassign interfacebloat ireturn lll loggercheck maintidx makezero mirror misspell musttag nakedret nestif nilerr nilnil nlreturn noctx nolintlint nonamedreturns nosprintfhostport paralleltest prealloc predeclared promlinter reassign revive rowserrcheck sqlclosecheck staticcheck stylecheck tagalign tagliatelle tenv testableexamples thelper tparallel typecheck unconvert unparam unused usestdlibvars wastedassign whitespace wrapcheck wsl zerologlint] 
INFO [loader] Go packages loading at mode 575 (compiled_files|deps|files|name|exports_file|imports|types_sizes) took 124.363166ms 
INFO [runner/filename_unadjuster] Pre-built 2 adjustments in 679.75µs 
INFO [linters_context] importas settings found, but no aliases listed. List aliases under alias: key. 
INFO [linters_context/goanalysis] analyzers took 7.192892032s with top 10 stages: the_only_name: 746.243165ms, buildir: 726.599623ms, buildssa: 481.265414ms, musttag: 291.126125ms, bidichk: 172.961ms, exhaustive: 166.152752ms, gocritic: 139.751375ms, wastedassign: 122.412875ms, dupl: 117.625583ms, goimports: 105.126376ms 
WARN [runner] Can't run linter goanalysis_metalinter: whitespace: failed to get line /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar:329: failed to get file /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar lines cache: can't get file /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar bytes from cache: can't read file /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar: open /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar: no such file or directory 
INFO [runner] processing took 1.497µs with stages: max_same_issues: 375ns, skip_dirs: 250ns, identifier_marker: 166ns, cgo: 125ns, skip_files: 42ns, path_prettifier: 42ns, autogenerated_exclude: 42ns, filename_unadjuster: 42ns, max_from_linter: 42ns, sort_results: 42ns, path_prefixer: 42ns, exclude-rules: 41ns, uniq_by_line: 41ns, path_shortener: 41ns, severity-rules: 41ns, nolint: 41ns, max_per_file_from_linter: 41ns, diff: 41ns, source_code: 0s, fixer: 0s, exclude: 0s 
INFO [runner] linters took 1.126762458s with stages: goanalysis_metalinter: 1.126732375s 
ERRO Running error: 1 error occurred:
	* can't run linter goanalysis_metalinter: whitespace: failed to get line /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar:329: failed to get file /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar lines cache: can't get file /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar bytes from cache: can't read file /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar: open /Users/david/dev/go/goyacc-tutorial/ast_calculator/yaccpar: no such file or directory
 
INFO Memory: 14 samples, avg is 167.2MB, max is 281.7MB 
INFO Execution took 1.265224291s                  

Code example or link to a public repository

git clone [email protected]:slrtbtfs/goyacc-tutorial.git
cd goyacc-tutorial
go mod init github.com/slrtbtfs/goyacc-tutorial
printf "linters:\n  enable-all: true\n" > .golangci.yaml
golangci-lint run -v ./calculator
perl -i -pe 's{^//line yacc.+}{}' calculator/y.go
golangci-lint run -v ./calculator

Validation

  • Yes, I've included all information above (version, config, etc.).
@theory theory added the bug Something isn't working label Jul 21, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 21, 2023

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

@ldez ldez added the area: cgo Related to CGO or line directives label Jul 21, 2023
@ldez
Copy link
Member

ldez commented Jul 22, 2023

Hello,

you have checked this box, can you give me the output of the whitespace linter?

@ldez ldez added the dependencies Relates to an upstream dependency label Jul 22, 2023
@ldez
Copy link
Member

ldez commented Jul 22, 2023

This linter is an old linter that doesn't follow our current standard for a linter, so there is no way to run it as a standalone linter or with go vet 😢

I'm currently discussing with the author of this linter about another topic and this linter is unmaintained.

For now, you have to disable whitespace linter.

@ldez ldez changed the title can't run linter goanalysis_metalinter: whitespace: failed to get line path/parse/yaccpar:329 whitespace: failed to get line path/parse/yaccpar:329 Jul 22, 2023
@theory
Copy link
Author

theory commented Jul 23, 2023

Thanks. I worked around this issue by using the -l option to goyacc to omit the //line lines.

@bombsimon
Copy link
Member

bombsimon commented Jul 24, 2023

The go/parser treats this the same way as compilers/debuggers are suggested to:

The //line directive specifies that the source line that follows should be recorded as having come from the given file path and line number. Successive lines are recorded using increasing line numbers, until the next directive. This directive typically appears in machine-generated code, so that compilers and debuggers will show lines in the original input to the generator.

So if you ask for the file.Position(pos).Filename for a node that is from a file with a //line comment the returned value will be based on this.

This code will print yaccpar. Removing the comment will instead print src.go.

package main

import (
	"fmt"
	"go/ast"
	"go/parser"
	"go/token"
)

const src = `package main

//line yaccpar:1
func main() {}`

func main() {
	fset := token.NewFileSet()
	file, _ := parser.ParseFile(fset, "src.go", src, parser.ParseComments)
	decl := file.Decls[0].(*ast.FuncDecl)
	fmt.Println(fset.Position(decl.Body.Pos()).Filename)
}

Thus this doesn't seem to be isolated to whitespace but any linter that would have a fixer where we would call GetLine from a token.Pos coming from a file/node with a //line comment.

Sine we pass each file to whitespace.Run I was hoping we could just keep track of the filename inside golangci-lint for later lookups but since we need the line for where to fix we get the wrong line reported based on the line comment as well.

My filename attempt
diff --git a/pkg/golinters/whitespace.go b/pkg/golinters/whitespace.go
index e5941fa5..0f509208 100644
--- a/pkg/golinters/whitespace.go
+++ b/pkg/golinters/whitespace.go
@@ -63,9 +63,19 @@ func NewWhitespace(settings *config.WhitespaceSettings) *goanalysis.Linter {
 }
 
 func runWhitespace(lintCtx *linter.Context, pass *analysis.Pass, wsSettings whitespace.Settings) ([]goanalysis.Issue, error) {
-	var messages []whitespace.Message
+	type whitespaceIssue struct {
+		whitespaceMessage whitespace.Message
+		filename          string
+	}
+
+	var messages []whitespaceIssue
 	for _, file := range pass.Files {
-		messages = append(messages, whitespace.Run(file, pass.Fset, wsSettings)...)
+		for _, message := range whitespace.Run(file, pass.Fset, wsSettings) {
+			messages = append(messages, whitespaceIssue{
+				whitespaceMessage: message,
+				filename:          pass.Fset.Position(file.Pos()).Filename,
+			})
+		}
 	}
 
 	if len(messages) == 0 {
@@ -73,10 +83,12 @@ func runWhitespace(lintCtx *linter.Context, pass *analysis.Pass, wsSettings whit
 	}
 
 	issues := make([]goanalysis.Issue, len(messages))
-	for k, i := range messages {
+	for k, wi := range messages {
+		i := wi.whitespaceMessage
+
 		issue := result.Issue{
 			Pos: token.Position{
-				Filename: i.Pos.Filename,
+				Filename: wi.filename,
 				Line:     i.Pos.Line,
 			},
 			LineRange:   &result.Range{From: i.Pos.Line, To: i.Pos.Line},
@@ -85,9 +97,9 @@ func runWhitespace(lintCtx *linter.Context, pass *analysis.Pass, wsSettings whit
 			Replacement: &result.Replacement{},
 		}
 
-		bracketLine, err := lintCtx.LineCache.GetLine(issue.Pos.Filename, issue.Pos.Line)
+		bracketLine, err := lintCtx.LineCache.GetLine(wi.filename, issue.Pos.Line)
 		if err != nil {
-			return nil, fmt.Errorf("failed to get line %s:%d: %w", issue.Pos.Filename, issue.Pos.Line, err)
+			return nil, fmt.Errorf("failed to get line %s:%d: %w", wi.filename, issue.Pos.Line, err)
 		}
 
 		switch i.Type { 

@bombsimon
Copy link
Member

I found the solution and it's simply to use PositionFor instead of Position when getting the token.Pos. I created ultraware/whitespace#7 to fix this.

@ldez
Copy link
Member

ldez commented Jul 24, 2023

@bombsimon whitespace is unmaintained 😉

ultraware/funlen#15 (comment)

@bombsimon
Copy link
Member

Yeah I saw the thread but I also saw someone merged a PR in funlen 4 hours ago so it doesn't seem completely dead. Maybe the change is simple enough to get a merge anyways. 🤞

@ldez
Copy link
Member

ldez commented Jul 24, 2023

Maybe my email had its effect...

ultraware/funlen#15 (comment)

@robinknaapen
Copy link

@bombsimon

Yes, our apologies for the delays. We are striving to maintain the packages. As said in ultraware/funlen#15 (comment)

We are open to pull request. And we will strive to discuss/review/maintain the repositores

@ldez
Copy link
Member

ldez commented Jul 24, 2023

so whitespace should be written to follow our current standard (the custom code inside golangci-lint should be removed)
@bombsimon can you handle that?

@bombsimon
Copy link
Member

so whitespace should be written to follow our current standard (the custom code inside golangci-lint should be removed) @bombsimon can you handle that?

Yes, I can do that. I have limited time the following ~1-2 weeks but I'll start and open a new PR when ready.

@bombsimon
Copy link
Member

@ldez Any suggestions on how to tackle the fixer if converting to analyzer and not returning any result to golangci-lint? Should I traverse the SuggestedFixes and use them before #1779 is fixed or do you have a better solution?

@ldez
Copy link
Member

ldez commented Jul 30, 2023

you can do something like that: https://github.com/4meepo/tagalign/blob/43dc5d806d929c9454a82a2116728b45e1b6f0f0/tagalign.go#L158-L191

@bombsimon
Copy link
Member

This is now fixed in #4003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cgo Related to CGO or line directives bug Something isn't working dependencies Relates to an upstream dependency
Projects
None yet
Development

No branches or pull requests

4 participants