-
Notifications
You must be signed in to change notification settings - Fork 830
Enable graph-based type checking, parallel optimizations and ILX gen in deterministic build #19028
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
base: main
Are you sure you want to change the base?
Conversation
❗ Release notes required
|
|
Good thing is I got exactly the same diff locally as in the CI, so it is already deterministic in this regard 😁 |
|
Ok so it's not a type parameter but actual char type. It just randomly gets emitted either as Char or char. Edit: Nope it was because of not unified type argument names between type declaration and extensions. |
eb10796 to
128da51
Compare
|
TODO: fsharp/src/Compiler/CodeGen/IlxGen.fs Line 2337 in 12efe3b
and fsharp/src/Compiler/CodeGen/IlxGen.fs Line 2757 in 12efe3b
Apart from stabilizing these generated names, the order of of generated IL also needs to be deterministic. Specifically, in FCS.dll generated types in PrivateImplementationDetails are sometimes in different order. Possibly relevant: fsharp/src/Compiler/CodeGen/IlxGen.fs Lines 2389 to 2394 in 12efe3b
|
f837b8d to
c032cc3
Compare
|
Ready for review, cc @nojaf |
nojaf
left a comment
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.
Can't say this feels cathartic just yet.
You danced around a problem without context, don't do that.
Nobody in the future will have any use for that.
PS: I do holy appreciate your effort here, sorry for sounding grumpy.
tests/FSharp.Compiler.ComponentTests/TypeChecks/Graph/DependencyResolutionTests.fs
Outdated
Show resolved
Hide resolved
I don't mind grumpy 😄 I was expecting it. This PR is another chaotic aftermath of a fight. Your input is the best in bringing order from the chaos, I noticed. |
5d68b92 to
caebe81
Compare
18aec62 to
45ffedc
Compare
it needs to be made compatible with parallel checking
513aa4f to
556f28c
Compare
T-Gro
left a comment
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.
Waiting for #19062
|
One thing where I do not have high confidence in the automated test suite (==> will need more manual testing and dogfooding) is the debugger support via pdbs. |


This as a follow up to #18998:
Necessary changes to enable determinism:
There are no additional tests for determinism, it is currently tested only by running the
test-determinismscript locally and in the CI.One issue needed investigation: type extensions with mismatched type argument names can break determinism:
Either
charorCharwill end up in the IL.This is related to #15287 and needs work: #19033