EncoderNLS.Convert doesn't always out the correct value for 'completed'

[GrabYourPitchforks]

Repro code:

var encoding = Encoding.GetEncoding("us-ascii", new EncoderReplacementFallback("hello"), DecoderFallback.ExceptionFallback);
var encoder = encoding.GetEncoder();

encoder.Convert(new[] { '\ud800' }, 0, 1, new byte[2], 0, 2, flush: false, out int charsUsed, out int bytesUsed, out bool completed);

Console.WriteLine($"charsUsed: {charsUsed}");
Console.WriteLine($"bytesUsed: {bytesUsed}");
Console.WriteLine($"completed: {completed}");

Expected output:

charsUsed: 1
bytesUsed: 0
completed: False

Actual output:

charsUsed: 1
bytesUsed: 0
completed: True

The culprit seems to be the line below.

https://github.com/dotnet/coreclr/blob/77fcaf6b738941a0c5dc3c00b70b49c7d9f63b69/src/System.Private.CoreLib/shared/System/Text/EncoderNLS.cs#L201

The values flush and this.HasState can never both be true at the same time because specifying flush = true mandates that the EncoderNLS instance not store any remaining high surrogate character for future invocations. Since this means (flush && this.HasState) == false always, by DeMorgan's theorem we have (!flush || !this.HasState) == true always, which means this clause isn't actually contributing to the final value for completed.

I believe the appropriate fix is to ignore the flush parameter entirely and to look only at three conditions: (a) all chars were consumed, (b) there's no leftover state on the EncoderNLS instance, and (c) there's no leftover state in the fallback buffer.

[GrabYourPitchforks]

/cc @tarekgh @layomia as FYI.

[tarekgh]

@GrabYourPitchforks Thanks for reporting it. I am wondering if you'll include the fix of this in your changes? Just asking to know if we need to work on it especially you marked it as 3.0 issue.

[GrabYourPitchforks]

Yes, I have this fixed in my branch. Just opened this for tracking purposes.

[GrabYourPitchforks]

This was fixed in dotnet/coreclr@43a5159.