Enable nullable reference types across FEx (convention change)#19
Open
GioAbq wants to merge 7 commits into
Open
Enable nullable reference types across FEx (convention change)#19GioAbq wants to merge 7 commits into
GioAbq wants to merge 7 commits into
Conversation
…rors) Enable nullable reference types across Layer 0 of the FEx nullable migration (convention change: nullable now ON globally). Agnostics.Abstractions defines the shared patterns: Guard helper annotated [NotNull]/[return: NotNull] (via a SysNotNull alias to avoid CS0104 with JetBrains.Annotations); Error/Result domain model made nullable where absence is valid; generic dictionary/set keys constrained 'notnull'; no blanket null-forgiving. Bumps DevConfigs to the PolySharp-enabled commit. Verified: all 4 L0 projects build green on every TFM.
L1: Agnostics, Agnostics.TestMocks, DependencyInjection.Abstractions, CLI, WebScraping, OneDrv.Abstractions L2: DependencyInjection, MVVM.Abstractions, Logging.Abstractions, PersistentStorage.Abstractions, Json L3: Core.Abstractions, Logging, PersistentStorage, OneDrv Idiomatic nullable annotations: nullable params/returns/fields, widened override/interface members, where T : notnull, [NotNullWhen]/[return: MaybeNull], L0 Guard/GuardProperty for [NotNull] propagation. null!/default! only for documented invariants and null-object no-ops. No #nullable directives, no blanket suppression. Verified green build (0 err/0 warn) on all TFMs per project.
L4: Core, Asyncx, Sqlx.Abstractions, Maui, RESXx, Platforms, Platforms.Windows, Telemetry, Common.Abstractions, PersistentStorage.Rx, Encryption, Logging.Web L5: MVVM, FileSystem, NuGetx, Telemetry.Sentry, Common, MVVM.Rx.Legacy, MVVM.Rx, Sqlx, EFCore, SecureStorage, AppSettings, Flurlx Idiomatic nullable annotations adapting to now-nullable L0-L3 APIs: widened module RegisterServices(TContainer?) + Guard re-narrow, where T : notnull on concurrent dictionaries, nullable returns/params, override/interface signature matches. null!/default! only for documented invariants and sibling-interface contracts (each commented). No #nullable directives, no blanket suppression. Verified green build (0 err/0 warn) on all TFMs per project incl. Maui/Windows.
L6: Webx, Flurlx.Newtonsoft, MSBuildx, Telemetry.Sentry.Web, Avaloniax L7: Downloader, Legacy, AzureStorage L8: FTPx, WPFx L9: Imaging.Windows L10: AzureDevOpsx Completes the src nullable migration. Honest nullable signatures over suppression, module RegisterServices(TContainer?)+Guard pattern, override/ interface matches. null!/default! only for documented invariants (WPF DependencyProperty statics, framework-init fields), each commented. No #nullable directives, no blanket suppression. Verified green (0 err/0 warn) all TFMs. Follow-up (flagged by Webx): IsNotNullOrEmptyReadOnlyCollection in Agnostics.Abstractions lacks [NotNullWhen(true)]; worked around locally.
…llable test: DependencyInjection.Tests, MVVM.Tests, FileSystem.Tests, Flurlx.Tests. Adapted test models/helpers to nullable (Child?/Name?/TestData.Name?), widened TestInitializeModule override to match now-nullable RegisterServices/GetModule, Task<TestData?> for the bulkhead lambda that returns null on rejection. FileSystem root-cause (revealed by null-tolerance tests): GetGitWorktreeKind(this DirectoryInfo?) and WhoIsLocking(string?) - both already null-check internally, so the honest param is nullable; L4-L5 pass had left them non-null. IDISP001 on GetModule() peek suppressed locally (default container owned by FExServiceProvider, not the test) - repo-standard #pragma pattern. Completes nullable migration: full `dotnet build FEx.slnx` green, 0 err / 0 warn.
…ct <Nullable> CLAUDE.md: nullable is now enabled globally via DevConfigs (PolySharp polyfills attributes on down-level TFMs); do not re-declare <Nullable> per project. Removed the now-redundant <Nullable>enable</Nullable> from the 3 samples and FEx.Building (inherited from DevConfigs/Directory.Build.props).
Clean fixes: removed redundant usings/base-ctor/casts/assignment, removed a redundant optional-param default (explicit overload exists), !b -> !b.Value. Justified suppressions (each commented; build of all TFMs incl. net481 stays green, so none hides a real error): - IDISP004 x4: Guard returns the same instance, does not create a disposable. - RedundantCast x3: cast required on down-level TFMs (oblivious object/string) to keep the IDictionary<,object?>/<,string?> contract - verified CS8619 without it. - SuggestVarOrType x2: var would infer non-null Type and break BaseType reassign. - ArrangeRedundantParentheses x2: parens needed so ! applies to the ?. result. - RedundantUsingDirective x1: CodeAnalysis using is used on net10. - ParameterOnlyUsedForPreconditionCheck x1: pre-existing (environmentId validated, not stored) - flagged for separate review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Enables
<Nullable>enable</Nullable>globally across the FEx library (convention change). Every project now compiles under nullable reference types with the zero-warning policy (TreatWarningsAsErrors=true), so all CS8xxx were build errors that had to be resolved.Why
Nullable reference types catch null-dereference bugs at compile time and make the public API's nullability explicit for consumers. Deliberate convention change (previously "nullable disabled by default").
Scope
srcprojects + 4 test projects annotated, migrated layer by layer (L0 foundational abstractions through L10 leaf projects, then tests).where T : notnull,[NotNullWhen]/[return: MaybeNull], andGuard/GuardProperty(which carry[NotNull]) to re-narrow.null!/default!used only for documented invariants and null-object no-ops; no blanket suppression, no#nullabledirectives (nullability stays props-driven).System.Diagnostics.CodeAnalysisnullable attributes so annotations compile on every TFM.Breaking changes
Some public API signatures were widened to nullable where the value is genuinely nullable (honest annotations over
= null!suppression), acceptable per FEx strategy (internal lib; consumers migrate at their own pace). A few behavior deltas are documented in commit messages (some debug-view / paginated-list constructors now throwArgumentNullExceptionon null instead of a deferred NRE; a couple of already-null-tolerant methods gained nullable params).Verification
dotnet build FEx.slnxgreen on all TFMs incl. net481 (0 errors, 0 warnings underTreatWarningsAsErrors).Depends on
DevConfigs
main@c3568ac(Enabled Nullable + PolySharp), pinned via the submodule pointer.Follow-ups (separate)
FuncProcessor.environmentIdis validated but never stored - pre-existing smell, flagged for review.IsNotNullOrEmptyReadOnlyCollectionin Agnostics.Abstractions lacks[NotNullWhen(true)](worked around locally).