Bugfixes for the OmniSharp ExternalAcces layer #53253
Conversation
333fred
commented
May 7, 2021
333fred
commented
- Make the completion provider class names constants so they can be used in pattern matching. I've added a unit test to make sure that these names are correct.
- Correctly handle default options arrays in OmniSharpPickMembersService.
1. Make the completion provider class names constants so they can be used in pattern matching. I've added a unit test to make sure that these names are correct. 2. Correctly handle default options arrays in OmniSharpPickMembersService.
|
@sharwell for a quick review. @jinujoseph, is this ok to take for 16.11? |
| internal const string ObjectCreationCompletionProvider = "Microsoft.CodeAnalysis.CSharp.Completion.Providers.ObjectCreationCompletionProvider"; | ||
| internal const string OverrideCompletionProvider = "Microsoft.CodeAnalysis.CSharp.Completion.Providers.OverrideCompletionProvider"; | ||
| internal const string PartialMethodCompletionProvider = "Microsoft.CodeAnalysis.CSharp.Completion.Providers.PartialMethodCompletionProvider"; | ||
| internal const string InternalsVisibleToCompletionProvider = "Microsoft.CodeAnalysis.CSharp.Completion.Providers.InternalsVisibleToCompletionProvider"; | ||
| internal const string TypeImportCompletionProvider = "Microsoft.CodeAnalysis.CSharp.Completion.Providers.TypeImportCompletionProvider"; | ||
| internal const string ExtensionMethodImportCompletionProvider = "Microsoft.CodeAnalysis.CSharp.Completion.Providers.ExtensionMethodImportCompletionProvider"; |
There was a problem hiding this comment.
These should all be static readonly to ensure they are not embedded as literals into external code.
... so they can be used in pattern matching ...
This will unfortunately not be possible without compromising the ability of the external access layer to adapt to changes.
There was a problem hiding this comment.
I'm going to keep these as constants. I don't think we have a real problem here: OmniSharp controls what version of Roslyn they take, no one is going to be upgrading it to a new version at runtime. We have tests now to ensure that these reflect the real names of the types, and the code in O# that uses these names has made pretty significant usage of pattern matching.
There was a problem hiding this comment.
The external access layer ships with Roslyn itself. The value of these strings is allowed to change as long as the following two requirements hold:
- The name of the field stays the same
- The value of the field is the new type name for the completion provider with the same concept
Embedding the strings in a binary outside of Roslyn breaks the ability to change the value in account for (2).
There was a problem hiding this comment.
In other words, Roslyn is allowed to change the constant value here and create an assembly binding redirection that forces the new value to appear at runtime. Any code referencing these as const would no longer behave correctly.
There was a problem hiding this comment.
In other words, Roslyn is allowed to change the constant value here and create an assembly binding redirection that forces the new value to appear at runtime.
There is no one to create such a binding redirect. OmniSharp is the host process here, and controls the final version of Roslyn being used. It's never going to be using a different version of the external access library than the rest of roslyn. While I get the theoretical concern here, I don't believe it matters for this external access library and I'm going to keep these as constants. Again, there is significant code in OmniSharp that already uses these as constants, and I'm not going to rewrite this code to a wildly different form to conform to theoretical concerns. I'll just not use these fields and leave them as the OmniSharp-copied constants.
There was a problem hiding this comment.
can i ask a clarifying question: what value do we get from constants here? they seem strictly more brittle than static readonlies.
There was a problem hiding this comment.
can i ask a clarifying question: what value do we get from constants here? they seem strictly more brittle than static readonlies.
Constants can appear in the arms of a switch expression, but static readonly cannot.
There was a problem hiding this comment.
Or any pattern matches: as mentioned, the completion code in OmniSharp makes quite a bit of use of pattern matching over these names today.
There was a problem hiding this comment.
Alright, I'll revert this. But ultimately, I'm not going to be changing the code in omnisharp here. It just means we're going to have constants there and not use these fields.
There was a problem hiding this comment.
@333fred maybe a situation to keep on the back of your mind it case future pattern matching improvements come through LDM 😄
|
yes Fred , will approve post review |
src/Tools/ExternalAccess/OmniSharp/Internal/PickMembers/OmniSharpPickMembersService.cs
Show resolved
Hide resolved
|
@jinujoseph for approval. |
