Skip to content

Commit aeaacfb

Browse files
koenbeukkoenbeuk
authored
Merge pull request #49 from EFNext/fix/expressive-properties
Fix bugs related to nullable types and generic parameters
2 parents 98d1657 + add27f1 commit aeaacfb

5 files changed

Lines changed: 258 additions & 3 deletions

‎src/ExpressiveSharp.Generator/Emitter/ReflectionFieldCache.cs‎

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,23 @@ public string EnsureMethodInfo(IMethodSymbol method)
6262
paramChecksBuilder.Append($" && m.GetParameters()[{i}].ParameterType.IsGenericType && !m.GetParameters()[{i}].ParameterType.IsGenericParameter");
6363
for (int t = 0; t < genericParam.TypeArguments.Length; t++)
6464
{
65-
if (genericParam.TypeArguments[t] is not ITypeParameterSymbol)
66-
paramChecksBuilder.Append($" && m.GetParameters()[{i}].ParameterType.GetGenericArguments()[{t}] == typeof({ResolveTypeFqn(genericParam.TypeArguments[t])})");
65+
var innerArg = genericParam.TypeArguments[t];
66+
if (innerArg is ITypeParameterSymbol)
67+
continue;
68+
69+
if (ContainsTypeParameter(innerArg))
70+
{
71+
// Pin the open-generic shape; closed FQN would leak the method type parameter.
72+
if (innerArg is INamedTypeSymbol { IsGenericType: true } innerGeneric)
73+
{
74+
var openFqn = innerGeneric.ConstructUnboundGenericType().ToDisplayString(_fullyQualifiedFormat);
75+
paramChecksBuilder.Append($" && m.GetParameters()[{i}].ParameterType.GetGenericArguments()[{t}].IsGenericType && m.GetParameters()[{i}].ParameterType.GetGenericArguments()[{t}].GetGenericTypeDefinition() == typeof({openFqn})");
76+
}
77+
}
78+
else
79+
{
80+
paramChecksBuilder.Append($" && m.GetParameters()[{i}].ParameterType.GetGenericArguments()[{t}] == typeof({ResolveTypeFqn(innerArg)})");
81+
}
6782
}
6883
}
6984
}
@@ -86,4 +101,21 @@ public string EnsureConstructorInfo(IMethodSymbol constructor)
86101
$"typeof({ResolveTypeFqn(p.Type)})"));
87102
return $"typeof({typeFqn}).GetConstructor(new global::System.Type[] {{ {paramTypes} }})";
88103
}
104+
105+
private static bool ContainsTypeParameter(ITypeSymbol type)
106+
{
107+
if (type is ITypeParameterSymbol)
108+
return true;
109+
if (type is INamedTypeSymbol named)
110+
{
111+
foreach (var arg in named.TypeArguments)
112+
{
113+
if (ContainsTypeParameter(arg))
114+
return true;
115+
}
116+
}
117+
if (type is IArrayTypeSymbol arr)
118+
return ContainsTypeParameter(arr.ElementType);
119+
return false;
120+
}
89121
}

‎src/ExpressiveSharp.Generator/Interpretation/ExpressivePropertyInterpreter.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,8 @@ private static bool IsNullablePropertyType(ITypeSymbol type)
245245

246246
private static string MakeNullableTypeFqn(ITypeSymbol type)
247247
{
248-
var fqn = type.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
248+
// Nullable format preserves inner ? (IEnumerable<Item?>) so the backing field matches the property.
249+
var fqn = type.ToDisplayString(FullyQualifiedNullableFormat);
249250
if (type.IsValueType)
250251
return $"global::System.Nullable<{fqn}>";
251252
return fqn.EndsWith("?") ? fqn : fqn + "?";

‎tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ExpressivePropertyTests.BodyUsingQueryableWhereOrderBy_EmitsValidExpressionTree.verified.txt‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
// <auto-generated/>
2+
#nullable disable
3+
4+
#nullable enable
5+
using ExpressiveSharp.Mapping;
6+
using Foo;
7+
8+
namespace ExpressiveSharp.Generated
9+
{
10+
static partial class Foo_BugB
11+
{
12+
// [ExpressiveProperty("Sorted")]
13+
// private IEnumerable<int> SortedExpr => Source.Where(x => x > 0).OrderBy(x => x);
14+
static global::System.Linq.Expressions.Expression<global::System.Func<global::Foo.BugB, global::System.Collections.Generic.IEnumerable<int>>> Sorted_Expression()
15+
{
16+
var p__this = global::System.Linq.Expressions.Expression.Parameter(typeof(global::Foo.BugB), "@this");
17+
var expr_2 = global::System.Linq.Expressions.Expression.Property(p__this, typeof(global::Foo.BugB).GetProperty("Source")); // Source
18+
var p_x_4 = global::System.Linq.Expressions.Expression.Parameter(typeof(int), "x"); // x => x > 0
19+
var expr_6 = global::System.Linq.Expressions.Expression.Constant(0, typeof(int)); // 0
20+
var expr_5 = global::System.Linq.Expressions.Expression.MakeBinary(global::System.Linq.Expressions.ExpressionType.GreaterThan, p_x_4, expr_6);
21+
var expr_7 = global::System.Linq.Expressions.Expression.Lambda<global::System.Func<int, bool>>(expr_5, p_x_4);
22+
var expr_3 = global::System.Linq.Expressions.Expression.Convert(expr_7, typeof(global::System.Linq.Expressions.Expression<global::System.Func<int, bool>>));
23+
var expr_1 = global::System.Linq.Expressions.Expression.Call(global::System.Linq.Enumerable.First(global::System.Linq.Enumerable.Where(typeof(global::System.Linq.Queryable).GetMethods(global::System.Reflection.BindingFlags.Public | global::System.Reflection.BindingFlags.NonPublic | global::System.Reflection.BindingFlags.Static), m => m.Name == "Where" && m.IsGenericMethodDefinition && m.GetGenericArguments().Length == 1 && m.GetParameters().Length == 2 && m.GetParameters()[0].ParameterType.IsGenericType && !m.GetParameters()[0].ParameterType.IsGenericParameter && m.GetParameters()[1].ParameterType.IsGenericType && !m.GetParameters()[1].ParameterType.IsGenericParameter && m.GetParameters()[1].ParameterType.GetGenericArguments()[0].IsGenericType && m.GetParameters()[1].ParameterType.GetGenericArguments()[0].GetGenericTypeDefinition() == typeof(global::System.Func<,>))).MakeGenericMethod(typeof(int)), new global::System.Linq.Expressions.Expression[] { expr_2, expr_3 });
24+
var p_x_9 = global::System.Linq.Expressions.Expression.Parameter(typeof(int), "x"); // x => x
25+
var expr_10 = global::System.Linq.Expressions.Expression.Lambda<global::System.Func<int, int>>(p_x_9, p_x_9);
26+
var expr_8 = global::System.Linq.Expressions.Expression.Convert(expr_10, typeof(global::System.Linq.Expressions.Expression<global::System.Func<int, int>>));
27+
var expr_0 = global::System.Linq.Expressions.Expression.Call(global::System.Linq.Enumerable.First(global::System.Linq.Enumerable.Where(typeof(global::System.Linq.Queryable).GetMethods(global::System.Reflection.BindingFlags.Public | global::System.Reflection.BindingFlags.NonPublic | global::System.Reflection.BindingFlags.Static), m => m.Name == "OrderBy" && m.IsGenericMethodDefinition && m.GetGenericArguments().Length == 2 && m.GetParameters().Length == 2 && m.GetParameters()[0].ParameterType.IsGenericType && !m.GetParameters()[0].ParameterType.IsGenericParameter && m.GetParameters()[1].ParameterType.IsGenericType && !m.GetParameters()[1].ParameterType.IsGenericParameter && m.GetParameters()[1].ParameterType.GetGenericArguments()[0].IsGenericType && m.GetParameters()[1].ParameterType.GetGenericArguments()[0].GetGenericTypeDefinition() == typeof(global::System.Func<,>))).MakeGenericMethod(typeof(int), typeof(int)), new global::System.Linq.Expressions.Expression[] { expr_1, expr_8 });
28+
var expr_11 = global::System.Linq.Expressions.Expression.Convert(expr_0, typeof(global::System.Collections.Generic.IEnumerable<int>));
29+
return global::System.Linq.Expressions.Expression.Lambda<global::System.Func<global::Foo.BugB, global::System.Collections.Generic.IEnumerable<int>>>(expr_11, p__this);
30+
}
31+
}
32+
}
33+
34+
35+
// ===
36+
37+
// <auto-generated/>
38+
#nullable enable
39+
40+
namespace Foo
41+
{
42+
partial class BugB
43+
{
44+
private global::System.Collections.Generic.IEnumerable<int>? _sorted;
45+
public global::System.Collections.Generic.IEnumerable<int> Sorted
46+
{
47+
get => _sorted ?? SortedExpr;
48+
init => _sorted = value;
49+
}
50+
}
51+
}

‎tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ExpressivePropertyTests.GenericReturnTypeWithInnerNullableReference_EmitsCorrectBackingFieldAnnotation.verified.txt‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// <auto-generated/>
2+
#nullable disable
3+
4+
#nullable enable
5+
using ExpressiveSharp.Mapping;
6+
using Foo;
7+
8+
namespace ExpressiveSharp.Generated
9+
{
10+
static partial class Foo_BugA
11+
{
12+
// [ExpressiveProperty("Items")]
13+
// private IEnumerable<Item?> ItemsExpr => new Item? []
14+
// {
15+
// null
16+
// };
17+
static global::System.Linq.Expressions.Expression<global::System.Func<global::Foo.BugA, global::System.Collections.Generic.IEnumerable<global::Foo.Item>>> Items_Expression()
18+
{
19+
var p__this = global::System.Linq.Expressions.Expression.Parameter(typeof(global::Foo.BugA), "@this");
20+
var expr_2 = global::System.Linq.Expressions.Expression.Constant(null, typeof(object)); // null
21+
var expr_1 = global::System.Linq.Expressions.Expression.Convert(expr_2, typeof(global::Foo.Item));
22+
var expr_0 = global::System.Linq.Expressions.Expression.NewArrayInit(typeof(global::Foo.Item), expr_1);
23+
var expr_3 = global::System.Linq.Expressions.Expression.Convert(expr_0, typeof(global::System.Collections.Generic.IEnumerable<global::Foo.Item>));
24+
return global::System.Linq.Expressions.Expression.Lambda<global::System.Func<global::Foo.BugA, global::System.Collections.Generic.IEnumerable<global::Foo.Item>>>(expr_3, p__this);
25+
}
26+
}
27+
}
28+
29+
30+
// ===
31+
32+
// <auto-generated/>
33+
#nullable enable
34+
35+
namespace Foo
36+
{
37+
partial class BugA
38+
{
39+
private global::System.Collections.Generic.IEnumerable<global::Foo.Item?>? _items;
40+
public global::System.Collections.Generic.IEnumerable<global::Foo.Item?> Items
41+
{
42+
get => _items ?? ItemsExpr;
43+
init => _items = value;
44+
}
45+
}
46+
}

‎tests/ExpressiveSharp.Generator.Tests/ExpressiveGenerator/ExpressivePropertyTests.cs‎

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -421,4 +421,129 @@ partial class Derived : Base {
421421

422422
Assert.AreEqual(1, result.Diagnostics.Count(d => d.Id == "EXP0035"));
423423
}
424+
425+
[TestMethod]
426+
public Task GenericReturnTypeWithInnerNullableReference_EmitsCorrectBackingFieldAnnotation()
427+
{
428+
// Outer non-nullable, inner ref-nullable — backing field must keep the inner ? to match the property.
429+
var compilation = CreateCompilation(
430+
"""
431+
#nullable enable
432+
using ExpressiveSharp.Mapping;
433+
434+
namespace Foo {
435+
public partial class BugA {
436+
[ExpressiveProperty("Items")]
437+
private IEnumerable<Item?> ItemsExpr => new Item?[] { null };
438+
}
439+
public class Item;
440+
}
441+
""");
442+
var result = RunExpressiveGenerator(compilation);
443+
444+
Assert.AreEqual(0, result.Diagnostics.Length,
445+
"Unexpected diagnostics: " + string.Join(", ", result.Diagnostics.Select(d => d.Id + ": " + d.GetMessage())));
446+
Assert.AreEqual(2, result.GeneratedTrees.Length);
447+
448+
var generated = string.Join("\n", result.GeneratedTrees.Select(t => t.ToString()));
449+
StringAssert.Contains(generated, "IEnumerable<global::Foo.Item?>? _items",
450+
"Backing field must preserve the inner ? on Item, otherwise the synthesized ?? trips CS8619.");
451+
452+
return Verifier.Verify(string.Join("\n\n// ===\n\n",
453+
result.GeneratedTrees.Select(t => t.ToString())));
454+
}
455+
456+
[TestMethod]
457+
public void GenericMethodWithClosedInnerTypeArg_PinsByExactEquality()
458+
{
459+
// Inner type arg is concrete (List<int>), exercising the else branch in EnsureMethodInfo.
460+
var compilation = CreateCompilation(
461+
"""
462+
#nullable enable
463+
using ExpressiveSharp.Mapping;
464+
465+
namespace Foo {
466+
public static class ListExt {
467+
public static T Pick<T>(this IEnumerable<T> src, List<int> filter) => default!;
468+
}
469+
public partial class ClosedInnerArg {
470+
public List<int> Filter { get; init; } = null!;
471+
472+
[ExpressiveProperty("Head")]
473+
private string HeadExpr => new[] { "x" }.Pick(Filter);
474+
}
475+
}
476+
""");
477+
var result = RunExpressiveGenerator(compilation);
478+
479+
Assert.AreEqual(0, result.Diagnostics.Length,
480+
"Unexpected diagnostics: " + string.Join(", ", result.Diagnostics.Select(d => d.Id + ": " + d.GetMessage())));
481+
482+
var generated = string.Join("\n", result.GeneratedTrees.Select(t => t.ToString()));
483+
StringAssert.Contains(generated, "GetGenericArguments()[0] == typeof(int)",
484+
"Closed inner type arg must be pinned by exact equality.");
485+
}
486+
487+
[TestMethod]
488+
public void GenericMethodWithArrayOfTypeParam_GeneratesValidCode()
489+
{
490+
// Inner type arg is T[] (IArrayTypeSymbol), exercising the array recursion in ContainsTypeParameter.
491+
var compilation = CreateCompilation(
492+
"""
493+
#nullable enable
494+
using ExpressiveSharp.Mapping;
495+
496+
namespace Foo {
497+
public static class ArrExt {
498+
public static int Total<T>(this IEnumerable<T[]> source) => 0;
499+
}
500+
public partial class ArrayOfTypeParam {
501+
public IEnumerable<int[]> Source { get; init; } = null!;
502+
503+
[ExpressiveProperty("Sum")]
504+
private int SumExpr => Source.Total();
505+
}
506+
}
507+
""");
508+
var result = RunExpressiveGenerator(compilation);
509+
510+
Assert.AreEqual(0, result.Diagnostics.Length,
511+
"Unexpected diagnostics: " + string.Join(", ", result.Diagnostics.Select(d => d.Id + ": " + d.GetMessage())));
512+
513+
var generated = string.Join("\n", result.GeneratedTrees.Select(t => t.ToString()));
514+
Assert.IsFalse(generated.Contains("typeof(T)") || generated.Contains("typeof(T["),
515+
"Array-of-type-parameter slot must not leak the open type parameter into typeof().");
516+
}
517+
518+
[TestMethod]
519+
public Task BodyUsingQueryableWhereOrderBy_EmitsValidExpressionTree()
520+
{
521+
// Queryable.Where's predicate-shaped parameter must not leak the open type parameter (TSource → CS0246).
522+
var compilation = CreateCompilation(
523+
"""
524+
#nullable enable
525+
using ExpressiveSharp.Mapping;
526+
527+
namespace Foo {
528+
public partial class BugB {
529+
public IQueryable<int> Source { get; init; } = null!;
530+
531+
[ExpressiveProperty("Sorted")]
532+
private IEnumerable<int> SortedExpr => Source.Where(x => x > 0).OrderBy(x => x);
533+
}
534+
}
535+
""");
536+
var result = RunExpressiveGenerator(compilation);
537+
538+
Assert.AreEqual(0, result.Diagnostics.Length,
539+
"Unexpected diagnostics: " + string.Join(", ", result.Diagnostics.Select(d => d.Id + ": " + d.GetMessage())));
540+
Assert.AreEqual(2, result.GeneratedTrees.Length);
541+
542+
var generated = string.Join("\n", result.GeneratedTrees.Select(t => t.ToString()));
543+
Assert.IsFalse(generated.Contains("TSource"),
544+
"Generated code must not reference the open type parameter TSource.");
545+
546+
return Verifier.Verify(string.Join("\n\n// ===\n\n",
547+
result.GeneratedTrees.Select(t => t.ToString())));
548+
}
424549
}

0 commit comments

Comments
 (0)