fix: Add null checks when mapping nullable sources to non-nullable targets with MayBeNull Attribute by Abhijithtv · Pull Request #2256 · riok/mapperly

Abhijithtv · 2026-05-01T08:45:19Z

Add null checks when mapping nullable sources to non-nullable targets with MayBeNull Attribute

Description

For target members, [MaybeNull] should not be considered when determining nullability.
Otherwise, it may lead to incorrect nullable analysis and result in CS8601 ("Possible null reference assignment") warnings.

Fixes #2192

Checklist

I did not use AI tools to generate this PR, or I have manually verified that the code is correct, optimal, and follows the project guidelines and architecture
I understand that low-quality, AI-generated PRs will be closed immediately without further explanation
The existing code style is followed
The commit message follows our guidelines
Performed a self-review of my code
Hard-to-understand areas of my code are commented
The documentation is updated (as applicable)
Unit tests are added/updated
Integration tests are added/updated (as applicable, especially if feature/bug depends on roslyn or framework version in use)

latonz · 2026-05-01T10:48:57Z

Thanks for the contribution! The fix turns out to be a bit more involved than it might initially appear, IMO also my original MayBeNull support PR was short-sighted. To handle this correctly, I think we need to support both MayBeNull (when reading values) and AllowNull (when writing values).

Abhijithtv · 2026-05-01T11:37:13Z

Thanks, that makes sense!

I checked and it looks like AllowNull isn't currently considered in target nullability checks (I also couldn’t find any tests covering it).

Would you prefer this to be handled in a separate PR focusing on AllowNull support?

Also, regarding the implementation: would you recommend modeling read and write nullability as separate properties (e.g., sourceMayBeNull vs targetAcceptsNull), or passing a context flag like isRead/isWrite?

latonz · 2026-05-01T11:39:44Z

As these two strongly belong together and the changes shouldn't get too big, I'd do it in the same PR.
I'd prefer separate properties/methods.

todo - add test for allow Null

latonz

Thanks for the updates, I added my feedback.

Please also update the 5-0.md breaking changes docs.

latonz · 2026-05-02T08:43:11Z

-            );
+            .HaveDiagnostic(
+                DiagnosticDescriptors.MapValueMethodTypeMismatch,
+                "Cannot assign method return type string of BuildValue() to B.Value of type string"


This error message is misleading (string mismatch vs string).

latonz · 2026-05-02T08:48:22Z

The test name no longer matches the assertion — it used to allow the nullable return, it now diagnoses it. Please rename to something like MethodToMaybeNullTargetPropertyShouldDiagnoseNullableReturnType.

latonz · 2026-05-02T08:50:09Z

Same as above, name says "should map", body now asserts a diagnostic. Please rename and consider whether this case (both sides annotated [MaybeNull]) is what users actually expect to break. Testing [AllowNull] as the way to opt into nullable writes would help here too.

latonz · 2026-05-02T08:51:14Z

@@ -92,6 +92,18 @@ private bool IsAccessible(ISymbol symbol, MemberVisibility visibility)
    }

    public bool IsNullable(ISymbol symbol)


Three near-identical helpers (IsNullable, IsReadNullable, IsWriteNullable) is a foot-gun, IsNullable now ignores both [MaybeNull] and [AllowNull], so any future call site that picks it without thinking will reintroduce the bug this PR fixes. Could the IsNullable methods be made private?

I have removed the IsNullable and replaced all its reverences with IsReadNullable and IsWriteNullable

latonz · 2026-05-02T08:55:36Z

+    public bool IsReadNullable => parameter.Type.IsNullable();
+    public bool IsWriteNullable => parameter.Type.IsNullable();


These ignore [MaybeNull] / [AllowNull] on the parameter symbol, while ConstructorParameterMember correctly delegates to SymbolAccessor.IsReadNullable/IsWriteNullable…

Abhijithtv · 2026-05-03T20:38:45Z

    public ITypeSymbol Type => parameter.Type;
    public INamedTypeSymbol? ContainingType => null;
-    public bool IsNullable => parameter.Type.IsNullable();
+    public bool IsReadNullable =>


For MethodMapping, is it sufficient to rely on the parameter type to determine nullability?

Currently, we do not store the parameter symbol within MethodMapping (I’m not sure whether this is feasible or intended). Could you provide some guidance on how nullability should be handled in this case?

We can't only rely on the type as that would ignore these attributes.

I think storing the symbol within the method parameter should be okay. I'd add helpers though (IsWriteNullable and IsReadNullable).

Abhijithtv · 2026-05-04T20:00:26Z

@latonz Thanks for the review.

I realized I forgot to mark the PR as a draft earlier. Your feedback was really helpful.
I believe the PR is ready for review now.

latonz · 2026-05-05T17:05:51Z

+- `MaybeNull` attributes are now respected for fields, properties and constructor parameters in read context.
+- `AllowNull` attributes are now respected for fields, properties and constructor parameters in write context.


instead of in read/write context IMO when reading/writing values may be clearer?

latonz · 2026-05-05T17:08:58Z

+            .GetReturnTypeAttributes()
+            .Any(a => string.Equals(a.AttributeClass?.Name, nameof(MaybeNullAttribute)));
+
+        var effectiveType = hasNullableAttribute


IMO SymbolAccessor.TryHasAttribute<MaybeNullAttribute>(methodSymbol.GetReturnTypeAttributes()) should be used.

latonz · 2026-05-05T17:10:26Z

+        bool hasNullableAttribute = methodSymbol
+            .GetReturnTypeAttributes()
+            .Any(a => string.Equals(a.AttributeClass?.Name, nameof(MaybeNullAttribute)));
+
+        var effectiveType = hasNullableAttribute


this logic is duplicated with the UserMethodMappingExtractor… extract into a new helper, e.g. SymbolAccessor.UpgradeReturnNullable(IMethodSymbol)

latonz · 2026-05-05T17:11:18Z

+    int Ordinal,
+    string Name,
+    ITypeSymbol Type,
+    IParameterSymbol? symbol = null,


Why is this lower cased?

latonz · 2026-05-05T17:15:56Z

    public ITypeSymbol Type => parameter.Type;
    public INamedTypeSymbol? ContainingType => null;
-    public bool IsNullable => parameter.Type.IsNullable();
+    public bool IsReadNullable =>


We can't only rely on the type as that would ignore these attributes.

I think storing the symbol within the method parameter should be okay. I'd add helpers though (IsWriteNullable and IsReadNullable).

latonz · 2026-05-05T17:18:25Z

+
+    private static string FormattedReturnType(IMethodSymbol methodSymbol)
+    {
+        bool hasNullableAttribute = methodSymbol


We use var across this codebase unless the type really adds some value or is necessary for the compilation. IMO it doesn't add any value here…

latonz · 2026-05-05T17:21:46Z

-    public bool IsAnyNullable() => Path.Any(p => p.IsNullable);
+    public bool IsAnyReadNullable() => Path.Any(x => x.IsReadNullable);
+
+    public bool IsWriteNullable() => Path[^1].IsWriteNullable;


Shouldn't this be moved to NonEmptyMemberPath?

Abhijithtv added 2 commits May 1, 2026 12:51

fix: Ignore MaybeNull property of target member

ed87c50

refactor: Refactor the code

f09ac6f

Abhijithtv changed the title ~~fix: Add null checks when mapping nullable sources to non-nullable targets with MayBeNull Attribute #2192~~ fix: Add null checks when mapping nullable sources to non-nullable targets with MayBeNull Attribute May 1, 2026

refactor: Add write and read null check

a8265f5

todo - add test for allow Null

latonz requested changes May 2, 2026

View reviewed changes

latonz added bug Something isn't working enhancement New feature or request labels May 2, 2026

Abhijithtv marked this pull request as draft May 3, 2026 09:16

Abhijithtv added 5 commits May 3, 2026 14:48

fix: Rework delegate mapping and failed test cases

467117e

fix: Include MaybeNull to decide the return type of method symbol

f9c3e0b

refactor: Remove redundant IsNullable and MemeberType

cbd86fc

feat:Add read and write null check for source parameter

9105d75

refactor: Test name based on context

3cb9ace

Abhijithtv commented May 3, 2026

View reviewed changes

Abhijithtv added 4 commits May 4, 2026 02:40

refactor: Refactor the code

15b3c81

refactor: Tried fix method with nullable method

4c6ff07

feat: Add tests

7368b77

chore: Update the breaking change doc

cc324eb

Abhijithtv marked this pull request as ready for review May 4, 2026 19:58

latonz self-requested a review May 5, 2026 16:44

latonz requested changes May 5, 2026

View reviewed changes

Abhijithtv marked this pull request as draft May 6, 2026 05:40

		@@ -92,6 +92,18 @@ private bool IsAccessible(ISymbol symbol, MemberVisibility visibility)
		}

		public bool IsNullable(ISymbol symbol)

		public bool IsReadNullable => parameter.Type.IsNullable();
		public bool IsWriteNullable => parameter.Type.IsNullable();

		- `MaybeNull` attributes are now respected for fields, properties and constructor parameters in read context.
		- `AllowNull` attributes are now respected for fields, properties and constructor parameters in write context.

Uh oh!

Conversation

Abhijithtv commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add null checks when mapping nullable sources to non-nullable targets with MayBeNull Attribute

Description

Checklist

Uh oh!

latonz commented May 1, 2026

Uh oh!

Abhijithtv commented May 1, 2026

Uh oh!

latonz commented May 1, 2026

Uh oh!

latonz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Abhijithtv commented May 4, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Abhijithtv commented May 1, 2026 •

edited

Loading