golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.89k stars 17.65k forks source link

x/tools/gopls: hover for format strings #70050

Open findleyr opened 1 week ago

findleyr commented 1 week ago

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 commented 1 week ago

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:

findleyr commented 1 week ago

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 commented 1 week ago

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 commented 6 days ago

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)