Get rid of the duplicated COMPLETION_EXECUTION_TIME in completion items

[jdneo]

• Now, the server will cache the COMPLETION_EXECUTION_TIME and only set it into the data field when the item is selected in the callback function.

fix #2621

Data size change

Triggering completion via 'S' in Spring Petclinic project, the response (textDocument/completion) data size can be reduced from 2.67MB to 2.54MB. (Directly copy the trace to a text file)

[rgrunber]

I can confirm this PR eliminates the the COMPLETION_EXECUTION_TIME from each completion item. It looks like this makes it so that if a client wants access to the completion execution time, they must contribute an ICompletionRankingProvider (presumably in a JDT-LS bundle contribution) to get access to it ?

Initially I was thinking, you would probably just return the execution time in the data field of the response to resolveCompletionItem. But I guess the wording of the data field implies it should not exist after resolve ?

     /**
	 * A data entry field that is preserved on a completion item between
	 * a completion and a completion resolve request.
	 */
	data?: [LSPAny](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#lspAny);

If that's the reason you went with reporting the values through java.completion.onDidSelect and only when a ranking provider is contributed, then feel free to merge. I just thought maybe there's a nicer way that uses less non-spec stuff.

[jdneo]

Ah yes, those concerns do make sense.

But I guess the wording of the data field implies it should not exist after resolve?

data does not exist in the resolveSupport capability and we could not update it during resolve according to the spec. But I believe that adding a new data entry won't have any impact on the client behavior in terms of the intellisense experience. (It's not critical things like textEdit or sortText). So maybe add it during resolve can be a good idea?

this makes it so that if a client wants access to the completion execution time, they must contribute an ICompletionRankingProvider (presumably in a JDT-LS bundle contribution) to get access to it?

Yes, that's the original intention to introduce COMPLETION_EXECUTION_TIME, but I directly put it into data field because that's how IntelliCode did previously.

I just thought maybe there's a nicer way that uses less non-spec stuff.

I'm not worry about it, because java.completion.onDidSelect command is an API of the LS itself, so the completion related restrictions should not be applied to it.

The discussion makes me thinking that: Is putting the execution time into the completion item's data field a good practice? Maybe we should make it part of the ICompletionRankingProvider API? Something like there will be an additional map of data feed into void onDidCompletionItemSelect(CompletionItem item)

[testforstephen]

The ranking provider may add many data properties to the completion items. You can store the data from ranking providers in the internal CompletionResponse cache instead of merging them to the lsp4j completion items. This will reduce duplication in the lsp4j response result.

https://github.com/eclipse/eclipse.jdt.ls/blob/b9fda0f3776ec0da832cdc3fbc7c63fb91ee06db/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/CompletionProposalRequestor.java#L250-L270

[jdneo]

You can store the data from ranking providers in the internal CompletionResponse cache instead of merging them to the lsp4j completion items.

Thanks for the input. Yes, IntelliCode contributes some other data entries which are now added into the CompletionItem data. I agree that we should avoid doing this and only append those data when onDidSelect happens. Because those data are only used by the registered ranking providers. Both server and client does not care about it actually. I'll address this problem in another separate PR.