gopls/internal/lsp/source: show both the original declaration and the…

… value of constants in hover

This change improves the hover information for constants by showing
both the original declaration and the value. The value is displayed
as an inline comment. If the original declaration and the value are
the same, there will be no inline comment.

Examples:

```go
const duration time.Duration = 15*time.Minute + 10*time.Second // 15m10s

const octal untyped int = 0o777 // 511

const expr untyped int = 2 << (0b111&0b101 - 2) // 16

const boolean untyped bool = (55 - 3) == (26 * 2) // true

const dec untyped int = 500
```

Other changes:

- Calls to `objectString` that format methods or variables have been
  replaced with `types.ObjectString`.
- The logic of inferred signature formatting has been extracted from
  `objectString` to a new function `inferredSignatureString`.
- Remove unused function `extractFieldList`.
- Port `gopls/internal/lsp/testdata/godef/a/g.go` to the new marker
  tests.
- Fix `-min_go` constraint for marker tests by replacing hardcoded
  `1.18` with an actual flag value.

Fixes golang/go#47453

Change-Id: Ib9012dae8554a3b646c3059d2f8967e425974fbf
GitHub-Last-Rev: 8cc75a7
GitHub-Pull-Request: #432
Reviewed-on: https://go-review.googlesource.com/c/tools/+/480695
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>

gopls/internal/lsp/regtest/marker.go

@@ -308,7 +308,7 @@ func RunMarkerTests(t *testing.T, dir string) {
 				if _, err := fmt.Sscanf(test.minGoVersion, "go1.%d", &go1point); err != nil {
 					t.Fatalf("parsing -min_go version: %v", err)
 				}
-				testenv.NeedsGo1Point(t, 18)
+				testenv.NeedsGo1Point(t, go1point)
 			}
 			config := fake.EditorConfig{
 				Settings: test.settings,

gopls/internal/lsp/source/hover.go

@@ -143,7 +143,7 @@ func hover(ctx context.Context, snapshot Snapshot, fh FileHandle, pp protocol.Po
 	// There's not much useful information to provide.
 	if selectedType != nil {
 		fakeObj := types.NewVar(obj.Pos(), obj.Pkg(), obj.Name(), selectedType)
-		signature := objectString(fakeObj, qf, nil)
+		signature := types.ObjectString(fakeObj, qf)
 		return rng, &HoverJSON{
 			Signature:  signature,
 			SingleLine: signature,
@@ -168,10 +168,14 @@ func hover(ctx context.Context, snapshot Snapshot, fh FileHandle, pp protocol.Po
 	docText := comment.Text()
 
 	// By default, types.ObjectString provides a reasonable signature.
-	signature := objectString(obj, qf, nil)
+	signature := objectString(obj, qf, declPos, declPGF.Tok, spec)
+	singleLineSignature := signature
+
 	// TODO(rfindley): we could do much better for inferred signatures.
 	if inferred := inferredSignature(pkg.GetTypesInfo(), ident); inferred != nil {
-		signature = objectString(obj, qf, inferred)
+		if s := inferredSignatureString(obj, qf, inferred); s != "" {
+			signature = s
+		}
 	}
 
 	// For "objects defined by a type spec", the signature produced by
@@ -214,7 +218,7 @@ func hover(ctx context.Context, snapshot Snapshot, fh FileHandle, pp protocol.Po
 				if (m.Obj().Exported() || m.Obj().Pkg() == pkg.GetTypes()) && len(m.Index()) == 1 {
 					b.WriteString(sep)
 					sep = "\n"
-					b.WriteString(objectString(m.Obj(), qf, nil))
+					b.WriteString(types.ObjectString(m.Obj(), qf))
 				}
 			}
 		}
@@ -321,7 +325,7 @@ func hover(ctx context.Context, snapshot Snapshot, fh FileHandle, pp protocol.Po
 	return rng, &HoverJSON{
 		Synopsis:          doc.Synopsis(docText),
 		FullDocumentation: docText,
-		SingleLine:        objectString(obj, qf, nil),
+		SingleLine:        singleLineSignature,
 		SymbolName:        linkName,
 		Signature:         signature,
 		LinkPath:          linkPath,
@@ -577,12 +581,10 @@ func hoverLit(pgf *ParsedGoFile, lit *ast.BasicLit, pos token.Pos) (protocol.Ran
 	}, nil
 }
 
-// objectString is a wrapper around the types.ObjectString function.
-// It handles adding more information to the object string.
-//
-// TODO(rfindley): this function does too much. We should lift the special
-// handling to callsites.
-func objectString(obj types.Object, qf types.Qualifier, inferred *types.Signature) string {
+// inferredSignatureString is a wrapper around the types.ObjectString function
+// that adds more information to inferred signatures. It will return an empty string
+// if the passed types.Object is not a signature.
+func inferredSignatureString(obj types.Object, qf types.Qualifier, inferred *types.Signature) string {
 	// If the signature type was inferred, prefer the inferred signature with a
 	// comment showing the generic signature.
 	if sig, _ := obj.Type().(*types.Signature); sig != nil && typeparams.ForSignature(sig).Len() > 0 && inferred != nil {
@@ -597,22 +599,65 @@ func objectString(obj types.Object, qf types.Qualifier, inferred *types.Signatur
 		str += "// " + types.TypeString(sig, qf)
 		return str
 	}
+	return ""
+}
+
+// objectString is a wrapper around the types.ObjectString function.
+// It handles adding more information to the object string.
+// If spec is non-nil, it may be used to format additional declaration
+// syntax, and file must be the token.File describing its positions.
+func objectString(obj types.Object, qf types.Qualifier, declPos token.Pos, file *token.File, spec ast.Spec) string {
 	str := types.ObjectString(obj, qf)
+
 	switch obj := obj.(type) {
 	case *types.Const:
-		str = fmt.Sprintf("%s = %s", str, obj.Val())
+		var (
+			declaration = obj.Val().String() // default formatted declaration
+			comment     = ""                 // if non-empty, a clarifying comment
+		)
 
-		// Try to add a formatted duration as an inline comment
-		typ, ok := obj.Type().(*types.Named)
-		if !ok {
-			break
+		// Try to use the original declaration.
+		switch obj.Val().Kind() {
+		case constant.String:
+			// Usually the original declaration of a string doesn't carry much information.
+			// Also strings can be very long. So, just use the constant's value.
+
+		default:
+			if spec, _ := spec.(*ast.ValueSpec); spec != nil {
+				for i, name := range spec.Names {
+					if declPos == name.Pos() {
+						if i < len(spec.Values) {
+							originalDeclaration := FormatNodeFile(file, spec.Values[i])
+							if originalDeclaration != declaration {
+								comment = declaration
+								declaration = originalDeclaration
+							}
+						}
+						break
+					}
+				}
+			}
 		}
-		pkg := typ.Obj().Pkg()
-		if pkg.Path() == "time" && typ.Obj().Name() == "Duration" {
-			if d, ok := constant.Int64Val(obj.Val()); ok {
-				str += " // " + time.Duration(d).String()
+
+		// Special formatting cases.
+		switch typ := obj.Type().(type) {
+		case *types.Named:
+			// Try to add a formatted duration as an inline comment.
+			pkg := typ.Obj().Pkg()
+			if pkg.Path() == "time" && typ.Obj().Name() == "Duration" {
+				if d, ok := constant.Int64Val(obj.Val()); ok {
+					comment = time.Duration(d).String()
+				}
 			}
 		}
+		if comment == declaration {
+			comment = ""
+		}
+
+		str += " = " + declaration
+		if comment != "" {
+			str += " // " + comment
+		}
 	}
 	return str
 }
@@ -708,28 +753,6 @@ func parseFull(ctx context.Context, snapshot Snapshot, fset *token.FileSet, pos
 	return pgf, fullPos, nil
 }
 
-// extractFieldList recursively tries to extract a field list.
-// If it is not found, nil is returned.
-func extractFieldList(specType ast.Expr) *ast.FieldList {
-	switch t := specType.(type) {
-	case *ast.StructType:
-		return t.Fields
-	case *ast.InterfaceType:
-		return t.Methods
-	case *ast.ArrayType:
-		return extractFieldList(t.Elt)
-	case *ast.MapType:
-		// Map value has a greater chance to be a struct
-		if fields := extractFieldList(t.Value); fields != nil {
-			return fields
-		}
-		return extractFieldList(t.Key)
-	case *ast.ChanType:
-		return extractFieldList(t.Value)
-	}
-	return nil
-}
-
 func formatHover(h *HoverJSON, options *Options) (string, error) {
 	signature := formatSignature(h, options)
 

gopls/internal/lsp/testdata/summary.txt.golden

@@ -16,7 +16,7 @@ SemanticTokenCount = 3
 SuggestedFixCount = 65
 FunctionExtractionCount = 27
 MethodExtractionCount = 6
-DefinitionsCount = 47
+DefinitionsCount = 46
 TypeDefinitionsCount = 18
 HighlightsCount = 69
 InlayHintsCount = 4

gopls/internal/lsp/testdata/summary_go1.18.txt.golden

@@ -16,7 +16,7 @@ SemanticTokenCount = 3
 SuggestedFixCount = 71
 FunctionExtractionCount = 27
 MethodExtractionCount = 6
-DefinitionsCount = 47
+DefinitionsCount = 46
 TypeDefinitionsCount = 18
 HighlightsCount = 69
 InlayHintsCount = 5

gopls/internal/regtest/marker/testdata/hover/const.txt

@@ -1,12 +1,86 @@
 This test checks hovering over constants.
+
+-- flags --
+-min_go=go1.17
+
 -- go.mod --
 module mod.com
 
-go 1.18
+go 1.17
+
 -- c.go --
 package c
 
+import (
+	"math"
+	"time"
+)
+
 const X = 0 //@hover("X", "X", bX)
+
+// dur is a constant of type time.Duration.
+const dur = 15*time.Minute + 10*time.Second + 350*time.Millisecond //@hover("dur", "dur", dur)
+
+// MaxFloat32 is used in another package.
+const MaxFloat32 = 0x1p127 * (1 + (1 - 0x1p-23))
+
+// Numbers.
+func _() {
+	const hex, bin = 0xe34e, 0b1001001
+
+	const (
+		// no inline comment
+		decimal = 153
+
+		numberWithUnderscore int64 = 10_000_000_000
+		octal                      = 0o777
+		expr                       = 2 << (0b111&0b101 - 2)
+		boolean                    = (55 - 3) == (26 * 2)
+	)
+
+	_ = decimal              //@hover("decimal", "decimal", decimalConst)
+	_ = hex                  //@hover("hex", "hex", hexConst)
+	_ = bin                  //@hover("bin", "bin", binConst)
+	_ = numberWithUnderscore //@hover("numberWithUnderscore", "numberWithUnderscore", numberWithUnderscoreConst)
+	_ = octal                //@hover("octal", "octal", octalConst)
+	_ = expr                 //@hover("expr", "expr", exprConst)
+	_ = boolean              //@hover("boolean", "boolean", boolConst)
+
+	const ln10 = 2.30258509299404568401799145468436420760110148862877297603332790
+
+	_ = ln10 //@hover("ln10", "ln10", ln10Const)
+}
+
+// Iota.
+func _() {
+	const (
+		a = 1 << iota
+		b
+	)
+
+	_ = a //@hover("a", "a", aIota)
+	_ = b //@hover("b", "b", bIota)
+}
+
+// Strings.
+func _() {
+	const (
+		str     = "hello" + " " + "world"
+		longStr = `Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur eget ipsum non nunc
+molestie mattis id quis augue. Mauris dictum tincidunt ipsum, in auctor arcu congue eu.
+Morbi hendrerit fringilla libero commodo varius. Vestibulum in enim rutrum, rutrum tellus
+aliquet, luctus enim. Nunc sem ex, consectetur id porta nec, placerat vel urna.`
+	)
+
+	_ = str     //@hover("str", "str", strConst)
+	_ = longStr //@hover("longStr", "longStr", longStrConst)
+}
+
+// Constants from other packages.
+func _() {
+	_ = math.Log2E //@hover("Log2E", "Log2E", log2eConst)
+}
+
 -- @bX/hover.md --
 ```go
 const X untyped int = 0
@@ -16,3 +90,68 @@ const X untyped int = 0
 
 
 [`c.X` on pkg.go.dev](https://pkg.go.dev/mod.com#X)
+-- @dur/hover.md --
+```go
+const dur time.Duration = 15*time.Minute + 10*time.Second + 350*time.Millisecond // 15m10.35s
+```
+
+dur is a constant of type time.Duration.
+-- @decimalConst/hover.md --
+```go
+const decimal untyped int = 153
+```
+
+no inline comment
+-- @hexConst/hover.md --
+```go
+const hex untyped int = 0xe34e // 58190
+```
+-- @binConst/hover.md --
+```go
+const bin untyped int = 0b1001001 // 73
+```
+-- @numberWithUnderscoreConst/hover.md --
+```go
+const numberWithUnderscore int64 = 10_000_000_000 // 10000000000
+```
+-- @octalConst/hover.md --
+```go
+const octal untyped int = 0o777 // 511
+```
+-- @exprConst/hover.md --
+```go
+const expr untyped int = 2 << (0b111&0b101 - 2) // 16
+```
+-- @boolConst/hover.md --
+```go
+const boolean untyped bool = (55 - 3) == (26 * 2) // true
+```
+-- @ln10Const/hover.md --
+```go
+const ln10 untyped float = 2.30258509299404568401799145468436420760110148862877297603332790 // 2.30259
+```
+-- @aIota/hover.md --
+```go
+const a untyped int = 1 << iota // 1
+```
+-- @bIota/hover.md --
+```go
+const b untyped int = 2
+```
+-- @strConst/hover.md --
+```go
+const str untyped string = "hello world"
+```
+-- @longStrConst/hover.md --
+```go
+const longStr untyped string = "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Curabitur e...
+```
+-- @log2eConst/hover.md --
+```go
+const math.Log2E untyped float = 1 / Ln2 // 1.4427
+```
+
+Mathematical constants.
+
+
+[`math.Log2E` on pkg.go.dev](https://pkg.go.dev/math#Log2E)

gopls/internal/regtest/marker/testdata/hover/hover.txt

@@ -15,7 +15,7 @@ func _() {
 }
 -- @abc/hover.md --
 ```go
-const abc untyped int = 42
+const abc untyped int = 0x2a // 42
 ```
 
 @hover("b", "abc", abc),hover(" =", "abc", abc)