-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Changes needed to create a F# language service based on Roslyn workspace model #8557
Conversation
…I to get at the list of metadata references for a Host Project.
@@ -18,6 +18,7 @@ namespace Microsoft.CodeAnalysis.Editor.Implementation.Classification | |||
[Export(typeof(ITaggerProvider))] | |||
[ContentType(ContentTypeNames.CSharpContentType)] | |||
[ContentType(ContentTypeNames.VisualBasicContentType)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we remove CSharpContentType
and VisualBasicContentType
now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes done.
LGTM |
👍 |
@@ -58,6 +58,8 @@ | |||
<InternalsVisibleToTest Include="Roslyn.Services.Editor.VisualBasic.UnitTests" /> | |||
<InternalsVisibleToTest Include="Roslyn.VisualStudio.Services.UnitTests" /> | |||
<InternalsVisibleToTest Include="RoslynETAHost" /> | |||
<InternalsVisibleToFSharp Include="FSharp.Editor" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to consider naming these the same way we do for C#/VB/TS. I think uniformity will make things a lot clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're also in the test section. Ideally all your stuff would always be next to the TypeScript stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean naming the F# dlls? Currently these are the names of the existing F# LS dlls. @otawfik-ms's plan is to start adding code to those existing dlls and refactor later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved all the F# IVTs next to the TypeScript ones.
👍 I would recommend renaming the F# dlls in the future. Just for consistency across all 4 languages. But it's a very minor thing. TS used this rewrite as the opportunity to make these types of changes, as they're usually much harder to do down the line. |
Agreed. |
Changes needed to create a F# language service based on Roslyn workspace model
Context:
We are planning to create a F# language service based on the Roslyn workspaces model to modernize the F# IDE (see dotnet/fsharp#913). In order to do that, the F# language service will need access to many of internal types that are in Roslyn Features\EditorFeatures\VS layers today. Currently those APIs are internal because they are not designed to be public. Their designs are very specific to the needs of C# and VB. In VS 2015, TypeScript took a bet on Roslyn and implemented their language service on top of the Roslyn workspace model. To do this, they had internals access and they went through the codebase cleaning up a bunch of places where we had assumed depedencies on the compiler model specific to C#\VB. This gives us a little more confidence about the APIs and puts us on a path to making them public. With this change I'm adding internals access to F# as well so that we can build up yet another language on top of these APIs. C# and VB are quite similar but with TypeScript and F# (different in significant ways) built on these APIs we think we'll have a good amount of data about the usage patterns that we can then start making these APIs public one by one.
Changes:
SyntacticClassificationTaggerProvider
work for the RoslynContentType which is now the base type for C#\VB\TypeScript\F#. TypeScript classifier is not implemented through IEditorClassificationService. I've verified that this type ignores if it cannot get the classification service for a document and so it shouldn't affect TypeScript.DiagnosticData
. This code only seems to run while bringing up a lightbulb and TypeScript didn't run into it because they don't have lightbulbs.@dotnet/roslyn-ide @jaredpar @KevinRansom @otawfik-ms @paulvanbrenk @vladima @mattwar @heejaechang