proposal: x/tools/go/analysis/analysistest: support expectations for related information

[tcard]

Proposal Details

At the moment, want comments let you express expectations for diagnostic messages and facts:

// want "diag" "diag2" x:"fact1" x:"fact2" y:"fact3"

But there is no equivalent for other diagnostics: category, URL, suggested fixes, and related information. The only way to check those is through the analysistest.Result returned from analysistest.Run, which defeats the purpose of the analysistest package, as it doesn't much improve on just using the base analysis/checker package.

To keep it minimal, this proposal is restricted to expectations about related information diagnostics.

I propose to extend the want comment format such that, after each message expectation pattern, a related section may follow, with one or more expectations to check against the list of related informations for the diagnostics matched by the preceding message expectation.

The format of each related information expectation is:

expectation = [ Pos ":" ] Message ;
Pos = absolutePos | relativePos ;
relativePos = [ "+" | "-" ] line ;
absolutePos = [ filename ":" ] line [ ":" column ] ;

filename = text ;   # matched as a suffix of token.Position.Filename
line = int ;        # matched exactly with token.Position.Line
column = int ;      # matched exactly with token.Position.Column
Message = pattern ; # between `` or "", just like the base message patterns

Each capitalized nonterminal is matched against the corresponding field in analysis.RelatedInformation.

Some extra rules:

• When Pos is specified, and a absolutePos, but Pos.file is unspecified, it is assumed to be the current file path.
• When Pos is a relativePos, line is interpreted as relative to the comment's file path and line.
• Otherwise, unspecified parts don't impose any constraint on the related information.
• Each expectation must be matched with at least one related information, and, once matched, it isn't considered for matching with other related informations. This is similar to how base diagnostics expectations behave.
• It is OK for some related information not to be matched with any expectation. This is in contrast with base diagnostics, which must all be matched with an expectation.

Examples

(in foo.go:23) // want "diag" related 20:"extra"

Or equivalently:

(in foo.go:23) // want "diag" related -3:"extra"

Matches:

foo.go:23: my diag
    foo.go:20:10: extra info # Matches line and message
    bar.go:23:15: not matched # OK not to be expected

foo.go:23: my diag
    foo.go:20:25: extra info # Matches line, any column is fine

Doesn't match:

foo.go:23: my diag
    bar.go:23: not matched
    # Missing matching related information

foo.go:23: my diag
    foo.go:9999: extra info  # Different line

foo.go:23: my diag
    bar.go:20: extra info  # Different file

---

(in foo.go:23) // want "diag" related "extra"

Doesn't match:

foo.go:23: my diag
    foo.go:20: extra info
    foo.go:10: more extra info # Expectation is "spent" on the first related information

---

(in foo.go:23) // want "diag" related "extra" "extra"

Matches:

foo.go:23: my diag
    foo.go:20: extra info
    foo.go:10: more extra info # Each related information matches an expectation

Doesn't match:

foo.go:23: my diag
    foo.go:20: extra info
   # Missing related information for one of the expectations

---

(in foo.go:23) // want "diag" related bar.go:20:"extra"

Matches:

foo.go:23: my diag
    bar.go:20:2-30:3: extra info # Matches file, line, and message

Doesn't match:

foo.go:23: my diag
    foo.go:20:2-30:3: extra info # Different file

[tcard]

If this proposal is accepted, I would appreciate the opportunity to implement it too.

[adonovan]

This proposal seems reasonable to me. A few thoughts:

Given that absolute line numbers in expectations are inherently fragile, it might be more robust to permit relative ones (e.g. +5) so that insertion of text far away does not require renumbering expectations throughout the file. This has been a problem in diagnostics that include relative information such as lostcancel's "this return statement may be reached without using the %s var defined on line %d".

Such diagnostics should arguably always have been expressed using the RelatedInformation feature, except that we had no way to test it (because of the gap this proposal aims to fix), and possibly also due to patchy client support. If major clients all support RelatedInformation, and analysistest does too, we should fully invest in it and audit our existing analyzers to make them use it consistently.

If we permit +%d relative line numbers, we'll want -%d too, but that conflicts with the proposed Pos "-" End separator. Do we need to test End positions at all? We haven't bothered to do so for Diagnostics.

[adonovan]

I applied the patch below to lostcancel.

• VS Code correctly displays the related information (though the UI is a little clunky: clicking on the related link moves the cursor but doesn't dismiss the popup so it's hard to tell that anything happened at all).
• Emacs+eglot does not display the information.
• Nor does Zed.
• Nor does Helix.
• I didn't try Vim.

Seems like client support for Related information remains patchy, probably because few servers dare to respond with a feature that has patchy client support.

@@ -164,19 +165,38 @@ func runFunc(pass *analysis.Pass, node ast.Node) {
        // single pass over the AST, but seldom is there more than one.)
        for v, stmt := range cancelvars {
                if ret := lostCancelPath(pass, g, v, stmt, sig); ret != nil {
-                       lineno := pass.Fset.Position(stmt.Pos()).Line
-                       pass.ReportRangef(stmt, "the %s function is not used on all paths (possible context leak)", v.Name())
-
                        pos, end := ret.Pos(), ret.End()
                        // golang/go#64547: cfg.Block.Return may return a synthetic
                        // ReturnStmt that overflows the file.
                        if pass.Fset.File(pos) != pass.Fset.File(end) {
                                end = pos
                        }
+
+                       // Report two diagnostics, each referencing the
+                       // other in its Related information.
+
+                       pass.Report(analysis.Diagnostic{
+                               Pos:     stmt.Pos(),
+                               End:     stmt.End(),
+                               Message: fmt.Sprintf("the %s function is not used on all paths (possible context leak)", v.Name()),
+                               Related: []analysis.RelatedInformation{{
+                                       Pos:     pos,
+                                       End:     end,
+                                       Message: "return statement reached without canceling context",
+                               }},
+                       })
+
                        pass.Report(analysis.Diagnostic{
-                               Pos:     pos,
-                               End:     end,
-                               Message: fmt.Sprintf("this return statement may be reached without using the %s var defined on line %d", v.Name(), lineno),
+                               Pos: pos,
+                               End: end,
+                               Message: fmt.Sprintf("this return statement may be reached without using the %s var defined on line %d",
+                                       v.Name(),
+                                       pass.Fset.Position(v.Pos()).Line),
+                               Related: []analysis.RelatedInformation{{
+                                       Pos:     v.Pos(),
+                                       End:     v.Pos() + token.Pos(len(v.Name())),
+                                       Message: fmt.Sprintf("declaration of var %s", v.Name()),
+                               }},
                        })
                }
        }

[tcard]

Thanks @adonovan.

This proposal seems reasonable to me. A few thoughts:

Given that absolute line numbers in expectations are inherently fragile, it might be more robust to permit relative ones (e.g. +5) so that insertion of text far away does not require renumbering expectations throughout the file.

Sounds good to me!

If we permit +%d relative line numbers, we'll want -%d too, but that conflicts with the proposed Pos "-" End separator. Do we need to test End positions at all? We haven't bothered to do so for Diagnostics.

Sounds reasonable. I myself haven't bothered testing them.

If major clients all support RelatedInformation, and analysistest does too, we should fully invest in it and audit our existing analyzers to make them use it consistently. [...] Seems like client support for Related information remains patchy, probably because few servers dare to respond with a feature that has patchy client support.

My motivating use case is a static analyzer that is primarily meant to be run during CI/CD workflows, through a standalone command provided by singlecheck. I would argue that ultimately most analyzers end up running in CI/CD workflows, regardless of whether they also were ran from an editor during development. In that context, related information is always going to be part of the output. So I don't think client support tells the whole story of how relevant related information is.

[adonovan]

Fair enough. I support this proposal, amended relative line numbers and dropping the "end" position.

[gopherbot]

Change https://go.dev/cl/712360 mentions this issue: gopls/internal/analysis/yield: improve diagnostic

[tcard]

Thank you. I've amended the proposal as discussed.

[aclements]

While relative line numbers are certainly less fragile than absolute line numbers, what if we had "anchor" comments instead? For example, we could add a new comment // anchor <name> [<name> ...] that can be used to "name" a line in an input file, and then we could define Pos in your syntax as a string that matches some anchor name.

[tcard]

@aclements Sounds good to me!

Is there any kind of "official" approval (or rejection) I should be waiting for before working on this?

[adonovan]

Is there any kind of "official" approval (or rejection) I should be waiting for before working on this?

I think this is a good idea, but there are definitely some nuances to it and potential for surprises, so the most helpful thing would be to prepare a prototype implementation--perhaps you made one already--to work out any details. Once everyone on this thread is happy with the proposal I'll bring it up at the next meeting. So by all means start work on it. Thanks!