Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AddNullMemberInitializers should check if a member is assigned before instantiating with ??= new() #640

Closed
TimothyMakkison opened this issue Aug 9, 2023 · 3 comments · Fixed by #1408
Labels
bug Something isn't working

Comments

@TimothyMakkison
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
When mapping paths, MapProperty initializes parents of members using ??= new() to prevent exceptions. This doesn't check if the member has already been set resulting in redundant code.

[MapProperty("NestedValue", "Nested.Value")]
[MapProperty("Parent", "Nested")] 
partial B Map(A source);

class A { public string NestedValue { get; init; } public C Parent { get; set; } }
class B { public C? Nested { get; set; } }
class C { public string Value { get; set; } }

// Generates
var target = new global::B();
target.Nested ??= new(); // redundant
target.Nested = source.Parent;
target.Nested.Value = source.NestedValue;
return target;

// Should generate
var target = new global::B();
target.Nested = source.Parent;
target.Nested.Value = source.NestedValue;
return target;

Describe the solution you'd like
AddNullMemberInitializers should check if the member is already set before creating a MemberNullAssignmentInitializerMapping.

@latonz latonz added the bug Something isn't working label Aug 9, 2023
@TimothyMakkison
Copy link
Collaborator Author

TimothyMakkison commented Aug 9, 2023

Just realised that the nullable initializer is required. Even if the source is known to not be null the targets setter could make the property null (very unlikely).

Instead the nullable initializer should be added after the assignment as it's currently redundant in these cases.

// Generates
var target = new global::B();
target.Nested = source.Parent; // nested setter could do something weird (rare)
target.Nested ??= new(); // should be after
target.Nested.Value = source.NestedValue;
return target;

I don't know if it's worth accounting for bizarre setters. WDYT

@latonz
Copy link
Contributor

latonz commented Aug 9, 2023

Is it really required? Isn't it only required when source.Parent is nullable? Otherwise the same problem would exist with target.Nested ??= new(); (a setter could assign null here too), but I wouldn't account such weird setters.

@TimothyMakkison
Copy link
Collaborator Author

Good point, no idea why I didn't think of that 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants