Fix Issue #414 - NullableExpressionVisitor improvement

[DocSvartz]

Fix Issue #414

[DocSvartz]

@andrerav @stagep
If we focus on annotations.
Then the correct solution would be to use another mechanism,but then it won't solve this problem #722.

Because the expected behavior in Nullable aware context was actually working by mistake (In the example, the error should occur in both cases.).

Which path should we follow:

1. Use nullable annotation.
2. Or treat all reference types as nullable types.

Example :

#nullable enable

        [TestMethod]
        public void MappinginNullContext()
        {
            var user = new User();
            var userDto = user.Adapt<UserDto>();
        }

        #region TestClasses

        public class User
        {
            public string? Id { get; set; }
            //public string? Name { get; set; } //  when uncommented - mapping  Property People  throw Exception because it has been null

            public People People { get; set; }
        }


        public class People
        {
            public string? Id { get; set; }
            public string? Name { get; set; }
        }

        public class UserDto
        {
            public string? Id { get; set; }
            public string? Name { get; set; }

            public PeopleDto People { get; set; }
        }

        public class PeopleDto
        {
            public string? Id { get; set; }
            public string? Name { get; set; }
        }

[stagep]

@DocSvartz @andrerav
Does the exception occur because the mapping attempts to map the properties of PeopleDto which is null? My expectation is that mapping of a reference type that is null stops at the null type and does not access the properties of the null type avoiding a NullReferenceException. Is your question asking if this behavior should be opt-in through an attribute on the potentially nullable type or should it be automatic?

Looking at my own code generated mappings, it always checks the reference type is not null before it proceeds to map properties of the reference type. Is this the behavior that you are referring to that was implemented "by mistake"?

[DocSvartz]

Does the exception occur because the mapping attempts to map the properties of PeopleDto which is null?
What matters here is whether the Source Properties can be null.

code which is currently being created from first case:

    .If ($var1 == null) {
        null
    } .Else {
        .New MapsterTest3.Test1+UserDto(){
            Id = $var1.Id,
            People = .If ($var1.People == null) { // Property People not Mark NullableAttribute and now Mapster considers this property maybe null
                null
            } .Else {
                .New MapsterTest3.Test1+PeopleDto(){
                    Id = ($var1.People).Id,
                    Name = ($var1.People).Name
                }

code which is currently being created from second case:

  .If ($var1 == null) {
        null
    } .Else {
        .New MapsterTest3.Test1+UserDto(){
            Id = $var1.Id,
            Name = $var1.Name,
            People = .New MapsterTest3.Test1+PeopleDto(){  // Property People automatic Mark NullableAttribute[1] == not null
                Id = ($var1.People).Id,
                Name = ($var1.People).Name
            }

Is your question asking if this behavior should be opt-in through an attribute on the potentially nullable type or should it be automatic?

The problem here is that both the first and second options when describing a type in a nullable aware context (#nullable enable) have never been null.

A variable of a reference type T must be initialized with non-null, and can never be assigned a value that might be null.

And NullabilityInfoContext will indeed define it as not null in both cases. It can also take into account other nullable attributes.

Looking at my own code generated mappings, it always checks the reference type is not null before it proceeds to map properties of the reference type. Is this the behavior that you are referring to that was implemented "by mistake"?

If you mean Mapster Tool. Then source generators by default create code of #nullable disable context.
and there this behavior is correct. (There may be something else going on there, but these examples mapsterTool also creates with and without check null 🤔)

Viewing the code of what happens during normal mapping is better done like this user.BuildAdapter().CreateMapExpression<UserDto>()

[DocSvartz]

@stagep @andrerav Mapster for the most part, it does not analyze the runtime state of variables. It creates mapping functions based on the Source and Destination types.
From this point of view, if we are provided with types whose members are explicitly specified as non-nullable, then it is quite expected that the processing function will be created for types that do not allow null values.
(In this case, the presence of null is already an error on the calling side - a violation of the established contract.)

I am not against the existence of behavior "by design", That's why I'm asking what we will stick to.

[stagep]

@DocSvartz Have been busy so not got to look at this. What version or branch of Mapster are you using in the above example? I see you have forked this repository so do I need to use a branch of your repository?

[DocSvartz]

@stagep everything I described happens on any version.
If you need a fixed version, you can use this PR. It was created on the current Development branch
Or do you need a fix based on NullabilityInfoContext ?

Current tests do not use nullable context. To notice this, you need to use #nullable enable in the file with the description of test classes.