From 7c33a561290d6022b3c6bb0bc9b3cc465183f4d2 Mon Sep 17 00:00:00 2001 From: Nikita Shoshin Date: Fri, 31 Mar 2023 16:22:40 +0000 Subject: [PATCH] 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: 8cc75a79d5651b3f5e1e3a0a7bfc4e3795d77763 GitHub-Pull-Request: golang/tools#432 Reviewed-on: https://go-review.googlesource.com/c/tools/+/480695 Run-TryBot: Robert Findley TryBot-Result: Gopher Robot Reviewed-by: Michael Knyszek gopls-CI: kokoro Reviewed-by: Robert Findley --- gopls/internal/lsp/regtest/marker.go | 2 +- gopls/internal/lsp/source/hover.go | 107 +++++++------ gopls/internal/lsp/testdata/godef/a/g.go | 6 - .../internal/lsp/testdata/godef/a/g.go.golden | 7 - .../internal/lsp/testdata/summary.txt.golden | 2 +- .../lsp/testdata/summary_go1.18.txt.golden | 2 +- .../regtest/marker/testdata/hover/const.txt | 141 +++++++++++++++++- .../regtest/marker/testdata/hover/hover.txt | 2 +- 8 files changed, 209 insertions(+), 60 deletions(-) delete mode 100644 gopls/internal/lsp/testdata/godef/a/g.go delete mode 100644 gopls/internal/lsp/testdata/godef/a/g.go.golden diff --git a/gopls/internal/lsp/regtest/marker.go b/gopls/internal/lsp/regtest/marker.go index 9fe63e2ccef..2d18496fc60 100644 --- a/gopls/internal/lsp/regtest/marker.go +++ b/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, diff --git a/gopls/internal/lsp/source/hover.go b/gopls/internal/lsp/source/hover.go index 3e197b484c1..0d1aa6bbbfe 100644 --- a/gopls/internal/lsp/source/hover.go +++ b/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) diff --git a/gopls/internal/lsp/testdata/godef/a/g.go b/gopls/internal/lsp/testdata/godef/a/g.go deleted file mode 100644 index dfef2fb8040..00000000000 --- a/gopls/internal/lsp/testdata/godef/a/g.go +++ /dev/null @@ -1,6 +0,0 @@ -package a - -import "time" - -// dur is a constant of type time.Duration. -const dur = 15*time.Minute + 10*time.Second + 350*time.Millisecond //@dur,hoverdef("dur", dur) diff --git a/gopls/internal/lsp/testdata/godef/a/g.go.golden b/gopls/internal/lsp/testdata/godef/a/g.go.golden deleted file mode 100644 index f7a2e1b0775..00000000000 --- a/gopls/internal/lsp/testdata/godef/a/g.go.golden +++ /dev/null @@ -1,7 +0,0 @@ --- dur-hoverdef -- -```go -const dur time.Duration = 910350000000 // 15m10.35s -``` - -dur is a constant of type time.Duration. - diff --git a/gopls/internal/lsp/testdata/summary.txt.golden b/gopls/internal/lsp/testdata/summary.txt.golden index e5c84a6cc2e..9e13d423834 100644 --- a/gopls/internal/lsp/testdata/summary.txt.golden +++ b/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 diff --git a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden b/gopls/internal/lsp/testdata/summary_go1.18.txt.golden index 6b4ee225f82..184695e658e 100644 --- a/gopls/internal/lsp/testdata/summary_go1.18.txt.golden +++ b/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 diff --git a/gopls/internal/regtest/marker/testdata/hover/const.txt b/gopls/internal/regtest/marker/testdata/hover/const.txt index cdb0e51e27d..6effaf49de6 100644 --- a/gopls/internal/regtest/marker/testdata/hover/const.txt +++ b/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) diff --git a/gopls/internal/regtest/marker/testdata/hover/hover.txt b/gopls/internal/regtest/marker/testdata/hover/hover.txt index f9cd3311bc8..f6a54329a6c 100644 --- a/gopls/internal/regtest/marker/testdata/hover/hover.txt +++ b/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)