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

Add lint-go-gopls #30729

Merged
merged 20 commits into from
Jun 5, 2024
Merged

Add lint-go-gopls #30729

merged 20 commits into from
Jun 5, 2024

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Apr 27, 2024

Uses gopls check <files> as a linter. Tested locally and brings up 149 errors currently for me. I don't think I want to fix them in this PR, but I would like at least to get this analysis running on CI.

List of errors:

modules/indexer/code/indexer.go:181:11: impossible condition: nil != nil
routers/private/hook_post_receive.go:120:15: tautological condition: nil == nil
services/auth/source/oauth2/providers.go:185:9: tautological condition: nil == nil
services/convert/issue.go:216:11: tautological condition: non-nil != nil
tests/integration/git_test.go:332:9: impossible condition: nil != nil
services/migrations/migrate.go:179:24-43: unused parameter: ctx
services/repository/transfer.go:288:48-69: unused parameter: doer
tests/integration/api_repo_tags_test.go:75:41-61: unused parameter: session
tests/integration/git_test.go:696:64-74: unused parameter: baseBranch
tests/integration/gpg_git_test.go:265:27-39: unused parameter: t
tests/integration/gpg_git_test.go:284:23-29: unused parameter: tmpDir
tests/integration/gpg_git_test.go:284:31-35: unused parameter: name
tests/integration/gpg_git_test.go:284:37-42: unused parameter: email

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 27, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 27, 2024
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 27, 2024
@silverwind
Copy link
Member Author

silverwind commented Apr 27, 2024

The gopls check command currently does not set a proper exit code when there are errors, so we would likely have to analyze the output.

silverwind pushed a commit that referenced this pull request Apr 27, 2024
Suggested by logs in #30729

- Remove `math/rand.Seed`
`rand.Seed is deprecated: As of Go 1.20 there is no reason to call Seed
with a random value.`
- Replace `math/rand.Read`
`rand.Read is deprecated: For almost all use cases, [crypto/rand.Read]
is more appropriate.`
- Replace `math/rand` with `math/rand/v2`, which is available since Go
1.22
@silverwind silverwind marked this pull request as draft April 27, 2024 17:15
@silverwind
Copy link
Member Author

silverwind commented Apr 27, 2024

Lint script added which performs error detection and error counting. It should now fail the CI like this and also be much less verbose with module downloads which it apparently does:

/Users/silverwind/git/gitea/modules/git/commit_info_nogogit.go:100:35-54: unused parameter: ctx [windows,amd64]
/Users/silverwind/git/gitea/modules/git/commit_info_nogogit.go:100:35-54: unused parameter: ctx
Found 149 'gopls check' errors
make: *** [Makefile:432: lint-go-gopls] Error 1

@silverwind silverwind marked this pull request as ready for review April 27, 2024 19:07
tools/lint-go-gopls.sh Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 27, 2024
@silverwind
Copy link
Member Author

silverwind commented Apr 27, 2024

Output and failure detection looks good now. I will add || true so we can merge it without fixing the errors.

@silverwind
Copy link
Member Author

Will post an update later on the outstanding issues. I think we should only merge this after all issues have been fixed.

@silverwind
Copy link
Member Author

49 errors left, #30735 will fix a few.

@silverwind
Copy link
Member Author

silverwind commented Apr 29, 2024

It seems gopls has no support for //nolint currently, so we need to get more creative for technical debt like this one:

//lint:ignore SA1019 We use IsAnInteractiveSession because IsWindowsService has a different permissions profile
isAnInteractiveSession, err := svc.IsAnInteractiveSession() //nolint:staticcheck

@silverwind
Copy link
Member Author

silverwind commented Apr 29, 2024

It seems all deprecations that are warned here already have //nolint comments, so I'll just ignore any error with "is deprecated". Some of these should defintely be looked at though.

@silverwind silverwind marked this pull request as draft April 29, 2024 21:40
silverwind added a commit that referenced this pull request Apr 30, 2024
As discovered by #30729.

---------

Co-authored-by: Giteabot <[email protected]>
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request May 9, 2024
As discovered by go-gitea/gitea#30729.

---------

Co-authored-by: Giteabot <[email protected]>
(cherry picked from commit 610802df85933e7a190a705bc3f7800da87ce868)

Conflicts:
	tests/integration/git_test.go
	trivial conflict because of https://codeberg.org/forgejo/forgejo/pulls/2834
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Jun 4, 2024
@silverwind silverwind marked this pull request as ready for review June 4, 2024 19:02
@silverwind
Copy link
Member Author

silverwind commented Jun 4, 2024

This is ready now. I fixed all non-deprecation errors and one golang deprecation as well.

For reference, here are the remaining deprecated errors that are ignored:

routers/api/v1/repo/branch.go:237:16-33: opt.OldBranchName is deprecated: true Name of the old branch to create from
routers/api/v1/repo/branch.go:238:37-54: opt.OldBranchName is deprecated: true Name of the old branch to create from
routers/api/v1/repo/branch.go:239:54-71: opt.OldBranchName is deprecated: true Name of the old branch to create from
routers/api/v1/repo/branch.go:520:14-29: form.BranchName is deprecated: true
modules/markup/common/footnote.go:200:13-29: util.FindClosure is deprecated: This function can not handle newlines. Many elements can be existed over multiple lines(e.g. link labels). Use text.Reader.FindClosure.
modules/markup/common/footnote.go:290:13-29: util.FindClosure is deprecated: This function can not handle newlines. Many elements can be existed over multiple lines(e.g. link labels). Use text.Reader.FindClosure.
models/db/sql_postgres_with_schema.go:42:25-38: driver.Execer is deprecated: Drivers should implement [ExecerContext] instead.
models/db/sql_postgres_with_schema.go:67:11-20: stmt.Exec is deprecated: Drivers should implement StmtExecContext instead (or additionally).

@silverwind silverwind requested a review from lunny June 4, 2024 19:03
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 5, 2024
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 5, 2024
@lunny lunny merged commit 8162222 into go-gitea:main Jun 5, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Jun 5, 2024
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 5, 2024
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 5, 2024
* giteaofficial/main:
  Add `lint-go-gopls` (go-gitea#30729)
  Make blockquote attention recognize more syntaxes (go-gitea#31240)
  Fix admin oauth2 custom URL settings (go-gitea#31246)
  Replace `gt-word-break` with `tw-break-anywhere` (go-gitea#31183)
  Make pasted "img" tag has the same behavior as markdown image (go-gitea#31235)
  Remove .segment from .project-column (go-gitea#31204)
  Fix overflow on push notification (go-gitea#31179)
  Fix NuGet Package API for $filter with Id equality  (go-gitea#31188)
  Fix overflow on notifications (go-gitea#31178)
  Update chroma to v2.14.0 (go-gitea#31177)
  Update air package path (go-gitea#31233)
  Bump `@github/relative-time-element` to v4.4.1 (go-gitea#31232)
@silverwind silverwind deleted the lint-go-gopls branch June 5, 2024 07:01
silverwind added a commit to silverwind/gitea that referenced this pull request Jun 6, 2024
* origin/main: (231 commits)
  Allow including `Reviewed-on`/`Reviewed-by` lines for custom merge messages (go-gitea#31211)
  Add `MAX_ROWS` option for CSV rendering (go-gitea#30268)
  Update `golang.org/x/net` (go-gitea#31260)
  Add replacement module for `mholt/archiver` (go-gitea#31267)
  Fix Activity Page Contributors dropdown (go-gitea#31264)
  Optimize runner-tags layout to enhance visual experience (go-gitea#31258)
  fix: allow actions artifacts storage migration to complete succesfully (go-gitea#31251)
  Add `lint-go-gopls` (go-gitea#30729)
  Make blockquote attention recognize more syntaxes (go-gitea#31240)
  Fix admin oauth2 custom URL settings (go-gitea#31246)
  Replace `gt-word-break` with `tw-break-anywhere` (go-gitea#31183)
  Make pasted "img" tag has the same behavior as markdown image (go-gitea#31235)
  Remove .segment from .project-column (go-gitea#31204)
  Fix overflow on push notification (go-gitea#31179)
  Fix NuGet Package API for $filter with Id equality  (go-gitea#31188)
  Fix overflow on notifications (go-gitea#31178)
  Update chroma to v2.14.0 (go-gitea#31177)
  Update air package path (go-gitea#31233)
  Bump `@github/relative-time-element` to v4.4.1 (go-gitea#31232)
  Add option for mailer to override mail headers (go-gitea#27860)
  ...
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code modifies/internal size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/code-linting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants