Skip to content

Commit 61d2b5b

Browse files
authored
fix: resolve builder StackOverflow, activator negative refCount, binding regression, and testing extensions (#4301)
## What's Changed ### Builder converter registration no longer causes StackOverflow Calling `WithFallbackConverter`, `WithConverter`, `WithSetMethodConverter`, or `WithConvertersFrom` on `IReactiveUIBuilder` previously caused a `StackOverflowException` due to infinite recursion. These methods now work correctly. ### ViewModelActivator no longer goes into a broken state on extra Deactivate calls If `Deactivate()` was called more times than `Activate()`, the internal reference count would go negative, preventing future activations from working. The activator now safely ignores extra deactivation calls. ### Two-way bindings work again with base types like System.Object Two-way bindings between a view model property of a derived type (e.g. `string`) and a view property of a base type (e.g. `object`) were throwing `ArgumentException`. This is now permitted as expected. ### TestScheduler extensions (AdvanceByMs, AdvanceToMs, etc.) are discoverable again After adding the `ReactiveUI.Testing.Reactive` package, `AdvanceByMs`, `AdvanceToMs`, `OnNextAt`, and related extensions are now available with just `using ReactiveUI.Testing;` -- no additional namespace import needed. Fixes #4293 Fixes #4283 Fixes #4298 Fixes #4297 --- ## Maintainer Notes - **IReactiveUIBuilder.cs**: Added 9 converter method signatures (`WithConverter` x4, `WithFallbackConverter` x2, `WithSetMethodConverter` x2, `WithConvertersFrom` x1) to the interface. These were previously only on the concrete `ReactiveUIBuilder` class, causing the extension methods in `BuilderMixins.cs` to resolve back to themselves instead of dispatching to the implementation. - **ViewModelActivator.cs**: Replaced `Interlocked.Decrement` in `Deactivate()` with a CAS (CompareExchange) loop that refuses to decrement below zero. The `ignoreRefCount` path now explicitly resets to 0. - **PropertyBinderImplementation.cs**: Changed the type assignability check from `&&` (mutual) to `||` (one-directional). The existing fallback conversion logic at lines 180-184 already handles one-directional assignment. - **TestSchedulerExtensions.cs**: Changed namespace from `ReactiveUI.Testing.Reactive` to `ReactiveUI.Testing`. The assembly/package name remains `ReactiveUI.Testing.Reactive` -- the move to a separate package is intentional to keep `ReactiveUI.Testing` free of the `Microsoft.Reactive.Testing` dependency. - **Test updates**: Added new interface members to `TestReactiveUIBuilder` mock in Blazor tests, removed now-unnecessary `using ReactiveUI.Testing.Reactive` from scheduler tests.
1 parent db239a9 commit 61d2b5b

13 files changed

Lines changed: 218 additions & 1896 deletions

dotnet-install.sh

Lines changed: 0 additions & 1888 deletions
This file was deleted.

src/ReactiveUI.Testing.Reactive/ReactiveUI.Testing.Reactive.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
<PackageDescription>Provides TestScheduler extensions for testing ReactiveUI applications with Microsoft.Reactive.Testing</PackageDescription>
88
<PackageId>ReactiveUI.Testing.Reactive</PackageId>
99
<PackageTags>mvvm;reactiveui;rx;reactive extensions;observable;LINQ;events;frp;test;testscheduler;</PackageTags>
10+
<IsPackable>true</IsPackable>
1011
</PropertyGroup>
1112
<ItemGroup>
1213
<Using Include="System" />

src/ReactiveUI.Testing.Reactive/TestSchedulerExtensions.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
using System.Reactive;
77
using Microsoft.Reactive.Testing;
88

9-
namespace ReactiveUI.Testing.Reactive;
9+
namespace ReactiveUI.Testing;
1010

1111
/// <summary>
1212
/// Extension methods for TestScheduler from Microsoft.Reactive.Testing.

src/ReactiveUI/Activation/ViewModelActivator.cs

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,30 @@ public IDisposable Activate()
9090
/// </param>
9191
public void Deactivate(bool ignoreRefCount = false)
9292
{
93-
if (Interlocked.Decrement(ref _refCount) == 0 || ignoreRefCount)
93+
if (ignoreRefCount)
94+
{
95+
Interlocked.Exchange(ref _refCount, 0);
96+
Interlocked.Exchange(ref _activationHandle, Disposable.Empty).Dispose();
97+
_deactivated.OnNext(Unit.Default);
98+
return;
99+
}
100+
101+
// Guard against going negative — only decrement if current value is > 0
102+
int current;
103+
int next;
104+
do
105+
{
106+
current = Volatile.Read(ref _refCount);
107+
if (current <= 0)
108+
{
109+
return;
110+
}
111+
112+
next = current - 1;
113+
}
114+
while (Interlocked.CompareExchange(ref _refCount, next, current) != current);
115+
116+
if (next == 0)
94117
{
95118
Interlocked.Exchange(ref _activationHandle, Disposable.Empty).Dispose();
96119
_deactivated.OnNext(Unit.Default);

src/ReactiveUI/Bindings/Property/PropertyBinderImplementation.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ internal PropertyBinderImplementation(
100100

101101
// Check if we have converters or if types are assignable
102102
var hasConverters = vmToViewConverterObj is not null && viewToVMConverterObj is not null;
103-
var typesAreAssignable = typeof(TVProp).IsAssignableFrom(typeof(TVMProp)) && typeof(TVMProp).IsAssignableFrom(typeof(TVProp));
103+
var typesAreAssignable = typeof(TVProp).IsAssignableFrom(typeof(TVMProp)) || typeof(TVMProp).IsAssignableFrom(typeof(TVProp));
104104

105105
if (!hasConverters && !typesAreAssignable)
106106
{

src/ReactiveUI/Builder/IReactiveUIBuilder.cs

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,6 +219,73 @@ IReactiveUIBuilder WithPlatformModule<T>()
219219
/// <returns>The builder instance for chaining.</returns>
220220
IReactiveUIBuilder WithCacheSizes(int smallCacheLimit, int bigCacheLimit);
221221

222+
/// <summary>
223+
/// Registers a typed binding converter using the concrete type.
224+
/// </summary>
225+
/// <typeparam name="TFrom">The source type for the conversion.</typeparam>
226+
/// <typeparam name="TTo">The target type for the conversion.</typeparam>
227+
/// <param name="converter">The converter instance to register.</param>
228+
/// <returns>The builder instance for chaining.</returns>
229+
IReactiveUIBuilder WithConverter<TFrom, TTo>(BindingTypeConverter<TFrom, TTo> converter);
230+
231+
/// <summary>
232+
/// Registers a typed binding converter using the interface.
233+
/// </summary>
234+
/// <param name="converter">The converter instance to register.</param>
235+
/// <returns>The builder instance for chaining.</returns>
236+
IReactiveUIBuilder WithConverter(IBindingTypeConverter converter);
237+
238+
/// <summary>
239+
/// Registers a typed binding converter via factory (lazy instantiation).
240+
/// </summary>
241+
/// <typeparam name="TFrom">The source type for the conversion.</typeparam>
242+
/// <typeparam name="TTo">The target type for the conversion.</typeparam>
243+
/// <param name="factory">The factory function that creates the converter.</param>
244+
/// <returns>The builder instance for chaining.</returns>
245+
IReactiveUIBuilder WithConverter<TFrom, TTo>(Func<BindingTypeConverter<TFrom, TTo>> factory);
246+
247+
/// <summary>
248+
/// Registers a typed binding converter via factory (interface, lazy instantiation).
249+
/// </summary>
250+
/// <param name="factory">The factory function that creates the converter.</param>
251+
/// <returns>The builder instance for chaining.</returns>
252+
IReactiveUIBuilder WithConverter(Func<IBindingTypeConverter> factory);
253+
254+
/// <summary>
255+
/// Registers a fallback binding converter.
256+
/// </summary>
257+
/// <param name="converter">The fallback converter instance to register.</param>
258+
/// <returns>The builder instance for chaining.</returns>
259+
IReactiveUIBuilder WithFallbackConverter(IBindingFallbackConverter converter);
260+
261+
/// <summary>
262+
/// Registers a fallback binding converter via factory (lazy instantiation).
263+
/// </summary>
264+
/// <param name="factory">The factory function that creates the fallback converter.</param>
265+
/// <returns>The builder instance for chaining.</returns>
266+
IReactiveUIBuilder WithFallbackConverter(Func<IBindingFallbackConverter> factory);
267+
268+
/// <summary>
269+
/// Registers a set-method binding converter.
270+
/// </summary>
271+
/// <param name="converter">The set-method converter instance to register.</param>
272+
/// <returns>The builder instance for chaining.</returns>
273+
IReactiveUIBuilder WithSetMethodConverter(ISetMethodBindingConverter converter);
274+
275+
/// <summary>
276+
/// Registers a set-method binding converter via factory (lazy instantiation).
277+
/// </summary>
278+
/// <param name="factory">The factory function that creates the set-method converter.</param>
279+
/// <returns>The builder instance for chaining.</returns>
280+
IReactiveUIBuilder WithSetMethodConverter(Func<ISetMethodBindingConverter> factory);
281+
282+
/// <summary>
283+
/// Imports all converters from a Splat dependency resolver into the builder.
284+
/// </summary>
285+
/// <param name="resolver">The Splat resolver to import converters from.</param>
286+
/// <returns>The builder instance for chaining.</returns>
287+
IReactiveUIBuilder WithConvertersFrom(IReadonlyDependencyResolver resolver);
288+
222289
/// <summary>
223290
/// Withes the views from assembly.
224291
/// </summary>

src/tests/ReactiveUI.Blazor.Tests/BlazorReactiveUIBuilderExtensionsTests.cs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,5 +259,23 @@ public IReactiveUIBuilder UsingSplatModule<T>(T registrationModule)
259259
public IReactiveUIBuilder WithSuspensionHost<TAppState>() => throw new NotImplementedException();
260260

261261
public IReactiveUIBuilder WithCacheSizes(int smallCacheLimit, int bigCacheLimit) => throw new NotImplementedException();
262+
263+
public IReactiveUIBuilder WithConverter<TFrom, TTo>(BindingTypeConverter<TFrom, TTo> converter) => throw new NotImplementedException();
264+
265+
public IReactiveUIBuilder WithConverter(IBindingTypeConverter converter) => throw new NotImplementedException();
266+
267+
public IReactiveUIBuilder WithConverter<TFrom, TTo>(Func<BindingTypeConverter<TFrom, TTo>> factory) => throw new NotImplementedException();
268+
269+
public IReactiveUIBuilder WithConverter(Func<IBindingTypeConverter> factory) => throw new NotImplementedException();
270+
271+
public IReactiveUIBuilder WithFallbackConverter(IBindingFallbackConverter converter) => throw new NotImplementedException();
272+
273+
public IReactiveUIBuilder WithFallbackConverter(Func<IBindingFallbackConverter> factory) => throw new NotImplementedException();
274+
275+
public IReactiveUIBuilder WithSetMethodConverter(ISetMethodBindingConverter converter) => throw new NotImplementedException();
276+
277+
public IReactiveUIBuilder WithSetMethodConverter(Func<ISetMethodBindingConverter> factory) => throw new NotImplementedException();
278+
279+
public IReactiveUIBuilder WithConvertersFrom(IReadonlyDependencyResolver resolver) => throw new NotImplementedException();
262280
}
263281
}

src/tests/ReactiveUI.Builder.Tests/ReactiveUIBuilderConverterTests.cs

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,65 @@ public async Task WithConvertersFrom_ReturnsBuilderForChaining()
125125
await Assert.That(result).IsSameReferenceAs(builder);
126126
}
127127

128+
[Test]
129+
public async Task WithFallbackConverter_Instance_ReturnsBuilderForChaining()
130+
{
131+
using var locator = new ModernDependencyResolver();
132+
var builder = locator.CreateReactiveUIBuilder();
133+
var converter = new TestFallbackConverter();
134+
135+
var result = builder.WithFallbackConverter(converter);
136+
137+
await Assert.That(result).IsSameReferenceAs(builder);
138+
}
139+
140+
[Test]
141+
public void WithFallbackConverter_Instance_WithNull_Throws()
142+
{
143+
using var locator = new ModernDependencyResolver();
144+
var builder = locator.CreateReactiveUIBuilder();
145+
146+
Assert.Throws<ArgumentNullException>(() =>
147+
builder.WithFallbackConverter((IBindingFallbackConverter)null!));
148+
}
149+
150+
[Test]
151+
public async Task WithFallbackConverter_Factory_ReturnsBuilderForChaining()
152+
{
153+
using var locator = new ModernDependencyResolver();
154+
var builder = locator.CreateReactiveUIBuilder();
155+
156+
var result = builder.WithFallbackConverter(() => new TestFallbackConverter());
157+
158+
await Assert.That(result).IsSameReferenceAs(builder);
159+
}
160+
161+
[Test]
162+
public void WithFallbackConverter_Factory_WithNull_Throws()
163+
{
164+
using var locator = new ModernDependencyResolver();
165+
var builder = locator.CreateReactiveUIBuilder();
166+
167+
Assert.Throws<ArgumentNullException>(() =>
168+
builder.WithFallbackConverter((Func<IBindingFallbackConverter>)null!));
169+
}
170+
171+
[Test]
172+
public async Task WithFallbackConverter_ViaInterfaceTypedVariable_DoesNotRecurse()
173+
{
174+
// Regression test for https://github.com/reactiveui/ReactiveUI/issues/4293
175+
// Calling WithFallbackConverter on an IReactiveUIBuilder-typed variable caused
176+
// infinite recursion (StackOverflowException) because the extension method in
177+
// BuilderMixins called itself instead of delegating to the interface method.
178+
using var locator = new ModernDependencyResolver();
179+
IReactiveUIBuilder builder = locator.CreateReactiveUIBuilder();
180+
var converter = new TestFallbackConverter();
181+
182+
var result = builder.WithFallbackConverter(converter);
183+
184+
await Assert.That(result).IsSameReferenceAs(builder);
185+
}
186+
128187
private sealed class TestTypedConverter : BindingTypeConverter<int, string>
129188
{
130189
public override int GetAffinityForObjects() => 1;
@@ -150,4 +209,15 @@ public bool TryConvertTyped(object? from, object? conversionHint, out object? re
150209
return from != null;
151210
}
152211
}
212+
213+
private sealed class TestFallbackConverter : IBindingFallbackConverter
214+
{
215+
public int GetAffinityForObjects(Type fromType, Type toType) => 1;
216+
217+
public bool TryConvert(Type fromType, object from, Type toType, object? conversionHint, [NotNullWhen(true)] out object? result)
218+
{
219+
result = from;
220+
return true;
221+
}
222+
}
153223
}

src/tests/ReactiveUI.Testing.Tests/SchedulerExtensionTests.cs

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77

88
using Microsoft.Reactive.Testing;
99

10-
using ReactiveUI.Testing.Reactive;
11-
1210
using TUnit.Core.Executors;
1311

1412
namespace ReactiveUI.Testing.Tests;

src/tests/ReactiveUI.Tests/API/ApiApprovalTests.ReactiveUI.DotNet10_0.verified.txt

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2040,7 +2040,14 @@ namespace ReactiveUI.Builder
20402040
ReactiveUI.Builder.IReactiveUIBuilder UsingSplatModule<T>(T registrationModule)
20412041
where T : Splat.Builder.IModule;
20422042
ReactiveUI.Builder.IReactiveUIBuilder WithCacheSizes(int smallCacheLimit, int bigCacheLimit);
2043+
ReactiveUI.Builder.IReactiveUIBuilder WithConverter(ReactiveUI.IBindingTypeConverter converter);
2044+
ReactiveUI.Builder.IReactiveUIBuilder WithConverter(System.Func<ReactiveUI.IBindingTypeConverter> factory);
2045+
ReactiveUI.Builder.IReactiveUIBuilder WithConverter<TFrom, TTo>(ReactiveUI.BindingTypeConverter<TFrom, TTo> converter);
2046+
ReactiveUI.Builder.IReactiveUIBuilder WithConverter<TFrom, TTo>(System.Func<ReactiveUI.BindingTypeConverter<TFrom, TTo>> factory);
2047+
ReactiveUI.Builder.IReactiveUIBuilder WithConvertersFrom(Splat.IReadonlyDependencyResolver resolver);
20432048
ReactiveUI.Builder.IReactiveUIBuilder WithExceptionHandler(System.IObserver<System.Exception> exceptionHandler);
2049+
ReactiveUI.Builder.IReactiveUIBuilder WithFallbackConverter(ReactiveUI.IBindingFallbackConverter converter);
2050+
ReactiveUI.Builder.IReactiveUIBuilder WithFallbackConverter(System.Func<ReactiveUI.IBindingFallbackConverter> factory);
20442051
ReactiveUI.Builder.IReactiveUIInstance WithInstance<T>(System.Action<T?> action);
20452052
ReactiveUI.Builder.IReactiveUIInstance WithInstance<T1, T2>(System.Action<T1?, T2?> action);
20462053
ReactiveUI.Builder.IReactiveUIInstance WithInstance<T1, T2, T3>(System.Action<T1?, T2?, T3?> action);
@@ -2066,6 +2073,8 @@ namespace ReactiveUI.Builder
20662073
ReactiveUI.Builder.IReactiveUIBuilder WithPlatformServices();
20672074
ReactiveUI.Builder.IReactiveUIBuilder WithRegistration(System.Action<Splat.IMutableDependencyResolver> configureAction);
20682075
ReactiveUI.Builder.IReactiveUIBuilder WithRegistrationOnBuild(System.Action<Splat.IMutableDependencyResolver> configureAction);
2076+
ReactiveUI.Builder.IReactiveUIBuilder WithSetMethodConverter(ReactiveUI.ISetMethodBindingConverter converter);
2077+
ReactiveUI.Builder.IReactiveUIBuilder WithSetMethodConverter(System.Func<ReactiveUI.ISetMethodBindingConverter> factory);
20692078
ReactiveUI.Builder.IReactiveUIBuilder WithSuspensionHost();
20702079
ReactiveUI.Builder.IReactiveUIBuilder WithSuspensionHost<TAppState>();
20712080
ReactiveUI.Builder.IReactiveUIBuilder WithTaskPoolScheduler(System.Reactive.Concurrency.IScheduler scheduler, bool setRxApp = true);

0 commit comments

Comments
 (0)