-
Notifications
You must be signed in to change notification settings - Fork 852
Bugfix :: SRTP - ApplyDefaults for TyparConstraint.MayResolveMember #19218
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
Merged
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
9023435
Srtp bugfix for curryN via applydefaults
T-Gro 27a51e8
Update azure-pipelines-PR.yml
T-Gro e979d4b
Apply suggestion from @T-Gro
T-Gro 04cfafc
Apply suggestion from @T-Gro
T-Gro c98d9f5
Apply suggestion from @T-Gro
T-Gro fa1f33e
Fix buildScript syntax in azure-pipelines-PR.yml
T-Gro ac6123b
Update regression-test-jobs.yml
T-Gro 31f3cb1
Merge branch 'main' into bugfix/srtp-resolution
T-Gro c18d6cf
Add release notes for SRTP resolution fix
T-Gro 0f0e96c
Update regression-test-jobs.yml
T-Gro df496c7
Fix formatting issues in regression test YAML
T-Gro e1eb4bf
Refactor script execution for cross-platform compatibility
T-Gro 01c2a1d
Merge branch 'main' into bugfix/srtp-resolution
T-Gro 1beb018
Fix script execution and extend timeout to 120 minutes
T-Gro 0f76fb5
Merge branch 'main' into bugfix/srtp-resolution
T-Gro 1e47dd8
Apply suggestion from @T-Gro
T-Gro 02e18b4
Merge branch 'main' into bugfix/srtp-resolution
T-Gro 31a7af6
Merge branch 'main' into bugfix/srtp-resolution
T-Gro 753189e
Merge branch 'main' into bugfix/srtp-resolution
T-Gro 6fbc3e1
Delete EXECUTIVE_SUMMARY.md
abonie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| # FSharpPlus curryN Regression Fix - Executive Summary | ||
|
|
||
| ## The Bug | ||
|
|
||
| FSharpPlus `curryN` pattern worked in F# SDK 8.0 but failed in SDK 9.0+ with error FS0030 (value restriction): | ||
|
|
||
| ```fsharp | ||
| let _x1 = curryN f1 100 // SDK 8: OK | SDK 9+: FS0030 error | ||
| ``` | ||
|
|
||
| ## Root Cause | ||
|
|
||
| In `CheckDeclarations.fs`, the `ApplyDefaults` function processes unsolved type variables at the end of type checking. The existing code only solved typars with `StaticReq <> None`: | ||
|
|
||
| ```fsharp | ||
| if (tp.StaticReq <> TyparStaticReq.None) then | ||
| ChooseTyparSolutionAndSolve cenv.css denvAtEnd tp | ||
| ``` | ||
|
|
||
| **The problem**: Some SRTP (Statically Resolved Type Parameter) typars have `MayResolveMember` constraints but `StaticReq=None`. These typars were being skipped, leaving them unsolved. When `CheckValueRestriction` ran next, it found unsolved typars and reported FS0030. | ||
|
|
||
| ## Why This Is a Bug | ||
|
|
||
| The `ApplyDefaults` code checks `StaticReq <> None` to identify SRTP typars that need solving. However, a typar may participate in an SRTP constraint (having a `MayResolveMember` constraint) without having `StaticReq` set. This can happen when: | ||
|
|
||
| 1. The typar is the **result type** of an SRTP method call, not the head type | ||
| 2. The typar is constrained through SRTP constraints but isn't directly marked with `^` | ||
|
|
||
| **Key insight from instrumentation:** | ||
| ``` | ||
| - ? (StaticReq=None, Constraints=[MayResolveMember, CoercesTo]) ← HAS SRTP CONSTRAINT! | ||
| [ApplyDefaults] After processing: 17 still unsolved ← SRTP typar SKIPPED because StaticReq=None | ||
| ``` | ||
|
|
||
| The condition `tp.StaticReq <> None` was too narrow - it missed typars that have SRTP constraints but no explicit static requirement. | ||
|
|
||
| ## Regression Analysis - Git Blame Evidence | ||
|
|
||
| **Root Cause: PR #15181 (commit `b73be1584`) - Nullness Checking Feature** | ||
|
|
||
| The regression was introduced by `FreshenTypar` added in PR #15181: | ||
|
|
||
| ```fsharp | ||
| // src/Compiler/Checking/NameResolution.fs:1600-1604 | ||
| let FreshenTypar (g: TcGlobals) rigid (tp: Typar) = | ||
| let clearStaticReq = g.langVersion.SupportsFeature LanguageFeature.InterfacesWithAbstractStaticMembers | ||
| let staticReq = if clearStaticReq then TyparStaticReq.None else tp.StaticReq // ← BUG! | ||
| ... | ||
| ``` | ||
|
|
||
| **The Mechanism:** | ||
|
|
||
| 1. **SDK 8**: `FreshenTypar` did not exist. When typars were freshened, `StaticReq` was preserved from the original typar. | ||
|
|
||
| 2. **SDK 9+**: When `InterfacesWithAbstractStaticMembers` is enabled (always on), `FreshenTypar` **clears `StaticReq` to `None`** unconditionally. | ||
|
|
||
| 3. **Effect**: SRTP typars still have `MayResolveMember` constraints, but lose their `StaticReq` marker. | ||
|
|
||
| 4. **Consequence**: `ApplyDefaults` checks `if tp.StaticReq <> None` → returns false → typar never solved → FS0030 error. | ||
|
|
||
| **The fix** adds an alternative check for `MayResolveMember` constraints directly, making `ApplyDefaults` robust against this `StaticReq` clearing. | ||
|
|
||
| ## The Fix | ||
|
|
||
| Added a check for `MayResolveMember` constraints in addition to `StaticReq`: | ||
|
|
||
| ```fsharp | ||
| let hasSRTPConstraint = tp.Constraints |> List.exists (function TyparConstraint.MayResolveMember _ -> true | _ -> false) | ||
| if (tp.StaticReq <> TyparStaticReq.None) || hasSRTPConstraint then | ||
| ChooseTyparSolutionAndSolve cenv.css denvAtEnd tp | ||
| ``` | ||
|
|
||
| ## Verification | ||
|
|
||
| - ✅ New curryN-style regression test passes | ||
| - ✅ FSharpPlus `curryN` pattern compiles without type annotations | ||
| - ✅ No regressions introduced in SRTP-related tests |
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
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
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.