`RedisValue.ToString()` behavior changed

[WeihanLi]

In 2.5.61 and previous versions, the StackExchange.Redis.RedisValue.Null.ToString() would return null

In 2.6.45 and the following versions, it would return string.Empty

dotnet-exec 'StackExchange.Redis.RedisValue.Null.ToString()' --reference "nuget:StackExchange.Redis,2.5.61"

dotnet-exec 'StackExchange.Redis.RedisValue.Null.ToString()' --reference "nuget:StackExchange.Redis,2.7.17"

It changed when the nullable reference type enabled

https://github.com/StackExchange/StackExchange.Redis/pull/2041/files#diff-9db71f7058393a2a3ebe750c4be6b20ebe6a2e9dc9f87a9c4d3a7f4b823b2147R251

[mgravell]

This gets into a philosophical point about which is preferable. The ToString() contract is string, not string?, so: we were historically in error, and that has been fixed. There are other ways of testing for null, and ToString() is never a good approach, since the value might be raw non-text bytes. I think this is a reasonable compromise.

[WeihanLi]

The ToString() contract is string, not string?

The ToString() is returning a string? from the base method, see https://github.com/dotnet/runtime/blob/main/src%2Flibraries%2FSystem.Private.CoreLib%2Fsrc%2FSystem%2FObject.cs#L39

[mgravell]

Curious. I have distinct recollections of that being string at various points inside the NRT timescale. Ok, I won't rule this out prematurely, then! Maybe reverting it will elicit which runtimes are unhappy.

[WeihanLi]

From my finding, it's string? since .NET Core 3.0.0, nullable reference types introduced in C# 8 starting from .NET Core 3.0.

Maybe string for netfx/netstandard/..., I do not think nullable reference types are enabled for netfx/netstandard.

Some source code as I found:

For netfx

code from reference source: https://referencesource.microsoft.com/#mscorlib/system/object.cs,40

For .NET Core

• https://github.com/dotnet/coreclr/blob/v3.0.0/src/System.Private.CoreLib/shared/System/Object.cs#L38
• https://github.com/dotnet/coreclr/blob/v3.1.0/src/System.Private.CoreLib/shared/System/Object.cs#L38
• https://github.com/dotnet/runtime/blob/v5.0.0/src/libraries/System.Private.CoreLib/src/System/Object.cs
• https://github.com/dotnet/runtime/blob/v6.0.0/src/libraries/System.Private.CoreLib/src/System/Object.cs
• https://github.com/dotnet/runtime/blob/v7.0.0/src/libraries/System.Private.CoreLib/src/System/Object.cs
• https://github.com/dotnet/runtime/blob/v8.0.0/src/libraries/System.Private.CoreLib/src/System/Object.cs

since there are some versions for the change, maybe it's not worth a revert, also kind of breaking change.

Fine with it now, just need some changes for the ToString() usage

[NickCraver]

I remember talking to the .NET team back when we did this and the overall message was "We know we had to make the contract string? after a lot of debate and examples in the BCL, but please do not return null if at all avoidable", and I went with that advice.

[mgravell]

The other consideration here: changing it back is also a change. Which prompts the question: in what valid scenario is this presenting a meaningful problem? The output of ToString() isn't usually intended for regular use, and in most scenarios: null and "" are effectively equivalent. As I said earlier: there are other (preferable) ways for checking for true null-ness.

[mgravell]

Or to put that more tersely, does anything make the acknowledgement "yes, it did" an insufficient reply here?

[WeihanLi]

I used the ToString() method instead of the string implicit convert in some places, think I should update the ToString() method usage with the string implicit convert

[WeihanLi]

In some cases, we treat null and string.Empty as different results, null means not exist, string.Empty means exists and the value is string.Empty

[WeihanLi]

The output of ToString() isn't usually intended for regular use, and in most scenarios: null and "" are effectively equivalent.

Does this mean that the breaking changes of ToString() may be acceptable for a non-major changes

[mgravell]

In some cases, we treat null and string.Empty as different results, null means not exist, string.Empty means exists and the value is string.Empty

and I'm saying "don't do that; that's not the right way to check for null/exists"

Does this mean that the breaking changes of ToString() may be acceptable for a non-major changes

I reject the premise of "breaking" - it is a change, but the usage where it is showing a difference is arguably / subjectively not a valid scenario; you simply shouldn't use ToString() for that purpose

[WeihanLi]

Get it, would update the ToString() usage in my code, thanks very much