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.

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

Improved nullable support #30

Merged
merged 12 commits into from
May 14, 2024
Merged

Improved nullable support #30

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a4d737c
added unit tests for nullable disabled
jkurdek May 8, 2024
88dfdcf
added integration tests for nullable disabled
jkurdek May 8, 2024
c5bc172
add nullable disabled support for reference types
jkurdek May 10, 2024
0d06066
wip: added support for nullable disabled on value types
jkurdek May 10, 2024
1116173
improved integration tests
jkurdek May 10, 2024
cd1f6fc
initialize support for indexers
jkurdek May 10, 2024
e85fede
add indexers support to SetterBuilder
jkurdek May 11, 2024
8c856ec
added additional tests for setter builder
jkurdek May 11, 2024
86521ee
cleaned up SetterBuilder
jkurdek May 11, 2024
5cfce1f
Fixed nullable access with BindingTransformer
jkurdek May 14, 2024
02b537a
Removed IsNullableValueType property
jkurdek May 14, 2024
2c80122
Cleaned up nullability
jkurdek May 14, 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
11 changes: 6 additions & 5 deletions src/Controls/src/BindingSourceGen/BindingCodeWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ public void AppendSetBindingInterceptor(int id, SetBindingInvocationDescription
Indent();

Append("handlers: ");
AppendHandlersArray(binding.SourceType, binding.Path);
AppendHandlersArray(binding.SourceType, binding.Path, binding.ConsiderAllReferenceTypesPotentiallyNullable);
AppendLine(")");

Unindent();
Expand Down Expand Up @@ -234,7 +234,7 @@ private void AppendSetterAction(SetBindingInvocationDescription binding, string
AppendLine('}');
}

var setter = Setter.From(binding.SourceType, binding.Path, sourceVariableName, assignedValueExpression);
var setter = Setter.From(binding.SourceType, binding.Path, binding.ConsiderAllReferenceTypesPotentiallyNullable, sourceVariableName, assignedValueExpression);
if (setter.PatternMatchingExpressions.Length > 0)
{
Append("if (");
Expand Down Expand Up @@ -274,20 +274,21 @@ private void AppendSetterAction(SetBindingInvocationDescription binding, string
}
}

private void AppendHandlersArray(TypeDescription sourceType, EquatableArray<IPathPart> path)
private void AppendHandlersArray(TypeDescription sourceType, EquatableArray<IPathPart> path, bool considerAllReferenceTypesPotentiallyNullable)
{
AppendLine($"new Tuple<Func<{sourceType}, object?>, string>[]");
AppendLine('{');

Indent();

string nextExpression = "source";
bool forceConditonalAccessToNextPart = false;
bool forceConditonalAccessToNextPart = !sourceType.IsValueType && considerAllReferenceTypesPotentiallyNullable;
foreach (var part in path)
{
var previousExpression = nextExpression;
nextExpression = AccessExpressionBuilder.Build(previousExpression, MaybeWrapInConditionalAccess(part, forceConditonalAccessToNextPart));
forceConditonalAccessToNextPart = part is Cast;
var isNullableReferenceType = part is MemberAccess memberAccess && !memberAccess.IsValueType;
Copy link
Owner

Choose a reason for hiding this comment

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

We need a check for IndexAccess here as well + a unit test that covers that case.

forceConditonalAccessToNextPart = part is Cast || considerAllReferenceTypesPotentiallyNullable && isNullableReferenceType;

// Some parts don't have a property name, so we can't generate a handler for them (for example casts)
if (part.PropertyName is string propertyName)
Expand Down
3 changes: 2 additions & 1 deletion src/Controls/src/BindingSourceGen/BindingSourceGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ static BindingDiagnosticsWrapper GetBindingForGeneration(GeneratorSyntaxContext
SourceType: BindingGenerationUtilities.CreateTypeNameFromITypeSymbol(lambdaSymbol.Parameters[0].Type, enabledNullable),
PropertyType: BindingGenerationUtilities.CreateTypeNameFromITypeSymbol(lambdaTypeInfo.Type, enabledNullable),
Path: new EquatableArray<IPathPart>([.. parts]),
SetterOptions: DeriveSetterOptions(lambdaBody, context.SemanticModel, enabledNullable));
SetterOptions: DeriveSetterOptions(lambdaBody, context.SemanticModel, enabledNullable),
ConsiderAllReferenceTypesPotentiallyNullable: !enabledNullable);
return new BindingDiagnosticsWrapper(binding, new EquatableArray<DiagnosticInfo>([.. diagnostics]));
}

Expand Down
10 changes: 7 additions & 3 deletions src/Controls/src/BindingSourceGen/BindingSourceGeneratorUtilities.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@ namespace Microsoft.Maui.Controls.BindingSourceGen;

internal static class BindingGenerationUtilities
{

internal static bool IsNullableValueType(ITypeSymbol typeInfo) =>
typeInfo is INamedTypeSymbol namedTypeSymbol
&& namedTypeSymbol.IsGenericType
&& namedTypeSymbol.ConstructedFrom.SpecialType == SpecialType.System_Nullable_T;

internal static bool IsTypeNullable(ITypeSymbol typeInfo, bool enabledNullable)
{
if (!enabledNullable && typeInfo.IsReferenceType)
Expand All @@ -16,9 +22,7 @@ internal static bool IsTypeNullable(ITypeSymbol typeInfo, bool enabledNullable)
return true;
}

return typeInfo is INamedTypeSymbol namedTypeSymbol
&& namedTypeSymbol.IsGenericType
&& namedTypeSymbol.ConstructedFrom.SpecialType == SpecialType.System_Nullable_T;
return IsNullableValueType(typeInfo);
}

internal static TypeDescription CreateTypeNameFromITypeSymbol(ITypeSymbol typeSymbol, bool enabledNullable)
Expand Down
20 changes: 14 additions & 6 deletions src/Controls/src/BindingSourceGen/GeneratorDataModels.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ public sealed record SetBindingInvocationDescription(
TypeDescription SourceType,
TypeDescription PropertyType,
EquatableArray<IPathPart> Path,
SetterOptions SetterOptions);
SetterOptions SetterOptions,
bool ConsiderAllReferenceTypesPotentiallyNullable);

public sealed record SourceCodeLocation(string FilePath, TextSpan TextSpan, LinePositionSpan LineSpan)
{
Expand Down Expand Up @@ -54,23 +55,30 @@ public override string ToString()

public sealed record SetterOptions(bool IsWritable, bool AcceptsNullValue = false);

public sealed record MemberAccess(string MemberName) : IPathPart
public sealed record MemberAccess(string MemberName, bool IsValueType = false, bool IsNullableValueType = false) : IPathPart
{
public string? PropertyName => MemberName;
public string PropertyName => MemberName;

public bool Equals(IPathPart other)
{
return other is MemberAccess memberAccess && MemberName == memberAccess.MemberName;
return other is MemberAccess memberAccess
&& MemberName == memberAccess.MemberName
&& IsValueType == memberAccess.IsValueType
&& IsNullableValueType == memberAccess.IsNullableValueType;
}
}

public sealed record IndexAccess(string DefaultMemberName, object Index) : IPathPart
public sealed record IndexAccess(string DefaultMemberName, object Index, bool IsValueType = false, bool IsNullableValueType = false) : IPathPart
{
public string? PropertyName => $"{DefaultMemberName}[{Index}]";

public bool Equals(IPathPart other)
{
return other is IndexAccess indexAccess && DefaultMemberName == indexAccess.DefaultMemberName && Index.Equals(indexAccess.Index);
return other is IndexAccess indexAccess
&& DefaultMemberName == indexAccess.DefaultMemberName
&& Index.Equals(indexAccess.Index)
&& IsValueType == indexAccess.IsValueType
&& IsNullableValueType == indexAccess.IsNullableValueType;
}
}

Expand Down
25 changes: 18 additions & 7 deletions src/Controls/src/BindingSourceGen/PathParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,10 @@ BinaryExpressionSyntax asExpression when asExpression.Kind() == SyntaxKind.AsExp
}

var member = memberAccess.Name.Identifier.Text;
IPathPart part = new MemberAccess(member);
var typeInfo = Context.SemanticModel.GetTypeInfo(memberAccess).Type;
var isReferenceType = typeInfo?.IsReferenceType ?? false;
var isNullableValueType = typeInfo != null ? BindingGenerationUtilities.IsNullableValueType(typeInfo) : false;
IPathPart part = new MemberAccess(member, !isReferenceType, isNullableValueType);
parts.Add(part);
return (diagnostics, parts);
}
Expand All @@ -55,12 +58,13 @@ BinaryExpressionSyntax asExpression when asExpression.Kind() == SyntaxKind.AsExp
}

var elementAccessSymbol = Context.SemanticModel.GetSymbolInfo(elementAccess).Symbol;
var (elementAccessDiagnostics, elementAccessParts) = HandleElementAccessSymbol(elementAccessSymbol, elementAccess.ArgumentList.Arguments, elementAccess.GetLocation());
var elementType = Context.SemanticModel.GetTypeInfo(elementAccess).Type;

var (elementAccessDiagnostics, elementAccessParts) = CreateIndexAccess(elementAccessSymbol, elementType, elementAccess.ArgumentList.Arguments, elementAccess.GetLocation());
if (elementAccessDiagnostics.Length > 0)
{
return (elementAccessDiagnostics, elementAccessParts);
}

parts.AddRange(elementAccessParts);
return (diagnostics, parts);
}
Expand All @@ -86,7 +90,10 @@ BinaryExpressionSyntax asExpression when asExpression.Kind() == SyntaxKind.AsExp
private (EquatableArray<DiagnosticInfo> diagnostics, List<IPathPart> parts) HandleMemberBindingExpression(MemberBindingExpressionSyntax memberBinding)
{
var member = memberBinding.Name.Identifier.Text;
IPathPart part = new MemberAccess(member);
var typeInfo = Context.SemanticModel.GetTypeInfo(memberBinding).Type;
var isReferenceType = typeInfo?.IsReferenceType ?? false;
var isNullableValueType = typeInfo != null ? BindingGenerationUtilities.IsNullableValueType(typeInfo) : false;
IPathPart part = new MemberAccess(member, !isReferenceType, isNullableValueType);
part = new ConditionalAccess(part);

return ([], new List<IPathPart>([part]));
Expand All @@ -95,7 +102,9 @@ BinaryExpressionSyntax asExpression when asExpression.Kind() == SyntaxKind.AsExp
private (EquatableArray<DiagnosticInfo> diagnostics, List<IPathPart> parts) HandleElementBindingExpression(ElementBindingExpressionSyntax elementBinding)
{
var elementAccessSymbol = Context.SemanticModel.GetSymbolInfo(elementBinding).Symbol;
var (elementAccessDiagnostics, elementAccessParts) = HandleElementAccessSymbol(elementAccessSymbol, elementBinding.ArgumentList.Arguments, elementBinding.GetLocation());
var elementType = Context.SemanticModel.GetTypeInfo(elementBinding).Type;

var (elementAccessDiagnostics, elementAccessParts) = CreateIndexAccess(elementAccessSymbol, elementType, elementBinding.ArgumentList.Arguments, elementBinding.GetLocation());
if (elementAccessDiagnostics.Length > 0)
{
return (elementAccessDiagnostics, elementAccessParts);
Expand Down Expand Up @@ -147,7 +156,7 @@ BinaryExpressionSyntax asExpression when asExpression.Kind() == SyntaxKind.AsExp
return (new EquatableArray<DiagnosticInfo>([DiagnosticsFactory.UnableToResolvePath(Context.Node.GetLocation())]), new List<IPathPart>());
}

private (EquatableArray<DiagnosticInfo>, List<IPathPart>) HandleElementAccessSymbol(ISymbol? elementAccessSymbol, SeparatedSyntaxList<ArgumentSyntax> argumentList, Location location)
private (EquatableArray<DiagnosticInfo>, List<IPathPart>) CreateIndexAccess(ISymbol? elementAccessSymbol, ITypeSymbol? typeSymbol, SeparatedSyntaxList<ArgumentSyntax> argumentList, Location location)
{
if (argumentList.Count != 1)
{
Expand All @@ -162,7 +171,9 @@ BinaryExpressionSyntax asExpression when asExpression.Kind() == SyntaxKind.AsExp
}

var name = GetIndexerName(elementAccessSymbol);
IPathPart part = new IndexAccess(name, indexValue);
var isReferenceType = typeSymbol?.IsReferenceType ?? false;
var isNullableValueType = typeSymbol != null ? BindingGenerationUtilities.IsNullableValueType(typeSymbol) : false;
Copy link
Owner

Choose a reason for hiding this comment

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

There's quite a lot of code duplication going on here and it's getting hard to read. We should consider moving this to a separate method at least.

IPathPart part = new IndexAccess(name, indexValue, !isReferenceType, isNullableValueType);

return ([], [part]);
}
Expand Down
47 changes: 32 additions & 15 deletions src/Controls/src/BindingSourceGen/SetterBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,14 @@ public sealed record Setter(string[] PatternMatchingExpressions, string Assignme
public static Setter From(
TypeDescription sourceTypeDescription,
EquatableArray<IPathPart> path,
bool considerAllReferenceTypesPotentiallyNullable = false,
string sourceVariableName = "source",
string assignedValueExpression = "value")
{
var builder = new SetterBuilder(sourceVariableName, assignedValueExpression);
var builder = new SetterBuilder(considerAllReferenceTypesPotentiallyNullable, sourceVariableName, sourceTypeDescription, assignedValueExpression);

if (path.Length > 0)
{
if (sourceTypeDescription.IsNullable)
{
builder.AddIsExpression("{}");
}

foreach (var part in path)
{
Expand All @@ -28,32 +25,47 @@ public static Setter From(

private sealed class SetterBuilder
{
private readonly string _sourceVariableName;
private readonly bool _considerAllReferenceTypesPotentiallyNullable;
private readonly string _assignedValueExpression;

private string _expression;
private int _variableCounter = 0;
private List<string>? _patternMatching;

private IPathPart? _currentPart;
private IPathPart? _previousPart;
private IPathPart _sourcePart;

public SetterBuilder(string sourceVariableName, string assignedValueExpression)
public SetterBuilder(bool considerAllReferenceTypesPotentiallyNullable, string sourceVariableName, TypeDescription sourceTypeDescription, string assignedValueExpression)
{
_sourceVariableName = sourceVariableName;
_considerAllReferenceTypesPotentiallyNullable = considerAllReferenceTypesPotentiallyNullable;
_assignedValueExpression = assignedValueExpression;

_sourcePart = new MemberAccess(sourceVariableName, sourceTypeDescription.IsValueType, sourceTypeDescription.IsValueType && sourceTypeDescription.IsNullable);
_currentPart = _sourcePart;

_expression = sourceVariableName;
}

public void AddPart(IPathPart nextPart)
{
_previousPart = HandlePreviousPart(nextPart);
var newPart = HandleCurrentPart(nextPart);
_previousPart = _currentPart;
_currentPart = newPart;

}
Copy link
Owner

Choose a reason for hiding this comment

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

Thinking out loud: this needs refactoring, the nextPart, newPart, _currentPart, _previousPart naming is confusing.


private IPathPart? HandlePreviousPart(IPathPart? nextPart)
private IPathPart? HandleCurrentPart(IPathPart? nextPart)
{
if (_previousPart is {} previousPart)

var previousReferenceType = _previousPart is MemberAccess previousPartAccess
&& !previousPartAccess.IsValueType || _previousPart is IndexAccess previousPartIndexAccess
&& !previousPartIndexAccess.IsValueType || _previousPart is ConditionalAccess;


if (_currentPart is { } currentPart && currentPart != _sourcePart)
{
if (previousPart is Cast { TargetType: var targetType })
if (currentPart is Cast { TargetType: var targetType })
{
AddIsExpression(targetType.GlobalName);

Expand All @@ -63,14 +75,19 @@ public void AddPart(IPathPart nextPart)
return innerPart;
}
}
else if (previousPart is ConditionalAccess { Part: var innerPart })
else if (currentPart is ConditionalAccess { Part: var innerPart })
{
AddIsExpression("{}");
_expression = AccessExpressionBuilder.Build(_expression, innerPart);
}
else if (previousReferenceType && _considerAllReferenceTypesPotentiallyNullable)
{
AddIsExpression("{}");
_expression = AccessExpressionBuilder.Build(_expression, currentPart);
}
else
{
_expression = AccessExpressionBuilder.Build(_expression, previousPart);
_expression = AccessExpressionBuilder.Build(_expression, currentPart);
}
}

Expand All @@ -94,7 +111,7 @@ private string CreateNextUniqueVariableName()

private string CreateAssignmentStatement()
{
HandlePreviousPart(nextPart: null);
HandleCurrentPart(nextPart: null);
return $"{_expression} = {_assignedValueExpression};";
}

Expand Down
18 changes: 12 additions & 6 deletions src/Controls/tests/BindingSourceGen.UnitTests/BindingCodeWriterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ public void BuildsWholeDocument()
new ConditionalAccess(new MemberAccess("B")),
new ConditionalAccess(new MemberAccess("C")),
]),
SetterOptions: new(IsWritable: true, AcceptsNullValue: false)));
SetterOptions: new(IsWritable: true, AcceptsNullValue: false),
ConsiderAllReferenceTypesPotentiallyNullable: false));

var code = codeWriter.GenerateCode();
AssertExtensions.CodeIsEqual(
Expand Down Expand Up @@ -140,7 +141,8 @@ public void CorrectlyFormatsSimpleBinding()
new ConditionalAccess(new MemberAccess("B")),
new ConditionalAccess(new MemberAccess("C")),
]),
SetterOptions: new(IsWritable: true, AcceptsNullValue: false)));
SetterOptions: new(IsWritable: true, AcceptsNullValue: false),
ConsiderAllReferenceTypesPotentiallyNullable: false));

var code = codeBuilder.ToString();
AssertExtensions.CodeIsEqual(
Expand Down Expand Up @@ -210,7 +212,8 @@ public void CorrectlyFormatsBindingWithoutAnyNullablesInPath()
new MemberAccess("B"),
new MemberAccess("C"),
]),
SetterOptions: new(IsWritable: true, AcceptsNullValue: false)));
SetterOptions: new(IsWritable: true, AcceptsNullValue: false),
ConsiderAllReferenceTypesPotentiallyNullable: false));

var code = codeBuilder.ToString();
AssertExtensions.CodeIsEqual(
Expand Down Expand Up @@ -276,7 +279,8 @@ public void CorrectlyFormatsBindingWithoutSetter()
new MemberAccess("B"),
new MemberAccess("C"),
]),
SetterOptions: new(IsWritable: false)));
SetterOptions: new(IsWritable: false),
ConsiderAllReferenceTypesPotentiallyNullable: false));

var code = codeBuilder.ToString();
AssertExtensions.CodeIsEqual(
Expand Down Expand Up @@ -339,7 +343,8 @@ public void CorrectlyFormatsBindingWithIndexers()
new ConditionalAccess(new IndexAccess("Indexer", "Abc")),
new IndexAccess("Item", 0),
]),
SetterOptions: new(IsWritable: true, AcceptsNullValue: false)));
SetterOptions: new(IsWritable: true, AcceptsNullValue: false),
ConsiderAllReferenceTypesPotentiallyNullable: false));

var code = codeBuilder.ToString();
AssertExtensions.CodeIsEqual(
Expand Down Expand Up @@ -417,7 +422,8 @@ public void CorrectlyFormatsBindingWithCasts()
new Cast(new TypeDescription("Z", IsValueType: true, IsNullable: true, IsGenericParameter: false)),
new ConditionalAccess(new MemberAccess("D")),
]),
SetterOptions: new(IsWritable: true, AcceptsNullValue: false)));
SetterOptions: new(IsWritable: true, AcceptsNullValue: false),
ConsiderAllReferenceTypesPotentiallyNullable: false));

var code = codeBuilder.ToString();

Expand Down
Loading
Loading