-
Notifications
You must be signed in to change notification settings - Fork 803
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
FCS namespace revamp #10971
FCS namespace revamp #10971
Conversation
release-notes.md
Outdated
@@ -50,16 +50,16 @@ This is a big update to FCS. There are significant trimmings and renamings of th | |||
Renamings: | |||
|
|||
```diff | |||
-type FSharp.Compiler.AbstractIL.Internal.Library.IFileSystem | |||
+type FSharp.Compiler.SourceCodeServices.IFileSystem | |||
-type FSharp.Compiler.AbstractIL.Library.IFileSystem |
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.
This is only correct if we plan on getting this in for FCS 39, which to my knowledge isn't the case? At least not that I'm aware of
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.
I don't mind - just let me know the schedule.
(TBH I'm ok with delaying 39 until this is in, even if it's not 100% complete it's still better to get the foundational renames into the ecosystem. But I don't know if there are people desperate to get 39 or not)
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.
We need to, at a minimum, release FCS 39 with VS 16.9. VS 16.9 bits are locked down for bug fixes only. So I highly doubt we'd incorporate this right now since it's helpful from a coherency standpoint - until we have a shipping cadence for FCS properly established - to have bits map to one another cleanly.
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.
FCS 39 would come with a bunch of bug fixes and improvements already, so folks definitely would want it
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.
Yep, eagerly anticipating a drop of 39 (since the nightlies aren't usable for packaging reasons :) ).
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.
OK great thank you. So I think that means we can integrate this to master and not into 16.9 release branch? I'll make sure the release notes are separate under 40.0
Aside: It did occur to me that perhaps the naming should be as follows to match the DLL, though I don't really mind
|
|
src/fsharp/TextLayoutRender.fs
Outdated
@@ -167,8 +167,7 @@ module LayoutRender = | |||
|
|||
let emitL f layout = renderL (taggedTextListR f) layout |> ignore | |||
|
|||
let toImmutableArray layout = |
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.
why this change?
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.
ImmutableArray is not in the default reference set for F# Interactive (i.e. the default reference set for the F# scripting model) and in my testing this is causing problems when using the API (though I could just reference it in my test scripts)
We can still use ImmutableArray but I just wanted to do it all-at-once intentionally (replacing all uses of IList<_>
by ImmutableArray<_>
for example) and also remembering to fix this problem.
I'll make a note of this at the top of this PR
I quite like this name.
|
This PR is ready (once green). It's large but nearly entirely routine. Would appreciate getting it merged fairly rapidly to minimise maintenance and get it integrated against all other ongoing work. |
Thank you for the quick merge!!! |
This a revamp of the namespace and naming of FCS following on from a design review with @TIHan @jonsequitur @cartermp @KevinRansom
Namespaces are:
Changes are documented in release-notes.md
Other internal cleanup changes:
vsintegration/src/FSharp.LanguageService/ProjectSitesAndFiles.fs
TODO in later PRs:
Switch to
ImmutableArray
instead ofIList
through the Symbols APIAdd an immutable, extensible, binary-compati-maintainable API to give binary-compatible access to the syntax tree