feat: C#12 UnsafeAccessor for private properties and fields

[TimothyMakkison]

C#12 UnsafeAccessor for private properties and fields

Description

WIP for UnsafeMemberAccessor, logic is mostly sound but not cleaned up.

Ramblings/Thoughts

• Used MethodMember successfully 👍
• Generated methods check the target type for conflicting member names.
• Should MemberVisbility.Accessible be removed and merged into Public, I have no idea why someone would want to only use it or need to disable it.
• SetterMemberPath needs the caller to check if IsMethod is true before either assigning to or invoking the result. (target.Set_value(target.Get_value() ?? new()) vs target.Value ??= new()) I did consider creating two functions, or overloads. Not sure what is better. Will sleep on it.
• Need to check/decide how private set; should be handled.
• I suspect it is now possible to assign to init properties after construction. Not sure if this should be explored.
• Struggled to format the generated mapper and accessor classes so I added SeparateByTrailingLineFeed not sure if I missed a helper function.
• Probably need to add using System.CompilerService; somewhere.
• Should the generated static class UnsafeAccessor appear before or after the code. Could the class names/generated extension names be improved?
• SymbolAccessor is initialized before ConfigurationReader so I pass in the MemberVisibility via SetMemberVisibility, this is hacky. I should probably create a AccesibilityContext class (just realied it would have to injected into SymbolAccessor. not sure what to do)
• Not using the .Net 8 branch because I haven't installed the preview yet.
• Haven't added a .Net 8 constraint, don't know if I can get away with using #if NET8_OR_GREATER preprocessors everywhere, or if I should check the compilation.

Update

• Use qualified namespace for attributes
• Prevented initializer from using a broken private setter function

Fixes #600

Checklist

• 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]

Thank you for this PR and sorry for the late review... Looks pretty good already 😊 Is it still in a draft state? I didn't do a complete review yet.

What do you think about #600 (comment), I think such an approach would lead to cleaner and improved readability.

Addressing the questions:

Need to check/decide how private set; should be handled.

Would treat them the same as private fields

I suspect it is now possible to assign to init properties after construction. Not sure if this should be explored.

I'd focus on other things for now and see if there is demand / a use case.

Should the generated static class UnsafeAccessor appear before or after the code?

I'd put it after the code, as these are only accessors and seem to be less relevant.

Could the class names/generated extension names be improved?

Probably upper-case the first char of the name of the target property in the accessor name?

SymbolAccessor is initialized before ConfigurationReader so I pass in the MemberVisibility via SetMemberVisibility, this is hacky. I should probably create a AccesibilityContext class (just realied it would have to injected into SymbolAccessor. not sure what to do)

I'll try some things here and come back to you...

Haven't added a .Net 8 constraint, don't know if I can get away with using #if NET8_OR_GREATER preprocessors everywhere, or if I should check the compilation.

I think it should be safe to check whether the UnsafeAccessorAttribute is available.

[TimothyMakkison]

Thank you for this PR and sorry for the late review... Looks pretty good already 😊 Is it still in a draft state? I didn't do a complete review yet.

It is nearish to completion 😅 I need to add xml comments, docs, review my logic etc. Not a fan of how I've handled MemberPath or SourceEmitter. Looking back, I misunderstood what Accessible was meant to do, hence why my enum is so different, I'll fix this.

Should the generated static class UnsafeAccessor appear before or after the code?

I'd put it after the code, as these are only accessors and seem to be less relevant.

👍

Could the class names/generated extension names be improved?

Probably upper-case the first char of the name of the target property in the accessor name?

How should a leading underscore _ be handled? Debated stripping it or capitalising the next letter.

Haven't added a .Net 8 constraint, don't know if I can get away with using #if NET8_OR_GREATER preprocessors everywhere, or if I should check the compilation.

I think it should be safe to check whether the UnsafeAccessorAttribute is available.

I might create a spearate pr to help this. There are a couple of version specific methods that mapperly will try to use like Enumerable.EnsureCapacity, A class caching this information would preventing redundant, repeat checks, improving performance.

What do you think about #600 (comment), I think such an approach would lead to cleaner and improved readability.

Thanks, it was really helpful. MethodAccessorMember was a massive help and simplified a lot of my logic. ExpressionSyntax Build(ExpressionSyntax source, bool nullConditional = false) does feel a little hacky, but I couldn't think of a nicer way to support getters, setters, method invocation and nullable paths. I deliberately created GetterMemberPath and SetterMemberPath due to the differences in getter/setter method logic and to prevent unwanted UnsafeAccessors from being added to the descriptor.

[latonz]

How should a leading underscore _ be handled? Debated stripping it or capitalising the next letter.

I'd strip it and capitalise the first letter after _.

[codecov]

Codecov Report

Merging #732 (9e07d83) into main (35a6bc7) will increase coverage by 0.07%.
The diff coverage is 91.51%.

@@            Coverage Diff             @@
##             main     #732      +/-   ##
==========================================
+ Coverage   91.38%   91.45%   +0.07%     
==========================================
  Files         202      214      +12     
  Lines        6721     7152     +431     
  Branches      832      868      +36     
==========================================
+ Hits         6142     6541     +399     
- Misses        388      406      +18     
- Partials      191      205      +14     

Files	Coverage Δ
src/Riok.Mapperly.Abstractions/MapperAttribute.cs	100.00% <100.00%> (ø)
...Riok.Mapperly/Configuration/MapperConfiguration.cs	92.85% <100.00%> (+0.54%)	⬆️
...apperly/Configuration/MapperConfigurationMerger.cs	79.48% <100.00%> (+1.10%)	⬆️
src/Riok.Mapperly/Descriptors/DescriptorBuilder.cs	100.00% <100.00%> (ø)
src/Riok.Mapperly/Descriptors/MapperDescriptor.cs	100.00% <100.00%> (ø)
...s/BuilderContext/MembersContainerBuilderContext.cs	100.00% <100.00%> (ø)
...lders/NewInstanceObjectMemberMappingBodyBuilder.cs	81.02% <100.00%> (+0.62%)	⬆️
...ingBodyBuilders/NewValueTupleMappingBodyBuilder.cs	75.00% <100.00%> (+0.18%)	⬆️
.../Descriptors/MappingBuilders/CtorMappingBuilder.cs	73.33% <100.00%> (ø)
...Mappings/MemberMappings/MemberAssignmentMapping.cs	57.57% <100.00%> (-1.25%)	⬇️
... and 32 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[TimothyMakkison]

I haven't read through my code yet but this PR is near completion.

• Added integration test
• Check for .NET 8 if Accessible is disabled - emitting a warning and generating normal code by reenabling it.
• UnsafeAccessor code hopefully won't be emitted for IQueryable but I may have missed something; the accessibility code is pretty messy.

Unfortunately, the preview is still broken for me, so I;ve never been able to run code containing UnsafeAccessor. I have no idea if this even works 😆. I should probably address this with the integration tests

[latonz]

Solid work 👍 Thank you!
I added my comments, feel free to discuss any provided feedback points 😊
Please don't forget the documentation updates.

[TimothyMakkison]

Thanks for the review, must have taken some time 😅
I've made the changes you've suggested, the only missing bits are GetOrBuildAccessor, possibly add a new compile constant and the accessor test without visibility.
Also fixed a pretty glaring issue with existing target types that my tests didn't catch 👍. See f99cfbb, I had to change IMemberAssignmentMapping etc.
I still need to rebase/squash, add a couple of xml comments and do the documentation, but it should be near completion.

Thanks again 😄

[latonz]

Thank you for the updates. Sorry for the slow reviews but this MR is huge 🙈 We should try to split future MR's into smaller ones to be better manageable.

[TimothyMakkison]

That should be the last of that changes 😅 no idea why csproj got into a funk.

Massive thanks @latonz 🙏. This PR is massive, thank you for taking the time to review this. I'll definitely have to break up #622 if I ever return to it.

[latonz]

Looks good to me now 🥳 Thank you!
Can you rebase onto main?
For future PR's we should definitely try to keep them smaller, it makes reviewing and merging easier for everyone involved.

[github-actions]

🎉 This PR is included in version 3.3.0-next.2 🎉

The release is available on:

• GitHub release
• v3.3.0-next.2

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version 3.3.0 🎉

The release is available on:

• GitHub release
• v3.3.0

Your semantic-release bot 📦🚀