Correct redaction of Array.Empty<char>() and new char[0] #4309
Conversation
ghost
assigned 
| @@ -181,6 +186,11 @@ public int Redact<T>(T value, Span<char> destination, string? format = null, IFo | |||
| return Redact(((IFormattable)value).ToString(format, provider), destination); | |||
| } | |||
|
|
|||
| if (value is char[] arrayOfChar) | |||
There was a problem hiding this comment.
I'm not sure whether this is semantically correct behavior. Why should a char array have special treatment over any other kind of array?
I think the test passing an empty array of char may need to be fixed instead.
There was a problem hiding this comment.
Because the redactor works with sequence of chars or strings represented as ReadOnlySpan<char>:
Converting an array of char to a string is a wasteful allocation because at the next steps the new string is converted back to a span.
Besides, when a user passes Array.Empty<char> or new char[0], the user expects the work to be performed on essentially the empty sequence, and not on the string System.Char[].
There was a problem hiding this comment.
The real issue is the overload resolution. The test is passing a char[] and that's binding to the generic version of the call instead of binding to the overload that takes a ReadOnlySpan. We didn't use to have a generic version of the call, so the test would bind to the ReadOnlySpan overload and things worked.
There was a problem hiding this comment.
Probably this usage is niche, I am not sure if we need this optimization. I don't think it is wrong though, we already have typechecks for iformattable, and it looks the same IMO.
There was a problem hiding this comment.
OK, then let's keep it.
@RussKie, please update the code to not use pattern matching and use the same pattern as the if checks for IFormattable and ISpanFormattable above. This pattern is recognized by the JIT and since this is a generic type, the JIT ends up generating code without any of those conditional statements being present. But this only happens when not using pattern matching.
Also, the same change needs to be applied to TryRedact.
There was a problem hiding this comment.
We can't directly cast, and have to do it via an object cast
There was a problem hiding this comment.
We may want to rename Redact<T> to RedactObject<T> to avoid the overload resolution problem.
Looking at this code, I'm also wondering why we don't have other TryRedact overloads to mirror the Redact overloads. Seems like an oversight...
@rafal-mz thought?
| @@ -31,38 +31,38 @@ public static void Redaction_Extensions_Return_Zero_On_Null_Input_Value() | |||
| [InlineData(10000)] | |||
| public static void User_Can_Get_String_From_IRedactor_Using_Extension_Method_With_Different_Input_Length(int length) | |||
| { | |||
| var data = new string('*', length); | |||
| string data = new string('*', length); | |||
There was a problem hiding this comment.
In many .NET repos we don't use var for integral types and when the type isn't obvious, see https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md points 10 and 11.
I rolled back this specific one.
| @@ -181,6 +186,11 @@ public int Redact<T>(T value, Span<char> destination, string? format = null, IFo | |||
| return Redact(((IFormattable)value).ToString(format, provider), destination); | |||
| } | |||
|
|
|||
| if (value is char[] arrayOfChar) | |||
There was a problem hiding this comment.
Probably this usage is niche, I am not sure if we need this optimization. I don't think it is wrong though, we already have typechecks for iformattable, and it looks the same IMO.
|
@rafal-mz What about adding more TryRedact overloads to cover the full set? Do you think that would make sense? |
RussKie
force-pushed
the
fix_4308_redaction
branch
from
6e79a07
to
b273601
Compare
RussKie
force-pushed
the
fix_4308_redaction
branch
from
30667e0
to
ef09b17
Compare
|
Ready for another round of review. |
|
Thank you |
Fixes #4308
Microsoft Reviewers: Open in CodeFlow