From 816222243af523316041692622be6f48ef068693 Mon Sep 17 00:00:00 2001 From: silverwind Date: Wed, 5 Jun 2024 03:22:38 +0200 Subject: [PATCH] Add `lint-go-gopls` (#30729) Uses `gopls check ` 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 ``` --- Makefile | 10 +++++++++- services/migrations/migrate.go | 2 +- services/repository/transfer.go | 4 ++-- tests/integration/api_repo_tags_test.go | 4 ++-- tests/integration/dump_restore_test.go | 2 +- tests/integration/gpg_git_test.go | 6 +++--- tools/lint-go-gopls.sh | 23 +++++++++++++++++++++++ 7 files changed, 41 insertions(+), 10 deletions(-) create mode 100755 tools/lint-go-gopls.sh diff --git a/Makefile b/Makefile index e9dc945206..d97360c9f4 100644 --- a/Makefile +++ b/Makefile @@ -36,6 +36,7 @@ XGO_PACKAGE ?= src.techknowlogick.com/xgo@latest GO_LICENSES_PACKAGE ?= github.com/google/go-licenses@v1 GOVULNCHECK_PACKAGE ?= golang.org/x/vuln/cmd/govulncheck@v1 ACTIONLINT_PACKAGE ?= github.com/rhysd/actionlint/cmd/actionlint@v1 +GOPLS_PACKAGE ?= golang.org/x/tools/gopls@v0.15.3 DOCKER_IMAGE ?= gitea/gitea DOCKER_TAG ?= latest @@ -213,6 +214,7 @@ help: @echo " - lint-go lint go files" @echo " - lint-go-fix lint go files and fix issues" @echo " - lint-go-vet lint go files with vet" + @echo " - lint-go-gopls lint go files with gopls" @echo " - lint-js lint js files" @echo " - lint-js-fix lint js files and fix issues" @echo " - lint-css lint css files" @@ -366,7 +368,7 @@ lint-frontend: lint-js lint-css lint-frontend-fix: lint-js-fix lint-css-fix .PHONY: lint-backend -lint-backend: lint-go lint-go-vet lint-editorconfig +lint-backend: lint-go lint-go-vet lint-go-gopls lint-editorconfig .PHONY: lint-backend-fix lint-backend-fix: lint-go-fix lint-go-vet lint-editorconfig @@ -424,6 +426,11 @@ lint-go-vet: @GOOS= GOARCH= $(GO) build code.gitea.io/gitea-vet @$(GO) vet -vettool=gitea-vet ./... +.PHONY: lint-go-gopls +lint-go-gopls: + @echo "Running gopls check..." + @GO=$(GO) GOPLS_PACKAGE=$(GOPLS_PACKAGE) tools/lint-go-gopls.sh $(GO_SOURCES_NO_BINDATA) + .PHONY: lint-editorconfig lint-editorconfig: @$(GO) run $(EDITORCONFIG_CHECKER_PACKAGE) $(EDITORCONFIG_FILES) @@ -864,6 +871,7 @@ deps-tools: $(GO) install $(GO_LICENSES_PACKAGE) $(GO) install $(GOVULNCHECK_PACKAGE) $(GO) install $(ACTIONLINT_PACKAGE) + $(GO) install $(GOPLS_PACKAGE) node_modules: package-lock.json npm install --no-save diff --git a/services/migrations/migrate.go b/services/migrations/migrate.go index 5bb3056161..21bdc68e73 100644 --- a/services/migrations/migrate.go +++ b/services/migrations/migrate.go @@ -176,7 +176,7 @@ func newDownloader(ctx context.Context, ownerName string, opts base.MigrateOptio // migrateRepository will download information and then upload it to Uploader, this is a simple // process for small repository. For a big repository, save all the data to disk // before upload is better -func migrateRepository(ctx context.Context, doer *user_model.User, downloader base.Downloader, uploader base.Uploader, opts base.MigrateOptions, messenger base.Messenger) error { +func migrateRepository(_ context.Context, doer *user_model.User, downloader base.Downloader, uploader base.Uploader, opts base.MigrateOptions, messenger base.Messenger) error { if messenger == nil { messenger = base.NilMessenger } diff --git a/services/repository/transfer.go b/services/repository/transfer.go index 3d0bce18d0..9e0ff7ae14 100644 --- a/services/repository/transfer.go +++ b/services/repository/transfer.go @@ -285,7 +285,7 @@ func transferOwnership(ctx context.Context, doer *user_model.User, newOwnerName } // changeRepositoryName changes all corresponding setting from old repository name to new one. -func changeRepositoryName(ctx context.Context, doer *user_model.User, repo *repo_model.Repository, newRepoName string) (err error) { +func changeRepositoryName(ctx context.Context, repo *repo_model.Repository, newRepoName string) (err error) { oldRepoName := repo.Name newRepoName = strings.ToLower(newRepoName) if err = repo_model.IsUsableRepoName(newRepoName); err != nil { @@ -347,7 +347,7 @@ func ChangeRepositoryName(ctx context.Context, doer *user_model.User, repo *repo // local copy's origin accordingly. repoWorkingPool.CheckIn(fmt.Sprint(repo.ID)) - if err := changeRepositoryName(ctx, doer, repo, newRepoName); err != nil { + if err := changeRepositoryName(ctx, repo, newRepoName); err != nil { repoWorkingPool.CheckOut(fmt.Sprint(repo.ID)) return err } diff --git a/tests/integration/api_repo_tags_test.go b/tests/integration/api_repo_tags_test.go index c6eeb404c0..a7f021ca4f 100644 --- a/tests/integration/api_repo_tags_test.go +++ b/tests/integration/api_repo_tags_test.go @@ -42,7 +42,7 @@ func TestAPIRepoTags(t *testing.T) { assert.Equal(t, setting.AppURL+"user2/repo1/archive/v1.1.zip", tags[0].ZipballURL) assert.Equal(t, setting.AppURL+"user2/repo1/archive/v1.1.tar.gz", tags[0].TarballURL) - newTag := createNewTagUsingAPI(t, session, token, user.Name, repoName, "gitea/22", "", "nice!\nand some text") + newTag := createNewTagUsingAPI(t, token, user.Name, repoName, "gitea/22", "", "nice!\nand some text") resp = MakeRequest(t, req, http.StatusOK) DecodeJSON(t, resp, &tags) assert.Len(t, tags, 2) @@ -72,7 +72,7 @@ func TestAPIRepoTags(t *testing.T) { MakeRequest(t, req, http.StatusNotFound) } -func createNewTagUsingAPI(t *testing.T, session *TestSession, token, ownerName, repoName, name, target, msg string) *api.Tag { +func createNewTagUsingAPI(t *testing.T, token, ownerName, repoName, name, target, msg string) *api.Tag { urlStr := fmt.Sprintf("/api/v1/repos/%s/%s/tags", ownerName, repoName) req := NewRequestWithJSON(t, "POST", urlStr, &api.CreateTagOption{ TagName: name, diff --git a/tests/integration/dump_restore_test.go b/tests/integration/dump_restore_test.go index bed2453054..47bb6f76e9 100644 --- a/tests/integration/dump_restore_test.go +++ b/tests/integration/dump_restore_test.go @@ -237,7 +237,7 @@ func (c *compareDump) assertLoadFiles(beforeFilename, afterFilename string, t re // // Given []Something{} create afterPtr, beforePtr []*Something{} // - sliceType := reflect.SliceOf(reflect.PtrTo(t.Elem())) + sliceType := reflect.SliceOf(reflect.PointerTo(t.Elem())) beforeSlice := reflect.MakeSlice(sliceType, 0, 10) beforePtr = reflect.New(beforeSlice.Type()) beforePtr.Elem().Set(beforeSlice) diff --git a/tests/integration/gpg_git_test.go b/tests/integration/gpg_git_test.go index 3ba4a5882c..047c049c7f 100644 --- a/tests/integration/gpg_git_test.go +++ b/tests/integration/gpg_git_test.go @@ -35,7 +35,7 @@ func TestGPGGit(t *testing.T) { defer os.Setenv("GNUPGHOME", oldGNUPGHome) // Need to create a root key - rootKeyPair, err := importTestingKey(tmpDir, "gitea", "gitea@fake.local") + rootKeyPair, err := importTestingKey() if !assert.NoError(t, err, "importTestingKey") { return } @@ -262,7 +262,7 @@ func TestGPGGit(t *testing.T) { }) } -func crudActionCreateFile(t *testing.T, ctx APITestContext, user *user_model.User, from, to, path string, callback ...func(*testing.T, api.FileResponse)) func(*testing.T) { +func crudActionCreateFile(_ *testing.T, ctx APITestContext, user *user_model.User, from, to, path string, callback ...func(*testing.T, api.FileResponse)) func(*testing.T) { return doAPICreateFile(ctx, path, &api.CreateFileOptions{ FileOptions: api.FileOptions{ BranchName: from, @@ -281,7 +281,7 @@ func crudActionCreateFile(t *testing.T, ctx APITestContext, user *user_model.Use }, callback...) } -func importTestingKey(tmpDir, name, email string) (*openpgp.Entity, error) { +func importTestingKey() (*openpgp.Entity, error) { if _, _, err := process.GetManager().Exec("gpg --import tests/integration/private-testing.key", "gpg", "--import", "tests/integration/private-testing.key"); err != nil { return nil, err } diff --git a/tools/lint-go-gopls.sh b/tools/lint-go-gopls.sh new file mode 100755 index 0000000000..4bb69f4c16 --- /dev/null +++ b/tools/lint-go-gopls.sh @@ -0,0 +1,23 @@ +#!/bin/bash +set -uo pipefail + +cd "$(dirname -- "${BASH_SOURCE[0]}")" && cd .. + +IGNORE_PATTERNS=( + "is deprecated" # TODO: fix these +) + +# lint all go files with 'gopls check' and look for lines starting with the +# current absolute path, indicating a error was found. This is neccessary +# because the tool does not set non-zero exit code when errors are found. +# ref: https://github.com/golang/go/issues/67078 +ERROR_LINES=$("$GO" run "$GOPLS_PACKAGE" check "$@" 2>/dev/null | grep -E "^$PWD" | grep -vFf <(printf '%s\n' "${IGNORE_PATTERNS[@]}")); +NUM_ERRORS=$(echo -n "$ERROR_LINES" | wc -l) + +if [ "$NUM_ERRORS" -eq "0" ]; then + exit 0; +else + echo "$ERROR_LINES" + echo "Found $NUM_ERRORS 'gopls check' errors" + exit 1; +fi