Inconsistent RegisterViewsForViewModels() behavior

[FreddyDgh]

Problem

There are a bunch of minor problems that result from a bug in the way Locator.CurrentMutable.RegisterViewsForViewModels() is coded. I think the biggest issue with it is that its behavior is not necessarily deterministic. While it should be deterministic on a given compiler/runtime, it is not deterministic across different compilers/runtimes.

The key problem that results from this is that when you call RegisterViewsForViewModels(), the DefaultViewLocator is not able to find your views, because the registration behavior with Splat is not guaranteed to be consistent, so it can't always find the service type that the view was registered under.

I believe the solution is to merge this pull request: #2356, however, the PR was closed.

This affects interfaces that directly derive from IViewFor the most, but it wouldn't be surprising to find more complex scenarios broken as well. For example, consider the following interface:

public interface ISpecializedViewFor<T> : IViewFor<T>

And let's say views are registered with:

public App()
{
	Locator.CurrentMutable.RegisterViewsForViewModels(Assembly.GetCallingAssembly());
}

We can then look at the following scenarios:

// Scenario A
public class SampleView : ISpecializedViewFor<SampleViewModel> { }

// Scenario B
public class SampleView : IViewFor<SampleViewModel>, ISpecializedViewFor<SampleViewModel> { }

// Scenario C
public class SampleView : ISpecializedViewFor<SampleViewModel>, IViewFor<SampleViewModel> { }

Compiling with .NET Core 3.1 on Windows x64 gave me the following results:

• Scenario A gets registered with something like Locator.CurrentMutable.Register(() => new SampleView(), typeof(ISpecializedViewFor<SampleViewModel>)); instead of Locator.CurrentMutable.Register(() => new SampleView(), typeof(IViewFor<SampleViewModel>)); and it therefore the view won't be found by the DefaultViewLocator.
• Scenario B DOES get registered correctly and can be found by the DefaultViewLocator, but Scenario C DOES NOT work. Please note that it is highly unintuitive that the order of the listed interfaces should matter for being able to locate the view.

Testing some sample code using different compilers led me to find that the above behavior is typical, but there was one compiler I tried where Scenarios A, B, and C all were registered with Locator.CurrentMutable.Register(() => new SampleView(), typeof(ISpecializedViewFor<SampleViewModel>)); in spite of the how they were declared. Thus, there was no way at all to have the view found by the DefaultViewLocator.

Furthermore, I believe the existing code actually will register a view using a non-generic interface. For example, say you have:

public interface ISomeViewFor : IViewFor { }

public class ViewA : ISomeViewFor { }
public class ViewB : ISomeViewFor { }

I can't recall if I actually tested this particular case or not, but I remember analyzing the code and I believe this will actually result in the following:

Locator.CurrentMutable.Register(() => new ViewA(), typeof(ISomeViewFor));
Locator.CurrentMutable.Register(() => new ViewB(), typeof(ISomeViewFor));

Based on code comments and documentation, I don't believe registration using non-generic interfaces is an intended behavior. My memory is that it won't be found by the default view locator, and even if it were, multiple views could be registered with the same service type, so the behavior would likely be undefined. I don't think this issue matters too much, but it does demonstrate some potentially unwanted side effects of the current code.

Summary

To summarize, the current behavior of RegisterViewsForViewModels() exhibits at least the following issues:

• The behavior is not guaranteed to be consistent across platforms.
• The most desired behavior of being able to implement SampleView : ISpecializedViewFor<SampleViewModel> does not work with the DefaultViewLocator.
• When trying to use the less desired behavior of explicitly implementing two interfaces (i.e. SampleView : IViewFor<SampleViewModel>, ISpecializedViewFor<SampleViewModel>), the order of the interface list matters, or on some platforms, may not work at all. That makes writing documentation on how to properly use RegisterViewsForViewModels() very difficult, because there is no guidance that you can give users that is guaranteed to work.
• It may register views implementing non-generic interfaces, which is probably not common, but also, is probably not desirable (verification/testing needed)

Next Steps

Again, I believe the most appropriate fix is #2356. While the change may not be 100% fail-safe in its own right, I believe it generally maintains backward compatibility while fixing the problems with the current code. Certainly, it isn't guaranteed that it won't break anyone (after all, the behavior is not consistent across platforms, and fixing that is basically the point, isn't it?). However, I don't believe that the PR as submitted would realistically break anyone. The most likely scenario that I could think of would be if someone had a view declared as in Scenario A, registered views using RegisterViewsForViewModels(), realized it didn't work, and wrote their own custom ViewLocator as a workaround that finds the view using ISpecializedViewFor<T> instead of IViewFor<T>. I'm not sure how likely this situation is to begin with, but even so, they would clearly have to be aware of the bug and I don't think it would surprise such people to see the bug fixed. It is pretty difficult to think of a realistic scenario where the PR would break someone who didn't explicitly encounter the bug and then deliberately try to workaround it.

Also, please keep in mind that the given PR would affect existing code like this:

public interface ICustomViewFor<T> : IViewFor<T> { }
public interface ICustomViewFor : IViewFor { }

But would NOT affect existing code like this

public class CustomViewForBase<T> : IViewFor<T> { }
public class CustomViewForBase : IViewFor { }

So, the PR does not affect people following the guidance in this documentation: https://reactiveui.net/docs/handbook/view-location/extending-iviewfor

Personally, I don't particularly see the argument for keeping code that can lead to bugs (even if the bugs won't affect 99% of people). However, if a different solution is desired, there are some possible alternatives to fixing RegisterViewsForViewModels():

1. You could define another extension method that is virtually identical to RegisterViewsForViewModels(), but include the changes from the PR and call it something else (or make an overload of the existing method). The upside is that it won't break any existing code. The downsides are that there will be multiple methods with very similar behavior, which would make it confusing for people to know which one is correct to call, and that people who do nothing will continue to call the existing code, which is less desirable. To remedy that, the existing method could be marked Obsolete/EditorBrowsableNever, but that would cause EVERYONE currently using RegisterViewsForViewModels() to get warnings. Furthermore, all existing documentation and samples would need to be updated to use the new method/overload. I'm not sure that's really worth it for this type of bug-fix.

2. You could also attempt to remedy the situation by modifying the DefaultViewLocator. I'm not sure how plausible this one actually is, since it's not necessarily consistent how the views are registered. However, I would expect that since the registration code only runs once, it would be better to add the code fix there instead of the ViewLocator, whose code runs every time that a view needs to be located.

[glennawatson]

Hi there,

Thanks for the issue and following up.

Really appreciate taking your time and explaining your problem to us. We just find changes which can have more of a bigger impact better discussed in an issue first.

I will have a play around with the scenarios your talking about and setup some unit tests first most likely is the way I'm going to approach this problem. Then I will evaluate solutions including the one you presented in #2356

Thanks