-
Notifications
You must be signed in to change notification settings - Fork 842
Improve compiler's type searching logic #1792
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
Conversation
src/absil/il.fs
Outdated
| static member IsSomePrimaryAssembly n = | ||
| n = PrimaryAssembly.Mscorlib.Name || | ||
| n = PrimaryAssembly.DotNetCore.Name || | ||
| n = PrimaryAssembly.PrivateCoreLib.Name |
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.
No ... please not this ... its bad enough that the compiler knows about mscorlib and System.Runtime ... let's not do propagate that error.
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.
@KevinRansom Will work on it
|
@KevinRansom I have removed the knowledge of Private.CoreLib. F# Interactive still runs correctly. I've also enabled lots more testing for .NET Core F# Compiler and F# Interactive in The .NET Core compiler and F# Interactive still have some problems on a number of the FSharpSuite tests. Most of the FSC_BASIC and FSI_BASIC tests that have not been enabled fail in one way or another, either failing to compile (usually the test needs to be adjusted) or failing to run through F# Interactive (this seems to indicate some limitation or difference in dynamic code generation on .NET Core). |
|
@KevinRansom This is ready to go in (assuming the CI stays green). I think it's important to get this in to lock down the status of the F# compiler and F# Interactive on .NET Core, at least for the FSharpSuite tests we have under CI (in addition to the FSharp.Core.Unittests, which we've always had under CI). |
|
@dotnet-bot test this please |
|
@KevinRansom I'll merge this now. I had to disable the added CoreCLR testing since it hit a testing timeout in CI of some kind, but it runs successfully by hand. I'll work that out tomorrow. |
The compiler has some logic to search for known types amongst available DLLs.
With this PR we get to enable several tests that uses F# Interactive on .NET Core and more tests for the F# compiler on .NET Core
Some of the known-type-search logic is OK, but swathes of it are not so good, e.g. this stuff
This improves the search by
just having one function to do the search
moving many "known types" out of Abstract IL (ILGlobals) and into the F# Compiler
TcGlobals. THe advantages of this is that getting started with ILGlobals is much simpler if the set of referenced types is simpler - in particular if the small set of types known to Abstract IL are all always in the primary assembly (Int32, String, Array etc.).This is particularly important for .NET Core. I don't trust the current code to correctly find some of the more obscure types for .NET Core, since the searching was based around various assumptions for Profile 7, 78, 259.
Overall this removes a fair chunk of fragile code from the compiler.
Separately this refactors
TcGlobalsto be a class instead of a record (which requires fewer lines of code)