You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Completion incorrectly shows class (not constructor) dartdocs for constructors for unimported items #3787
I had a quick look at this, and believe it's because of how we fetch the documentation lazily for LSP with suggestion sets. We send two completions to the client (MyClass and MyClass() for the class/constructor respectively), but when we "resolve" them to get the import edit and documentation, we only have MyClass to look the element up (via requestedLibraryElement.exportNamespace.get(requestedName) so both the class and the constructor get the same thing.
I think this problem might go away when we migrate to the NotImportedContributor.
My only potential concern is performance. Your idea of making the location conditional based on the protocol being used protects legacy protocol clients, so there's no danger there. For LSP clients we'll probably need to measure the difference to see whether this causes problems.
There is a third option that I can think of, which is to change the completion engine to use a protocol-agnostic form of completion suggestion and then translate it as required by whichever protocol is being used as a last step. If we did that the new representation could store the Element directly and the work to convert it into a protocol Element could be delayed. That might improve performance on both sides by avoiding the translation of the data for all of the suggestions that we aren't sending across the wire.
If you want to do the presumably faster task of adding an optional field so that there's less work to do I'm OK with that. We can always come back and explore the other idea later. If you do that, let's mark it as experimental, just as a reminder to ourselves.
My only potential concern is performance. Your idea of making the location conditional based on the protocol being used protects legacy protocol clients, so there's no danger there. For LSP clients we'll probably need to measure the difference to see whether this causes problems.
Do you mean performance of including the extra data in the payload? If so, LSP will have to pay this price to have this fix in any case (although only for not-imported items, and only for the "page" of items we're fetching based on maxCompletionItems).
There is a third option that I can think of, which is to change the completion engine to use a protocol-agnostic form of completion suggestion and then translate it as required by whichever protocol is being used as a last step.
Yeah, that probably makes more sense, I'll take a look. Thanks!
Do you mean performance of including the extra data in the payload?
I meant the overall performance of code completion, wherever the time is spent, but increasing the size of the payload is known to have an impact on the total performance, so yes, that too.
By the way, I pinged Konstantin and Sam to get their opinions of the idea of a protocol-agnostic representation. Haven't heard back from them yet, but we might.
SGTM - I haven't gotten to looking at this properly yet (and probably won't until next week now), so if there are other ideas/feedback before then let me know (or have them post here). Thanks!
@bwilkerson I started taking a look at adding a non-protocol CompletionSuggestion class, although there are some things that make it awkward that I hadn't thought about:
protocol.CompletionSuggestionKind is used in some types (like OpType) that are used by plugins and difficult to change (so for now, I kept using the protocol version for kind and only introduced a new CompletionSuggestion kind)
In some places we need to combine results from Plugins (which will be protocol classes) with those from the server. This is done while they're still Builders (so building can be avoided if they'll be dropped when a subset of completions are returned). Today the plugin results are wrapped in a ValueCompletionSuggestionBuilder (a builder that returns a fixed value) but since this change would change the build() method for builders to return the new type, this doesn't work (and we can't correctly convert a protocol suggestion from a plugin into a "server" suggestion easily because of Element).
To handle this for now, I made SuggestionBuilder have both a server.CompletionSuggestion build() and a protocol.CompletionSuggestion buildProtocol() and plugins will be wrapped in a class that only supports the latter, and most code that would previously call build() can now call buildProtocol().. This doesn't seem like a good solution though.
There are some places that currently produce "synthetic" protocol.Elements - that is, elements for the protocol that didn't originate from a real Element (see here and here) that we now lose (the resulting protocol suggestion has a null element whereas it had these values before)
So, I'm now wondering if this is the best approach (at least if the only current goal is to attach some additional info to these classes). Another idea I had was to just extend these protocol classes with another class that adds some additional fields (like Element, and maybe later some URIs for #4116) and create instances of that class, but it would leave all the protocol/JSON in-tact, and anyone that needs the extra data could do an is check.
protocol.CompletionSuggestionKind is used in some types (like OpType) that are used by plugins and difficult to change ...
I'm not suggesting that we do so, but if we were decide to pursue a non-protocol suggestion class then I'd have no problem with forking classes like OpType and completely abandoning analyzer_plugin until/unless such a time as we decide to support it. (A fairly large portion of the completion engine is already not available to plugins.)
In some places we need to combine results from Plugins (which will be protocol classes) with those from the server.
Do you mean results from builders in analyzer_plugin, or do you mean suggestions that are returned by an actual plugin via the plugin protocol? If you mean the former then the solution above (which again, I'm not advocating for the moment) would solve the problem. If you mean the latter then I'll need to understand more about how that works.
There are some places that currently produce "synthetic" protocol.Elements ...
Interesting. I don't see a good alternative for the first case, but I have to wonder why were not building the element from the LabelElement associated with the label. My guess is that it was convenient to do so and that we could store the LabelElement just as well.
Another idea I had was to just extend these protocol classes ...
Yeah, we probably want to take a more pragmatic approach for the time being. Extending these classes might be a good solution, but that depends on where the suggestion objects are being created.
Another expedient solution would be to use an Expando to associate the extra information with the suggestions. The protocol objects should be short lived, so it shouldn't create any problems to do it that way.
Do you mean results from builders in analyzer_plugin, or do you mean suggestions that are returned by an actual plugin via the plugin protocol? If you mean the former then the solution above (which again, I'm not advocating for the moment) would solve the problem. If you mean the latter then I'll need to understand more about how that works.
We collect a list of CompletionSuggestionBuilders for server, and then wrap the plugins results in a ValueCompletionSuggestionBuilder (which is just a builder that returns the result directly, with no actual building). This combined list of builders is then filtered/scored before returning a subset to the user. So we need some way of having a combined list of server+plugin results somewhere. Ofc we could just build() the server results and convert them to protocol classes and then add the plugins results to that, but it means we'd build() all results and not only those being returned to the client. I suspect building the CompletionSuggestions is probably not the most expensive part of completion, but adding more work to it would certainly make it some amount slower.
Interesting. I don't see a good alternative for the first case, but I have to wonder why were not building the element from the LabelElement associated with the label. My guess is that it was convenient to do so and that we could store the LabelElement just as well.
The Label we have here is only an AstNode and I can't see an obvious way to get to an Element. The other example was for the call method of a Function - I thought that there wouldn't be an Element for that (since it's not real), although I do see some code that is getting an element for this, so if you think it's worth going this route (rather than below) I can try wiring that up.
Yeah, we probably want to take a more pragmatic approach for the time being. Extending these classes might be a good solution, but that depends on where the suggestion objects are being created.
Another expedient solution would be to use an Expando to associate the extra information with the suggestions. The protocol objects should be short lived, so it shouldn't create any problems to do it that way.
I think both of these would work. My feeling is that extending the classes would be slightly "safer" (in terms of typing.. I recently fixed a bug where I'd used Expando's badly and was missing data) and I think they're all created in places where we can easily change this, but I don't know if there are other pros/cons to consider. I'll start on classes but if you have a preference for Expando's it won't be a difficult swap. Thanks!
Ofc we could just build() the server results and convert them to protocol classes and then add the plugins results to that, but it means we'd build() all results and not only those being returned to the client. I suspect building the CompletionSuggestions is probably not the most expensive part of completion, but adding more work to it would certainly make it some amount slower.
That would work, and I wouldn't object, but I'd rather not build the protocol objects when we don't need to. The alternative would be to write a wrapper around the protocol suggestions to make them look like non-protocol suggestions enough to be able to sort them and "convert" them back to protocol objects (by unwrapping them). Not that it matters if we're not implementing the non-protocol objects at this point.
The Label we have here is only an AstNode and I can't see an obvious way to get to an Element.
Again, it doesn't really matter, but I think label.label.staticElement should get it for you.
The alternative would be to write a wrapper around the protocol suggestions to make them look like non-protocol suggestions enough to be able to sort them and "convert" them back to protocol objects (by unwrapping them).
I thought about that, but "translating" them back instead of "unwrapping" them, which failed because we couldn't produce a proper Element to re-create all the fields on protocol.Element. Oh well, I think the new class is probably a simpler route for now - it was trivial to implement and would be trivial to switch if we go with something else in future.
changed the title [-]Completion incorrectly shows class (not constructor) dartdocs for constructors for suggestion sets[/-][+]Completion incorrectly shows class (not constructor) dartdocs for constructors for unimported items[/+]on Nov 15, 2022
Activity
DanTup commentedon Jan 17, 2022
I had a quick look at this, and believe it's because of how we fetch the documentation lazily for LSP with suggestion sets. We send two completions to the client (
MyClassandMyClass()for the class/constructor respectively), but when we "resolve" them to get theimportedit and documentation, we only haveMyClassto look the element up (viarequestedLibraryElement.exportNamespace.get(requestedName)so both the class and the constructor get the same thing.I think this problem might go away when we migrate to the
NotImportedContributor.5 remaining items
bwilkerson commentedon Nov 10, 2022
My only potential concern is performance. Your idea of making the location conditional based on the protocol being used protects legacy protocol clients, so there's no danger there. For LSP clients we'll probably need to measure the difference to see whether this causes problems.
There is a third option that I can think of, which is to change the completion engine to use a protocol-agnostic form of completion suggestion and then translate it as required by whichever protocol is being used as a last step. If we did that the new representation could store the
Elementdirectly and the work to convert it into a protocolElementcould be delayed. That might improve performance on both sides by avoiding the translation of the data for all of the suggestions that we aren't sending across the wire.If you want to do the presumably faster task of adding an optional field so that there's less work to do I'm OK with that. We can always come back and explore the other idea later. If you do that, let's mark it as experimental, just as a reminder to ourselves.
DanTup commentedon Nov 10, 2022
Do you mean performance of including the extra data in the payload? If so, LSP will have to pay this price to have this fix in any case (although only for not-imported items, and only for the "page" of items we're fetching based on maxCompletionItems).
Yeah, that probably makes more sense, I'll take a look. Thanks!
bwilkerson commentedon Nov 10, 2022
I meant the overall performance of code completion, wherever the time is spent, but increasing the size of the payload is known to have an impact on the total performance, so yes, that too.
bwilkerson commentedon Nov 10, 2022
By the way, I pinged Konstantin and Sam to get their opinions of the idea of a protocol-agnostic representation. Haven't heard back from them yet, but we might.
DanTup commentedon Nov 10, 2022
SGTM - I haven't gotten to looking at this properly yet (and probably won't until next week now), so if there are other ideas/feedback before then let me know (or have them post here). Thanks!
DanTup commentedon Nov 15, 2022
@bwilkerson I started taking a look at adding a non-protocol
CompletionSuggestionclass, although there are some things that make it awkward that I hadn't thought about:protocol.CompletionSuggestionKindis used in some types (likeOpType) that are used by plugins and difficult to change (so for now, I kept using the protocol version forkindand only introduced a newCompletionSuggestionkind)Builders (so building can be avoided if they'll be dropped when a subset of completions are returned). Today the plugin results are wrapped in aValueCompletionSuggestionBuilder(a builder that returns a fixed value) but since this change would change thebuild()method for builders to return the new type, this doesn't work (and we can't correctly convert a protocol suggestion from a plugin into a "server" suggestion easily because ofElement).SuggestionBuilderhave both aserver.CompletionSuggestion build()and aprotocol.CompletionSuggestion buildProtocol()and plugins will be wrapped in a class that only supports the latter, and most code that would previously callbuild()can now callbuildProtocol().. This doesn't seem like a good solution though.protocol.Elements- that is, elements for the protocol that didn't originate from a realElement(see here and here) that we now lose (the resulting protocol suggestion has anullelement whereas it had these values before)So, I'm now wondering if this is the best approach (at least if the only current goal is to attach some additional info to these classes). Another idea I had was to just
extendthese protocol classes with another class that adds some additional fields (likeElement, and maybe later some URIs for #4116) and create instances of that class, but it would leave all the protocol/JSON in-tact, and anyone that needs the extra data could do anischeck.Interested in your thoughts/opinions.
bwilkerson commentedon Nov 15, 2022
I'm not suggesting that we do so, but if we were decide to pursue a non-protocol suggestion class then I'd have no problem with forking classes like
OpTypeand completely abandoninganalyzer_pluginuntil/unless such a time as we decide to support it. (A fairly large portion of the completion engine is already not available to plugins.)Do you mean results from builders in
analyzer_plugin, or do you mean suggestions that are returned by an actual plugin via the plugin protocol? If you mean the former then the solution above (which again, I'm not advocating for the moment) would solve the problem. If you mean the latter then I'll need to understand more about how that works.Interesting. I don't see a good alternative for the first case, but I have to wonder why were not building the element from the
LabelElementassociated with the label. My guess is that it was convenient to do so and that we could store theLabelElementjust as well.Yeah, we probably want to take a more pragmatic approach for the time being. Extending these classes might be a good solution, but that depends on where the suggestion objects are being created.
Another expedient solution would be to use an
Expandoto associate the extra information with the suggestions. The protocol objects should be short lived, so it shouldn't create any problems to do it that way.DanTup commentedon Nov 15, 2022
I mean in the completion handler:
https://github.com/dart-lang/sdk/blob/a2adab7dfdc069175abfc667c9f75a43811c69d1/pkg/analysis_server/lib/src/handler/legacy/completion_get_suggestions2.dart#L77-L84
We collect a list of
CompletionSuggestionBuilders for server, and then wrap the plugins results in aValueCompletionSuggestionBuilder(which is just a builder that returns the result directly, with no actual building). This combined list of builders is then filtered/scored before returning a subset to the user. So we need some way of having a combined list of server+plugin results somewhere. Ofc we could justbuild()the server results and convert them to protocol classes and then add the plugins results to that, but it means we'dbuild()all results and not only those being returned to the client. I suspect building theCompletionSuggestions is probably not the most expensive part of completion, but adding more work to it would certainly make it some amount slower.The
Labelwe have here is only anAstNodeand I can't see an obvious way to get to an Element. The other example was for thecallmethod of aFunction- I thought that there wouldn't be anElementfor that (since it's not real), although I do see some code that is getting an element for this, so if you think it's worth going this route (rather than below) I can try wiring that up.I think both of these would work. My feeling is that extending the classes would be slightly "safer" (in terms of typing.. I recently fixed a bug where I'd used Expando's badly and was missing data) and I think they're all created in places where we can easily change this, but I don't know if there are other pros/cons to consider. I'll start on classes but if you have a preference for Expando's it won't be a difficult swap. Thanks!
bwilkerson commentedon Nov 15, 2022
That would work, and I wouldn't object, but I'd rather not build the protocol objects when we don't need to. The alternative would be to write a wrapper around the protocol suggestions to make them look like non-protocol suggestions enough to be able to sort them and "convert" them back to protocol objects (by unwrapping them). Not that it matters if we're not implementing the non-protocol objects at this point.
Again, it doesn't really matter, but I think
label.label.staticElementshould get it for you.SGTM
DanTup commentedon Nov 15, 2022
I thought about that, but "translating" them back instead of "unwrapping" them, which failed because we couldn't produce a proper
Elementto re-create all the fields onprotocol.Element. Oh well, I think the new class is probably a simpler route for now - it was trivial to implement and would be trivial to switch if we go with something else in future.I've sent https://dart-review.googlesource.com/c/sdk/+/269960 for review. Thanks!
[analysis_server] Use ElementLocation to properly resolve documentati…
DanTup commentedon Nov 15, 2022
Fixed by dart-lang/sdk@fc9581d.
[-]Completion incorrectly shows class (not constructor) dartdocs for constructors for suggestion sets[/-][+]Completion incorrectly shows class (not constructor) dartdocs for constructors for unimported items[/+]