x/tools/gopls: show hex/binary literal with which a const was declared on mouse over

[soypat]

What did you do?

Mouse over a hex/binary notation integer

const hex, bin, oct = 0xe34e, 0b10101, 071

// Mouse over variable name `hex` or `bin` shows decimal representation
c := hex * bin

What did you expect to see?

The literal used to declare the const. Usually when one declares a const in hex, binary or octal representation it carries with it a significance, to aid the reader of the code. If we wrote programs for machines gofmt would convert these literals to decimals or whatever the computer like.

With the philosophy out of the way, I expected to see:

• const hex untyped int = 0xe34e (58190)
• const bin untyped int = 0b10101 (21).

What did you see instead?

The usual gopls help string of

• const hex untyped int = 58190
• const bin untyped int = 21.

[hyangah]

Isn't it a duplicate of #45802? Can you please check if the fix in golang.org/x/tools/gopls@v0.7.1-pre.1 meets what you want?

[soypat]

@hyangah negative. The issues are similar in nature though. I'm not asking for literal evaluation but rather that gopls show me how the author initialized a const integer since there are around 5 ways of initializing the same number and usually non-decimal form carries useful information about certain bits (i.e. bitfields, masks)

[gopherbot]

Change https://golang.org/cl/381961 mentions this issue: internal/lsp/source: show an original declaration and a value for variables and constants

[soypat]

Hey there! I'd like to bump this issue. This is something I regularly run into. I use Go for firmware/embedded systems and hexadecimals plague my code. It'd be really nice to get this mouse-over information

[soypat]

I wonder, to me it would suffice if the decimal representation was shown next to a hexadecimal representation. This is similar to how time.Duration is displayed on mouse hover:

const time.Millisecond time.Duration = 1000000 // 1ms

[ShoshinNikita]

IMO, it would be even better to always show the original declaration for constants. For example, consider these declarations:

// From src/math/bits.go
const (
	mask     = 0x7FF
	shift    = 64 - 11 - 1
	bias     = 1023
	fracMask = 1<<shift - 1
)

const (
	timeout = time.Minute + 30 * time.Second
)

Here is a comparison between the current hover and the proposed one:

Show only the value (now)	Show both the original declaration and the value
const mask untyped int = 2047 const shift untyped int = 52 const bias untyped int = 1023 const fracMask untyped int = 4503599627370495 const timeout time.Duration = 90000000000 // 1m30s	const mask untyped int = 0x7FF // 2047 const shift untyped int = 64 - 11 - 1 // 52 const bias untyped int = 1023 const fracMask untyped int = 1<<shift - 1 // 4503599627370495 const timeout time.Duration = time.Minute + 30*time.Second // 1m30s

I also checked the GoLand behavior. It shows both the original declaration and the value:

Screenshots

---

I could successfully implement this feature without any serious issues. However, I noticed that the hover logic had been significantly rewritten. And I am not sure that the function internal/lsp/source.objectString (that I used in my original PR) is still a good place to implement this feature. Especially considering this TODO:

// TODO(rfindley): this function does too much. We should lift the special
// handling to callsites.

@findleyr I would like to hear your opinion on this matter.

[findleyr]

@ShoshinNikita first of all, I'm very sorry about https://go.dev/cl/381961, which appears to have gotten stuck in limbo. If you would be interested in seeing this through, we would greatly appreciate the contribution. You can send the CL to me and I'll be sure to get it in expediently.

Yes, I had to refactor hover to disentangle the places where we depended on implicit relationships between packages or parsed files (so that we can break these relationships to improve gopls' scaling). I hope that it is easier to contribute to now, so please let me know if you find otherwise.

If you want to put the logic into the const block of objectString, that's OK. I'll eventually get around to that TODO. That TODO means this: objectString looks almost entirely like types.ObjectString, except when it doesn't: when the object is a const or when the type has inferred arguments (and the latter only occurs at one call site). It would be simpler to avoid the abstraction and just handle the special case(s) at the call site. Specifically, I think we'd just need to handle the const case in the one location where we format a general object string (here).

But as I said, it would be fine if you just modify the const case of objectString.

[ShoshinNikita]

@findleyr no problem. The original PR was too outdated. So, I decided to create a new one - https://go.dev/cl/480695

[soypat]

Just curious: how would it render a constant made out of other constants and literals?

[ShoshinNikita]

@soypat as it was declared:

const fracMask untyped int = 1<<shift - 1 // 4503599627370495

[gopherbot]

Change https://go.dev/cl/480695 mentions this issue: gopls/internal/lsp/source: show both the original declaration and the value of constants in hover