Fix ValueOption optional args with ?param caller syntax#19742
Conversation
❗ Release notes required
|
When the SupportValueOptionsAsOptionalParameters language feature is enabled, do not force option<_> on caller-side ?name = expr arguments during overload inference. Leave the type as a fresh inference variable and let AdjustCalledArgTypeForOptionals (CalleeSide) reconcile against option<_> or voption<_>. Pre-LangVersion-10 behaviour is preserved. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
01ce17d to
a53024c
Compare
T-Gro
left a comment
There was a problem hiding this comment.
Clean, well-scoped fix. The root cause was that TcMethodApplication_SplitSynArguments eagerly constrained the caller argument type to option<_> when the ?param = expr syntax was used, preventing ValueOption from ever unifying correctly.
The fix correctly gates on SupportValueOptionsAsOptionalParameters to leave the type as a fresh inference variable when the feature is enabled, letting later unification in AdjustCalledArgTypeForOptionals (CalleeSide branch) match the actual declared parameter type — whether option<_> or voption<_>.
Key points verified:
- Correctness: The CalleeSide branch at line 382 passes through
calledArgTyunchanged (which already handles both option/voption), so the deferred unification naturally resolves to the right wrapping type. - Backwards compat: LangVersion < 10 preserves old behavior (early
option<_>constraint). - Test coverage: Methods, constructors, both option variants, and langversion gating are all exercised.
- No inference regressions: Since
None/SomeandValueNone/ValueSomeare unambiguous on their own, removing the early constraint doesn't create inference ambiguity for practical cases.
LGTM ✅
| |> withLangVersion90 | ||
| |> typecheck | ||
| |> shouldFail | ||
| |> withDiagnosticMessageMatches "voption" |
There was a problem hiding this comment.
Any specific reason to underspecify the diagnostic message here? Other negative tests in this file are way more specific, would be safer to ensure the expected error is triggered.
Fix caller-side
?name = exprsyntax for ValueOption optional parameters whenSupportValueOptionsAsOptionalParametersis enabled.Fixes #19711