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

Jump to bottom

x/tools/gopls: unusedparams should ignore unused params in functions satisfying the http.HandlerFunc type #44152

Closed
PSalant726 opened this issue Feb 7, 2021 · 5 comments
Closed

x/tools/gopls: unusedparams should ignore unused params in functions satisfying the http.HandlerFunc type #44152

PSalant726 opened this issue Feb 7, 2021 · 5 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/unplanned

Comments

@PSalant726
Copy link

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

  • Run go version to get version of Go from the VS Code integrated terminal.
    • 1.15.7 darwin/amd64
  • Run gopls -v version to get version of Gopls from the VS Code integrated terminal.
    • 0.6.5
  • Run code -v or code-insiders -v to get version of VS Code or VS Code Insiders.
    • 1.53.0
  • Check your installed extensions to get the version of the VS Code Go extension
    • 0.22.0

Share the Go related settings you have added/edited

"[go]": {
        "editor.codeActionsOnSave": { "source.organizeImports": true },
        "editor.formatOnSave": true
},
"[go.mod]": {
        "editor.codeActionsOnSave": { "source.organizeImports": true },
        "editor.formatOnSave": true
},
"go.coverOnTestPackage": false,
"go.formatTool": "goimports",
"go.lintFlags": ["--fast", "--max-same-issues=0"],
"go.lintTool": "golangci-lint",
"go.playground": { "run": false },
"go.testFlags": ["-count=1", "-cover"],
"go.toolsManagement.autoUpdate": true,
"gopls": {
        "ui.codelenses": { "generate": false, "test": true },
        "ui.completion.usePlaceholders": true,
        "ui.diagnostic.analyses": { "unusedparams": true }
}

Describe the bug

With code like the below example, where a function (previewHandler) satisfies the http.HanderlFunc type, but does not use or modify the http.Request (second argument), the unusedparams analyzer included with gopls should not warn on the issue.

package main

import (
  "log"
  "net/http"
  "text/template"

  "github.com/gorilla/mux"
)

func NewRouterWithRoutes() *mux.Router {
  var (
    router = mux.NewRouter()
    get    = router.Methods(http.MethodGet).Subrouter()
  )

  get.HandleFunc(PathPreview, previewHandler)

  return router
}

func previewHandler(w http.ResponseWriter, r *http.Request) {  // WARNING: potentially unused parameter: 'r' (unusedparams)
  if err := templates.ExecuteTemplate(w, "preview.html", &Page{}); err != nil {
    http.Error(w, err.Error(), http.StatusInternalServerError)
    log.Println(err)
  }
}
@stamblerre stamblerre transferred this issue from golang/vscode-go Feb 7, 2021
@hyangah
Copy link
Contributor

hyangah commented Feb 7, 2021

Don't people use _ or unnamed params to make it clear the params aren't used in the body?
The unusedparams analyzer already ignores parameters that do not have a name or are underscored.

cc @stamblerre

@PSalant726
Copy link
Author

Don't people use _ or unnamed params to make it clear the params aren't used in the body?

Yes, some do, but this isn't strictly enforced by the compiler. I don't believe that gopls should warn on functions that implement such a commonly used type.

@hyangah hyangah changed the title unusedparams should ignore unused params in functions satisfying the http.HandlerFunc type x/tools/gopls: unusedparams should ignore unused params in functions satisfying the http.HandlerFunc type Feb 7, 2021
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Feb 7, 2021
@gopherbot gopherbot added this to the Unreleased milestone Feb 7, 2021
@giter
Copy link

giter commented Jan 15, 2022 •
edited

I don't think we should use _ to mark a large number of context.Context

@findleyr
Copy link
Contributor

I think there are benefits to leaving this diagnostic for unused parameters, even in http.HandlerFunc. Imagine the following scenario, where there are two writers in scope.

image

In this case, the unusedparams diagnostic makes it more noticeable that the wrong writer is being used. This is admittedly a bit contrived, but not unreasonable, IMO.

Generally, I think the unusedparams analyzer falls into the category of "opinionated" analyzers, that not everyone agrees with. This is why it is disabled by default. When using this analyzer, I sometimes do need to rename parameters to _ just to make the analyzer happy. However, in those cases it arguably improves the clarity of the code:

  • It becomes locally evident that the parameter is not used.
  • There is one fewer name in scope.
  • It is clear to the next reader that the parameter is intentionally unused.

To add to that, there is a nontrivial cost to keeping a list of signatures that are exempted from unused params diagnostics, both in terms of maintenance, and in terms of the cognitive overhead for users, who may be confused by a missing diagnostic.

Based on this, I think we will likely decline to make this change.

@PSalant726
Copy link
Author

Based on feedback from @hyangah and @findleyr , and after continued use of unusedparams in several projects, I no longer believe this omission should be implemented.

If others disagree, I would encourage them to reopen this issue and continue to push it forward.

@findleyr findleyr closed this as not planned Won't fix, can't repro, duplicate, stale Aug 14, 2022
@golang golang locked and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/unplanned
Development

No branches or pull requests
6 participants
@giter @hyangah @stamblerre @gopherbot @PSalant726 @findleyr