Revert changes to EncoderNLS/DecoderNLS.Convert (#752)
authorLevi Broderick <GrabYourPitchforks@users.noreply.github.com>
Fri, 27 Dec 2019 18:37:16 +0000 (13:37 -0500)
committerJan Kotas <jkotas@microsoft.com>
Fri, 27 Dec 2019 18:37:16 +0000 (10:37 -0800)
src/libraries/System.Memory/tests/EncodingExtensions/EncodingExtensionsTests.cs
src/libraries/System.Private.CoreLib/src/System/Text/DecoderNLS.cs
src/libraries/System.Private.CoreLib/src/System/Text/EncoderNLS.cs
src/libraries/System.Text.Encoding/tests/Decoder/DecoderConvert2.cs
src/libraries/System.Text.Encoding/tests/Encoder/EncoderConvert2.cs

index 3760ca3..24a7918 100644 (file)
@@ -85,7 +85,7 @@ namespace System.Text.Tests
             inputData = Encoding.UTF8.GetBytes(new string('x', 20_000_000)).Concat(new byte[] { 0xE0, 0xA0 }).ToArray();
             EncodingExtensions.Convert(decoder, inputData, writer, flush: false, out charsUsed, out completed);
             Assert.Equal(20_000_000, charsUsed);
-            Assert.False(completed);
+            Assert.True(completed);
 
             // Then, a large input with flushing and leftover data (should be replaced).
 
@@ -133,7 +133,7 @@ namespace System.Text.Tests
                 new byte[] { 0xF4, 0x80 }); // U+100000 (continues on next line)
             EncodingExtensions.Convert(decoder, inputData, writer, flush: false, out charsUsed, out completed);
             Assert.Equal(0, charsUsed);
-            Assert.False(completed);
+            Assert.True(completed);
 
             // Then, input with flushing and leftover data (should be replaced).
 
@@ -177,7 +177,7 @@ namespace System.Text.Tests
             inputData = new string('x', 20_000_000) + '\ud800';
             EncodingExtensions.Convert(encoder, inputData, writer, flush: false, out bytesUsed, out completed);
             Assert.Equal(20_000_000, bytesUsed);
-            Assert.False(completed);
+            Assert.True(completed);
 
             // Then, a large input with flushing and leftover data (should be replaced).
 
@@ -224,7 +224,7 @@ namespace System.Text.Tests
                 new char[] { '\udbc0' }); // U+100000 (continues on next line)
             EncodingExtensions.Convert(encoder, inputData, writer, flush: false, out bytesUsed, out completed);
             Assert.Equal(0, bytesUsed);
-            Assert.False(completed);
+            Assert.True(completed);
 
             // Then, input with flushing and leftover data (should be replaced).
 
index 49aa166..61dfd38 100644 (file)
@@ -207,12 +207,10 @@ namespace System.Text
             charsUsed = _encoding.GetChars(bytes, byteCount, chars, charCount, this);
             bytesUsed = _bytesUsed;
 
-            // Per MSDN, "The completed output parameter indicates whether all the data in the input
-            // buffer was converted and stored in the output buffer." That means we've successfully
-            // consumed all the input _and_ there's no pending state or fallback data remaining to be output.
+            // See comment in EncoderNLS.Convert for the details of the logic below.
 
             completed = (bytesUsed == byteCount)
-                && !this.HasState
+                && (!flush || !this.HasState)
                 && (_fallbackBuffer is null || _fallbackBuffer.Remaining == 0);
         }
 
index 5c813dc..5135bc0 100644 (file)
@@ -194,12 +194,39 @@ namespace System.Text
             bytesUsed = _encoding.GetBytes(chars, charCount, bytes, byteCount, this);
             charsUsed = _charsUsed;
 
-            // Per MSDN, "The completed output parameter indicates whether all the data in the input
-            // buffer was converted and stored in the output buffer." That means we've successfully
-            // consumed all the input _and_ there's no pending state or fallback data remaining to be output.
+            // If the 'completed' out parameter is set to false, it means one of two things:
+            // a) this call to Convert did not consume the entire source buffer; or
+            // b) this call to Convert did consume the entire source buffer, but there's
+            //    still pending data that needs to be written to the destination buffer.
+            //
+            // In either case, the caller should slice the input buffer, provide a fresh
+            // destination buffer, and call Convert again in a loop until 'completed' is true.
+            //
+            // The caller *must* specify flush = true on the final iteration(s) of the loop
+            // and iterate until 'completed' is set to true. Otherwise data loss may occur.
+            //
+            // Technically, the expected logic is detailed below.
+            //
+            // If 'flush' = false, the 'completed' parameter MUST be set to false if not all
+            // elements of the source buffer have been consumed. The 'completed' parameter MUST
+            // be set to true once the entire source buffer has been consumed and there is no
+            // pending data for the destination buffer. (In other words, the 'completed' parameter
+            // MUST be set to true if passing a zero-length source buffer and an infinite-length
+            // destination buffer will make no forward progress.) The 'completed' parameter value
+            // is undefined for the case where all source data has been consumed but there remains
+            // pending data for the destination buffer.
+            //
+            // If 'flush' = true, the 'completed' parameter is set to true IF AND ONLY IF:
+            // a) all elements of the source buffer have been transcoded into the destination buffer; AND
+            // b) there remains no internal partial read state within this instance; AND
+            // c) there remains no pending data for the destination buffer.
+            //
+            // In other words, if 'flush' = true, then when 'completed' is set to true it should mean
+            // that all data has been converted and that this instance is indistinguishable from a
+            // freshly-reset instance.
 
             completed = (charsUsed == charCount)
-                && !this.HasState
+                && (!flush || !this.HasState)
                 && (_fallbackBuffer is null || _fallbackBuffer.Remaining == 0);
         }
 
index 9d87dbb..b2b7d06 100644 (file)
@@ -165,7 +165,7 @@ namespace System.Text.Tests
             }
 
             Decoder decoder = Encoding.Unicode.GetDecoder();
-            VerificationHelper(decoder, bytes, 0, bytes.Length, chars, 0, chars.Length, false, bytes.Length, chars.Length / 2, false, "006.1");
+            VerificationHelper(decoder, bytes, 0, bytes.Length, chars, 0, chars.Length, false, bytes.Length, chars.Length / 2, true, "006.1");
             decoder.Reset();
             // There will be 1 byte left unconverted after previous statement, and set flush = false should left this 1 byte in the buffer.
             VerificationHelper(decoder, bytes, 0, bytes.Length, chars, 0, chars.Length, true, bytes.Length, chars.Length / 2 + 1, true, "006.2");
@@ -288,8 +288,8 @@ namespace System.Text.Tests
             char[] chars = new char[32];
             Decoder decoder = Encoding.UTF8.GetDecoder();
 
-            VerificationHelper(decoder, bytes, 0, 1, chars, 0, 10, false, 1, 0, false, "011.1"); // incomplete since still state in Decoder
-            VerificationHelper(decoder, bytes, 1, 2, chars, 0, 10, false, 2, 1, false, "011.2"); // incomplete since still state in Decoder
+            VerificationHelper(decoder, bytes, 0, 1, chars, 0, 10, false, 1, 0, true, "011.1"); // complete since all source bytes consumed and no pending data for destination
+            VerificationHelper(decoder, bytes, 1, 2, chars, 0, 10, false, 2, 1, true, "011.2"); // complete since all source bytes consumed and no pending data for destination
             VerificationHelper(decoder, bytes, 3, 6, chars, 1, 1, false, 2, 1, false, "011.3"); // incomplete since not all bytes consumed
             VerificationHelper(decoder, bytes, 5, 4, chars, 2, 10, false, 4, 2, true, "011.4"); // complete since all bytes consumed and no leftover state
             VerificationHelper(decoder, bytes, 9, 1, chars, 4, 10, true, 1, 1, true, "011.5"); // complete since all bytes consumed and no leftover state
index 7236db1..2844e91 100644 (file)
@@ -224,7 +224,7 @@ namespace System.Text.Tests
             VerificationHelper(encoder, chars, 1, 3, bytes, 1, 3, false, 1, 3, expectedCompleted: false);
             VerificationHelper(encoder, chars, 1, 3, bytes, 1, 5, true, 3, 5, expectedCompleted: true);
 
-            VerificationHelper(encoder, chars, 0, 1, bytes, 0, bytes.Length, false, 1, 0, expectedCompleted: false);
+            VerificationHelper(encoder, chars, 0, 1, bytes, 0, bytes.Length, false, 1, 0, expectedCompleted: true);
             VerificationHelper(encoder, chars, 1, 1, bytes, 0, bytes.Length, true, 1, 4, expectedCompleted: true);
         }
 
@@ -259,7 +259,7 @@ namespace System.Text.Tests
             Encoder encoder = Encoding.UTF8.GetEncoder();
 
             VerificationHelper(encoder, chars, 0, 1, bytes, 0, 1, true, 1, 1, expectedCompleted: true);
-            VerificationHelper(encoder, chars, 0, 2, bytes, 0, 1, false, 2, 1, expectedCompleted: false);
+            VerificationHelper(encoder, chars, 0, 2, bytes, 0, 1, false, 2, 1, expectedCompleted: true);
             VerificationHelper(encoder, chars, 2, 1, bytes, 0, 5, false, 1, 4, expectedCompleted: true);
             VerificationHelper(encoder, chars, 1, 2, bytes, 0, 7, false, 2, 4, expectedCompleted: true);
             VerificationHelper(encoder, chars, 0, 5, bytes, 0, 7, false, 5, 7, expectedCompleted: true);
@@ -268,7 +268,7 @@ namespace System.Text.Tests
             VerificationHelper(encoder, chars, 1, 3, bytes, 1, 5, false, 3, 5, expectedCompleted: true);
             VerificationHelper(encoder, chars, 1, 3, bytes, 1, 5, true, 3, 5, expectedCompleted: true);
 
-            VerificationHelper(encoder, chars, 0, 2, bytes, 0, bytes.Length, false, 2, 1, expectedCompleted: false);
+            VerificationHelper(encoder, chars, 0, 2, bytes, 0, bytes.Length, false, 2, 1, expectedCompleted: true);
             VerificationHelper(encoder, chars, 2, 2, bytes, 0, bytes.Length, false, 2, 5, expectedCompleted: true);
             VerificationHelper(encoder, chars, 1, 1, bytes, 0, bytes.Length, true, 1, 3, expectedCompleted: true);
         }