-
Notifications
You must be signed in to change notification settings - Fork 832
Fix SignatureHash to include constant values in hash computation #18771
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add hashConst function with exhaustive pattern matching over Const type - Modify hashNonMemberVal to include literal constant value in hash for deterministic builds - This ensures MVIDs change when constant values change (fixes #18758) Co-authored-by: T-Gro <[email protected]>
Copilot
AI
changed the title
[WIP] SignatureHash: include constant value in hash for MVID calculation + test
Fix SignatureHash to include constant values in hash computation
Jul 21, 2025
Copilot finished work on behalf of
T-Gro
July 21, 2025 15:58
T-Gro
approved these changes
Jul 22, 2025
tests/fsharp/TestResults/_pkrvmq0rgcvqdmg_2025-07-21_15_40_57.trx
Outdated
Show resolved
Hide resolved
Co-authored-by: T-Gro <[email protected]>
❗ Release notes required
Warning No PR link found in some release notes, please consider adding it.
|
Copilot finished work on behalf of
T-Gro
July 22, 2025 10:28
T-Gro
reviewed
Jul 22, 2025
tests/fsharp/TestResults/_pkrvmq0rgcvqdmg_2025-07-21_15_41_19.trx
Outdated
Show resolved
Hide resolved
Co-authored-by: T-Gro <[email protected]>
Copilot finished work on behalf of
T-Gro
July 22, 2025 10:42
|
/run fantomas |
T-Gro
approved these changes
Jul 25, 2025
🔧 CLI Command Report
✅ Patch applied: |
Ran fantomas formatting - minor whitespace and indentation fixes applied to TypeHashing.fs in commit a6ce065. |
abonie
approved these changes
Jul 25, 2025
T-Gro
approved these changes
Jul 29, 2025
This was referenced Aug 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
SignatureHash was not including the actual values of literal constants in its hash computation, only their names and types. This caused MVIDs to remain unchanged when constant values changed, which is incorrect for deterministic builds and assembly identity.
For example, these two modules would produce the same MVID despite having different constant values:
Solution
This PR modifies the
hashNonMemberValfunction inTypeHashing.fsto include literal constant values in the hash computation:hashConstfunction with exhaustive pattern matching over allConstcases to deterministically hash constant valueshashNonMemberValto check forVal.LiteralValueand include the constant value hash when presentval_constisSome)Verification
Manual testing confirms the fix works correctly:
Related
Fixes #18758
The implementation uses exhaustive pattern matching over the
Constdiscriminated union to ensure all constant types are handled correctly and deterministically.Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
1k9vsblobprodcus379.vsblob.vsassets.io/home/REDACTED/work/_temp/ghcca-node/node/bin/node --enable-source-maps /home/REDACTED/work/_temp/copilot-developer-action-main/dist/index.js(dns block)dotnet restore ./FSharp.Compiler.Service.sln(dns block)4vyvsblobprodcus361.vsblob.vsassets.iodotnet restore ./FSharp.Compiler.Service.sln(dns block)If you need me to access, download, or install something from one of these locations, you can either:
This pull request was created as a result of the following prompt from Copilot chat.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.