Roslyn: Improve completion support for obsolete members

[sharwell]

This is a design issue related to completion. Implementation issue(s) will be created according to the approved design, once available.

Original issues:

• #16817 by @CanisLupus
• #19583 by @davidfowl
• #20442 by @mkrueger
• #26478 by @xCyborg

Options for improving the use of these members include, but is not limited to the following:

• Formatting, e.g. strike-through, gray text, etc.
• Custom glyph(s)
• Modified sort order
• Filtering to exclude the items altogether

[xCyborg]

I hope someone is on this.

[gundermanc]

To answer @CyrusNajmabadi's question from the previous thread, yes, VS's completion is able to present strikethrough text encoded using the unicode facilities for strikethrough, however, typing the regular text didn't match the strikethrough version from the completion list when I tried it.

My guess is that you could probably get the desired behavior using the right combination of CompletionItem properties:

• Display text should be strikethrough
• Sort text and filter text and insert text should be non-strikethrough so we can find the item on TYPECHAR and insert the right text.

The same thing looks possible in language server protocol by providing both a label and sort text.

@AmadeusW is the feature owner for completion and might know more.

AFAIK we don't currently support dimming of items in the in-proc completion API. LSP protocol does have a property indicating deprecation status though, so, it's possible to add support for that in both places.

[CyrusNajmabadi]

When you are bolding items, you look at the display text though right? so we would lose bolding here, right?

Note: my preference would be to not have the language set strikethrough, but instead expose a boolean stating if this is deprecated/obsolete. Then, the platform would apply the styling and could do the right thing for all clients in a consistent fashion. Would that make sense for you @gundermanc ?

[airbreather]

In my ideal world, it works like this (n.b.: I haven't seriously looked at how other languages / IDEs handle this):

• If ObsoleteAttribute.IsError is true, then the member should be filtered out.
  
  
  
  • Reasoning: the compiler will always emit an error unless you change the member itself, so I see no reason to have the IDE help you add errors to your code.
  
  
• Else if the completion item would necessarily cause a diagnostic whose severity is Error, then it should be in the list, but with ~strikethrough~ (perhaps put it to the end of the list to reduce clutter on the "real" members).
  
  
  
  • Reasoning: the compiler would emit an error, but there tend to be auto-fixes in these situations, so I can't justify hiding them outright. ~Strikethrough~ feels like the next-strongest clue that the member should not be used.
  
  
• Else if the completion item would necessarily cause a diagnostic whose severity is anything other than Error, then I think gray text makes sense here.
  
  
  
  • Reasoning: grey text makes it clear that this member is not in the same league as everything else, but I perceive it as being less forceful than ~strikethrough~.
  
  
  • Perhaps we could skip gray text for "Hidden" too..
  
  
• Else (the completion item would not necessarily cause any diagnostics), let's leave it alone.
  
  
  
  • Reasoning: if the situation isn't even bad enough to give it any markings after completion, then I interpret it to mean that it's not bad enough to do anything special before completion either.
  
  

In table form:
|ObsoleteAttribute.IsError|Severity|Action|
|-|-|-|
|true||filter out|
|false|Error|~strikethrough~|
|false|Warning|gray text|
|false|Info|gray text|
|false|Hidden|gray text (or, *perhaps, do nothing)|
|false|None (i.e., suppressed)|do nothing|

Perhaps another alternative would be to decorate the list item the same way that it would be decorated in the text editor after completion? e.g., if the completion would result in an error, then give it a red squiggly underline in the list too.

Regardless, as an expert user, I would also appreciate being able to (opt-in) filter out all obsolete items from completion lists.

[gundermanc]

Would that make sense for you @gundermanc ?

@AmadeusW, as completion owner can you chime in?

[PathogenDavid]

• Reasoning: the compiler will always emit an error unless you change the member itself, so I see no reason to have the IDE help you add errors to your code.

Personally I'd want to see it there anyway because I think it would cause confusion for people who knew about the now-obsolete API and the obsolete message would hopefully point me to the replacement API.

[jasonmalinowski]

Design meeting notes: we agreed we should go ahead with strikethrough, since VS code implemented that. If we get feedback we could also add a filter, but we seem to get be getting a small number of users using filters today.

@genlu will chat with the editor team next to see if this is already supported.

[gjrfytn]

@jasonmalinowski will it also be possible to show obsolete members in the end of the suggestion list?
An option to hide them completely sounds great for me too (without the need to hide them every time suggestions show up, I mean some kind of persistent filter).

[jasonmalinowski]

Possibly! I'll let @genlu speak to the details. (I was just the note taker.)

[AmadeusW]

Looks like LSP spec offers CompletionItemTag.Deprecated - are we going to go with this for the LSP implementation? I can add a property to the VS CompletionItem

[CyrusNajmabadi]

are we going to go with this for the LSP implementation

Yup.

I can add a property to the VS CompletionItem

Awesome! Let us know when it's available. Thanks!

[genlu]

An option to hide them completely sounds great for me too (without the need to hide them every time suggestions show up, I mean some kind of persistent filter).

I think this make sense, and making it the default would be my preference..

[DamianEdwards]

Apparently VS Code has a feature like this for symbols in the editor, not just in completion. Would be great to have that in the VS editor too:

Original tweet that brought this to my attention: https://twitter.com/JustinNoelDev/status/1337007045011234817