Enable in-memory cross project referencing for C# -> F# projects #11301
Conversation
| @@ -82,6 +82,15 @@ type public FSharpProjectOptions = | |||
| /// Compute the project directory. | |||
| member internal ProjectDirectory: string | |||
|
|
|||
| and [<NoComparison>] public FSharpReferencedProject = | |||
| internal | |||
There was a problem hiding this comment.
Could you please make it public instead?
There was a problem hiding this comment.
I don't want the internal structure to be public. But, we should be able to create different kinds of FSharpReferencedProject instances.
| and [<NoComparison>] public FSharpReferencedProject = | ||
| internal | ||
| | FSharp of projectFileName: string * options: FSharpProjectOptions | ||
| | IL of projectFileName: string * stamp: DateTime * lazyData: Lazy<byte []> |
There was a problem hiding this comment.
Could you make it an interface? We can't use Roslyn compilation that produces a byte array but would be happy to use an implementation that looks at R# symbols.
There was a problem hiding this comment.
The use of the byte array isn't because of the Roslyn compilation, it's our metadata reader.
There are currently two functions for opening an ILModuleReader:
val internal OpenILModuleReader: string -> ILReaderOptions -> ILModuleReader
val internal OpenILModuleReaderFromBytes: fileName:string -> assemblyContents: byte[] -> options: ILReaderOptions -> ILModuleReaderI'm using the latter at the moment. Using the former is a little tricky as it tries to look at the last write time of a file which in this case, we can't because it's in-memory.
I think we need a new function to open an ILModuleReader that doesn't require a byte array and doesn't require the file to be on-disk.
There was a problem hiding this comment.
What do you think about something like this?
type FSharpReferencedProject =
| FSharp of projectFileName: string * options: FSharpProjectOptions
| IL of projectFileName: string * metadata: IILMetadata
and IILMetadata =
abstract Timestamp: DateTime
abstract CreateReader: unit -> ILModuleReader // or `ILReaderOptions -> ILModuleReader`There was a problem hiding this comment.
An IILMetadata would be better, though I am hesitant to create an ILModuleReader directly like that as a public API, even though ILModuleReader is public.
I need to investigate whether or not we could use System.Reflection.PortableExecutable.PEReader.
There was a problem hiding this comment.
What makes me want to reuse this logic is changing the module reader directly via shim currently makes it impossible to distinguish module reads from projects and from scripts. Using FSharpReferencedProject would make it a better experience, but we only can provide a custom module reader, not a PEReader or a metadata stream. Could you consider an option allowing passing a custom reader, something like I've suggested above, please?
There was a problem hiding this comment.
To make sure I understand, you already have a module reader and you just want to pass it to a FSharpReferencedProject?
There was a problem hiding this comment.
Yes, I currently create it in IAssemblyReader.GetILModuleReader(filename: string, readerOptions: ILReaderOptions) implementation that is used in the assembly reader shim.
There was a problem hiding this comment.
Ok, then I think it makes sense to allow you to create a FSharpReferencedProject with an ILModuleReader. I will make another PR.
There was a problem hiding this comment.
@TIHan Have you looked into adding this case by any chance? I'll be happy to work on implementing it if we agree on some design.
|
This is ready for review, though I am going to add some tests. |
| let peRef = createPEReference referencedProject comp ct | ||
| referencedProjects.Add(peRef) | ||
| with | ||
| | _ -> () |
There was a problem hiding this comment.
Hmm, I don't know about this. We should allow any exception like that to bubble up somehow IMO. Otherwise we might further propagate issues like this: #9575
The issue there is that we're crashing somehow, but an exception was swallowed, so there's no traceability.
There was a problem hiding this comment.
I would like to allow the exception to get propagated, but we can't have it take down VS if it does.
There was a problem hiding this comment.
Right, but that might happen anyways. In the issue I linked, an exception is not propagated, but because of some unknown reason, it causes a corruption in Roslyn that ultimately crashes VS anyways. Except this time there's no stack trace with any causality.
There was a problem hiding this comment.
I think we can get rid of the try/with safely. When emitting the compilation, I'm setting tolerateErrors to true which means:
/// <summary>
/// Tolerate errors, producing a PE stream and a success result even in the presence of (some) errors.
/// </summary>It sounds like it doesn't guarantee a success result and a PE stream, but it will do it as best as it can.
|
Added tests. The tests also cover if the in-memory C# PEStream will be able to get GCed. This is now 100% done. |
|
This is great! Thanks guys. |
…net#11301) * Added FSharpReferencedProject * Touching up the API * Refactoring and using a Stream instead of a byte[] * Removing a few API endpoints * Fixing build * Updating surface area * Added ConditionalWeakTable for compilation emission * Just in case, put a try/with around creating a pe reference * Remove try/with * Adding C# in-memory reference for compiler service tests
This allows cross project referencing of C#/IL projects to be supported directly in FCS. This means, making a change in a C# project will now be able to be reflected in a F# project that depends on it without having to compile the C# project on-disk.
In VS specifically, there are still a few quirks as semantic analysis doesn't get fired in a F# project when it's dependent C# project changes, but it will correct itself as soon as the user does something in the F# project or any of its dependent F# projects.
API Changes:
FSharpReferencedProject.FSharpProjectOptions's fieldReferencedProjectstype has been changed toFSharpReferencedProject [].Remaining work: