Make F# compiler framework agnostic #8043
Conversation
src/fsharp/CompileOps.fs
Outdated
|
|
||
| let findPrimaryAssembly (data: TcConfigBuilder) = | ||
| let assemblies = | ||
| data.referencedDLLs |
There was a problem hiding this comment.
We may not have the correct paths at this time because assembly resolution has not happened yet. This means we need to find the primary assembly after assembly resolution.
The problem with that is assembly resolution requires knowledge of a primary assembly... @KevinRansom is that right?
src/fsharp/CompileOptions.fs
Outdated
| yield CompilerOption | ||
| ("targetprofile", tagString, | ||
| OptionString (SetTargetProfile tcConfigB), None, | ||
| OptionString (fun _ -> ()), None, // TODO: Add deprecation message. |
There was a problem hiding this comment.
"--targetprofile is now deprecated and will have no effect. The F# compiler discovers the appropriate target framework profile automatically."
src/fsharp/CompileOps.fs
Outdated
| let hasNoPiaLocalTypes (reader: ILModuleReader) = | ||
| reader.ILModuleDef.TypeDefs.AsArray | ||
| |> Array.exists (fun (ilTypeDef: ILTypeDef) -> | ||
| ilTypeDef.CustomAttrs.AsArray | ||
| |> Array.exists (fun attr -> attr.Method.MethodRef.DeclaringTypeRef.Name = "System.Runtime.InteropServices.TypeIdentifierAttribute")) |
There was a problem hiding this comment.
O(n^2) - are these arrays small?
There was a problem hiding this comment.
Also: do we need to do AsArray? What's the underlying type? Seq.exists may be an alternative here.
There was a problem hiding this comment.
The underlying type is an array and these arrays will be small. It's fine that we iterate like this.
IMO, this should be an ImmutableArray.
src/fsharp/CompileOps.fs
Outdated
| assemblies | ||
| |> List.choose (fun path -> | ||
| let reader = OpenILModuleReader path readerSettings | ||
| if isCandidate reader && reader.ILModuleDef.HasManifest then |
There was a problem hiding this comment.
It may be faster to change order since isCandidate may be expensive
There was a problem hiding this comment.
Maybe. Most assemblies have a manifest so isCandidate will always get called, but it doesn't hurt to change the order.
There was a problem hiding this comment.
My hypothesis on isCandidate, it should be very quick on all assemblies because it first checks to see if the assembly uses any other assemblies. Which almost all assemblies do, except for the primary one.
There was a problem hiding this comment.
so in a refs directory (ie dotnet-root/packs/Microsoft.NETCore.App.Ref/3.1.0/ref/netcoreapp3.1), there are two dlls that I'd consider a 'primary assembly': mscorlib.dll and netstandard.dll. do both of those pass the candidate checks here? if so, then we'll always hit the multiple-candidates stage on line 2598.
There was a problem hiding this comment.
It's fine we that we have multiple candidates; it just shortens the list. The one we actually pick to be primary is the one that contains the type definition of System.Object.
There was a problem hiding this comment.
Hmm, I read my code incorrectly. In the case you have @baronfel , isCandidate should not return true for mscorlib.dll and netstandard.dll inside of that directory because they do not contain the actual type definition for System.Object.
| match candidates with | ||
| | [] -> error(InternalError("Primary assembly not found.", Range.range0)) | ||
| | [candidate] -> candidate | ||
| | _ -> error(InternalError("Two or more possible primary assemblies were found. Consider fixing the assembly references.", Range.range0)) |
There was a problem hiding this comment.
Are we sure this is an internal error and never surfaces? "consider fixing" implies this may be seen by the user. Also: how?
There was a problem hiding this comment.
This is not an internal error. I have this as a placeholder for now and will be changed to a more proper error by adding one in FSComp.txt.
|
Just spent a few (more) hours today debugging issues due to |
|
@TIHan should this be marked as WIP? |
|
We'll take care of this sometime in the future. |
|
😢 |
|
@eiriktsarpalis , yeah ... but priorities. |
This is to make the F# compiler framework agnostic, meaning we will not have to rely on targetprofile if we provide a DLL that it considers to be the primary assembly. We will determine a primary assembly based on the rules specified by Rolsyn here: http://sourceroslyn.io/#Microsoft.CodeAnalysis/ReferenceManager/CommonReferenceManager.Binding.cs,536628815dfdb8a2