[tools] gopls/internal/test/marker: add optional error to typedef marker
Hongxiang Jiang (Gerrit)
Hongxiang Jiang has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal/test/marker: add optional error to typedef marker
The typedef marker now accept optional input parameter error to
verify if the typedef returns an expected error from gopls.
Also migrate regression test for golang/go#60544 from integration
to marker test.
For golang/go#76723
Change diff
diff --git a/gopls/internal/test/integration/misc/definition_test.go b/gopls/internal/test/integration/misc/definition_test.go
index efe7df8..f7024d1 100644
--- a/gopls/internal/test/integration/misc/definition_test.go
+++ b/gopls/internal/test/integration/misc/definition_test.go
@@ -321,37 +321,6 @@
}
}
-func TestGoToTypeDefinition_Issue60544(t *testing.T) {
- const mod = `
--- go.mod --
-module mod.com
-
-go 1.19
--- main.go --
-package main
-
-func F[T comparable]() {}
-`
-
- Run(t, mod, func(t *testing.T, env *Env) {
- env.OpenFile("main.go")
-
- // TypeDefinition of comparable should not panic.
- loc := env.RegexpSearch("main.go", "comparable")
- locs, err := env.Editor.TypeDefinitions(env.Ctx, loc) // doesn't panic
- if err != nil {
- t.Fatal(err)
- }
-
- // For extra credit, check the actual location.
- got := fmt.Sprint(locs)
- wantSubstr := "builtin.go"
- if !strings.Contains(got, wantSubstr) {
- t.Errorf("TypeDefinitions('comparable') = %v, want substring %q", got, wantSubstr)
- }
- })
-}
-
// Test for golang/go#47825.
func TestImportTestVariant(t *testing.T) {
const mod = `
diff --git a/gopls/internal/test/marker/doc.go b/gopls/internal/test/marker/doc.go
index 44f2a63..040cb4f 100644
--- a/gopls/internal/test/marker/doc.go
+++ b/gopls/internal/test/marker/doc.go
@@ -310,8 +310,10 @@
request at the given location, and asserts that the result includes
exactly one token with the given token type and modifier string.
- - typedef(src, want ...location): performs a textDocument/typeDefinition request
- at the src location, and checks that the results equals want.
+ - typedef(src, want ...location, err=stringMatcher): performs a
+ textDocument/typeDefinition request at the src location, and checks that the
+ results equals want. If err is set, the implementation query must fail with
+ the expected error.
- workspacesymbol(query, golden): makes a workspace/symbol request for the
given query, formats the response with one symbol per line, and compares
diff --git a/gopls/internal/test/marker/marker_test.go b/gopls/internal/test/marker/marker_test.go
index c5ff7a2..f07229f 100644
--- a/gopls/internal/test/marker/marker_test.go
+++ b/gopls/internal/test/marker/marker_test.go
@@ -618,7 +618,7 @@
"quickfixerr": actionMarkerFunc(quickfixErrMarker),
"symbol": actionMarkerFunc(symbolMarker),
"token": actionMarkerFunc(tokenMarker),
- "typedef": actionMarkerFunc(typedefMarker),
+ "typedef": actionMarkerFunc(typedefMarker, "err"),
"workspacesymbol": actionMarkerFunc(workspaceSymbolMarker),
"mcptool": actionMarkerFunc(mcpToolMarker, "location", "output"),
}
@@ -1715,11 +1715,18 @@
}
}
+// typedefMarker implements the @typedef marker.
func typedefMarker(mark marker, loc protocol.Location, want ...protocol.Location) {
+ wantErr := namedArgFunc(mark, "err", convertStringMatcher, stringMatcher{})
+
env := mark.run.env
got, err := env.Editor.TypeDefinitions(env.Ctx, loc)
- if err != nil {
- mark.errorf("typeDefinition request failed: %v", err)
+ if err != nil && wantErr.empty() {
+ mark.errorf("typeDefinition at %s failed: %v", loc, err)
+ return
+ }
+ if !wantErr.empty() {
+ wantErr.checkErr(mark, err)
return
}
diff --git a/gopls/internal/test/marker/testdata/typedef/issue60544.txt b/gopls/internal/test/marker/testdata/typedef/issue60544.txt
new file mode 100644
index 0000000..cc5ab00
--- /dev/null
+++ b/gopls/internal/test/marker/testdata/typedef/issue60544.txt
@@ -0,0 +1,10 @@
+Regression test for golang/go#60544.
+
+-- go.mod --
+module mod.com
+
+go 1.19
+-- main.go --
+package main
+
+func F[T comparable]() {}//@typedef(re"comparable()", BUILTIN)
diff --git a/gopls/internal/test/marker/testdata/typedef/typedef.txt b/gopls/internal/test/marker/testdata/typedef/typedef.txt
index 559abab..b9737ca 100644
--- a/gopls/internal/test/marker/testdata/typedef/typedef.txt
+++ b/gopls/internal/test/marker/testdata/typedef/typedef.txt
@@ -40,20 +40,34 @@
_ = s.x.xx.field2 //@typedef(re"field2()", Int)
}
+// type def returns function return types' location.
+
func F1() Int { return 0 }
-func F2() (Int, float64) { return 0, 0 }
-func F3() (Struct, int, bool, error) { return Struct{}, 0, false, nil }
-func F4() (**int, Int, bool, *error) { return nil, 0, false, nil }
-func F5() (int, float64, error, Struct) { return 0, 0, nil, Struct{} }
-func F6() (int, float64, ***Struct, error) { return 0, 0, nil, nil }
+func F2() error { return nil }
+func F3() (Int, float64) { return 0, 0 }
+func F4() (Int, error) { return 0, nil }
+func F5() (Struct, int, bool, error) { return Struct{}, 0, false, nil }
+func F6() (**int, Int, bool, *error) { return nil, 0, false, nil }
+func F7() (int, float64, error, Struct) { return 0, 0, nil, Struct{} }
+func F8() (int, float64, ***Struct, error) { return 0, 0, nil, nil }
+
+// type def returns error.
+
+func E1() {}
+func E2() (int, bool) { return 0, true }
func _() {
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)
+ F2() //@typedef(re"F2()", BUILTIN)
+ F3() //@typedef(re"F3()", Int)
+ F4() //@typedef(re"F4()", Int, BUILTIN)
+ F5() //@typedef(re"F5()", Struct, BUILTIN)
+ F6() //@typedef(re"F6()", Int, BUILTIN)
+ F7() //@typedef(re"F7()", Struct, BUILTIN)
+ F8() //@typedef(re"F8()", Struct, BUILTIN)
+
+ E1() //@typedef(re"E1()", err="cannot find type name(s)")
+ E2() //@typedef(re"E2()", err="cannot find type name(s)")
f := func() Int { return 0 }
f() //@typedef(re"f()", Int)
Change information
- M gopls/internal/test/integration/misc/definition_test.go
- M gopls/internal/test/marker/doc.go
- M gopls/internal/test/marker/marker_test.go
- A gopls/internal/test/marker/testdata/typedef/issue60544.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
- 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 and added 2 comments![]( "Open in Gerrit")
Votes added by Madeline Kalil
| Code-Review | +2 |
2 comments
results equals want. If err is set, the implementation query must fail withtype definition
func F5() (Struct, int, bool, error) { return Struct{}, 0, false, nil }General question: why can you jump to definition on basic types like int, bool, but can't do a type def query on these? Shouldn't they jump to the same place, in the same way that type def and normal def on "error" jump to the same place?
Related details
- 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 uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- 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 voted and added 2 comments![]( "Open in Gerrit")
Votes added by Hongxiang Jiang
| Commit-Queue | +1 |
2 comments
results equals want. If err is set, the implementation query must fail withHongxiang Jiangtype definition
Oops. Done.
func F5() (Struct, int, bool, error) { return Struct{}, 0, false, nil }General question: why can you jump to definition on basic types like int, bool, but can't do a type def query on these? Shouldn't they jump to the same place, in the same way that type def and normal def on "error" jump to the same place?
I think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...
I'm happy to change the behavior, we can discuss this offline.
Related details
- Alan Donovan
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 1 comment![]( "Open in Gerrit")
func F5() (Struct, int, bool, error) { return Struct{}, 0, false, nil }Hongxiang JiangGeneral question: why can you jump to definition on basic types like int, bool, but can't do a type def query on these? Shouldn't they jump to the same place, in the same way that type def and normal def on "error" jump to the same place?
I think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...
I'm happy to change the behavior, we can discuss this offline.
I think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...
This is historically correct, but perhaps built-in types were rejected because TypeDefinition could (until now) report only a single type, so ignoring built-in types helped us locate the one type of interest. But if we can now report on all the named types in (say) the result tuple of a function call, then I see no reason not to treat them all consistently. We should of course check the ergonomics in VS Code (and I can check in Emacs+eglot) before committing.
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")
- Code-Review: +2 by Madeline Kalil
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Hongxiang Jiang
| Commit-Queue | +1 |
1 comment
func F5() (Struct, int, bool, error) { return Struct{}, 0, false, nil }Hongxiang JiangGeneral question: why can you jump to definition on basic types like int, bool, but can't do a type def query on these? Shouldn't they jump to the same place, in the same way that type def and normal def on "error" jump to the same place?
Alan DonovanI think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...
I'm happy to change the behavior, we can discuss this offline.
I think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...This is historically correct, but perhaps built-in types were rejected because TypeDefinition could (until now) report only a single type, so ignoring built-in types helped us locate the one type of interest. But if we can now report on all the named types in (say) the result tuple of a function call, then I see no reason not to treat them all consistently. We should of course check the ergonomics in VS Code (and I can check in Emacs+eglot) before committing.
SG. I will add a TODO here in case I forgot about this. I'm happy to have another patch later to make the behavior consistent between all these types.
Comment added in `gopls/internal/golang/type_definition.go`
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 added 1 comment![]( "Open in Gerrit")
func F5() (Struct, int, bool, error) { return Struct{}, 0, false, nil }Hongxiang JiangGeneral question: why can you jump to definition on basic types like int, bool, but can't do a type def query on these? Shouldn't they jump to the same place, in the same way that type def and normal def on "error" jump to the same place?
Alan DonovanI think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...
I'm happy to change the behavior, we can discuss this offline.
Hongxiang JiangI think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...This is historically correct, but perhaps built-in types were rejected because TypeDefinition could (until now) report only a single type, so ignoring built-in types helped us locate the one type of interest. But if we can now report on all the named types in (say) the result tuple of a function call, then I see no reason not to treat them all consistently. We should of course check the ergonomics in VS Code (and I can check in Emacs+eglot) before committing.
SG. I will add a TODO here in case I forgot about this. I'm happy to have another patch later to make the behavior consistent between all these types.
Comment added in `gopls/internal/golang/type_definition.go`
I have a CL that I need to rebase on top of your work and finish that will fix this in gopls: https://go-review.git.corp.google.com/c/tools/+/729740
Related details
- 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 added 1 comment![]( "Open in Gerrit")
func F5() (Struct, int, bool, error) { return Struct{}, 0, false, nil }Hongxiang JiangGeneral question: why can you jump to definition on basic types like int, bool, but can't do a type def query on these? Shouldn't they jump to the same place, in the same way that type def and normal def on "error" jump to the same place?
Alan DonovanI think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...
I'm happy to change the behavior, we can discuss this offline.
Hongxiang JiangI think this is more of an existing behavior. Gopls ignores basic types like this...
I think people think it's less useful to provide type def info for these built-in types...This is historically correct, but perhaps built-in types were rejected because TypeDefinition could (until now) report only a single type, so ignoring built-in types helped us locate the one type of interest. But if we can now report on all the named types in (say) the result tuple of a function call, then I see no reason not to treat them all consistently. We should of course check the ergonomics in VS Code (and I can check in Emacs+eglot) before committing.
Madeline KalilSG. I will add a TODO here in case I forgot about this. I'm happy to have another patch later to make the behavior consistent between all these types.
Comment added in `gopls/internal/golang/type_definition.go`
I have a CL that I need to rebase on top of your work and finish that will fix this in gopls: https://go-review.git.corp.google.com/c/tools/+/729740
Feel free to review this change and the previous change so we can merge the CLs.
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/test/marker: add optional error to typedef marker
The typedef marker now accept optional input parameter error to
verify if the typedef returns an expected error from gopls.
Also migrate regression test for golang/go#60544 from integration
to marker test.
For golang/go#76723
- M gopls/internal/golang/type_definition.go
- M gopls/internal/test/integration/misc/definition_test.go
- M gopls/internal/test/marker/doc.go
- M gopls/internal/test/marker/marker_test.go
- A gopls/internal/test/marker/testdata/typedef/issue60544.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. |