Repository navigation
fix(nullability): converters told the compiler Avalonia never passes null, and it does - #227
Conversation
…null, and it does Avalonia calls IValueConverter.Convert with a null `value` for any unset binding and a null `parameter` on every binding without a ConverterParameter — which is most of them. Eleven of the twelve converters here declared those parameters non-nullable anyway, so the annotation asserted something the framework contradicts on the first call. Same for the two `Equals(object obj)` overrides, which the runtime calls with null. The bodies were already null-safe (`is` patterns, an explicit `value == null` guard in ListToStringConverter, three null branches at the top of NaturalCompare), so nothing about behaviour changes — only the declaration stops lying. ListToStringConverter's null guard in particular read as dead code before this: the compiler had been told value could not be null. Signature-only: 24 insertions, 24 deletions, no line added or removed. Desktop warnings 235 -> 193 on cold builds of merge-base and branch; the distinct first-party warning set differs by exactly the 42 removed lines (36 CS8767, 4 CS8769, 2 CS8765) with no additions and no line-number shift. The correct shape was already in the tree — BoolOrConverter, FirmwareOption.Equals and DaqifiStreamingDevice.Equals are all hand-written and all get it right. This brings the portomatic-scaffolded stragglers into line with them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR |
PR Summary by QodoAlign converter and override signatures with nullable contracts
AI Description
Diagram
High-Level Assessment
Files changed (14)
|
|
Qodo-clean, CI green — ready for review. Verified on head |
Not merging — for review.
Eleven of the twelve value converters in this app declared
valueandparameterasnon-nullable. Avalonia passes
nullforparameteron every binding that has noConverterParameter— which is nearly all of them — andnullforvalueon any bindingthat has not resolved yet. The annotation asserted something the framework contradicts on
the first call. Same story for the two
Equals(object obj)overrides: the runtime callsEqualswith null.This declares those parameters the way the interface declares them. Nothing else.
The class
One rule, applied everywhere it holds: a parameter's nullability must match the member
being implemented or overridden.
CS8767NaturalStringComparer.Compare(implicit interface impl)CS8769EnumDescriptionConverter(explicit interface impl)CS8765AbstractChannel.Equals,LoggingSession.Equals(override)NaturalSortHelper.NaturalCompareis annotated too, though it warned about nothing: thecomparer that delegates to it now accepts
string?, andNaturalComparealready opens withthree null branches. Leaving it non-nullable would have traded two
CS8767for twoCS8604.Why this is behaviour-preserving
Nullable reference annotations are erased at runtime — this is metadata, not IL. And every
body was already null-safe before the change:
ispatterns, which are null-safe by construction;ListToStringConverteropens with an explicitif (value == null) return string.Empty;;NotNullToVisibilityConverterliterally returnsvalue != null;NaturalCompareopens with three null branches;Equalsoverrides useis/is notpatterns.So no null check was added, removed, or moved. The interesting case is
ListToStringConverter: its null guard was unreachable-looking before this, because thecompiler had been told
valuecould not be null. The guard was right and the signature waswrong. That is the shape of the whole change.
No renames, no removals, no member moved — so there is no XAML-binding exposure here. Every
converter keeps its type name and its interface methods; only parameter annotations change.
Measurement
Both numbers measured in the same worktree, cold:
obj/andbin/removed tree-wide(including
third_party/oxyplot-avalonia/obj, which only re-emits its 20 warnings when itsown
obj/is cold) and then built with the same command.origin/main, 5414c07)third_party/Diffing the distinct first-party warning set on both sides, the delta is a pure
deletion: 42 lines removed, 0 added, 0 changed, no line-number shift (the diff is
signature-only, so no line was added or removed from any file). Attribution, all 42:
CS8767—BoolAndToVisibilityConverter(1),BrushColorMatchConverter(1),BoolToActiveStatusConverter(4),BoolToStatusColorConverter(4),ConnectionTypeToColorConverter(4),ListToStringConverter(4),NotNullToVisibilityConverter(4),OxyColorToBrushConverter(4),BooleanToInverseBoolConverter(4),IntToVisibilityConverter(4),NaturalSortHelper(2)CS8769—EnumDescriptionConverterCS8765—AbstractChannel,LoggingSessionEvery other warning code is unchanged in both count and text:
CS861878,CS860115,CS862213,CS860013,CS86259,CS86029,CS86048,CS06188,CS86036,CA14165,MVVMTK00345,CS00672,AVLN30011,CS02191.Diffstat
24 for 24, no line added or removed — every changed line is a signature gaining a
?. Notnet-additive; not net-smaller either, because there is nothing to delete here. What gets
smaller is the set of false claims the code makes about itself.
Deliberately out of scope
EnumDescriptionConverter.cs(46): CS8603 Possible null reference returnsits in a filethis PR touches and survives it. It is a return-nullability issue, not a parameter one,
and fixing it means choosing whether the converter may return null — a behaviour decision.
Different class, different PR.
#pragma, noNoWarn, no attribute. The 42 warnings are gonebecause the declarations are now true.
Verification
dotnet build Daqifi.Avalonia.Desktop— green, 193 warnings, 0 errorsdotnet restore Daqifi.Avalonia.iOSthendotnet build Daqifi.Avalonia.iOS -c Release -r iossimulator-arm64 --no-restore— green,194 warnings, 0 errors (no merge-base pair measured for iOS; reported as an absolute only)
dotnet test Daqifi.Avalonia.Tests— 462 passed, 0 failedpackages.lock.jsonunchanged in all six lock-file projects (restore was run without-r)Prior art in this repo
The correct shape was already present, in every file that was written by hand rather than
scaffolded:
BoolOrConverter.Convertalready takesobject? parameter, andFirmwareOption.Equals/DaqifiStreamingDevice.Equalsalready takeobject? obj. Allfourteen files changed here carry the
// Generated by portomatic scaffoldheader. Thisbrings the scaffolded stragglers into line with the hand-written ones.