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.

Sign up for GitHub

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: "source" and "code" in publishDiagnostics are reversed #65168

Closed
hyangah opened this issue Jan 19, 2024 · 6 comments
Closed

x/tools/gopls: "source" and "code" in publishDiagnostics are reversed #65168

hyangah opened this issue Jan 19, 2024 · 6 comments
Labels
gopls/analysis Issues related to running analysis in gopls gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@hyangah
Copy link
Contributor

hyangah commented Jan 19, 2024 •
edited

During the investigation of #65167, I noticed that the source/code fields look strange.
Shouldn't they be reversed?

Moreover, it looks like VS Code uses "code" as the identifiers of diagnostics. I am guessing this is used to dedup diagnostics, which is why we saw the editor didn't send code action for SA1006 in #65167.
https://code.visualstudio.com/api/references/vscode-api#Diagnostic

{
    "uri": "file:///Users/hakim/projects/telemetry/cmd/gotelemetry/main.go",
    "version": 407,
    "diagnostics": [
        {
            "range": {...},
            "severity": 2,
            "code": "default",
            "source": "SA1006",
            "message": "printf-style function with dynamic format string and no further arguments should use print-style function instead"
        },
        {
            "range": {...},
            "severity": 2,
            "code": "default",
            "codeDescription": {
                "href": "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf"
            },
            "source": "printf",
            "message": "fmt.Fprintln arg list ends with redundant newline"
        },
        {
            "range": {...},
            "severity": 2,
            "code": "default",
            "codeDescription": {
                "href": "https://pkg.go.dev/golang.org/x/tools/go/analysis/passes/printf"
            },
            "source": "printf",
            "message": "fmt.Fprintln arg list ends with redundant newline"
        }
    ]
}

EDIT: the duplication based on code-only was from misunderstanding. It looks like vscode does not dedup. :-)
@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 Jan 19, 2024
@gopherbot gopherbot added this to the Unreleased milestone Jan 19, 2024
@hyangah hyangah added the gopls/analysis Issues related to running analysis in gopls label Jan 19, 2024
@findleyr
Copy link
Contributor

This is WAI, as best I can tell. The "source" is the analyzer, which may set a more specific code if it reports multiple types of diagnostics.

@findleyr
Copy link
Contributor

I seem to consistently get SA1006 diagnostics:

image

I don't think SA1006 returns diagnostics when the strings are constants.

@findleyr
Copy link
Contributor

Also, note that as of https://go.dev/cl/556755, the "code" is meaningful for gopls' behavior. @adonovan we should probably confirm we're happy with the codes we produce since they are prominent in the UI.

@hyangah
Copy link
Contributor Author

hyangah commented Jan 19, 2024

I think at least for staticcheck, we need to consider to use staticcheck analyzer name as the code. I think we decided to use "code" to formulate the codeDescription.href field.

@findleyr
Copy link
Contributor

#57906 has precise semantics we should use for codeHref. I'm not sure if gopls conforms to those heuristics.

@hyangah
Copy link
Contributor Author

hyangah commented Jan 19, 2024

Assuming each of the staticcheck analysis belongs to an Analyzer, SA1006 is also an Analyzer. Yes, this is working as intended. Gopls can handle staticcheck url computation specially (for example, map SA1006 analysis result to https://staticcheck.dev/docs/checks#SA1006).

Closing. Sorry for the noise.

@hyangah hyangah closed this as not planned Won't fix, can't repro, duplicate, stale Jan 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
gopls/analysis Issues related to running analysis in gopls 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
Unreleased
Development

No branches or pull requests
3 participants
@hyangah @gopherbot @findleyr