Skip to content

Improve nullable annotations in [ObservableProperty] generated code #944

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Sep 9, 2024
Merged

Improve nullable annotations in [ObservableProperty] generated code #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9abe628
Mark old property value as nullable if needed
Sergio0694 Sep 9, 2024
8e23ac1
Add unit tests for new nullable annotations
Sergio0694 Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 26 additions & 15 deletions ...munityToolkit.Mvvm.SourceGenerators/ComponentModel/ObservablePropertyGenerator.Execute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -834,9 +834,6 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf
{
using ImmutableArrayBuilder<StatementSyntax> setterStatements = ImmutableArrayBuilder<StatementSyntax>.Rent();

// Get the property type syntax
TypeSyntax propertyType = IdentifierName(propertyInfo.TypeNameWithNullabilityAnnotations);

string getterFieldIdentifierName;
ExpressionSyntax getterFieldExpression;
ExpressionSyntax setterFieldExpression;
Expand Down Expand Up @@ -872,7 +869,7 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf
// <PROPERTY_TYPE> __oldValue = <FIELD_EXPRESSIONS>;
setterStatements.Add(
LocalDeclarationStatement(
VariableDeclaration(propertyType)
VariableDeclaration(GetMaybeNullPropertyType(propertyInfo))
.AddVariables(
VariableDeclarator(Identifier("__oldValue"))
.WithInitializer(EqualsValueClause(setterFieldExpression)))));
Expand Down Expand Up @@ -1001,6 +998,9 @@ public static MemberDeclarationSyntax GetPropertySyntax(PropertyInfo propertyInf
Argument(LiteralExpression(SyntaxKind.StringLiteralExpression, Literal(propertyInfo.PropertyName))))));
}

// Get the property type syntax
TypeSyntax propertyType = IdentifierName(propertyInfo.TypeNameWithNullabilityAnnotations);

// Generate the inner setter block as follows:
//
// if (!global::System.Collections.Generic.EqualityComparer<<PROPERTY_TYPE>>.Default.Equals(<FIELD_EXPRESSION>, value))
Expand Down Expand Up @@ -1128,17 +1128,8 @@ public static ImmutableArray<MemberDeclarationSyntax> GetOnPropertyChangeMethods
Comment($"/// <remarks>This method is invoked right before the value of <see cref=\"{propertyInfo.PropertyName}\"/> is changed.</remarks>")), SyntaxKind.OpenBracketToken, TriviaList())))
.WithSemicolonToken(Token(SyntaxKind.SemicolonToken));

// Prepare the nullable type for the previous property value. This is needed because if the type is a reference
// type, the previous value might be null even if the property type is not nullable, as the first invocation would
// happen when the property is first set to some value that is not null (but the backing field would still be so).
// As a cheap way to check whether we need to add nullable, we can simply check whether the type name with nullability
// annotations ends with a '?'. If it doesn't and the type is a reference type, we add it. Otherwise, we keep it.
TypeSyntax oldValueTypeSyntax = propertyInfo.IsReferenceTypeOrUnconstraindTypeParameter switch
{
true when !propertyInfo.TypeNameWithNullabilityAnnotations.EndsWith("?")
=> IdentifierName($"{propertyInfo.TypeNameWithNullabilityAnnotations}?"),
_ => parameterType
};
// Get the type for the 'oldValue' parameter (which can be null on first invocation)
TypeSyntax oldValueTypeSyntax = GetMaybeNullPropertyType(propertyInfo);

// Construct the generated method as follows:
//
Expand Down Expand Up @@ -1224,6 +1215,26 @@ public static ImmutableArray<MemberDeclarationSyntax> GetOnPropertyChangeMethods
onPropertyChanged2Declaration);
}

/// <summary>
/// Gets the <see cref="TypeSyntax"/> for the type of a given property, when it can possibly be <see langword="null"/>.
/// </summary>
/// <param name="propertyInfo">The input <see cref="PropertyInfo"/> instance to process.</param>
/// <returns>The type of a given property, when it can possibly be <see langword="null"/></returns>
private static TypeSyntax GetMaybeNullPropertyType(PropertyInfo propertyInfo)
{
// Prepare the nullable type for the previous property value. This is needed because if the type is a reference
// type, the previous value might be null even if the property type is not nullable, as the first invocation would
// happen when the property is first set to some value that is not null (but the backing field would still be so).
// As a cheap way to check whether we need to add nullable, we can simply check whether the type name with nullability
// annotations ends with a '?'. If it doesn't and the type is a reference type, we add it. Otherwise, we keep it.
return propertyInfo.IsReferenceTypeOrUnconstraindTypeParameter switch
{
true when !propertyInfo.TypeNameWithNullabilityAnnotations.EndsWith("?")
=> IdentifierName($"{propertyInfo.TypeNameWithNullabilityAnnotations}?"),
_ => IdentifierName(propertyInfo.TypeNameWithNullabilityAnnotations)
};
}

/// <summary>
/// Gets a <see cref="CompilationUnitSyntax"/> instance with the cached args of a specified type.
/// </summary>
Expand Down
Loading