Skip to content

fix: Add null checks when mapping nullable sources to non-nullable targets with MayBeNull and AllowNull Attribute#2256

Merged
latonz merged 15 commits into
riok:mainfrom
Abhijithtv:fix/target-ignore-maybenull
May 11, 2026
Merged

fix: Add null checks when mapping nullable sources to non-nullable targets with MayBeNull and AllowNull Attribute#2256
latonz merged 15 commits into
riok:mainfrom
Abhijithtv:fix/target-ignore-maybenull

Conversation

@Abhijithtv
Copy link
Copy Markdown
Contributor

@Abhijithtv Abhijithtv commented May 1, 2026

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)

@Abhijithtv 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
@latonz
Copy link
Copy Markdown
Contributor

latonz commented May 1, 2026

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Contributor

latonz commented May 1, 2026

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.

Copy link
Copy Markdown
Contributor

@latonz latonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, I added my feedback.

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

Comment thread test/Riok.Mapperly.Tests/Mapping/ObjectPropertyValueMethodTest.cs Outdated
Comment thread test/Riok.Mapperly.Tests/Mapping/ObjectPropertyValueMethodTest.cs Outdated
Comment thread test/Riok.Mapperly.Tests/Mapping/ObjectPropertyValueMethodTest.cs Outdated
Comment thread src/Riok.Mapperly/Descriptors/SymbolAccessor.cs Outdated
Comment thread src/Riok.Mapperly/Descriptors/SymbolAccessor.cs
Comment thread src/Riok.Mapperly/Symbols/Members/ParameterSourceMember.cs Outdated
@latonz latonz added bug Something isn't working enhancement New feature or request labels May 2, 2026
@Abhijithtv Abhijithtv marked this pull request as draft May 3, 2026 09:16
Comment thread src/Riok.Mapperly/Symbols/Members/ParameterSourceMember.cs
@Abhijithtv Abhijithtv marked this pull request as ready for review May 4, 2026 19:58
@Abhijithtv
Copy link
Copy Markdown
Contributor Author

@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 latonz self-requested a review May 5, 2026 16:44
Comment thread docs/docs/breaking-changes/5-0.md Outdated
Comment thread src/Riok.Mapperly/Descriptors/MappingBodyBuilders/SourceValueBuilder.cs Outdated
Comment thread src/Riok.Mapperly/Descriptors/MappingBodyBuilders/SourceValueBuilder.cs Outdated
Comment thread src/Riok.Mapperly/Symbols/MethodParameter.cs Outdated
Comment thread src/Riok.Mapperly/Symbols/Members/ParameterSourceMember.cs
Comment thread src/Riok.Mapperly/Descriptors/MappingBodyBuilders/SourceValueBuilder.cs Outdated
Comment thread src/Riok.Mapperly/Symbols/Members/MemberPath.cs Outdated
@Abhijithtv Abhijithtv marked this pull request as draft May 6, 2026 05:40
@Abhijithtv Abhijithtv marked this pull request as ready for review May 6, 2026 17:58
@Abhijithtv Abhijithtv requested a review from latonz May 7, 2026 16:08
@Abhijithtv
Copy link
Copy Markdown
Contributor Author

@latonz
I have updated the PR based on the feedback.

Copy link
Copy Markdown
Contributor

@latonz latonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. One small feedback point left.
Could you please also rebase? Afterwards I'll merge it 😊

Comment thread src/Riok.Mapperly/Symbols/Members/NonEmptyMemberPath.cs Outdated
@Abhijithtv Abhijithtv force-pushed the fix/target-ignore-maybenull branch from 737c17d to c37630f Compare May 11, 2026 18:23
@Abhijithtv
Copy link
Copy Markdown
Contributor Author

Thanks for the updates. One small feedback point left. Could you please also rebase? Afterwards I'll merge it 😊

I have updated the PR and rebased the branch.

@Abhijithtv Abhijithtv requested a review from latonz May 11, 2026 18:29
@latonz latonz changed the title fix: Add null checks when mapping nullable sources to non-nullable targets with MayBeNull Attribute fix: Add null checks when mapping nullable sources to non-nullable targets with MayBeNull and AllowNull Attribute May 11, 2026
@latonz latonz merged commit 80b97d9 into riok:main May 11, 2026
17 checks passed
@latonz
Copy link
Copy Markdown
Contributor

latonz commented May 11, 2026

@Abhijithtv thank you for this contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CS8601 while mapping properties annotated with MaybeNullAttribute

2 participants