golangci-lint integration issue

[butuzov]

TL/DR
Warning message generated by nolintlint linter, brakes vscode-go problem reporting. this is caused by npm child_process executor logic. Current workaround to enable no error code argument supplied to vscode-go golangci-lint linter argumens (--issues-exit-code=0) .

---

What version of Go, VS Code & VS Code Go extension are you using?

• Run go version to get version of Go
  • go version go1.14.5 darwin/amd64
• Run code -v or code-insiders -v to get version of VS Code or VS Code Insiders
  • 1.47.2
• Check your installed extensions to get the version of the VS Code Go extension
  • none + golang.go
• Run go env to get the go development environment details
  • doesn't matter.

Share the Go related settings you have added/edited

"go.lintTool": "golangci-lint",
"go.lintFlags": [
        "-c", ".golangci.yml"   # example https://github.com/butuzov/dots/blob/master/.golangci.yml
]

Describe the bug

This is an integration bug (found while working with golangci-lint) and prevents getting Problems found by linter.

It occurs if combinations of factors happen, If linter output has:

1. Error_Code, StdErr and StdOut: it's (vscode-go) fails to report an code problems.
2. Error_Code and StdOut: everything works.
3. StdOut & StdErr, but no Error_Code: everything works.

Steps to reproduce the behavior:

1. Create example.go

   package main
   
   func main() {}
   
   // there are no such linter "deaccode", it's a typo, but `nolintlint` linter will show warning (perfectly fine). 
   // and this is braks vsc-go 
   
   //nolint:deacdcode
   func add(a, b int) int {
   	a += 10
   	b += 11
   	return a + b
   }
   
   func nouse() {}

2. Run VSCode against folder with this file with default settings () to get failed examples.

3. Run VSCode against folder with this file with default settings + --issues-exit-code=0 in order to get working example.

An example of call to golangci-lint

golangci-lint run --disable-all -E=nolintlint -E=deadcode -E=wsl example.go

Suggestions?

1. Add --issues-exit-code=0 to list of arguments passed to golangci-lint in addition to existing ones (added in steals mode by vscode-go)
2. Adjust logic at https://github.com/golang/vscode-go/blob/master/src/util.ts#L726:L729 as per

Root of the problem

1. node js child_process:

• report an error, if stderr not empty, and return code is > 0
• doesn't change err if stderr has output and return code is 0

test node.js script

const { execFile } = require('child_process');

let args = ['run', '--disable-all', '-E=nolintlint', '-E=deadcode', '-E=wsl'];
['issue.go', 'noissue.go'].forEach((file) => {
	['1', '0'].forEach((error_code_report) => {
		let testArgs = args.slice();
		testArgs.push(`--issues-exit-code=${error_code_report}`, file);

		execFile('golangci-lint', testArgs, (err, stdout, stderr) => {
			console.info('='.repeat(60));
			console.debug(testArgs);
			console.log('err', err);
			console.log('stdOut exists', stdout.length > 0);
			console.log('stdErr exists', stderr.length > 0);
		});
	});
});

run it

node test.js

against next files https://gist.github.com/butuzov/4a682fd28bc89c1bdb6f1cc7996e9cfb

[gopherbot]

Change https://golang.org/cl/244801 mentions this issue: src/goLint.ts: Fixes golangci-lint integration golang/vscode-go#411

[hyangah]

@butuzov thanks for the bug report.

From vscode-go's code, golangci-lint is supposed to output the lint result using stdout. But this nolintlint linter outputs its result in stderr and results in exit code 3 in case of an error. This is different from other linters golangci-lint provides.

Unfortunately, I am not familiar with golangci-lint and I still want to understand what's special about nolintlint and why it behaves differently from other golangci-lint linters. Is there an upstream issue that discusses this?
If not, can you please file an issue upstream first?

We are happy to help to apply the solution once the golangci-lint community comes up with an agreed workaround.

By the way, from the golangci-lint source, I think the issues-exit-code is 0 by default already. So, I am curious to learn how the linked PR changes the default behavior.

[butuzov]

First, I also thought that issue on golangci-lint side, but here are results I found:

1. nolintlint is (kinda) weird linter that reports about not used lintes in //nolint: directive.
2. nolintlint, not failing, but is a place where bug is triggered.
3. stderr if from log/warning message generated to report about an unknown linter and absolutely legit message in this place
4. golangci-lint argument --verbose will turn on an info messages, as for error and warning messages that are shown by default.
5. there are at least 26 places in golangci-lint code where warning is called

P.S.
re: issues-exit-code, not really

golangci-lint run --help | grep issues-exit-code
>       --issues-exit-code int           Exit code when issues were found (default 1)
golangci-lint --version
> golangci-lint has version 1.29.0 built from 6a68907 on 2020-07-24T16:57:30Z

[butuzov]

Regarding my PR.

1. If there are errors found by golangci-lint:
   • Error code is 0, and results are parsed from stdout, err from child_process is empty/null/false
2. If there is no error found by golangci-lint:
   • Error code is 0, results (no results) are parsed, err from child_process is empty/null/false
3. If there are errors found by golangci-lint and warning:
   • Error code is 0, results (no results) are parsed from stdout, err from child_process is empty/null/false because stderr (warning) is ignored because of return code is 0.

[hyangah]

Thanks for explaining the problem.

In my opinion, the issue lies in the logic of runTool in util.ts - that completely ignores stdout when stderr is set and it sees a non-zero exit code. It will break with any tools that utilizes stderr to output non-critical error or log messages. That code needs improvement for long term.

But if it's perfectly ok to set golangci-lint's exit code to 0 unconditionally, I agree the proposed workaround for golangci-lint is a low-risk workaround that does not break other tools invoked using runTool.

[butuzov]

@hyangah thanks for the correction on commit message and spell issue.

And yeah, I don't know if code is used somewhere else (with exact settings err && stderr && !useStdErr). That's why js code included to test child_process included.