Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove private reflection from OmniSharp #2150

Merged
merged 5 commits into from
May 18, 2021

Conversation

333fred
Copy link
Contributor

@333fred 333fred commented May 7, 2021

This updates OmniSharp to consume the new ExternalAccess assemblies that were added to Roslyn, providing a more structured form of internals access that will give us compile-time diagnostics and avoid the overhead of reflection-based calls. As a part of this, I'm updating our Roslyn dependency to 3.11, and I'm removing the deprecated AutoComplete handler in favor of the modern completion handlers.

This updates OmniSharp to consume the new ExternalAccess assemblies that were added to Roslyn, providing a more structured form of internals access that will give us compile-time diagnostics and avoid the overhead of reflection-based calls. As a part of this, I'm updating our Roslyn dependency to 3.11, and I'm removing the deprecated AutoComplete handler in favor of the modern completion handlers.
@333fred
Copy link
Contributor Author

333fred commented May 7, 2021

This is in draft and tests will fail until dotnet/roslyn#53253 is merged and we can get a version with the bugfix in it, as the pickmembers service is currently failing.

@333fred 333fred marked this pull request as ready for review May 12, 2021 06:46
@333fred
Copy link
Contributor Author

333fred commented May 12, 2021

@filipw @JoeRobich for review.

@filipw
Copy link
Member

filipw commented May 12, 2021

@filipw @JoeRobich for review.

I have a feeling it would be good to do one last release with everything we have so far before merging this

@filipw
Copy link
Member

filipw commented May 12, 2021

there is also a bunch of reflection calls here https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.Script/ScriptProjectProvider.cs#L127-L140 looks like this is not part of the current change

@333fred
Copy link
Contributor Author

333fred commented May 13, 2021

there is also a bunch of reflection calls here https://github.com/OmniSharp/omnisharp-roslyn/blob/master/src/OmniSharp.Script/ScriptProjectProvider.cs#L127-L140 looks like this is not part of the current change

Unfortunately, I didn't see these when I was creating the external access library, and didn't include them in it. We can look at them as a follow up.

@333fred
Copy link
Contributor Author

333fred commented May 13, 2021

Unfortunately, I didn't see these when I was creating the external access library, and didn't include them in it. We can look at them as a follow up.

@filipw the things being used by ScriptProjectProvider are compiler APIs, not IDE APIs. ExternalAccess cannot provide IVT to them as it doesn't have IVT to them itself. Those are going to have to stay as is.

@filipw
Copy link
Member

filipw commented May 13, 2021

@filipw the things being used by ScriptProjectProvider are compiler APIs, not IDE APIs. ExternalAccess cannot provide IVT to them as it doesn't have IVT to them itself. Those are going to have to stay as is.

ah I see, makes sense!

given that we reset binder flags there and you could use it for all kinds of mad things (e.g ignore accessibility checks), I can imagine it's probably not one that will ever be exposed to anyone anyway 😅

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! looks good to me, pending a 'current' release before the merge #2150 (comment)

@333fred
Copy link
Contributor Author

333fred commented May 14, 2021

given that we reset binder flags there and you could use it for all kinds of mad things (e.g ignore accessibility checks), I can imagine it's probably not one that will ever be exposed to anyone anyway 😅

Yeah, the compiler layer gives IVT to no-one, and that's not going to be changing at any point... ever :).

@JoeRobich
Copy link
Member

thanks! looks good to me, pending a 'current' release before the merge

Release created - https://github.com/OmniSharp/omnisharp-roslyn/releases/tag/v1.37.9

@JoeRobich JoeRobich merged commit a130407 into OmniSharp:master May 18, 2021
@333fred 333fred deleted the remove-private-reflection branch May 18, 2021 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants