[tools] gopls/internal/server: support type def with selected range
Hongxiang Jiang (Gerrit)
Hongxiang Jiang has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal/server: support type def with selected range
The logical validation for textDocumentPositionParam is moved
to protocol level ensures the validation will be done for all
LSP method param who extends textDocumentPositionParam.
gopls type def will be able to interpret the selected range
from the lsp client. If the selected range evaludate to an
expression, gopls will return the type of the expression.
gopls will return multiple locations for types.Signature or
Tuple without any special filering for type error.
Fix golang/go#76723
Change diff
diff --git a/gopls/internal/golang/hover.go b/gopls/internal/golang/hover.go
index a9e9b0c..620b2c7 100644
--- a/gopls/internal/golang/hover.go
+++ b/gopls/internal/golang/hover.go
@@ -708,9 +708,9 @@
case *types.Func:
sig := obj.Signature()
if sig.Recv() != nil {
- tname := typeToObject(sig.Recv().Type())
- if tname != nil { // beware typed nil
- recv = tname
+ tnames := typeToObjects(sig.Recv().Type())
+ if len(tnames) == 1 { // beware empty slice
+ recv = tnames[0]
}
}
case *types.Var:
diff --git a/gopls/internal/golang/identifier.go b/gopls/internal/golang/identifier.go
index 2f63483..d7bbb66 100644
--- a/gopls/internal/golang/identifier.go
+++ b/gopls/internal/golang/identifier.go
@@ -62,49 +62,33 @@
return exported
}
-// typeToObject returns the relevant type name for the given type, after
-// unwrapping pointers, arrays, slices, channels, and function signatures with
-// a single non-error result, and ignoring built-in named types.
-func typeToObject(typ types.Type) *types.TypeName {
+// typeToObjects returns the underlying type objects for the given type.
+// It unwraps composite types (pointers, slices, function signatures, etc.)
+// and ignores basic types.
+func typeToObjects(typ types.Type) []*types.TypeName {
switch typ := typ.(type) {
case *types.Alias:
- return typ.Obj()
+ return []*types.TypeName{typ.Obj()}
case *types.Named:
// TODO(rfindley): this should use typeparams.NamedTypeOrigin.
- return typ.Obj()
+ return []*types.TypeName{typ.Obj()}
case *types.Pointer:
- return typeToObject(typ.Elem())
+ return typeToObjects(typ.Elem())
case *types.Array:
- return typeToObject(typ.Elem())
+ return typeToObjects(typ.Elem())
case *types.Slice:
- return typeToObject(typ.Elem())
+ return typeToObjects(typ.Elem())
case *types.Chan:
- return typeToObject(typ.Elem())
- case *types.Signature:
- // Try to find a return value of a named type. If there's only one
- // such value, jump to its type definition.
- var res *types.TypeName
-
- results := typ.Results()
- for v := range results.Variables() {
- obj := typeToObject(v.Type())
- if obj == nil || hasErrorType(obj) {
- // Skip builtins. TODO(rfindley): should comparable be handled here as well?
- continue
- }
- if res != nil {
- // The function/method must have only one return value of a named type.
- return nil
- }
-
- res = obj
+ return typeToObjects(typ.Elem())
+ case *types.Tuple:
+ var res []*types.TypeName
+ for v := range typ.Variables() {
+ res = append(res, typeToObjects(v.Type())...)
}
return res
+ case *types.Signature:
+ return typeToObjects(typ.Results())
default:
return nil
}
}
-
-func hasErrorType(obj types.Object) bool {
- return types.IsInterface(obj.Type()) && obj.Pkg() == nil && obj.Name() == "error"
-}
diff --git a/gopls/internal/golang/type_definition.go b/gopls/internal/golang/type_definition.go
index 48b62fe..003dc0f 100644
--- a/gopls/internal/golang/type_definition.go
+++ b/gopls/internal/golang/type_definition.go
@@ -19,7 +19,7 @@
)
// TypeDefinition handles the textDocument/typeDefinition request for Go files.
-func TypeDefinition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, position protocol.Position) ([]protocol.Location, error) {
+func TypeDefinition(ctx context.Context, snapshot *cache.Snapshot, fh file.Handle, rng protocol.Range) ([]protocol.Location, error) {
ctx, done := event.Start(ctx, "golang.TypeDefinition")
defer done()
@@ -27,11 +27,11 @@
if err != nil {
return nil, err
}
- pos, err := pgf.PositionPos(position)
+ start, end, err := pgf.RangePos(rng)
if err != nil {
return nil, err
}
- cur, ok := pgf.Cursor.FindByPos(pos, pos)
+ cur, ok := pgf.Cursor.FindByPos(start, end)
if !ok {
return nil, fmt.Errorf("no enclosing syntax") // can't happen
}
@@ -69,13 +69,19 @@
if t == nil {
return nil, fmt.Errorf("no enclosing expression has a type")
}
- tname := typeToObject(t)
- if tname == nil {
+ tnames := typeToObjects(t)
+ if len(tnames) == 0 {
return nil, fmt.Errorf("cannot find type name from type %s", t)
}
- loc, err := ObjectLocation(ctx, pkg.FileSet(), snapshot, tname)
- if err != nil {
- return nil, err
+
+ var locs []protocol.Location
+ for _, t := range tnames {
+ loc, err := ObjectLocation(ctx, pkg.FileSet(), snapshot, t)
+ if err != nil {
+ return nil, err
+ }
+ locs = append(locs, loc)
}
- return []protocol.Location{loc}, nil
+
+ return locs, nil
}
diff --git a/gopls/internal/protocol/generate/output.go b/gopls/internal/protocol/generate/output.go
index 4c954f8..7b96af8 100644
--- a/gopls/internal/protocol/generate/output.go
+++ b/gopls/internal/protocol/generate/output.go
@@ -90,7 +90,7 @@
}
}
-func genCase(_ *Model, method string, param, result *Type, dir string) {
+func genCase(model *Model, method string, param, result *Type, dir string) {
out := new(bytes.Buffer)
fmt.Fprintf(out, "\tcase %q:\n", method)
var p string
@@ -106,6 +106,31 @@
out.WriteString("\t\tif err := UnmarshalJSON(raw, ¶ms); err != nil {\n")
out.WriteString("\t\t\treturn nil, true, fmt.Errorf(\"%%w: %%s\", jsonrpc2.ErrParse, err)\n\t\t}\n")
p = ", ¶ms"
+
+ var extendPositionParam bool
+ for _, struc := range model.Structures {
+ if struc.Name == nm {
+ if contains := slices.ContainsFunc(struc.Extends, func(t *Type) bool {
+ return t.Name == "TextDocumentPositionParams"
+ }); contains {
+ extendPositionParam = true
+ break
+ }
+ }
+ }
+
+ // If the parameter extends the TextDocumentPositionParam, verify the
+ // position is within the provided range.
+ if extendPositionParam {
+ out.WriteString(` if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %%v is outside the provided range %%v.", params.Position, params.Range)
+ }
+ }
+`)
+
+ }
+
}
if notNil(result) {
fmt.Fprintf(out, "\t\tresp, err := %%s.%s(ctx%s)\n", fname, p)
diff --git a/gopls/internal/protocol/tsserver.go b/gopls/internal/protocol/tsserver.go
index cf6dce7..bc8316c 100644
--- a/gopls/internal/protocol/tsserver.go
+++ b/gopls/internal/protocol/tsserver.go
@@ -387,6 +387,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.Completion(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -398,6 +403,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.Declaration(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -409,6 +419,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.Definition(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -474,6 +489,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.DocumentHighlight(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -529,6 +549,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.Hover(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -540,6 +565,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.Implementation(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -562,6 +592,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.InlineCompletion(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -584,6 +619,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.LinkedEditingRange(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -595,6 +635,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.Moniker(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -617,6 +662,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.PrepareCallHierarchy(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -628,6 +678,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.PrepareRename(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -639,6 +694,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.PrepareTypeHierarchy(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -672,6 +732,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.References(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -738,6 +803,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.SignatureHelp(ctx, ¶ms)
if err != nil {
return nil, true, err
@@ -749,6 +819,11 @@
if err := UnmarshalJSON(raw, ¶ms); err != nil {
return nil, true, fmt.Errorf("%w: %s", jsonrpc2.ErrParse, err)
}
+ if params.Range != (Range{}) {
+ if !params.Range.Contains(params.Position) {
+ return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
+ }
+ }
resp, err := server.TypeDefinition(ctx, ¶ms)
if err != nil {
return nil, true, err
diff --git a/gopls/internal/server/definition.go b/gopls/internal/server/definition.go
index 34863ff..964fa40 100644
--- a/gopls/internal/server/definition.go
+++ b/gopls/internal/server/definition.go
@@ -54,10 +54,23 @@
if err != nil {
return nil, err
}
+
+ var rng protocol.Range
+ if params.Range == (protocol.Range{}) {
+ // No selection range was provided.
+ // Default to an empty range at the position.
+ rng = protocol.Range{
+ Start: params.Position,
+ End: params.Position,
+ }
+ } else {
+ rng = params.Range
+ }
+
defer release()
switch kind := snapshot.FileKind(fh); kind {
case file.Go:
- return golang.TypeDefinition(ctx, snapshot, fh, params.Position)
+ return golang.TypeDefinition(ctx, snapshot, fh, rng)
default:
return nil, fmt.Errorf("can't find type definitions for file type %s", kind)
}
diff --git a/gopls/internal/server/hover.go b/gopls/internal/server/hover.go
index 1081db4..0df8afb 100644
--- a/gopls/internal/server/hover.go
+++ b/gopls/internal/server/hover.go
@@ -6,7 +6,6 @@
import (
"context"
- "fmt"
"golang.org/x/tools/gopls/internal/file"
"golang.org/x/tools/gopls/internal/golang"
@@ -35,8 +34,6 @@
}
defer release()
- // TODO(hxjiang): apply the range detection to all handler that accept
- // TextDocumentPositionParams.
var rng protocol.Range
if params.Range == (protocol.Range{}) {
// No selection range was provided.
@@ -46,9 +43,6 @@
End: params.Position,
}
} else {
- if !params.Range.Contains(params.Position) {
- return nil, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)
- }
rng = params.Range
}
diff --git a/gopls/internal/test/integration/fake/editor.go b/gopls/internal/test/integration/fake/editor.go
index ad78e29..88b676e 100644
--- a/gopls/internal/test/integration/fake/editor.go
+++ b/gopls/internal/test/integration/fake/editor.go
@@ -950,6 +950,7 @@
params := &protocol.TypeDefinitionParams{}
params.TextDocument.URI = loc.URI
params.Position = loc.Range.Start
+ params.Range = loc.Range
return e.Server.TypeDefinition(ctx, params)
}
diff --git a/gopls/internal/test/marker/testdata/typedef/expression.txt b/gopls/internal/test/marker/testdata/typedef/expression.txt
new file mode 100644
index 0000000..988aec3
--- /dev/null
+++ b/gopls/internal/test/marker/testdata/typedef/expression.txt
@@ -0,0 +1,26 @@
+This test checks textDocument/typeDefinition over a range reveals the expression type.
+
+-- go.mod --
+module mod.com
+
+go 1.19
+
+-- a/a.go --
+package a
+
+import "mod.com/b"
+
+func foo() (b.Bar, b.Baz, error) {
+ return b.Bar{}, "", nil
+}
+
+func main() {
+ _, _, _ = foo()//@typedef("foo()", Bar, Baz, BUILTIN)
+}
+
+-- b/b.go --
+package b
+
+type Bar struct {}//@loc(Bar, "Bar")
+
+type Baz string//@loc(Baz, "Baz")
diff --git a/gopls/internal/test/marker/testdata/typedef/typedef.txt b/gopls/internal/test/marker/testdata/typedef/typedef.txt
index 940b275..559abab 100644
--- a/gopls/internal/test/marker/testdata/typedef/typedef.txt
+++ b/gopls/internal/test/marker/testdata/typedef/typedef.txt
@@ -14,8 +14,8 @@
value Struct
point *Struct
)
- _ = value //@typedef("value", Struct)
- _ = point //@typedef("point", Struct)
+ _ = value //@typedef(re"value()", Struct)
+ _ = point //@typedef(re"point()", Struct)
var (
array [3]Struct
@@ -23,10 +23,10 @@
ch chan Struct
complex [3]chan *[5][]Int
)
- _ = array //@typedef("array", Struct)
- _ = slice //@typedef("slice", Struct)
- _ = ch //@typedef("ch", Struct)
- _ = complex //@typedef("complex", Int)
+ _ = array //@typedef(re"array()", Struct)
+ _ = slice //@typedef(re"slice()", Struct)
+ _ = ch //@typedef(re"ch()", Struct)
+ _ = complex //@typedef(re"complex()", Int)
var s struct {
x struct {
@@ -36,8 +36,8 @@
}
}
}
- _ = s.x.xx.field1 //@typedef("field1", Struct)
- _ = s.x.xx.field2 //@typedef("field2", Int)
+ _ = s.x.xx.field1 //@typedef(re"field1()", Struct)
+ _ = s.x.xx.field2 //@typedef(re"field2()", Int)
}
func F1() Int { return 0 }
@@ -48,15 +48,15 @@
func F6() (int, float64, ***Struct, error) { return 0, 0, nil, nil }
func _() {
- F1() //@typedef("F1", Int)
- F2() //@typedef("F2", Int)
- F3() //@typedef("F3", Struct)
- F4() //@typedef("F4", Int)
- F5() //@typedef("F5", Struct)
- F6() //@typedef("F6", Struct)
+ F1() //@typedef(re"F1()", Int)
+ F2() //@typedef(re"F2()", Int)
+ F3() //@typedef(re"F3()", BUILTIN, Struct)
+ F4() //@typedef(re"F4()", BUILTIN, Int)
+ F5() //@typedef(re"F5()", BUILTIN, Struct)
+ F6() //@typedef(re"F6()", BUILTIN, Struct)
f := func() Int { return 0 }
- f() //@typedef("f", Int)
+ f() //@typedef(re"f()", Int)
}
// https://github.com/golang/go/issues/38589#issuecomment-620350922
@@ -64,7 +64,7 @@
type myFunc func(int) Int //@loc(myFunc, "myFunc")
var foo myFunc
- _ = foo() //@typedef("foo", myFunc), diag(")", re"not enough arguments")
+ _ = foo() //@typedef(re"foo()", myFunc), diag(")", re"not enough arguments")
}
func _() {
@@ -73,21 +73,21 @@
// In this example it's the r-paren of a call expr.
func() Struct {
panic(0)
- }() //@typedef(")", Struct)
+ }() //@typedef(re"\\)()", Struct)
// And in this one, it's the composite literal enclosing the
// KeyValueExpr denoted by the colon (which must not be adjacent
// to either they key or the value!).
- _ = Struct{Field : ""} //@typedef(":", Struct)
+ _ = Struct{Field : ""} //@typedef(re":", Struct)
}
// edge case: type-switch implicits.
// y in "switch y" has no type; use x instead.
func _(x any) {
- switch y := x.(type) { //@typedef("y", BUILTIN)
+ switch y := x.(type) { //@typedef(re"y()", BUILTIN)
case Int:
- _ = y //@typedef("y", Int)
+ _ = y //@typedef(re"y()", Int)
case Struct:
- _ = y //@typedef("y", Struct)
+ _ = y //@typedef(re"y()", Struct)
}
}
Change information
- M gopls/internal/golang/hover.go
- M gopls/internal/golang/identifier.go
- M gopls/internal/golang/type_definition.go
- M gopls/internal/protocol/generate/output.go
- M gopls/internal/protocol/tsserver.go
- M gopls/internal/server/definition.go
- M gopls/internal/server/hover.go
- M gopls/internal/test/integration/fake/editor.go
- A gopls/internal/test/marker/testdata/typedef/expression.txt
- M gopls/internal/test/marker/testdata/typedef/typedef.txt
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
var extendPositionParam bool
for _, struc := range model.Structures {
if struc.Name == nm {
if contains := slices.ContainsFunc(struc.Extends, func(t *Type) bool {
return t.Name == "TextDocumentPositionParams"
}); contains {
extendPositionParam = true
break
}
}
}
// If the parameter extends the TextDocumentPositionParam, verify the
// position is within the provided range.
if extendPositionParam {
out.WriteString(` if params.Range != (Range{}) {
if !params.Range.Contains(params.Position) {
return nil, true, fmt.Errorf("illegal, position %%v is outside the provided range %%v.", params.Position, params.Range)
}
}This is not the most elegant and efficient code I have ever written, but I think this is for generator so I did not bother making a map....
Also, if you think the validation should be done method by method, I can change the implementation as well. I use the generated code to make sure I don't miss any LSP method who extend `TextDocumentPositionParam`
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Donovan (Gerrit)
Alan Donovan added 13 comments![]( "Open in Gerrit")
// typeToObjects returns the underlying type objects for the given type.type name
// and ignores basic types.The unwrapping of Signatures is a little more subtle than the other cases. Perhaps:
"It unwraps composite types (pointers, slices, etc), and accumulates names from each parameter of a function type."
return []*types.TypeName{typ.Obj()}typ.Obj().Origin()
return []*types.TypeName{typ.Obj()}typ.Obj.Origin()
(and delete the TODO)
cur, ok := pgf.Cursor.FindByPos(start, end)Historically we used to use PathEnclosingInterval for the very first step of mapping a "sloppy" selection to a syntax tree. In the move to cursors, I replaced some of those calls (including this one) by FindByPos. But in CL 716340 we added astutil.Select, which provides the "sloppy" semantics of PathEnclosingInterval but for Cursors. So you can use it here:
```
cur, _, _, err := astutil.Select(pgf.Cursor, start, end)
```
I think the effect of the change should be that if you include adjacent spaces in the selection e.g `⌊ foo ⌋`, then the selection will still refer to `foo`, not to the enclosing node, which could be quite a lot larger.
var extendPositionParam bool
for _, struc := range model.Structures {
if struc.Name == nm {
if contains := slices.ContainsFunc(struc.Extends, func(t *Type) bool {
return t.Name == "TextDocumentPositionParams"
}); contains {
extendPositionParam = true
break
}
}
}
This would be clearer (and self-documenting) hoisted up to L94 in this form:
```
extends := func(x, y string) bool { ... }
...
if extends(nm, "TextDocumentPositionParams") {
out.WriteString(...)
}
``` if params.Range != (Range{}) {if !params.Range.Empty() && !params.Range.Contains(params.Position) {
return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)delete
if params.Range == (protocol.Range{}) {.Empty()
var rng protocol.Range
if params.Range == (protocol.Range{}) {
// No selection range was provided.
// Default to an empty range at the position.
rng = protocol.Range{
Start: params.Position,
End: params.Position,
}
} else {
rng = params.Range
}Won't every RPC that takes a TDPP want this logic? Perhaps it should be folded into the generator.
if params.Range == (protocol.Range{}) {(ditto, move into generator?)
func TestGoToTypeDefinition_Issue38589(t *testing.T) {Was this test redundant w.r.t. the marker tests?
Related details
- Hongxiang Jiang
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil added 3 comments![]( "Open in Gerrit")
return nil, fmt.Errorf("cannot find type name from type %s", t)nit: name(s)
var extendPositionParam bool
for _, struc := range model.Structures {
if struc.Name == nm {
if contains := slices.ContainsFunc(struc.Extends, func(t *Type) bool {
return t.Name == "TextDocumentPositionParams"
}); contains {
extendPositionParam = true
break
}
}
}
// If the parameter extends the TextDocumentPositionParam, verify the
// position is within the provided range.
if extendPositionParam {
out.WriteString(` if params.Range != (Range{}) {
if !params.Range.Contains(params.Position) {
return nil, true, fmt.Errorf("illegal, position %%v is outside the provided range %%v.", params.Position, params.Range)
}
}This is not the most elegant and efficient code I have ever written, but I think this is for generator so I did not bother making a map....
Also, if you think the validation should be done method by method, I can change the implementation as well. I use the generated code to make sure I don't miss any LSP method who extend `TextDocumentPositionParam`
I'm not very familiar with how the generator works, how would doing the validation method by method be different?
{re: `F1`, wantError: true},
{re: `F2`, wantError: true},
{re: `F3`, wantError: true},These will no longer result in errors, right? Maybe we should update this test / change the name instead of deleting, unless the marker tests cover this sufficiently.
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
Related details
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted and added 13 comments![]( "Open in Gerrit")
Votes added by Hongxiang Jiang
| Commit-Queue | +1 |
13 comments
// typeToObjects returns the underlying type objects for the given type.Hongxiang Jiangtype name
Done
The unwrapping of Signatures is a little more subtle than the other cases. Perhaps:
"It unwraps composite types (pointers, slices, etc), and accumulates names from each parameter of a function type."
Done
return []*types.TypeName{typ.Obj()}Hongxiang Jiangtyp.Obj().Origin()
I think you mean `typ.Origin().Obj()`
typ.Obj.Origin()
(and delete the TODO)
Done
cur, ok := pgf.Cursor.FindByPos(start, end)Historically we used to use PathEnclosingInterval for the very first step of mapping a "sloppy" selection to a syntax tree. In the move to cursors, I replaced some of those calls (including this one) by FindByPos. But in CL 716340 we added astutil.Select, which provides the "sloppy" semantics of PathEnclosingInterval but for Cursors. So you can use it here:
```
cur, _, _, err := astutil.Select(pgf.Cursor, start, end)
```
I think the effect of the change should be that if you include adjacent spaces in the selection e.g `⌊ foo ⌋`, then the selection will still refer to `foo`, not to the enclosing node, which could be quite a lot larger.
Sorry I did not fully get your comment. I take a look of both function and I think
`astutil.Select(pgf.Cursor, start, end)` is the same as `pgf.Cursor.FindByPos(start, end)` right?
other then return two more cursor (the first and the last enclosed by the range). The first return value of `astutil.Select` is the same as the first return value of `pgf.Cursor.FindByPos(start, end)`.
Is this comment a suggestion indicating, we should encourage using `astutil.Select` in favor of `pgf.Cursor.FindByPos`?
return nil, fmt.Errorf("cannot find type name from type %s", t)Hongxiang Jiangnit: name(s)
Done
var extendPositionParam bool
for _, struc := range model.Structures {
if struc.Name == nm {
if contains := slices.ContainsFunc(struc.Extends, func(t *Type) bool {
return t.Name == "TextDocumentPositionParams"
}); contains {
extendPositionParam = true
break
}
}
}
This would be clearer (and self-documenting) hoisted up to L94 in this form:
```
extends := func(x, y string) bool { ... }...
if extends(nm, "TextDocumentPositionParams") {
out.WriteString(...)
}
```
Done
var extendPositionParam bool
for _, struc := range model.Structures {
if struc.Name == nm {
if contains := slices.ContainsFunc(struc.Extends, func(t *Type) bool {
return t.Name == "TextDocumentPositionParams"
}); contains {
extendPositionParam = true
break
}
}
}
// If the parameter extends the TextDocumentPositionParam, verify the
// position is within the provided range.
if extendPositionParam {
out.WriteString(` if params.Range != (Range{}) {
if !params.Range.Contains(params.Position) {
return nil, true, fmt.Errorf("illegal, position %%v is outside the provided range %%v.", params.Position, params.Range)
}
}Madeline KalilThis is not the most elegant and efficient code I have ever written, but I think this is for generator so I did not bother making a map....
Also, if you think the validation should be done method by method, I can change the implementation as well. I use the generated code to make sure I don't miss any LSP method who extend `TextDocumentPositionParam`
I'm not very familiar with how the generator works, how would doing the validation method by method be different?
No difference.... But sometime people forgot to run the validation. So run this systemically make sure it can be captured by generator...
if params.Range != (Range{}) {Hongxiang Jiangif !params.Range.Empty() && !params.Range.Contains(params.Position) {
I have some other opinion over this comment. In VSCode, in it's nature there are three pointers.
One is where your mouse is, two is where your cursor is.
```
func foo () int {}
^ ^
cursor. mouse
```
So it is possible we will receive a position {pos: 1:6, range: 1:8 ~ 1:8}.
The range is technically empty because it is selecting empty range but it is illegal because the range is not including the position.
return nil, true, fmt.Errorf("illegal, position %v is outside the provided range %v.", params.Position, params.Range)Hongxiang Jiangdelete
Done
if params.Range == (protocol.Range{}) {Hongxiang Jiang.Empty()
Same above.
var rng protocol.Range
if params.Range == (protocol.Range{}) {
// No selection range was provided.
// Default to an empty range at the position.
rng = protocol.Range{
Start: params.Position,
End: params.Position,
}
} else {
rng = params.Range
}Won't every RPC that takes a TDPP want this logic? Perhaps it should be folded into the generator.
This is a little bit tricky though. I actually think about this.
In generator, we call each LSP method by `server.METHOD_NAME(METHOD_PARAM)`. We do our best to make sure the `METHOD_PARAM` is the same as it's param declared in the LSP.
One thing I think we can do in the generator is, generator will always set `params.Range` and set `params.Pos` to zero value. And we can mark `Deprecated: position is deprecated, use range instead`
Do you think this is better? So we basically eliminate the use of position entirely and replace all the caller by `param.Rng`.
A larger change is need, I will create a follow up CL, but should be straightforward.
if params.Range == (protocol.Range{}) {Hongxiang Jiang(ditto, move into generator?)
Acknowledged. Discussed above.
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hongxiang Jiang
| Commit-Queue | +1 |
1 comment
cur, ok := pgf.Cursor.FindByPos(start, end)Hongxiang JiangHistorically we used to use PathEnclosingInterval for the very first step of mapping a "sloppy" selection to a syntax tree. In the move to cursors, I replaced some of those calls (including this one) by FindByPos. But in CL 716340 we added astutil.Select, which provides the "sloppy" semantics of PathEnclosingInterval but for Cursors. So you can use it here:
```
cur, _, _, err := astutil.Select(pgf.Cursor, start, end)
```
I think the effect of the change should be that if you include adjacent spaces in the selection e.g `⌊ foo ⌋`, then the selection will still refer to `foo`, not to the enclosing node, which could be quite a lot larger.
Sorry I did not fully get your comment. I take a look of both function and I think
`astutil.Select(pgf.Cursor, start, end)` is the same as `pgf.Cursor.FindByPos(start, end)` right?
other then return two more cursor (the first and the last enclosed by the range). The first return value of `astutil.Select` is the same as the first return value of `pgf.Cursor.FindByPos(start, end)`.
Is this comment a suggestion indicating, we should encourage using `astutil.Select` in favor of `pgf.Cursor.FindByPos`?
After making the change, the test start failing. I think the issue is:
```
if !CursorValid(curStart) {
return noCursor, noCursor, noCursor, fmt.Errorf("no syntax selected")
}
```
it is possible that the input selection does not enclose any syntax node. when the input selection is empty (a position), it can not enclose any syntax node. But there should be a syntax node encloses the position.
I wonder should we make the change to `astutil.Select`.
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang uploaded new patchset![]( "Open in Gerrit")
- Alan Donovan
- Hongxiang Jiang
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 3 comments![]( "Open in Gerrit")
var rng protocol.Range
if params.Range == (protocol.Range{}) {
// No selection range was provided.
// Default to an empty range at the position.
rng = protocol.Range{
Start: params.Position,
End: params.Position,
}
} else {
rng = params.Range
}Hongxiang JiangWon't every RPC that takes a TDPP want this logic? Perhaps it should be folded into the generator.
This is a little bit tricky though. I actually think about this.
In generator, we call each LSP method by `server.METHOD_NAME(METHOD_PARAM)`. We do our best to make sure the `METHOD_PARAM` is the same as it's param declared in the LSP.
One thing I think we can do in the generator is, generator will always set `params.Range` and set `params.Pos` to zero value. And we can mark `Deprecated: position is deprecated, use range instead`
Do you think this is better? So we basically eliminate the use of position entirely and replace all the caller by `param.Rng`.
A larger change is need, I will create a follow up CL, but should be straightforward.
Plan to address this in CL 728881.
func TestGoToTypeDefinition_Issue38589(t *testing.T) {Was this test redundant w.r.t. the marker tests?
Addressed in CL 728880.
{re: `F1`, wantError: true},
{re: `F2`, wantError: true},
{re: `F3`, wantError: true},These will no longer result in errors, right? Maybe we should update this test / change the name instead of deleting, unless the marker tests cover this sufficiently.
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Alan Donovan (Gerrit)
Alan Donovan added 1 comment![]( "Open in Gerrit")
cur, ok := pgf.Cursor.FindByPos(start, end)Hongxiang JiangHistorically we used to use PathEnclosingInterval for the very first step of mapping a "sloppy" selection to a syntax tree. In the move to cursors, I replaced some of those calls (including this one) by FindByPos. But in CL 716340 we added astutil.Select, which provides the "sloppy" semantics of PathEnclosingInterval but for Cursors. So you can use it here:
```
cur, _, _, err := astutil.Select(pgf.Cursor, start, end)
```
I think the effect of the change should be that if you include adjacent spaces in the selection e.g `⌊ foo ⌋`, then the selection will still refer to `foo`, not to the enclosing node, which could be quite a lot larger.
Hongxiang JiangSorry I did not fully get your comment. I take a look of both function and I think
`astutil.Select(pgf.Cursor, start, end)` is the same as `pgf.Cursor.FindByPos(start, end)` right?
other then return two more cursor (the first and the last enclosed by the range). The first return value of `astutil.Select` is the same as the first return value of `pgf.Cursor.FindByPos(start, end)`.
Is this comment a suggestion indicating, we should encourage using `astutil.Select` in favor of `pgf.Cursor.FindByPos`?
After making the change, the test start failing. I think the issue is:
```
if !CursorValid(curStart) {
return noCursor, noCursor, noCursor, fmt.Errorf("no syntax selected")
}
```it is possible that the input selection does not enclose any syntax node. when the input selection is empty (a position), it can not enclose any syntax node. But there should be a syntax node encloses the position.
I wonder should we make the change to `astutil.Select`.
astutil.Select(pgf.Cursor, start, end) is the same as pgf.Cursor.FindByPos(start, end) right?
Not quite: if start, end are exactly the Pos, End of some Node both operations will behave the same. But Select implements the fuzzy matching of whitespace similar to PathEnclosingInterval, whereas FindByPos does not. For example if you consider the selection ` foo ` with it spaces, Select will return the identifier whereas FindByPos will return its parent node. Select is appropriate only for interpreting the actual text selection provided by the user, which may be "sloppy".
> Is this comment a suggestion indicating, we should encourage using astutil.Select in favor of pgf.Cursor.FindByPos?
Yes, for the "front line" interpretation of LSP-provided selections, but it should rarely be used within the guts of analysis or refactoring.
After making the change, the test start failing. I think the issue is:
> if !CursorValid(curStart) {
return noCursor, noCursor, noCursor, fmt.Errorf("no syntax selected")
}
it is possible that the input selection does not enclose any syntax node. when the input selection is empty (a position), it can not enclose any syntax node. But there should be a syntax node encloses the position.
> I wonder should we make the change to astutil.Select.
I'm not sure exactly what change you are proposing. Select should always return some node, even if just the enclosing file.
In any case, you needn't use Select in this CL, it was just a suggestion about a general trend towards Cursors and Select and away from Node and PathEnclosingInterval.
Related details
- Hongxiang Jiang
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang added 1 comment![]( "Open in Gerrit")
cur, ok := pgf.Cursor.FindByPos(start, end)I will create another CL to change from `FindByPos` to `astutil.Select(pgf.Cursor, start, end)`. We can continue our discussion on that CL.
Related details
- Alan Donovan
- Madeline Kalil
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Madeline Kalil (Gerrit)
Madeline Kalil voted Code-Review+2![]( "Open in Gerrit")
- Alan Donovan
- Hongxiang Jiang
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hongxiang Jiang (Gerrit)
Hongxiang Jiang submitted the change![]( "Open in Gerrit")
Change information
gopls/internal/server: support type def with selected range
The logical validation for textDocumentPositionParam is moved
to protocol level ensures the validation will be done for all
LSP method param who extends textDocumentPositionParam.
gopls type def will be able to interpret the selected range
from the lsp client. If the selected range evaludate to an
expression, gopls will return the type of the expression.
gopls will return multiple locations for types.Signature or
Tuple without any special filering for type error.
vscode-go CL 727560
For golang/go#76723
- M gopls/internal/golang/hover.go
- M gopls/internal/golang/identifier.go
- M gopls/internal/golang/type_definition.go
- M gopls/internal/protocol/generate/output.go
- M gopls/internal/protocol/tsserver.go
- M gopls/internal/server/definition.go
- M gopls/internal/server/hover.go
- M gopls/internal/test/integration/fake/editor.go
- M gopls/internal/test/integration/misc/definition_test.go
- A gopls/internal/test/marker/testdata/typedef/expression.txt
- M gopls/internal/test/marker/testdata/typedef/typedef.txt
Code-Review: +2 by Madeline Kalil
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |