Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Feb 6, 2023

In light of my investigation of the determinism of #14494, I found out that the mvid is stable (when using graph type checking) with these tweaks.

For TypedTree.fsi I'm just updating the signature with what is actually used. It is a bit weird that these were different in the first place.

For local.fsi, the _ leads to using the internal name
image, which again I don't think should have been there in the first place.

I believe the remaining determinism problem is with the pdb generation. But that is still a hunch at this point.

@nojaf nojaf requested a review from a team as a code owner February 6, 2023 13:52
@vzarytovskii
Copy link
Member

So, this is the fix for the FCS itself, right? Do you have any suggestions how can we fix it for everyone without turning off the graph checking?
We also should have a separate issue for investigating PDB issue, I thinkg.

@auduchinok
Copy link
Member

Would it be possible to keep these signatures as is, but to fix the logic that causes the issues? Or at least we should later add tests for these cases, so they assert that they still work with determinism enabled.

@nojaf
Copy link
Contributor Author

nojaf commented Feb 8, 2023

Would it be possible to keep these signatures as is, but to fix the logic that causes the issues?

In theory yes, but I'm unable to reproduce this problem outside of FCS or FSharp.Core.
On my local machine, I created a project where I have both problems in 50 different file pairs.
These all have no relation and are processed in parallel. After compiling 50 times, the mvid is exactly the same.

At this point, I'd rather have this merged in and wait for a more obvious case to appear in the wild.

@safesparrow
Copy link
Contributor

At this point, I'd rather have this merged in and wait for a more obvious case to appear in the wild.

Regardless of whether it's merged or not, we should use the case we have at hands as it can help us solve the general problem.
If we do merge it, we can always use a historical revision of the codebase for determinism testing, but that makes it slightly more complicated for an average contributor.

@vzarytovskii vzarytovskii merged commit 2917555 into dotnet:main Feb 14, 2023
@nojaf nojaf deleted the fix-signature-files branch February 14, 2023 10:24
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.

6 participants