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

Change golangci-lint lint mode to project #3509

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

wandering-tales
Copy link
Contributor

@wandering-tales wandering-tales commented Apr 26, 2024

It seems list_of_files is not the right choice as default lint mode for the golangci-lint linter, for several reasons.

In such mode, the changed .go files are sent in a single call to the linter.

The first issue with that is that, as the linter Quick Start guide explicitly states, files must come from the same package.

The second issue is that the linter tries to compile each file via types.Checker and, unless the source file is self-contained and has no references to things declared in other source files of the codebase, the compilation will fail, even if the references are towards the same package.

Such compilation errors are reported by golangci-lint and labeled as typecheck (which is not a linter) (see why do you have typecheck errors? section of the documentation).

The same problem has also been reported in golangci-lint #1574 by an user who configured his editor to run the linter on single files. As commented by a core maintainer here, typecheck is not included in the "fast" linters list and, therefore, cannot be run on single files.

Proposed Changes

  1. Change golangci-lint lint mode to project

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@wandering-tales
Copy link
Contributor Author

wandering-tales commented Apr 26, 2024

Besides, it looks like the typecheck "linter" has been added in #2700. That PR was meant to solve a completely different issue, but it also added/removed linters in the golangci-lint configuration template.

Said that, as an alternative, the solution to this issue might simply be removing typecheck from the linters list.

It seems `list_of_files` is not the right choice as default lint
mode for the `golangci-lint` linter, for several reasons.

In such mode, the changed `.go` files are sent in a single call to the
linter.

The first issue with that is that, as the linter
[Quick Start](https://golangci-lint.run/welcome/quick-start/) guide
explicitly states, files must come from the same package.

The second issue is that the linter tries to compile each file via
`types.Checker` and, unless the source file is self-contained and has no
references to things declared in other source files of the codebase, the
compilation will fail, even if the references are towards the same
package.

Such compilation errors are reported by `golangci-lint` and labeled as
`typecheck` (which is not a linter) (see
[why do you have `typecheck` errors?](https://golangci-lint.run/welcome/faq/#why-do-you-have-typecheck-errors)
section of the documentation).

The same problem has also been reported in
[`golangci-lint oxsecurity#1574`](golangci/golangci-lint#1574)
by an user who configured his editor to run the linter on single files.
As commented by a core maintainer
[here](golangci/golangci-lint#1574 (comment))
`typecheck` is not included in the "fast" linters list and, therefore,
cannot be run on single files.
@nvuillam
Copy link
Member

Mode "list_of_files" is critical to VALIDATE_ALL_CODEBASE=false users, who want to analyze only files updated within a PR

Using project mode will be default analyze all go files undepending VALIDATE_ALL_CODEBASE mode, i'm not sure go project users will like it

But your argumentation is convincing, can I summarize it by "not using project mode by default with golangci-lint generates crappy results" ? ^^

If so, i'll merge the PR :)

@wandering-tales
Copy link
Contributor Author

wandering-tales commented Apr 29, 2024

Actually, I am not convinced it's just about "crappy results", as MegaLinter simply fails. typecheck is included in the default list of golangci-lint linters, but it cannot be run on a single file.

If you feel that setting project might be too aggressive as a change to make the default linters works, the alternative, as I suggested, is to remove typecheck from that list as all the other linters could be run on single files. WDYT?

@nvuillam
Copy link
Member

nvuillam commented Apr 29, 2024

@wandering-tales I never wrote a single line in go, that's why I strongly rely on your opinion :)

Is typecheck a relevant rule ?

@wandering-tales
Copy link
Contributor Author

@wandering-tales I never wrote a single line in go, that's why I strongly rely on your opinion :)

Is typecheck a relevant rule ?

typecheck checks for compiling errors and it's not a real linter. Errors returned by typecheck are merely meant to warn the user that any attempt to build the analyzed file(s) would fail. However, by reading again the documentation, I confirm it cannot be disabled:

[...] It cannot be disabled because of that.

And more further on running it on a single file or a group of files:

As a consequence, the code to analyze should compile. It means that if you try to run an analysis on a single file or a group of files or a package or a group of packages, with dependencies on other files or packages of your project, as it doesn't compile (because of the missing pieces of code), it also cannot be analyzed.

Said that, I fear the only viable option to make golangci-lint work are the changes in this PR, unfortunately at the expense of performance.

Copy link
Member

@nvuillam nvuillam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wandering-tales I'm convinced by your explanations, thanks for your analysis and the PR :)

@nvuillam nvuillam enabled auto-merge (squash) April 29, 2024 12:27
@nvuillam nvuillam merged commit 56c1b8b into oxsecurity:main Apr 29, 2024
6 checks passed
@wandering-tales
Copy link
Contributor Author

@wandering-tales I'm convinced by your explanations, thanks for your analysis and the PR :)

Many thanks to you for the quick feedback!

@nvuillam
Copy link
Member

nvuillam commented Apr 29, 2024

You can already check with beta version :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants