[tools] gopls/internal/golang/completion: report types in unimported completion
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
gopls/internal/golang/completion: report types in unimported completion
pkgIDmatches handled only the Function, Variable, Struct and Constant symbol
kinds, so an interface or a named non-struct type from a workspace package was
dropped, and a struct type was reported as "var". Report every type as
"type (from ...)", as the other two search paths already do.
All three paths gave a type VariableCompletion, so editors drew a variable icon
beside an item whose detail read "type". Use InterfaceCompletion for an
interface and ClassCompletion otherwise. The standard library and module cache
indexes do not record whether a type is an interface, so those two paths always
use ClassCompletion.
Fixes golang/go#81369
Change diff
diff --git a/gopls/internal/golang/completion/unimported.go b/gopls/internal/golang/completion/unimported.go
index 79ec3d7..2fcc6cf 100644
--- a/gopls/internal/golang/completion/unimported.go
+++ b/gopls/internal/golang/completion/unimported.go
@@ -233,9 +233,15 @@
}
kind = protocol.FunctionCompletion
detail = fmt.Sprintf("func (from %q)", pkg.PkgPath)
- case protocol.Variable, protocol.Struct:
+ case protocol.Variable:
kind = protocol.VariableCompletion
detail = fmt.Sprintf("var (from %q)", pkg.PkgPath)
+ case protocol.Interface:
+ kind = protocol.InterfaceCompletion
+ detail = fmt.Sprintf("type (from %q)", pkg.PkgPath)
+ case protocol.Struct, protocol.Class:
+ kind = protocol.ClassCompletion
+ detail = fmt.Sprintf("type (from %q)", pkg.PkgPath)
case protocol.Constant:
kind = protocol.ConstantCompletion
detail = fmt.Sprintf("const (from %q)", pkg.PkgPath)
@@ -284,7 +290,7 @@
kind = protocol.VariableCompletion
detail = fmt.Sprintf("var (from %q)", candpkg)
case stdlib.Type:
- kind = protocol.VariableCompletion
+ kind = protocol.ClassCompletion
detail = fmt.Sprintf("type (from %q)", candpkg)
default:
continue
@@ -331,7 +337,7 @@
kind = protocol.ConstantCompletion
detail = fmt.Sprintf("const (from %s)", cand.ImportPath)
case modindex.Type: // might be a type alias
- kind = protocol.VariableCompletion
+ kind = protocol.ClassCompletion
detail = fmt.Sprintf("type (from %s)", cand.ImportPath)
default:
continue
diff --git a/gopls/internal/test/marker/testdata/completion/unimported.txt b/gopls/internal/test/marker/testdata/completion/unimported.txt
index be10595..7835266 100644
--- a/gopls/internal/test/marker/testdata/completion/unimported.txt
+++ b/gopls/internal/test/marker/testdata/completion/unimported.txt
@@ -21,6 +21,13 @@
package foo
type StructFoo struct{ F int }
+type ArrayFoo [16]byte
+type InterfaceFoo interface{}
+type FuncFoo func(int)
+
+const ConstFoo = 1
+
+var VarFoo = 2
-- baz/baz.go --
package baz
@@ -41,6 +48,10 @@
signature.Foo //@complete(re"()Foo", signaturefoo)
context.Bac //@complete(re"() \\/\\/", contextBackground)
+
+ // Every kind of exported declaration of a workspace package that is not
+ // imported yet.
+ foo. //@complete(re"foo.() \\/\\/", fooArray, fooConst, fooFunc, fooInterface, fooStruct, fooVar)
}
// Create markers for unimported std lib packages. Only for use by this test.
@@ -49,7 +60,14 @@
/* httptrace */ //@item(httptrace, "httptrace", "\"net/http/httptrace\"", "package")
/* httputil */ //@item(httputil, "httputil", "\"net/http/httputil\"", "package")
-/* ring.Ring */ //@item(ringring, "Ring", "type (from \"container/ring\")", "var")
+/* ring.Ring */ //@item(ringring, "Ring", "type (from \"container/ring\")", "type")
+
+/* foo.ArrayFoo */ //@item(fooArray, "ArrayFoo", "type (from \"unimported.test/foo\")", "type")
+/* foo.ConstFoo */ //@item(fooConst, "ConstFoo", "const (from \"unimported.test/foo\")", "const")
+/* foo.FuncFoo */ //@item(fooFunc, "FuncFoo", "func (from \"unimported.test/foo\")", "func")
+/* foo.InterfaceFoo */ //@item(fooInterface, "InterfaceFoo", "type (from \"unimported.test/foo\")", "interface")
+/* foo.StructFoo */ //@item(fooStruct, "StructFoo", "type (from \"unimported.test/foo\")", "type")
+/* foo.VarFoo */ //@item(fooVar, "VarFoo", "var (from \"unimported.test/foo\")", "var")
/* signature.Foo */ //@item(signaturefoo, "Foo", "func (from \"unimported.test/signature\")", "func")
Change information
- M gopls/internal/golang/completion/unimported.go
- M gopls/internal/test/marker/testdata/completion/unimported.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. |
Gopher Robot (Gerrit)
Gopher Robot added 1 comment![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 79 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)
Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
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. |
Gopher Robot (Gerrit)
Message from Gopher Robot![]( "Open in Gerrit")
Congratulations on opening your first change. Thank you for your contribution!
Next steps:
A maintainer will review your change and provide feedback. See
https://go.dev/doc/contribute#review for more info and tips to get your
patch through code review.
Most changes in the Go project go through a few rounds of revision. This can be
surprising to people new to the project. The careful, iterative review process
is our way of helping mentor contributors and ensuring that their contributions
have a lasting impact.
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. |
Sergei Efremov (Gerrit)
Sergei Efremov added 2 comments![]( "Open in Gerrit")
I spotted some possible problems with your PR:
1. You have a long 79 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Done
Fixed line length in the commit message body.
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. |
Gerrit Bot (Gerrit)
Gerrit Bot 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. |
Peter Weinberger (Gerrit)
Peter Weinberger added 3 comments![]( "Open in Gerrit")
Thanks for trying this. I made a couple of comments, but I'm not sure this is solving the whole problem. Does type F func() report 'type' even if it is satisfied from the workspace? (i think unimported completions from stdlib and module cache do report 'type')
case protocol.Struct, protocol.Class:these two cases could be combined.
const ConstFoo = 1what exactly is this test testing? do we need ConstFoo and VarFoo?
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. |
Sergei Efremov (Gerrit)
Sergei Efremov added 4 comments![]( "Open in Gerrit")
Thanks for trying this. I made a couple of comments, but I'm not sure this is solving the whole problem. Does type F func() report 'type' even if it is satisfied from the workspace? (i think unimported completions from stdlib and module cache do report 'type')
Yes, you are right. As you can see in tests. type FuncFoo func is still shows as 'func'.
Before going further I'd like a check that I have read this right.
To be exact, the code I am referring to is the case *ast.FuncType inside the
`*ast.TypeSpec` branch of `symbolizeFile` in `gopls/internal/cache/symbols/symbols.go`,
and the same case in typeDetails in `gopls/internal/golang/symbols.go`. Both assign `protocol.Function`.
A real function never reaches either of those lines: a function declaration is an
`*ast.FuncDecl` and is handled in a separate branch. Anything reaching the
`*ast.TypeSpec` code is a type declaration, so the `*ast.FuncType` there is the right hand side of `type F func()`, not a function.
The effect shows up in completion. A declaration like `type FuncFoo func(int)`
carries the same kind as a real function, so pkgIDmatches in
`gopls/internal/golang/completion/unimported.go` cannot tell the two apart and
offers FuncFoo as `func (from "example.com/foo")`. The other two search paths
report a type: stdlibMatches offers net/http.HandlerFunc as
`type (from "net/http")` from stdlib.Type, and modcacheMatches does the same from
modindex.Type.
Is that intended, or should the two *ast.FuncType cases assign protocol.Class?
Thanks for trying this. I made a couple of comments, but I'm not sure this is solving the whole problem. Does type F func() report 'type' even if it is satisfied from the workspace? (i think unimported completions from stdlib and module cache do report 'type')
Yes, you are right in case of
case protocol.Struct, protocol.Class:these two cases could be combined.
Will merge them. Before I do - is the concern the duplicated body, or that the interface icon is only available for workspace packages and so is inconsistent with the stdlib and modcache paths? Asking because if it's the latter I'll drop InterfaceCompletion entirely rather than keep it behind a conditional.
const ConstFoo = 1what exactly is this test testing? do we need ConstFoo and VarFoo?
Mostly for completeness. The @complete marker lists every item, so the test reads
as a table of declaration kind against the label completion reports for it. If you find it as dead weight - I can drop it.
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. |
Peter Weinberger (Gerrit)
Peter Weinberger added 2 comments![]( "Open in Gerrit")
Sergei EfremovThanks for trying this. I made a couple of comments, but I'm not sure this is solving the whole problem. Does type F func() report 'type' even if it is satisfied from the workspace? (i think unimported completions from stdlib and module cache do report 'type')
Yes, you are right. As you can see in tests. type FuncFoo func is still shows as 'func'.
Before going further I'd like a check that I have read this right.
To be exact, the code I am referring to is the case *ast.FuncType inside the
`*ast.TypeSpec` branch of `symbolizeFile` in `gopls/internal/cache/symbols/symbols.go`,
and the same case in typeDetails in `gopls/internal/golang/symbols.go`. Both assign `protocol.Function`.A real function never reaches either of those lines: a function declaration is an
`*ast.FuncDecl` and is handled in a separate branch. Anything reaching the
`*ast.TypeSpec` code is a type declaration, so the `*ast.FuncType` there is the right hand side of `type F func()`, not a function.The effect shows up in completion. A declaration like `type FuncFoo func(int)`
carries the same kind as a real function, so pkgIDmatches in
`gopls/internal/golang/completion/unimported.go` cannot tell the two apart and
offers FuncFoo as `func (from "example.com/foo")`. The other two search paths
report a type: stdlibMatches offers net/http.HandlerFunc as
`type (from "net/http")` from stdlib.Type, and modcacheMatches does the same from
modindex.Type.Is that intended, or should the two *ast.FuncType cases assign protocol.Class?
I think the big question is what the Detail field should be for all possible circumstances. If getting there is too much for one CL that's ok. What the code does now doesn't really matter if it's wrong. (It might be that if the changes are too extensive, maybe it's not worth the effort, but that's a later decision.)
case protocol.Struct, protocol.Class:Sergei Efremovthese two cases could be combined.
Will merge them. Before I do - is the concern the duplicated body, or that the interface icon is only available for workspace packages and so is inconsistent with the stdlib and modcache paths? Asking because if it's the latter I'll drop InterfaceCompletion entirely rather than keep it behind a conditional.
It was just the duplicated code.
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. |
Sergei Efremov (Gerrit)
Sergei Efremov added 2 comments![]( "Open in Gerrit")
Sergei EfremovThanks for trying this. I made a couple of comments, but I'm not sure this is solving the whole problem. Does type F func() report 'type' even if it is satisfied from the workspace? (i think unimported completions from stdlib and module cache do report 'type')
Peter WeinbergerYes, you are right. As you can see in tests. type FuncFoo func is still shows as 'func'.
Before going further I'd like a check that I have read this right.
To be exact, the code I am referring to is the case *ast.FuncType inside the
`*ast.TypeSpec` branch of `symbolizeFile` in `gopls/internal/cache/symbols/symbols.go`,
and the same case in typeDetails in `gopls/internal/golang/symbols.go`. Both assign `protocol.Function`.A real function never reaches either of those lines: a function declaration is an
`*ast.FuncDecl` and is handled in a separate branch. Anything reaching the
`*ast.TypeSpec` code is a type declaration, so the `*ast.FuncType` there is the right hand side of `type F func()`, not a function.The effect shows up in completion. A declaration like `type FuncFoo func(int)`
carries the same kind as a real function, so pkgIDmatches in
`gopls/internal/golang/completion/unimported.go` cannot tell the two apart and
offers FuncFoo as `func (from "example.com/foo")`. The other two search paths
report a type: stdlibMatches offers net/http.HandlerFunc as
`type (from "net/http")` from stdlib.Type, and modcacheMatches does the same from
modindex.Type.Is that intended, or should the two *ast.FuncType cases assign protocol.Class?
I think the big question is what the Detail field should be for all possible circumstances. If getting there is too much for one CL that's ok. What the code does now doesn't really matter if it's wrong. (It might be that if the changes are too extensive, maybe it's not worth the effort, but that's a later decision.)
I gave it a try. It seems fine, and my local tests are passing. But you're right: the big question is what we want to see. If it's F func(), we should report 'type', and the patch does that. I'm not confident I've found everything this affects, though. I'm less optimistic after you pointed out that gap 😊
case protocol.Struct, protocol.Class:Sergei Efremovthese two cases could be combined.
Peter WeinbergerWill merge them. Before I do - is the concern the duplicated body, or that the interface icon is only available for workspace packages and so is inconsistent with the stdlib and modcache paths? Asking because if it's the latter I'll drop InterfaceCompletion entirely rather than keep it behind a conditional.
It was just the duplicated code.
Ok, simplified.
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. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergei Efremov (Gerrit)
Sergei Efremov added 1 comment![]( "Open in Gerrit")
// A named function type declares a type, not a function.Added here explicit switch for visibility.
It could be deleted and `*ast.FuncType` be process by default rule.
Related details
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger added 6 comments![]( "Open in Gerrit")
Sergei EfremovThanks for trying this. I made a couple of comments, but I'm not sure this is solving the whole problem. Does type F func() report 'type' even if it is satisfied from the workspace? (i think unimported completions from stdlib and module cache do report 'type')
Peter WeinbergerYes, you are right. As you can see in tests. type FuncFoo func is still shows as 'func'.
Before going further I'd like a check that I have read this right.
To be exact, the code I am referring to is the case *ast.FuncType inside the
`*ast.TypeSpec` branch of `symbolizeFile` in `gopls/internal/cache/symbols/symbols.go`,
and the same case in typeDetails in `gopls/internal/golang/symbols.go`. Both assign `protocol.Function`.A real function never reaches either of those lines: a function declaration is an
`*ast.FuncDecl` and is handled in a separate branch. Anything reaching the
`*ast.TypeSpec` code is a type declaration, so the `*ast.FuncType` there is the right hand side of `type F func()`, not a function.The effect shows up in completion. A declaration like `type FuncFoo func(int)`
carries the same kind as a real function, so pkgIDmatches in
`gopls/internal/golang/completion/unimported.go` cannot tell the two apart and
offers FuncFoo as `func (from "example.com/foo")`. The other two search paths
report a type: stdlibMatches offers net/http.HandlerFunc as
`type (from "net/http")` from stdlib.Type, and modcacheMatches does the same from
modindex.Type.Is that intended, or should the two *ast.FuncType cases assign protocol.Class?
Sergei EfremovI think the big question is what the Detail field should be for all possible circumstances. If getting there is too much for one CL that's ok. What the code does now doesn't really matter if it's wrong. (It might be that if the changes are too extensive, maybe it's not worth the effort, but that's a later decision.)
I gave it a try. It seems fine, and my local tests are passing. But you're right: the big question is what we want to see. If it's F func(), we should report 'type', and the patch does that. I'm not confident I've found everything this affects, though. I'm less optimistic after you pointed out that gap 😊
Done
progress (and a misunderstanding on my part(
Sergei Efremovthese two cases could be combined.
Peter WeinbergerWill merge them. Before I do - is the concern the duplicated body, or that the interface icon is only available for workspace packages and so is inconsistent with the stdlib and modcache paths? Asking because if it's the latter I'll drop InterfaceCompletion entirely rather than keep it behind a conditional.
Sergei EfremovIt was just the duplicated code.
Ok, simplified.
Marked as resolved.
case protocol.Struct, protocol.Class:Sergei Efremovthese two cases could be combined.
Peter WeinbergerWill merge them. Before I do - is the concern the duplicated body, or that the interface icon is only available for workspace packages and so is inconsistent with the stdlib and modcache paths? Asking because if it's the latter I'll drop InterfaceCompletion entirely rather than keep it behind a conditional.
Sergei EfremovIt was just the duplicated code.
Ok, simplified.
I misunderstood what you said. I didn't mean for you to give up on InterfaceCompletion. Sorry for the confusion
// A named function type declares a type, not a function.Added here explicit switch for visibility.
It could be deleted and `*ast.FuncType` be process by default rule.
i think that would be ok.
const ConstFoo = 1Sergei Efremovwhat exactly is this test testing? do we need ConstFoo and VarFoo?
Mostly for completeness. The @complete marker lists every item, so the test reads
as a table of declaration kind against the label completion reports for it. If you find it as dead weight - I can drop it.
Yes, i think it's distracting to have the test test more than it has to.
Related details
- Sergei Efremov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sergei Efremov (Gerrit)
Sergei Efremov added 3 comments![]( "Open in Gerrit")
case protocol.Struct, protocol.Class:Sergei Efremovthese two cases could be combined.
Peter WeinbergerWill merge them. Before I do - is the concern the duplicated body, or that the interface icon is only available for workspace packages and so is inconsistent with the stdlib and modcache paths? Asking because if it's the latter I'll drop InterfaceCompletion entirely rather than keep it behind a conditional.
Sergei EfremovIt was just the duplicated code.
Peter WeinbergerOk, simplified.
I misunderstood what you said. I didn't mean for you to give up on InterfaceCompletion. Sorry for the confusion
Reverted it back.
// A named function type declares a type, not a function.Peter WeinbergerAdded here explicit switch for visibility.
It could be deleted and `*ast.FuncType` be process by default rule.
i think that would be ok.
Droped the case.
const ConstFoo = 1Sergei Efremovwhat exactly is this test testing? do we need ConstFoo and VarFoo?
Peter WeinbergerMostly for completeness. The @complete marker lists every item, so the test reads
as a table of declaration kind against the label completion reports for it. If you find it as dead weight - I can drop it.
Yes, i think it's distracting to have the test test more than it has to.
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Peter Weinberger
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Weinberger (Gerrit)
Peter Weinberger 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. |
Peter Weinberger (Gerrit)
Peter Weinberger voted Code-Review+2![]( "Open in Gerrit")
| Code-Review | +2 |
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. |
Sergei Efremov (Gerrit)
Sergei Efremov added 3 comments![]( "Open in Gerrit")
case protocol.Struct, protocol.Class:Sergei Efremovthese two cases could be combined.
Peter WeinbergerWill merge them. Before I do - is the concern the duplicated body, or that the interface icon is only available for workspace packages and so is inconsistent with the stdlib and modcache paths? Asking because if it's the latter I'll drop InterfaceCompletion entirely rather than keep it behind a conditional.
Sergei EfremovIt was just the duplicated code.
Peter WeinbergerOk, simplified.
Sergei EfremovI misunderstood what you said. I didn't mean for you to give up on InterfaceCompletion. Sorry for the confusion
Reverted it back.
Done
// A named function type declares a type, not a function.Peter WeinbergerAdded here explicit switch for visibility.
It could be deleted and `*ast.FuncType` be process by default rule.
Sergei Efremovi think that would be ok.
Droped the case.
Done
const ConstFoo = 1Sergei Efremovwhat exactly is this test testing? do we need ConstFoo and VarFoo?
Peter WeinbergerMostly for completeness. The @complete marker lists every item, so the test reads
as a table of declaration kind against the label completion reports for it. If you find it as dead weight - I can drop it.
Sergei EfremovYes, i think it's distracting to have the test test more than it has to.
Removed ConstFoo and VarFoo.
Done
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 and added 1 comment![]( "Open in Gerrit")
Votes added by Hongxiang Jiang
| Commit-Queue | +1 |
1 comment
The test failure is not really relevant to your change, re-run.
I will take a quick scan this week since Peter has already done the review.
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. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Code-Review | +1 |
| Commit-Queue | +1 |
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. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
gopls/internal/golang/completion: report types in unimported completion
pkgIDmatches handled only the Function, Variable, Struct and Constant symbol
kinds, so an interface or a named non-struct type from a workspace package
was dropped, and a struct type was reported as "var". Report every type as
"type (from ...)", as the other two search paths already do.
All three paths gave a type VariableCompletion, so editors drew a variable
icon beside an item whose detail read "type". Use InterfaceCompletion for an
interface and ClassCompletion otherwise. The standard library and module
cache indexes do not record whether a type is an interface, so those two
paths always use ClassCompletion.
Fixes golang/go#81369
- M gopls/internal/cache/symbols/symbols.go
- M gopls/internal/golang/completion/unimported.go
- M gopls/internal/golang/symbols.go
- M gopls/internal/test/integration/misc/package_symbols_test.go
- M gopls/internal/test/marker/testdata/completion/unimported.txt
Code-Review: +2 by Peter Weinberger, +1 by Hyang-Ah Hana Kim
TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |