reaqtive / reaqtor

Reaqtor is a framework for reliable, stateful, distributed, and scalable event processing based on Rx.
https://reaqtive.net/
MIT License
627 stars 40 forks source link

StringSegment test fails due to .NET 6.0 fixing a bug in string.Remove #124

Closed idg10 closed 2 years ago

idg10 commented 2 years ago

Nuqleon's StringSegment provides direct equivalents to various members of System.String, including a form of Remove that takes a single argument. The current version of this:

https://github.com/reaqtive/reaqtor/blob/de07519c630b0b1dba97efb1eb58bb4aa1aa9751/Nuqleon/Core/BCL/Nuqleon.StringSegment/System/StringSegment.cs#L1022-L1032

has tests that verify maintains bug-for-bug compatibility with the corresponding System.String.Remove method. This now causes us to fail builds because the .NET team went and fixed one of those bugs:

https://github.com/dotnet/runtime/issues/50094

The bug here is that the single-int-argument overload of Remove had edge case behaviour that was inconsistent with other overloads: with the two-ints overload. For example, this is allowed:

string r = "foo".Remove(3, 0);

and sets r to "foo". I.e., removing zero characters at the end of the string is allowed as a no-op. But this, which is, arguably, logically equivalent:

string r = "foo".Remove(3);

fails on .NET FX and everything up to and including .NET 5.0, throwing an ArgumentOutOfRangeException. In .NET 6.0, https://github.com/dotnet/runtime/pull/50096 means that this second example no longer throw, and now does the same thing as the first example.

This gives us a problem. StringSegment is intended to produce the same logical behaviour as string.

Options include:

  1. Remove StringSegment—nothing in this codebase appears to depend on it, and as it says in https://github.com/reaqtive/reaqtor/blob/main/Nuqleon/Core/BCL/Nuqleon.StringSegment/README.md this was introduced before Span<T> was available, "which may provide a valid alternative."
  2. Use conditional compilation to make StringSegment behaviour reproduce the different string.Remove behaviours for different targets (but what do we do with netstandard2.0?)
  3. Preserve the current behaviour, modifying the tests to handle the case where string and StringSegment now differ
  4. Modify the behaviour to match .NET 6.0's string.Remove(int), modifying the tests to handle the case where string and StringSegment now differ
HowardvanRooijen commented 2 years ago

+1 for Option 1 (if it's not being used, plus it was on the backlog for upgrading to Span as part of the modernisation efforts).

HowardvanRooijen commented 2 years ago

Option 5. Don't upgrade to .NET 6.0, and move into the "Museum" folder. ✅