Skip to content

Bugfix :: Nullness :: Eliminate spurious warnings when working with CLI events#19221

Merged
T-Gro merged 13 commits intomainfrom
tgro/event-nullness-fix
Jan 19, 2026
Merged

Bugfix :: Nullness :: Eliminate spurious warnings when working with CLI events#19221
T-Gro merged 13 commits intomainfrom
tgro/event-nullness-fix

Conversation

@T-Gro
Copy link
Member

@T-Gro T-Gro commented Jan 19, 2026

Fix FS3261 nullness warning when implementing INotifyPropertyChanged or ICommand CLIEvent properties. (Issue #18361, Issue #18349)

Fixes #18361
Fixes #18349

T-Gro added 8 commits January 17, 2026 20:55
…sue #18361)

This test reproduces the FS3261 nullness warning that occurs when implementing
INotifyPropertyChanged interface with F# Event and [<CLIEvent>] attribute.

The test documents the current buggy behavior:
- INotifyPropertyChanged with PropertyChangedEventHandler produces spurious FS3261
- F#-defined interfaces with EventHandler do not produce the warning

When the bug is fixed, the test should be updated to use shouldSucceed instead.
Add failing TDD test for issue #18349 - ICommand CanExecuteChanged
CLIEvent produces FS3261 nullness warning with --checknulls enabled.

The test uses System.Windows.Input.ICommand from BCL and
demonstrates the same nullness mismatch bug as #18361
(INotifyPropertyChanged).
Add 'not null' constraints to the delegate type parameters in:
- IDelegateEvent<'Delegate>
- IEvent<'Delegate,'Args>
- DelegateEvent<'Delegate>
- Event<'Delegate,'Args>

These constraints are guarded with #if BUILDING_WITH_LKG || BUILD_FROM_SOURCE
to ensure bootstrap compatibility with the LKG compiler.

This change is part of the fix for nullness warnings with CLIEvent properties
(#18361, #18349). While these constraints alone don't fix the FS3261 warning,
they are the correct FSharp.Core changes needed for proper nullness support.
A compiler-side fix is also needed to fully resolve the issue.
Suppress spurious nullness warnings (FS3261) when implementing interfaces like
INotifyPropertyChanged or ICommand using F# Events with --checknulls enabled.

The fix has three parts:
1. ConstraintSolver.fs: Suppress delegate nullness warnings in SolveNullnessEquiv
   and SolveNullnessSubsumesNullness when both types are delegates (same type,
   different nullness)
2. infos.fs: Strip nullness from delegate type in FindDelegateTypeOfPropertyEvent
3. FSharp.Core: Add 'not null' constraint to DelegateEvent and Event types

The warning was spurious because:
- C# interfaces like INotifyPropertyChanged lack nullable annotations (predate NRT)
- F# treats unannotated C# parameters as nullable
- F#'s event AddHandler expects non-null delegates
- In practice, event handlers are never null

Fixes #18361
Fixes #18349
Subtask 2: Study how byref types are handled specially in SolveTypeSubsumesType
(lines 1562-1573) to understand the correct pattern for adding delegate-specific
handling.

Key findings documented:
- Pattern match guard structure: tyconRefEq + type-specific check
- Nullness is intentionally skipped by omitting SolveNullnessSubsumesNullness call
- Byref case precedes generic TType_app case in pattern match order
- Template for delegate-specific handling following same pattern
…ssSubsumesNullness

Remove the isDelegateTy checks and suppressDelegateWarning logic from the
generic nullness-solving functions. This was a hack that polluted low-level
type unification with domain-specific exceptions.

The proper fix will be added at the correct abstraction level in
SolveTypeEqualsType and SolveTypeSubsumesType following the byref pattern.
Following the byref special handling pattern (Option B from VISION.md),
add TType_app pattern matches for delegate types in SolveTypeEqualsType
and SolveTypeSubsumesType that skip nullness checking.

This is the proper architectural approach - handling delegate types at
the correct abstraction level rather than in low-level nullness functions.

Fixes #18361
Fixes #18349
@github-actions
Copy link
Contributor

github-actions bot commented Jan 19, 2026

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/FSharp.Core docs/release-notes/.FSharp.Core/10.0.200.md
src/Compiler docs/release-notes/.FSharp.Compiler.Service/10.0.200.md

@T-Gro T-Gro marked this pull request as ready for review January 19, 2026 12:15
@T-Gro T-Gro requested a review from a team as a code owner January 19, 2026 12:15
@T-Gro T-Gro requested a review from abonie January 19, 2026 12:22
@T-Gro T-Gro enabled auto-merge (squash) January 19, 2026 12:22
@github-project-automation github-project-automation bot moved this from New to In Progress in F# Compiler and Tooling Jan 19, 2026
@T-Gro
Copy link
Member Author

T-Gro commented Jan 19, 2026

/azp-run

@T-Gro
Copy link
Member Author

T-Gro commented Jan 19, 2026

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@T-Gro T-Gro merged commit 966a2c0 into main Jan 19, 2026
40 checks passed
@T-Gro T-Gro deleted the tgro/event-nullness-fix branch January 20, 2026 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

2 participants