x/tools/gopls: Hover/DocumentHighlight for format strings

[findleyr]

This just occurred to me, when looking up a rarely used format verb for the nth time at pkg.go.dev/fmt: we should offer hover information for format strings. To start, this could simply show documentation for the active format verb under the cursor, but we could also extend it to explain more complicated formatting expressions.

We could extract the tricky bits of detecting and parsing format strings from the printf analyzer, though that begs the question of why we can't detect printf wrappers... @adonovan I wonder what you think about this idea, and more generally about the concept of exposing facts from the analysis framework to other gopls functionality.

Note that accurate detection of printf wrappers is not strictly necessary--that's more of a thought experiment. For hover, we could use a much weaker heuristic to guess whether a string is likely to be a format string.

[adonovan]

Format strings are only one small step removed from the language spec in the sense that every Go file uses them and every Go reader is expected to recognize them, or at least the dozen or so most common ones. Just as I would find it odd to serve a hover box over the go keyword that says it starts a new goroutine, it seems to me redundant to show a dialog over %q saying it quotes a string. Though it may benefit the beginner, I expect it would do so at the cost of the Go programmers who have passed that phase and find the extra popups a nuisance.

To your bigger implied questions:

• Yes, I'm sure there's a lot we could do with better integration, though the mechanics seem tricky: hover depends only on typed syntax, whereas facts depend on analysis. Perhaps, just as analyses can report SuggestedFixes, which we tie together with diagnostics, they could also [waves hands] contribute information to hover reports.

• A somewhat related issue for interpreting "little languages" in string literals such as fmt, SQL, and text/template (and perhaps third-party ones like https://github.com/a-h/templ if that is the de facto victor), is proposal: text/template: annotations to enable static checking of syntax and semantics #64543. I think templ has a VS Code extension for completions, but I assume that only works in standalone files. If raw string literals are commonly used, it would be neat if we could parse the strings, interpret their nonstandard types (as we do with fmt), and show both diagnostics and cross-references in a general way, at the correct portion of the string literal.

[findleyr]

Though it may benefit the beginner, I expect it would do so at the cost of the Go programmers who have passed that phase and find the extra popups a nuisance.

I'm not following this argument. FWIW, this issue is premised on the fact that this would help me, and I'm not a beginner! We offer hover for builtins, even though they're part of the spec... Certainly one uses make more than %p. You may argue that 'make' depends on its arguments, but so does the behavior of %p! (And for the record, I never have to look up %q, even though its behavior is rather more complicated than others). In fact, I'd go so far as to say it would be nice to offer completion after typing %.

Unfortunately, you may be right that the hover could be a nuisance, but then aren't all hovers a nuisance? In my editor, I only trigger hover explicitly.

[adonovan]

Fair point. I remember I once asked you why we bothered with hover on built-ins and you responded that they're really useful.

I do think there would be real value in tying each %v formatting operation with its operand, as when they are numerous it can be hard to see the correspondence at a glance (which is why Python now has f-strings). Perhaps DocumentHighlight is the ideal LSP UI? I still have concerns about efficiency if lightweight queries such as Hover and Highlight were to invoke the analysis framework, even if only in particular cases with a restricted subset of analyzers. We hack up a little experiment.

[xzbdmw]

I'd go so far as to say it would be nice to offer completion after typing %

It would be so much better if

type Bar struct {}
var foo int
var bar Bar
fmt.Printf("%foo, %bar")

can expands to

type Bar struct {}
var foo int
var bar Bar
fmt.Printf("%d, %v", foo, bar)

via completion then a (range) code action, given that go has rejected f-string proposal, it is always a pain if arguments are too many. (why we need to specify the various %d %v etc.. if complier already knows the type)

[xzbdmw]

Perhaps DocumentHighlight is the ideal LSP UI? I still have concerns about efficiency if lightweight queries such as Hover and Highlight were to invoke the analysis framework, even if only in particular cases with a restricted subset of analyzers. We hack up a little experiment.

DocumentHighlight should solve the "counting" process quite well, I'm writing a lot long printf so I want to implement this feature, where should I start?

(after moving the undeclared analyzer to gopls/internal, the response speed improved quit a lot, I guess it is a good thing to slowly factor the printf analyzer out?)

[adonovan]

DocumentHighlight should solve the "counting" process quite well, I'm writing a lot long printf so I want to implement this feature, where should I start?

You'll need a function that, given a string literal and an offset, returns an index within the denoted string; and a function that, given a logical format string and an offset, returns the index of the % conversion (if any) that it falls within. The simplest way to do the second one would be to follow the approach in #68279 (comment), which isn't perfect, but is good enough for >99.9% of strings.

Once you have those, hooking into internal/golang/highlight.go for CallExpr shouldn't be hard. The only tricky bit is how to know which functions to apply it to: obviously fmt.Printf, but what about all the wrappers? We could use the analysis framework, but there are a lot of tricky problems there. Since this is only highlighting, it needn't be perfect: perhaps it's enough to observe that the format string contains a %, or approximately as many %s as arguments?

[findleyr]

Starting with DocumentHighlight makes sense to me.

after moving the undeclared analyzer to gopls/internal, the response speed improved quit a lot, I guess it is a good thing to slowly factor the printf analyzer out

FWIW, that's because there's a 1s debouncing before we run analysis.

[xzbdmw]

I think this can be simpler without parsing format string and args. The format syntax is %[flags][width][.precision][verb], what we need for DocumentHighlight is only the range of the placeholder, that is, a range starts with '%' and ends with a valid verb, and details sits between them can be ignored. So we can just find the '%', find the next verb, get the range & index, then we are nearly done.

or approximately as many %s as arguments

The main use case is to writing the arg list without counting which % I'm matching, so I think it is better to not assume any args, but try to highlight one if there is a match.

Update: let's include all information, since hover may want to show what is the meaning of a precision/width/flag.

[xzbdmw]

Should we also paint the format string with semantic highlight? Once we have a iter describing each element of a string in order, highlighting becomes free too, though vscode's textmate rule and treesitter-printf (I guess many people doesn't know it exists) can detect something similar. But it may benefit other editors.

[gopherbot]

Change https://go.dev/cl/632598 mentions this issue: gopls/internal/highlight: DocumentHighlight for format strings

[xzbdmw]

#68279 (comment), This method has limitations that can't handle explicit argument indexes, eg. fmt.Sprintf("%[2]d %[1]d\n", 11, 22), also directives of %T, %p, will not hit wrapper logic, because

go/src/fmt/print.go

Lines 695 to 704 in c46ba1f

	// Special processing considerations.
	// %T (the value's type) and %p (its address) are special; we always do them first.
	switch verb {
	case 'T':
	p.fmt.fmtS(reflect.TypeOf(arg).String())
	return
	case 'p':
	p.fmtPointer(reflect.ValueOf(arg), 'p')
	return
	}

This can be ignored for DocumentHighlight( just replace them with %s is fine) but may be surprised for hovering usage.
Is these acceptable or should I go another way?

[adonovan]

Good point about %T and %p; sorry I didn't notice that sooner. (I added a warning at the other issue.)

Is these acceptable or should I go another way?

Feel free to take another approach, such as writing a parser for the format string (with a suitable test suite---perhaps you can devise a test where your parser is cross-checked against the actual implementation of fmt.Printf to ensure that it computes the correct answer).