Revert change to a CompletionItem property name which pythia depends on #59724
Conversation
|
I'm really confsued waht pythia is doing here. Are they accessing internal impl details of our completion items? |
|
|
||
| public static CompletionDescription GetDescription(CompletionItem item) | ||
| => CommonCompletionItem.GetDescription(item); | ||
|
|
||
| public static bool TryGetInsertionText(CompletionItem item, [NotNullWhen(true)]out string? insertionText) | ||
| => SymbolCompletionItem.TryGetInsertionText(item, out insertionText); |
There was a problem hiding this comment.
this isn't ok. We can't expose internal impl details to 3rd parties. :(
InsertionText is not part of the completion API and should never be depended on. It can only be used by a particualr completion provider with the specific items they create.
There was a problem hiding this comment.
I agree. Unfortunately, that ship has sailed long ago, when we decided to use the property bag to store provider specific data, instead of adopting a proper type hierarchy for completion item. Now we ended up having dependencies on stuff in our property bag everywhere (from pythia to editor)
This makes our code very fragile and hard to maintain, also extremely difficult to prevent similar misuse in the future. Given everything around Completion is public API, I'm not sure what is a good way to fix the root cause w/o breaking the back-compat. If you have any suggestions, I'm all ears :)
There was a problem hiding this comment.
This makes our code very fragile and hard to maintain, also extremely difficult to prevent similar misuse in the future. Given everything around Completion is public API, I'm not sure what is a good way to fix the root cause w/o breaking the back-compat. If you have any suggestions, I'm all ears :)
This does not break backcompat. We never doc'ed that anyone coudl use this property bag for this purpose. All we doc'ed was:
/// <summary>
/// Additional information attached to a completion item by it creator.
/// </summary>We have never said that the items on this will not change in the future or that anyone can depend on any particular item in that, or htat we will accept any particular item in there. It is specifically about a completion provider adding stuff to its own items that it can read out later.
There was a problem hiding this comment.
That's not what I meant by breaking backcompat, I was thinking of a more significant change to CompletionItem so that each provider can define their own item sub-type, and such item objects can be shared with other providers easily if needed w/o exposing implementation details like the content of property bag (like how pythia is using our SymbolCompletionItem)
You are right changing content of the property bag isn't breaking our public API, but since our partner teams in VS take dependencies on it over time (by simply looking at our code), we are still obliged to avoid/fix any such break caused by it (and we don't have a better way to prevent this in the future) :(
Also add required helpers so we can remove the dependency
ghost
added this to the 
The change was made in #59553. Also add required helpers so we can remove the dependency.
Fix https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1487315