StringValues implicit conversion to string should match ToString()

Description

Signature of static implicit operator string? (StringValues values) does not match public override string ToString() in the same struct. This may be due to a long-standing bug in the original StringValues implementation which could return null. We never handled that case higher in the stack i.e. we aren't expecting null from the implicit conversion.

Result in [main] Update dependencies from dotnet/efcore dotnet/runtime by dotnet-maestro · Pull Request #35547 · dotnet/aspnetcore (github.com) will be a lot of redundant-seeming .ToString() calls. That will likely result in needless attempts to remove them later because we all "know" implicit conversions to string and ToString() are semantically identical.

Please update https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.Extensions.Primitives/src/StringValues.cs#L72-L75 to use ToString() and return string. I can submit a PR if agreed.

Configuration

I'm currently building with Microsoft.Extensions.Primitives v7.0.0-alpha.1.21425.1 but the PR has probably moved on from that. Not seeing a change in the StringValues code however.

Problem is new in 7.0.0 but otherwise not specific to a configuration.

Regression?

Kind-of. We didn't need the ToString() calls in existing ASP.NET Core code.

Other information

Work around is to add ToString() but the number of calls required is very quickly piling up as I work on [main] Update dependencies from dotnet/efcore dotnet/runtime by dotnet-maestro · Pull Request #35547 · dotnet/aspnetcore (github.com)](dotnet/aspnetcore#35547).

@maxkoshevoi

/cc @maxkoshevoi

Spicy take: we should consider obsoleting the implicit conversion operator and providing a fixer that rewrites the call sites to an explicit ToArray or ToString invocation.

StringValues sv = GetStringValues();
string s1 = sv; // Is this an O(1) call? O(n)? Who knows!
string s2 = sv; // Another O(n) call? Spin the wheel and find out!

StringValues sv = GetStringValues();
string s1 = sv.ToString(); // caller is explicitly aware that this can allocate a string instance
string s2 = sv.ToString(); // caller is explicitly aware that this may duplicate the work done by the line above

@GrabYourPitchforks

You want to make the problem worse @GrabYourPitchforks❔ The implicit operator and ToString() are of the same complexity today and many implicit operators allocate.

and many implicit operators allocate.

Implicit operators should not be performing work beyond complexity O(1). I would consider any implicit operator which does "complex" work to be a bug or a pit of failure that should be corrected, preferably through obsoletion as suggested here.

Obsoleting the implicit operator would impact more call sites in dotnet/aspnetcore and would seem to be worse than handling the null case w/in the implicit operator. Maybe [Obsolete] next week and fix this issue to unblock dependency flow❔

(Personally, I consider implicit conversions to string as shorthand for ToString() and expect either may allocate in some cases for some types.)

The annotation on the implicit conversion is reflecting what the implementation already does; it changed nothing about the implementation or whether it could return null or not... it merely informed consumers of the fact that it can return null.

Work around is to add ToString()

The work around for the nullability warning? The work around for the nullability warning is to suppress the warning, e.g. by adding ! to the use. The code didn't change in the implementation; this is just a new warning reflecting the annotation reflecting the actual contract of the existing method.

(That said, I'd much prefer consuming code use ToString() explicitly and that this operator didn't exist, especially with it apparently not working the way folks want it to. I'd consider consuming code changing to use ToString() a positive change rather than a workaround.)

The annotation on the implicit conversion is reflecting what the implementation already does

My suggestion was to change the implementation as well as the annotation. The current implementation can return null but ToString() can't. That discrepancy isn't a feature.

My suggestion was to change the implementation as well as the annotation.

I understand. And I'm suggesting that the way to unblock the dependency flow is to suppress the warnings, not to rush in a change in behavior that hasn't changed.

I'd much prefer consuming code use ToString() explicitly

I'm going w/ this while the conversation in this issue continues. ! would just preserve potential nullability bugs in dotnet/aspnetcore.

Another way to look at this issue: We could fix the potential bugs in one place (the implicit conversion) or in numerous locations all over dotnet/aspnetcore.

We could fix the potential bugs in one place (the implicit conversion)

And potentially introduce bugs for anyone actually depending on null being a valid return value, since as you said, it's been able to do so for a long time. I know you say ASP.NET doesn't depend on that. Maybe others do, I have no idea. So, while that's being discussed, don't block on such a change being made, especially if there's serious discussion about obsoleting the conversion.

The implicit conversion has been explicitly documented as it can return null for almost 2 years:

https://github.com/dotnet/extensions/pull/2780/files#diff-d870217f901c4af40871170d474711d39a0b2a00c7da5fb12f7d88f6843c5f5cR68

runtime/src/libraries/Microsoft.Extensions.Primitives/src/StringValues.cs

Lines 65 to 72 in 7994e59

    
           /// <summary> 
        
           /// Defines an implicit conversion of a given <see cref="StringValues"/> to a string, with multiple values joined as a comma separated string. 
        
           /// </summary> 
        
           /// <remarks> 
        
           /// Returns <c>null</c> where <see cref="StringValues"/> has been initialized from an empty string array or is <see cref="StringValues.Empty"/>. 
        
           /// </remarks> 
        
           /// <param name="values">A <see cref="StringValues"/> to implicitly convert.</param> 
        
           public static implicit operator string? (StringValues values)

Changing this behavior because we actually annotate it correctly doesn't make any sense IMO.

I can get behind obsoleting the implicit conversion though.

I don't think we should obsolete the implicit conversion. This is used pervasively in ASP.NET Core applications today.

PS: I agree with Stephen about unblocking the build.

@davidfowl

What say you @davidfowl about changing the implementation and annotation (and doc comments)❔

Handle more nullable expressions

I think we should close this issue has "won't fix". This is a behavior breaking change that I don't think we are willing to make at this point. This behavior has existed for years without complaints. Now that we've annotated it for nullable doesn't give us a reason to change the behavior.

Also, there are places where ASP.NET code is actually relying on this behavior of returning null from the implicit conversion. Here's one such example:

https://github.com/dotnet/aspnetcore/blob/b849efe77a282c4fd0baf460d68142d89a1b874d/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs#L384

        private static string GetConnectionToken(HttpContext context) => context.Request.Query["id"];

https://github.com/dotnet/aspnetcore/blob/b849efe77a282c4fd0baf460d68142d89a1b874d/src/SignalR/common/Http.Connections/src/Internal/HttpConnectionDispatcher.cs#L67-L71

            var connectionToken = GetConnectionToken(context);
            if (connectionToken != null)
            {
                _manager.TryGetConnection(connectionToken, out connectionContext);
            }

there are places where ASP.NET code is actually relying on this behavior of returning null from the implicit conversion.

Actually, there are no such places e.g. the GetConnectionToken(...) implementation returns a non-nullable string. We just have leftover null checks that were not removed when we annotated our code.

[main] Update dependencies from dotnet/efcore dotnet/runtime (#35547)

@GrabYourPitchforks

We don't want to make the breaking change after we have enabled nullability.

Closing this issue. But if we wanted to obsolete the implicit conversion (recommended by @GrabYourPitchforks) we should log a new issue for that.

Navigation Menu

Repository navigation

`StringValues` implicit conversion to `string` should match `ToString()` #58223

Description

Configuration

Regression?

Other information

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	/// <summary>
	/// Defines an implicit conversion of a given <see cref="StringValues"/> to a string, with multiple values joined as a comma separated string.
	/// </summary>
	/// <remarks>
	/// Returns <c>null</c> where <see cref="StringValues"/> has been initialized from an empty string array or is <see cref="StringValues.Empty"/>.
	/// </remarks>
	/// <param name="values">A <see cref="StringValues"/> to implicitly convert.</param>
	public static implicit operator string? (StringValues values)

StringValues implicit conversion to string should match ToString() #58223

Description

Description

Configuration

Regression?

Other information

Activity

dougbu commented on Aug 27, 2021

GrabYourPitchforks commented on Aug 27, 2021

dougbu commented on Aug 27, 2021

GrabYourPitchforks commented on Aug 27, 2021

dougbu commented on Aug 27, 2021

dougbu commented on Aug 27, 2021

stephentoub commented on Aug 27, 2021

dougbu commented on Aug 27, 2021

stephentoub commented on Aug 27, 2021

dougbu commented on Aug 27, 2021

stephentoub commented on Aug 27, 2021

eerhardt commented on Aug 27, 2021

davidfowl commented on Aug 27, 2021

dougbu commented on Aug 27, 2021

eerhardt commented on Aug 27, 2021

dougbu commented on Aug 27, 2021

maryamariyan commented on Sep 2, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`StringValues` implicit conversion to `string` should match `ToString()` #58223