Fix #19657: MeasureAnnotatedAbbreviation over ref type + | null#19775
Fix #19657: MeasureAnnotatedAbbreviation over ref type + | null#19775T-Gro wants to merge 4 commits into
Conversation
…eference type with | null Add four tests in NullableReferenceTypesTests.fs covering: - MeasureAnnotatedAbbreviation over string accepts | null in let bindings, parameters, returns, type abbreviations, and inline instantiations (fails today). - MeasureAnnotatedAbbreviation over user-defined reference class accepts | null (fails today). - MeasureAnnotatedAbbreviation over value types still rejects | null (non-regression, passes today). - Nullness flow and not-null constraints work through the abbreviation (fails today; compilation does not reach the flow assertions). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MeasureAnnotatedAbbreviation tycons over a reference type (e.g. UMX-style `type string<[<Measure>] 'm> = string`) previously failed the reference-type constraint check used by the `| null` syntax, producing FS0043. The surface type is a TType_app over a MeasureableReprTycon, which isRefTy does not recognise. Strip measure equations before testing reference semantics so the underlying erased representation is consulted, consistent with TypeNullNever and IsReferenceTyparTy. Value-type erasure still correctly fails FS0043. Also corrects a Sprint 1 test expectation that listed a phantom diagnostic which the compiler does not emit (the matched-on null case is fully covered). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Rename strippedTy -> underlyingTy for consistency with SolveTypeIsNonNullableValueType - Condense comment to single line - Add test: not-struct constraint satisfied by MeasureAnnotatedAbbreviation - Add test: MAA over obj, interface, and chained abbreviation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
T-Gro
added
AI-Auto-Resolve-CI
❗ Release notes required
Warning No PR link found in some release notes, please consider adding it.
|
github-actions
Bot
added
the
AI-Tooling-Check-Bypassed
T-Gro
left a comment
T-Gro
left a comment
There was a problem hiding this comment.
Expert Review: PR #19775 — Fix #19657: MeasureAnnotatedAbbreviation + | null
Overall Assessment: ✅ LGTM
Clean, minimal, correct bug fix with excellent test coverage.
Type System Correctness ✅
The fix is sound. isRefTy does not strip measure equations internally — it checks structural forms via stripTyEqns, but not measure equations. A [<MeasureAnnotatedAbbreviation>] type like type string<[<Measure>] 'm> = string stores its equation as a measure equation, so isRefTy couldn't see through it.
The fix correctly applies stripTyEqnsAndMeasureEqns before the isRefTy check, exactly as the sister functions SolveTypeIsNonNullableValueType (line 2932) and SolveTypeRequiresDefaultConstructor (line 3003) already do. The error message correctly preserves the original ty for user display.
Test Coverage — Excellent ✅
Six tests covering:
- Positive:
string, custom reference types,obj, interfaces, chained abbreviations - Negative: value types (
int,DateTime) correctly rejected - Nullness flow: widening/narrowing semantics preserved, null-check narrowing works
- Constraint interaction:
not structconstraint satisfaction verified
Struct Awareness — Correctly Preserved ✅
The negative test verifies int<kg> | null and DateTime<s> | null still produce errors (both Error 3260 and Error 43). The fix doesn't break the value-type path because isRefTy g underlyingTy returns false for structs.
Feature Gating — Not Required ✅
Pure bug fix — | null and MeasureAnnotatedAbbreviation are both existing features. No language version gate needed.
Backward Compatibility — Safe ✅
Previously-rejected code now compiles. No previously-accepted code is rejected. No binary format or public API changes.
Minor Note (informational)
isRefTy could arguably be enhanced to internally call stripTyEqnsAndMeasureEqns to prevent this class of bug at other call sites — but that's a broader refactoring beyond this PR's scope.
1 participant
Fixes #19657
SolveTypeIsReferenceTypedid not strip measure equations before callingisRefTy, unlike its siblings (SolveTypeIsNonNullableValueType,SolveTypeRequiresDefaultConstructor). This caused[<MeasureAnnotatedAbbreviation>]types over reference types (e.g. FSharp.UMXstring<'m>) to be rejected by| nulland'T : not structconstraints.