-
Notifications
You must be signed in to change notification settings - Fork 833
Fix nullness flow for type aliases after null pattern #18852
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
base: main
Are you sure you want to change the base?
Conversation
❗ Release notes requiredCaution No release notes found for the changed paths (see table below). Please make sure to add an entry with an informative description of the change as well as link to this pull request, issue and language suggestion if applicable. Release notes for this repository are based on Keep A Changelog format. The following format is recommended for this repository:
If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request. You can open this PR in browser to add release notes: open in github.dev
|
…patterns - Add tryRefineToNonNullPreservingAbbrev helper to TypedTreeOps.fs - Modify TcMatchClause in CheckExpressions.fs to use new helper - Preserves type aliases while refining nullness after null patterns Co-authored-by: T-Gro <[email protected]>
- Modified helper function to preserve type structure - Tests still show type variable not being refined correctly - May need different approach to handle defaulting interaction Co-authored-by: T-Gro <[email protected]>
- Modify removeNull function in TcMatchClause to preserve original type structure - Use stripTyEqns only for decision making, apply refinement to original type - Directly construct refined types with KnownWithoutNull nullness - This preserves type aliases like 'objnull = obj | null' during refinement Co-authored-by: T-Gro <[email protected]>
null
pattern (preserve aliases); fix defaulting case (failwith ""
-> obj|null)objnull
from failwith
after null pattern
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot : You broke the compiler, this change only makes null elimination worse:
e.g .this now fails:
let map f (arr:_ ProvidedArray) : _ ProvidedArray =
match arr with
| null -> null
| notNull -> notNull |> Array.map f
src/Compiler/TypedTree/TypeProviders.fs(30,33): error FS3261: (NETCORE_ENGINEERING_TELEMETRY=Build) Nullness warning: A non-nullable ''a array' was expected but this expression is nullable. Consider either changing the target to also be nullable, or use pattern matching to safely handle the null case of this expression.
You also created this massive tryRefineToNonNullPreservingAbbrev
but did not use it.
and your changes in CheckExpressions.fs on line 10740 are not doing the real thing, are not even abbreviation related, and just breaking what worked.
Start from scratch and try a different approach which will only fix the bug in case of a type abbreviation, not break what already used to work.
@copilot : See the comment above. |
…oach Co-authored-by: T-Gro <[email protected]>
I've reverted all previous changes and implemented a targeted fix (commit 6ebd419). The new approach only affects actual type abbreviations detected via |
objnull
from failwith
after null pattern
Problem
When nullness checking is enabled, variables in non-null branches of match expressions should be refined to non-null types, but type aliases were not being preserved during this refinement process.
Consider this example:
Previously, the type alias
objnull
would be stripped during nullness refinement, and the variablex
would lose its connection to the original type alias.Root Cause
The issue was in the
removeNull
function withinTcMatchClause
inCheckExpressions.fs
. The original implementation used:This approach stripped type abbreviations using
stripTyEqns
and then applied the nullness refinement to the stripped type, losing the original type alias information.Solution
Modified the nullness refinement logic with a targeted approach that only affects type abbreviations:
This surgical approach:
tcref.Deref.IsTypeAbbrev
KnownWithoutNull
Impact
objnull = obj | null
are now preserved in non-null branchesFixes #18488
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.