Skip to content

Commit ce7ae6c

Browse files
committed
fix: ViewLocatorDispatch contract ordering, thread safety, and defensive improvements
- Group dispatch branches by ViewModel type so contract-specific checks are emitted before the default branch (prevents shadowing) - Escape contract strings with SymbolDisplay.FormatLiteral for safe codegen - Use Interlocked.CompareExchange for thread-safe singleton view caching - Fix strategyDoc for SingleInstanceView without parameterless constructor - Move [ExcludeFromViewRegistration] check inside IViewFor<T> guard to prevent EnsureNotNull crash in compilations without ReactiveUI.Binding - Add tests for grouped dispatch and contract-only dispatch (100% coverage)
1 parent a0741ec commit ce7ae6c

11 files changed

Lines changed: 507 additions & 34 deletions

src/ReactiveUI.Binding.SourceGenerators/Generators/ViewLocatorDispatchGenerator.cs

Lines changed: 132 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System.Text;
77

88
using Microsoft.CodeAnalysis;
9+
using Microsoft.CodeAnalysis.CSharp;
910

1011
using ReactiveUI.Binding.SourceGenerators.Helpers;
1112
using ReactiveUI.Binding.SourceGenerators.Models;
@@ -145,35 +146,41 @@ private static bool __RegisterViewDispatch()
145146
{
146147
""");
147148

149+
// Group registrations by ViewModel FQN so contract-specific checks
150+
// are emitted before the default (no-contract) branch within a single
151+
// type-switch block. Without grouping, a default branch emitted first
152+
// would unconditionally match and shadow contract-specific branches.
153+
var vmOrder = new List<string>(registrations.Count);
154+
var vmGroupIndices = new Dictionary<string, List<int>>(registrations.Count);
148155
for (var i = 0; i < registrations.Count; i++)
149156
{
150-
var reg = registrations[i];
151-
var resolverMethodName = "__ResolveView_" + i;
157+
var vmFqn = registrations[i].ViewModelFullyQualifiedName;
158+
if (!vmGroupIndices.TryGetValue(vmFqn, out var indices))
159+
{
160+
indices = new List<int>();
161+
vmGroupIndices[vmFqn] = indices;
162+
vmOrder.Add(vmFqn);
163+
}
164+
165+
indices.Add(i);
166+
}
167+
168+
for (var g = 0; g < vmOrder.Count; g++)
169+
{
170+
var vmFqn = vmOrder[g];
171+
var indices = vmGroupIndices[vmFqn];
152172

153173
sb.AppendLine();
154174

155-
if (reg.Contract is not null)
175+
if (indices.Count == 1)
156176
{
157-
sb.Append($$"""
158-
// {{reg.ViewModelFullyQualifiedName}} -> {{reg.ViewFullyQualifiedName}} [contract: "{{reg.Contract}}"]
159-
if (instance is {{reg.ViewModelFullyQualifiedName}})
160-
{
161-
if (contract == "{{reg.Contract}}")
162-
{
163-
return {{resolverMethodName}}(contract);
164-
}
165-
}
166-
""");
177+
// Single registration per VM: emit the compact form.
178+
EmitSingleRegistrationDispatch(sb, registrations, indices[0]);
167179
}
168180
else
169181
{
170-
sb.Append($$"""
171-
// {{reg.ViewModelFullyQualifiedName}} -> {{reg.ViewFullyQualifiedName}}
172-
if (instance is {{reg.ViewModelFullyQualifiedName}})
173-
{
174-
return {{resolverMethodName}}(contract);
175-
}
176-
""");
182+
// Multiple registrations for same VM: group into single type-switch.
183+
EmitGroupedDispatch(sb, registrations, vmFqn, indices);
177184
}
178185
}
179186

@@ -196,6 +203,100 @@ private static bool __RegisterViewDispatch()
196203
""");
197204
}
198205

206+
/// <summary>
207+
/// Emits a dispatch branch for a single registration (one view per VM type).
208+
/// Preserves the compact output format used by existing tests.
209+
/// </summary>
210+
/// <param name="sb">The string builder.</param>
211+
/// <param name="registrations">All registrations.</param>
212+
/// <param name="index">The registration index.</param>
213+
private static void EmitSingleRegistrationDispatch(StringBuilder sb, List<ViewRegistrationInfo> registrations, int index)
214+
{
215+
var reg = registrations[index];
216+
var resolverMethodName = "__ResolveView_" + index;
217+
218+
if (reg.Contract is not null)
219+
{
220+
var escapedLiteral = SymbolDisplay.FormatLiteral(reg.Contract, true);
221+
sb.Append($$"""
222+
// {{reg.ViewModelFullyQualifiedName}} -> {{reg.ViewFullyQualifiedName}} [contract: {{escapedLiteral}}]
223+
if (instance is {{reg.ViewModelFullyQualifiedName}})
224+
{
225+
if (contract == {{escapedLiteral}})
226+
{
227+
return {{resolverMethodName}}(contract);
228+
}
229+
}
230+
""");
231+
}
232+
else
233+
{
234+
sb.Append($$"""
235+
// {{reg.ViewModelFullyQualifiedName}} -> {{reg.ViewFullyQualifiedName}}
236+
if (instance is {{reg.ViewModelFullyQualifiedName}})
237+
{
238+
return {{resolverMethodName}}(contract);
239+
}
240+
""");
241+
}
242+
}
243+
244+
/// <summary>
245+
/// Emits a grouped dispatch branch for a VM type with multiple registrations.
246+
/// Contract-specific checks are emitted first, with the default (no-contract) branch last.
247+
/// </summary>
248+
/// <param name="sb">The string builder.</param>
249+
/// <param name="registrations">All registrations.</param>
250+
/// <param name="vmFqn">The fully qualified VM type name.</param>
251+
/// <param name="indices">The registration indices for this VM type.</param>
252+
private static void EmitGroupedDispatch(StringBuilder sb, List<ViewRegistrationInfo> registrations, string vmFqn, List<int> indices)
253+
{
254+
sb.Append($$"""
255+
// {{vmFqn}} — multiple views
256+
if (instance is {{vmFqn}})
257+
{
258+
""");
259+
260+
// Contract-specific branches first
261+
for (var j = 0; j < indices.Count; j++)
262+
{
263+
var idx = indices[j];
264+
var reg = registrations[idx];
265+
if (reg.Contract is not null)
266+
{
267+
var escapedLiteral = SymbolDisplay.FormatLiteral(reg.Contract, true);
268+
var resolverMethodName = "__ResolveView_" + idx;
269+
sb.AppendLine().Append($$"""
270+
// -> {{reg.ViewFullyQualifiedName}} [contract: {{escapedLiteral}}]
271+
if (contract == {{escapedLiteral}})
272+
{
273+
return {{resolverMethodName}}(contract);
274+
}
275+
""");
276+
}
277+
}
278+
279+
// Default (no-contract) branch last
280+
for (var j = 0; j < indices.Count; j++)
281+
{
282+
var idx = indices[j];
283+
var reg = registrations[idx];
284+
if (reg.Contract is null)
285+
{
286+
var resolverMethodName = "__ResolveView_" + idx;
287+
sb.AppendLine().Append($$"""
288+
// -> {{reg.ViewFullyQualifiedName}} (default)
289+
return {{resolverMethodName}}(contract);
290+
""");
291+
break; // Only one default per VM (deduplicated earlier)
292+
}
293+
}
294+
295+
sb.AppendLine().Append("""
296+
}
297+
""");
298+
}
299+
199300
/// <summary>
200301
/// Generates a per-view-model resolver method.
201302
/// </summary>
@@ -206,11 +307,13 @@ private static void GenerateResolverMethod(StringBuilder sb, ViewRegistrationInf
206307
{
207308
var methodName = "__ResolveView_" + index;
208309

209-
var strategyDoc = reg.IsSingleInstance
210-
? " /// Returns a cached singleton instance (marked with [SingleInstanceView])."
211-
: reg.HasParameterlessConstructor
212-
? " /// Tries the service locator first, then falls back to direct construction."
213-
: " /// Service locator only — no direct construction available.";
310+
var strategyDoc = (reg.IsSingleInstance, reg.HasParameterlessConstructor) switch
311+
{
312+
(true, true) => " /// Returns a cached singleton instance (marked with [SingleInstanceView]).",
313+
(true, false) => " /// Service locator only — [SingleInstanceView] without parameterless constructor.",
314+
(false, true) => " /// Tries the service locator first, then falls back to direct construction.",
315+
(false, false) => " /// Service locator only — no direct construction available.",
316+
};
214317

215318
sb.AppendLine().Append($$"""
216319
@@ -245,7 +348,10 @@ private static void GenerateResolverMethod(StringBuilder sb, ViewRegistrationInf
245348
// Fallback: singleton construction ({{reg.ViewFullyQualifiedName}} has [SingleInstanceView]).
246349
if ({{fieldName}} == null)
247350
{
248-
{{fieldName}} = new {{reg.ViewFullyQualifiedName}}();
351+
System.Threading.Interlocked.CompareExchange(
352+
ref {{fieldName}},
353+
new {{reg.ViewFullyQualifiedName}}(),
354+
null);
249355
}
250356
251357
return {{fieldName}};

src/ReactiveUI.Binding.SourceGenerators/Helpers/ViewRegistrationExtractor.cs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,6 @@ internal static class ViewRegistrationExtractor
3333
return null;
3434
}
3535

36-
// Skip types marked with [ExcludeFromViewRegistration]
37-
if (HasAttribute(typeSymbol, Constants.ExcludeFromViewRegistrationAttributeMetadataName, semanticModel.Compilation))
38-
{
39-
return null;
40-
}
41-
4236
var iViewForGeneric = semanticModel.Compilation.GetTypeByMetadataName(
4337
Constants.IViewForGenericMetadataName);
4438

@@ -54,6 +48,14 @@ internal static class ViewRegistrationExtractor
5448
&& SymbolEqualityComparer.Default.Equals(iface.OriginalDefinition, iViewForGeneric)
5549
&& iface.TypeArguments.Length == 1)
5650
{
51+
// Check [ExcludeFromViewRegistration] only after confirming IViewFor<T> is
52+
// resolvable, so attribute resolution via EnsureNotNull cannot throw in
53+
// compilations that don't reference ReactiveUI.Binding.
54+
if (HasAttribute(typeSymbol, Constants.ExcludeFromViewRegistrationAttributeMetadataName, semanticModel.Compilation))
55+
{
56+
return null;
57+
}
58+
5759
var viewModelType = iface.TypeArguments[0];
5860
var viewModelFqn = viewModelType.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
5961
var viewFqn = typeSymbol.ToDisplayString(SymbolDisplayFormat.FullyQualifiedFormat);
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
//HintName: GeneratedBinderRegistration.g.cs
2+
// <auto-generated/>
3+
#pragma warning disable
4+
5+
namespace ReactiveUI.Binding.Generated
6+
{
7+
/// <summary>
8+
/// Auto-generated binder registration. Registers high-affinity
9+
/// ICreatesObservableForProperty implementations detected at compile time.
10+
/// </summary>
11+
[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
12+
internal static class __GeneratedBinderRegistration
13+
{
14+
/// <summary>
15+
/// Registers all generated binders with the Splat service locator.
16+
/// </summary>
17+
internal static void Initialize()
18+
{
19+
// Generated binder registrations will be added here in future phases.
20+
// Each per-kind binder provides high-affinity observation for detected types.
21+
// Detected types for kind: INPC
22+
}
23+
}
24+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
//HintName: GeneratedBindingsAttributes.g.cs
2+
// <auto-generated/>
3+
#pragma warning disable
4+
5+
namespace ReactiveUI.Binding
6+
{
7+
[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
8+
internal static partial class __ReactiveUIGeneratedBindings
9+
{
10+
}
11+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
//HintName: ViewDispatch.g.cs
2+
// <auto-generated/>
3+
#pragma warning disable
4+
5+
namespace ReactiveUI.Binding
6+
{
7+
internal static partial class __ReactiveUIGeneratedBindings
8+
{
9+
/// <summary>
10+
/// Triggers view dispatch registration when the generated bindings class is loaded.
11+
/// </summary>
12+
private static readonly bool __viewDispatchRegistered = __RegisterViewDispatch();
13+
14+
/// <summary>
15+
/// Registers the source-generated view dispatch function with
16+
/// <see cref="global::ReactiveUI.Binding.DefaultViewLocator"/>.
17+
/// Called once via static field initializer when this class is first accessed.
18+
/// </summary>
19+
/// <returns>Always returns <see langword="true"/>.</returns>
20+
private static bool __RegisterViewDispatch()
21+
{
22+
global::ReactiveUI.Binding.DefaultViewLocator.SetGeneratedViewDispatch(
23+
__TryResolveView);
24+
return true;
25+
}
26+
27+
/// <summary>
28+
/// Compile-time generated type-switch dispatch for view resolution.
29+
/// Attempts to resolve a view for the given view model instance without reflection.
30+
/// </summary>
31+
/// <param name="instance">The view model instance to resolve a view for.</param>
32+
/// <param name="contract">The contract string (empty string for default).</param>
33+
/// <returns>The resolved view, or <see langword="null"/> if no generated mapping exists.</returns>
34+
private static global::ReactiveUI.Binding.IViewFor __TryResolveView(
35+
object instance, string contract)
36+
{
37+
// global::TestApp.DashboardViewModel — multiple views
38+
if (instance is global::TestApp.DashboardViewModel)
39+
{
40+
// -> global::TestApp.CompactDashboardView [contract: "compact"]
41+
if (contract == "compact")
42+
{
43+
return __ResolveView_1(contract);
44+
}
45+
// -> global::TestApp.DashboardView (default)
46+
return __ResolveView_0(contract);
47+
}
48+
49+
// No compile-time mapping found; fall back to runtime resolution.
50+
return null;
51+
}
52+
53+
/// <summary>
54+
/// Resolves a view for <see cref="global::TestApp.DashboardViewModel"/>.
55+
/// Tries the service locator first, then falls back to direct construction.
56+
/// </summary>
57+
/// <param name="contract">The contract string (empty string for default).</param>
58+
/// <returns>The resolved view, or <see langword="null"/> if resolution fails.</returns>
59+
private static global::ReactiveUI.Binding.IViewFor __ResolveView_0(string contract)
60+
{
61+
// Normalize contract: empty string means no contract (null for Splat lookup).
62+
string svcContract = contract.Length == 0 ? null : contract;
63+
64+
// Prefer service-locator-registered view (supports DI-configured instances).
65+
var view = global::Splat.AppLocator.Current
66+
.GetService<global::ReactiveUI.Binding.IViewFor<global::TestApp.DashboardViewModel>>(
67+
svcContract);
68+
if (view != null)
69+
{
70+
return view;
71+
}
72+
// Fallback: direct construction (global::TestApp.DashboardView has a parameterless constructor).
73+
return new global::TestApp.DashboardView();
74+
}
75+
76+
/// <summary>
77+
/// Resolves a view for <see cref="global::TestApp.DashboardViewModel"/>.
78+
/// Tries the service locator first, then falls back to direct construction.
79+
/// </summary>
80+
/// <param name="contract">The contract string (empty string for default).</param>
81+
/// <returns>The resolved view, or <see langword="null"/> if resolution fails.</returns>
82+
private static global::ReactiveUI.Binding.IViewFor __ResolveView_1(string contract)
83+
{
84+
// Normalize contract: empty string means no contract (null for Splat lookup).
85+
string svcContract = contract.Length == 0 ? null : contract;
86+
87+
// Prefer service-locator-registered view (supports DI-configured instances).
88+
var view = global::Splat.AppLocator.Current
89+
.GetService<global::ReactiveUI.Binding.IViewFor<global::TestApp.DashboardViewModel>>(
90+
svcContract);
91+
if (view != null)
92+
{
93+
return view;
94+
}
95+
// Fallback: direct construction (global::TestApp.CompactDashboardView has a parameterless constructor).
96+
return new global::TestApp.CompactDashboardView();
97+
}
98+
}
99+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
//HintName: GeneratedBinderRegistration.g.cs
2+
// <auto-generated/>
3+
#pragma warning disable
4+
5+
namespace ReactiveUI.Binding.Generated
6+
{
7+
/// <summary>
8+
/// Auto-generated binder registration. Registers high-affinity
9+
/// ICreatesObservableForProperty implementations detected at compile time.
10+
/// </summary>
11+
[global::System.Diagnostics.CodeAnalysis.ExcludeFromCodeCoverage]
12+
internal static class __GeneratedBinderRegistration
13+
{
14+
/// <summary>
15+
/// Registers all generated binders with the Splat service locator.
16+
/// </summary>
17+
internal static void Initialize()
18+
{
19+
// Generated binder registrations will be added here in future phases.
20+
// Each per-kind binder provides high-affinity observation for detected types.
21+
// Detected types for kind: INPC
22+
}
23+
}
24+
}

0 commit comments

Comments
 (0)