Override all Read methods in NullTextReader and NullStreamReader

[bgrainger]

This is a fast-follow to #61898 (comment):

There are a few other TextReader-derived types in dotnet/runtime, e.g. a couple in System.Console, and NullTextReader in corelib. We should likely override the new overloads on those as well. (I'm not sure why NullTextReader doesn't already override most of the virtuals, but it seems it should, for perf.) Same goes for NullStreamReader.

CC @adamsitnik

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is a fast-follow to #61898 (comment):

There are a few other TextReader-derived types in dotnet/runtime, e.g. a couple in System.Console, and NullTextReader in corelib. We should likely override the new overloads on those as well. (I'm not sure why NullTextReader doesn't already override most of the virtuals, but it seems it should, for perf.) Same goes for NullStreamReader.

CC @adamsitnik

Author:	bgrainger
Assignees:	-
Labels:	area-System.IO, community-contribution
Milestone:	-

[stephentoub]

I wonder if we even need NullTextReader? Seems like public static readonly TextReader Null = new NullTextReader(); could be changed to instead construct a NullStreamReader. It's possible there's some reason I'm missing not to do that, but it'd be nice to avoid the duplication if possible. @adamsitnik, thoughts?

[bgrainger]

I wondered this, too, as I refactored the classes to be almost identical; the only thing I can think of changing/breaking is code that (essentially) performs TextReader.Null is TextReader and not StreamReader (or similar switch/case or if/else).

If setting TextReader.Null to a NullStreamReader (and deleting NullTextReader) is desirable, let me know and I'm happy to make the change.

[adamsitnik]

The scenario provided by @bgrainger is valid, but I hope that nobody does that.

I've taken a look at how these two properties are typically used and I think that it's relatively safe to reduce the code duplication and remove NullTextReader.

[adamsitnik]

Thank you for your contribution @bgrainger ! PTAL at my comments.

[adamsitnik]

The scenario provided by @bgrainger is valid, but I hope that nobody does that.

I've taken a look at how these two properties are typically used and I think that it's relatively safe to reduce the code duplication and remove NullTextReader.

[adamsitnik]

Overall the code looks good, but for some reason the tests are failing on mono (the CI has not finished so I am not 100% sure that it's mono bug):

  Discovering: System.IO.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.IO.Tests (found 694 of 700 test cases)
  Starting:    System.IO.Tests (parallel test collections = on, max threads = 6)
    System.IO.Tests.NullTests.TestCanceledNullTextReaderAsync(input: null) [FAIL]
      Assert.Throws() Failure
      Expected: typeof(System.OperationCanceledException)
      Actual:   typeof(System.NullReferenceException): Object reference not set to an instance of an object
      ---- System.NullReferenceException : Object reference not set to an instance of an object
      Stack Trace:
        /_/src/libraries/System.IO/tests/Stream/Stream.NullTests.cs(184,0): at System.IO.Tests.NullTests.<>c__DisplayClass11_0.<<TestCanceledNullTextReaderAsync>b__0>d.MoveNext()
        --- End of stack trace from previous location ---
        ----- Inner Stack Trace -----
        /_/src/libraries/System.IO/tests/Stream/Stream.NullTests.cs(184,0): at System.IO.Tests.NullTests.<>c__DisplayClass11_0.<<TestCanceledNullTextReaderAsync>b__0>d.MoveNext()
        --- End of stack trace from previous location ---
      Assert.Throws() Failure

Is there any chance you could build it and try to repro locally? https://github.com/dotnet/runtime/blob/main/docs/workflow/building/mono/README.md

I have no idea why it fails only on Mono, but I guess it's caused by the fact that two out of three arugments provided by NullReaders refer to the same object instance and somewhere in our stack (not in your PR) there is a bug.

runtime/src/libraries/System.IO/tests/Stream/Stream.NullTests.cs

Lines 197 to 203 in 57bfe47

	public static IEnumerable<object[]> NullReaders
	{
	get
	{
	yield return new object[] { TextReader.Null };
	yield return new object[] { StreamReader.Null };
	yield return new object[] { StringReader.Null };

[bgrainger]

I believe it's actually 3 out of 3 now. StringReader.Null always inherited from the base class, so it was the same object as TextReader.Null. But now they all are the same object.

I could throw on a .Distinct() to ensure that all the properties are read, but that only unique objects are tested, but I'm not sure if that would stop null being passed as the input to the test method.

[bgrainger]

Just had a thought that perhaps this is a static field initialisation problem (only on mono)? If the base class static fields have to be initialised before the derived class, then TextReader.Null would be set to null, which would cause the test failure. The fix would be to make NullStreamReader internal and simply construct a second instance in TextReader.Null. (StreamReader.Null technically wouldn't need to be a second instance, but could be = (StreamReader) TextReader.Null but that feels unnecessarily brittle to me.)

[bgrainger]

I could throw on a .Distinct()

That didn't fix the broken builds; I'll move ahead with the static field theory. (The Distinct() is still useful to avoid unnecessary testing.)

[bgrainger]

Changing the static field initialisation has resolved the "Libraries Test Run release mono" build failures.

[adamsitnik]

LGTM, thank you @bgrainger !