Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 30, 2023

When you think a compiler problem is solved,
Take caution before you get involved,
Check your code and run some tests,
For what seems fixed, may still protest.

I believe this to be one of my better attempts at making --test:GraphBasedChecking deterministic.

The initial intent was to solve #14850 and not get mixed up in any graph-based type-checking adventure.
But alas, my Stockholm syndrome kicked in.

In 97f3d0a I first try to illustrate the problem case.
This is a repeat of the run-through of this comment.
In short, the typars of a Val inside the ModuleOrNamespaceType will only get a pretty name if that function is used somewhere. A side effect of this is an impact in the letters used for pretty naming the typars (of the ValRef).

To overcome this, in 03085c3, I try to prettify the typar names of the signature data (ModuleOrNamespaceType) in PostInferenceChecks.fs.
Because this gets pretty names at the point the function is defined, this will overcome the problem that the pretty naming happens when the function is later invocated in another file. (CheckValRef will already have the proper names).

I believe this change would solve #14850, but it wouldn't solve the race condition of hash constraints.

For example: val someGenericFunction: #exn list -> unit in a signature file would still have ? after checking the signature file. In graph-based type-checking multiple files could already use this information in parallel, so the naming would kick in again when the function is first invocated.

That is why in d6b30e0 I'm calling the name logic again, right after the signature file was checked.

After some local testing (with some wonderful Thread.Sleep (rnd.Next(100, 500)) during the type-checking of each file) I found out that I was able to produce the exact same binary over 50 times in a row with graph-based checking. Sequential checking produced that exact same binary.

This doesn't prove anything, but I do believe this is a step in the right direction.
The big question of course remains if these changes are sensible or not.

Fingers crossed,

Florian

@nojaf nojaf marked this pull request as ready for review April 4, 2023 11:24
@nojaf nojaf requested a review from a team as a code owner April 4, 2023 11:24
@nojaf
Copy link
Contributor Author

nojaf commented Apr 4, 2023

After seeing this pass for all builds (I'm talking about Build Determinism_Debug experimental_features, which has passed even when the full fsharp-ci didn't), I would like to have this change.

It resolves the original problem of #14850 and would unblock us to start using --test:GraphBasedChecking in combination with --deterministic+.

If we have this in a nightly dotnet 8 SDK we can start doing extensive testing on our end to ensure that the binary produced by sequential and graph-based type-checking is the same.

@nojaf nojaf requested a review from vzarytovskii April 6, 2023 11:57
@T-Gro T-Gro merged commit 3ba4eec into dotnet:main Apr 7, 2023
@dsyme
Copy link
Contributor

dsyme commented Apr 11, 2023

@nojaf Amazing! Great to see the second take on this nailed it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants