Fixes ReactiveValidationObject NullReferenceException and enhances performance and thread-safety of ValidationText

[thargy]

What kind of change does this PR introduce?
This pull request fixes #142 and implements #141. There is no good reason for ValidationText to be mutable, as the current mutators (Add and Clear) are not used by the library and introduce potential thread-safety issues when validating asynchronously.

What is the current behavior?
#142 describes a bug based on the nullability of ReactiveValidationObject.SelectInvalidPropertyValidations, the fix is included in this PR as it is dependent on changes implemented for #141

#141 describes the benefits of making ValidationText immutable. As well as thread-safety, we can provide the ValidationText.Empty and ValidationText.None singletons for improved performance and memory utilisation. The current behaviour creates a lot of unnecessary lists.

What is the new behavior?
This PR introduces a pseudo-immutable version of ValidationText which exposes new Create factory methods. The factory methods will automatically detect a None or Empty equivalent and return the new corresponding singleton instead of a new instance, reducing memory utilisation.

The underlying List<string> is replaced with a string[] to improve performance and memory utilisation.

Currently, the underlying array is not readonly as the Add() and Clear() mutators (now marked as obsolete) are implemented for backwards compatibility. Using those methods will break thread-safety (which is not a breaking change). If those methods are not used, the class is thread-safe.

The existing constructors are marked as obsolete to encourage adoption of the factory methods.

TODO comments are included for changes that can be made when the existing constructors/methods are finally removed in future.

What might this PR break?
The provided implementation should be backwards compatible, it marks certain methods/constructors obsolete which will produce warnings in consumers.

Please check if the PR fulfills these requirements

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:
Existing usages of the now obsolete constructors have been removed in the library and replaced with calls to the factory methods. As such these frequently return one of the new singletons. Trying to call Add() or Clear() on the singletons results in an InvalidOperationException to prevent more serious bugs.

Where consumers created their own ValidationText instances this is not a problem, as the singletons will not be in use in existing code. However, attempting to modify ValidationText instances provided by the library itself may result in the new exception. This is considered unlikely in practice.

[codecov]

Codecov Report

Merging #144 into main will increase coverage by 0.36%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #144      +/-   ##
==========================================
+ Coverage   61.57%   61.94%   +0.36%     
==========================================
  Files          16       16              
  Lines         877      896      +19     
==========================================
+ Hits          540      555      +15     
- Misses        337      341       +4     

Impacted Files	Coverage Δ
Components/BasePropertyValidation.cs	93.50% <0.00%> (-0.25%)	⬇️
Helpers/ReactiveValidationObject.cs	97.36% <0.00%> (ø)
Collections/ValidationText.cs	79.06% <0.00%> (+2.87%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15ad58c...bdf6a81. Read the comment docs.

[thargy]

@worldbeater does this need tests adding for the Create factory? Not sure where the best place to add such tests would be, also I'm a bit scuppered by #145 at the moment (though I can temporarily delete the UAP target to test).

[worldbeater]

Worth adding a few unit tests for the factory methods yeah as we are exposing them as public API @thargy. This should make codecov happier. You could place the tests into ValidationContextTests or into PropertyValidationTests I guess as we keep the tests for low-level stuff there. Although creating a separate class e.g. ValidationTextTests also makes sense.

Removing the UAP target and not including this particular .csproj change in the diff (e.g. via git reset **/*.csproj before committing) might be a good way to go too. Worth noting that the project builds successfully in our CI/CD environment so probably some packages might be missing or conflicting on your machine, but the CI runs the tests in different environments for us anyway.

[worldbeater]

Would be happy to see the unit tests for the factories but in general LGTM! Thanks for this.

[thargy]

Updating this now.

[todo]

When Add() & Clear() are obsolesced this should be made read-only.

ReactiveUI.Validation/src/ReactiveUI.Validation/Collections/ValidationText.cs

Lines 29 to 34 in 0111a81

	// TODO When Add() & Clear() are obsolesced this should be made read-only.
	private /* readonly */ string[] _texts;
	/// <summary>
	/// Initializes a new instance of the <see cref="ValidationText"/> class.
	/// </summary>

---

This comment was generated by todo based on a TODO comment in 0111a81 in #144. cc @thargy.

[thargy]

I believe this is now done! After careful review, I added both an Empty and None singleton, this allowed backwards compatibility with the existing constructors. I added tests for the Create factories, though they also cover much of the rest of the class too, they also indicate the desired functionality.

For example, None+None = None, None+Empty=Empty, Empty+Empty=VT with 2 string.Empty items, etc.

The PR should improve thread safety and memory utilisation quite noticeably as the None and Empty singletons are a very common use case in the codebase (and elsewhere). This will result in a lot fewer object allocations in real-world scenarios.

[worldbeater]

Sorry for those boring suggestions! Just making sure we didn't miss anything. Huge thanks for the PR!

[todo]

When Add() & Clear() are obsolesced this should be made read-only.

ReactiveUI.Validation/src/ReactiveUI.Validation/Collections/ValidationText.cs

Lines 29 to 34 in 17f0792

	// TODO When Add() & Clear() are obsolesced this should be made read-only.
	private /* readonly */ string[] _texts;
	/// <summary>
	/// Initializes a new instance of the <see cref="ValidationText"/> class.
	/// </summary>

---

This comment was generated by todo based on a TODO comment in 17f0792 in #144. cc @thargy.

[todo]

When Add() & Clear() are obsolesced this should be made read-only.

ReactiveUI.Validation/src/ReactiveUI.Validation/Collections/ValidationText.cs

Lines 29 to 34 in 2c54af4

	// TODO When Add() & Clear() are obsolesced this should be made read-only.
	private /* readonly */ string[] _texts;
	/// <summary>
	/// Initializes a new instance of the <see cref="ValidationText"/> class.
	/// </summary>

---

This comment was generated by todo based on a TODO comment in 2c54af4 in #144. cc @thargy.

[glennawatson]

Lets see if we can remove some of these TODOs and put them into a github issue.

[thargy]

Thanks @worldbeater & @glennawatson for taking it over the line, I'd gone to bed!

[worldbeater]

Should be available on NuGet as 1.8.6 now.