Code Review Feedback

* Move additional `ReadOnlySpan<byte>` `ParseContext` overloads next to
the existing overload
* Pass `ReadOnlySpan<byte>` parameters by `ref`
* Update `MessageParsingHelpers` to add checks for parsing from
`ReadOnlySpan<byte>`, remove tests from `CodedInputStreamTest`

csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs

@@ -926,39 +926,6 @@ namespace Google.Protobuf
             }
         }
 
-        [Test]
-        public void TestParseFromSpan()
-        {
-            var testMessage = new TestAllTypes
-            {
-                RepeatedBool = { true, false },
-                SingleDouble = 42.0d,
-                SingleString = "testing"
-            };
-
-            ReadOnlySpan<byte> messagesBytes = testMessage.ToByteArray().AsSpan();
-            TestAllTypes parsedMessage = TestAllTypes.Parser.ParseFrom(messagesBytes);
-
-            Assert.AreEqual(testMessage, parsedMessage);
-        }
-
-        [Test]
-        public void TestMergeFromSpan()
-        {
-            var testMessage = new TestAllTypes
-            {
-                RepeatedBool = { true, false },
-                SingleDouble = 42.0d,
-                SingleString = "testing"
-            };
-
-            ReadOnlySpan<byte> messagesBytes = testMessage.ToByteArray().AsSpan();
-            var mergedMessage = new TestAllTypes();
-            mergedMessage.MergeFrom(messagesBytes);
-
-            Assert.AreEqual(testMessage, mergedMessage);
-        }
-
         /// <returns>A serialized big message</returns>
         private static byte[] GenerateBigSerializedMessage()
         {

csharp/src/Google.Protobuf.Test/MessageParsingHelpers.cs

@@ -41,32 +41,40 @@ namespace Google.Protobuf
     {
         public static void AssertReadingMessage<T>(MessageParser<T> parser, byte[] bytes, Action<T> assert) where T : IMessage<T>
         {
-            var parsedStream = parser.ParseFrom(bytes);
+            var parsedBuffer = parser.ParseFrom(bytes);
+            assert(parsedBuffer);
 
             // Load content as single segment
-            var parsedBuffer = parser.ParseFrom(new ReadOnlySequence<byte>(bytes));
+            parsedBuffer = parser.ParseFrom(new ReadOnlySequence<byte>(bytes));
             assert(parsedBuffer);
 
             // Load content as multiple segments
             parsedBuffer = parser.ParseFrom(ReadOnlySequenceFactory.CreateWithContent(bytes));
             assert(parsedBuffer);
 
-            assert(parsedStream);
+            // Load content as ReadOnlySpan
+            ReadOnlySpan<byte> bytesSpan = bytes.AsSpan();
+            parsedBuffer = parser.ParseFrom(ref bytesSpan);
+            assert(parsedBuffer);
         }
 
         public static void AssertReadingMessage(MessageParser parser, byte[] bytes, Action<IMessage> assert)
         {
-            var parsedStream = parser.ParseFrom(bytes);
+            var parsedBuffer = parser.ParseFrom(bytes);
+            assert(parsedBuffer);
 
             // Load content as single segment
-            var parsedBuffer = parser.ParseFrom(new ReadOnlySequence<byte>(bytes));
+            parsedBuffer = parser.ParseFrom(new ReadOnlySequence<byte>(bytes));
             assert(parsedBuffer);
 
             // Load content as multiple segments
             parsedBuffer = parser.ParseFrom(ReadOnlySequenceFactory.CreateWithContent(bytes));
             assert(parsedBuffer);
 
-            assert(parsedStream);
+            // Load content as ReadOnlySpan
+            ReadOnlySpan<byte> bytesSpan = bytes.AsSpan();
+            parsedBuffer = parser.ParseFrom(ref bytesSpan);
+            assert(parsedBuffer);
         }
 
         public static void AssertReadingMessageThrows<TMessage, TException>(MessageParser<TMessage> parser, byte[] bytes)
@@ -76,6 +84,12 @@ namespace Google.Protobuf
             Assert.Throws<TException>(() => parser.ParseFrom(bytes));
 
             Assert.Throws<TException>(() => parser.ParseFrom(new ReadOnlySequence<byte>(bytes)));
+
+            Assert.Throws<TException>(() => 
+              {
+                ReadOnlySpan<byte> bytesSpan = bytes.AsSpan();
+                parser.ParseFrom(ref bytesSpan);
+              });
         }
 
         public static void AssertRoundtrip<T>(MessageParser<T> parser, T message, Action<T> additionalAssert = null) where T : IMessage<T>
@@ -87,8 +101,12 @@ namespace Google.Protobuf
             message.WriteTo(bufferWriter);
             Assert.AreEqual(bytes, bufferWriter.WrittenSpan.ToArray(), "Both serialization approaches need to result in the same data.");
 
+            var parsedBuffer = parser.ParseFrom(bytes);
+            Assert.AreEqual(message, parsedBuffer);
+            additionalAssert?.Invoke(parsedBuffer);
+
             // Load content as single segment
-            var parsedBuffer = parser.ParseFrom(new ReadOnlySequence<byte>(bytes));
+            parsedBuffer = parser.ParseFrom(new ReadOnlySequence<byte>(bytes));
             Assert.AreEqual(message, parsedBuffer);
             additionalAssert?.Invoke(parsedBuffer);
 
@@ -97,10 +115,11 @@ namespace Google.Protobuf
             Assert.AreEqual(message, parsedBuffer);
             additionalAssert?.Invoke(parsedBuffer);
 
-            var parsedStream = parser.ParseFrom(bytes);
-
-            Assert.AreEqual(message, parsedStream);
-            additionalAssert?.Invoke(parsedStream);
+            // Load content as ReadOnlySpan
+            ReadOnlySpan<byte> bytesSpan = bytes.AsSpan();
+            parsedBuffer = parser.ParseFrom(ref bytesSpan);
+            Assert.AreEqual(message, parsedBuffer);
+            additionalAssert?.Invoke(parsedBuffer);
         }
 
         public static void AssertWritingMessage(IMessage message)

csharp/src/Google.Protobuf/MessageExtensions.cs

@@ -85,8 +85,8 @@ namespace Google.Protobuf
         /// <param name="message">The message to merge the data into.</param>
         /// <param name="span">Span containing the data to merge, which must be protobuf-encoded binary data.</param>
         [SecuritySafeCritical]
-        public static void MergeFrom(this IMessage message, ReadOnlySpan<byte> span) =>
-            MergeFrom(message, span, false, null);
+        public static void MergeFrom(this IMessage message, ref ReadOnlySpan<byte> span) =>
+            MergeFrom(message, ref span, false, null);
 
         /// <summary>
         /// Merges length-delimited data from the given stream into an existing message.
@@ -304,9 +304,9 @@ namespace Google.Protobuf
         }
 
         [SecuritySafeCritical]
-        internal static void MergeFrom(this IMessage message, ReadOnlySpan<byte> data, bool discardUnknownFields, ExtensionRegistry registry)
+        internal static void MergeFrom(this IMessage message, ref ReadOnlySpan<byte> data, bool discardUnknownFields, ExtensionRegistry registry)
         {
-            ParseContext.Initialize(data, out ParseContext ctx);
+            ParseContext.Initialize(ref data, out ParseContext ctx);
             ctx.DiscardUnknownFields = discardUnknownFields;
             ctx.ExtensionRegistry = registry;
             ParsingPrimitivesMessages.ReadRawMessage(ref ctx, message);

csharp/src/Google.Protobuf/MessageParser.cs

@@ -134,10 +134,10 @@ namespace Google.Protobuf
         /// <param name="data">The data to parse.</param>
         /// <returns>The parsed message.</returns>
         [SecuritySafeCritical]
-        public IMessage ParseFrom(ReadOnlySpan<byte> data)
+        public IMessage ParseFrom(ref ReadOnlySpan<byte> data)
         {
             IMessage message = factory();
-            message.MergeFrom(data, DiscardUnknownFields, Extensions);
+            message.MergeFrom(ref data, DiscardUnknownFields, Extensions);
             return message;
         }
 
@@ -334,10 +334,10 @@ namespace Google.Protobuf
         /// <param name="data">The data to parse.</param>
         /// <returns>The parsed message.</returns>
         [SecuritySafeCritical]
-        public new T ParseFrom(ReadOnlySpan<byte> data)
+        public new T ParseFrom(ref ReadOnlySpan<byte> data)
         {
             T message = factory();
-            message.MergeFrom(data, DiscardUnknownFields, Extensions);
+            message.MergeFrom(ref data, DiscardUnknownFields, Extensions);
             return message;
         }
 

csharp/src/Google.Protobuf/ParseContext.cs

@@ -65,6 +65,29 @@ namespace Google.Protobuf
             ctx.state = state;
         }
 
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        internal static void Initialize(ref ReadOnlySpan<byte> input, out ParseContext ctx)
+        {
+            Initialize(ref input, DefaultRecursionLimit, out ctx);
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        internal static void Initialize(ref ReadOnlySpan<byte> input, int recursionLimit, out ParseContext ctx)
+        {
+            ctx.buffer = input;
+            ctx.state = default;
+            ctx.state.lastTag = 0;
+            ctx.state.recursionDepth = 0;
+            ctx.state.sizeLimit = DefaultSizeLimit;
+            ctx.state.recursionLimit = recursionLimit;
+            ctx.state.currentLimit = int.MaxValue;
+            ctx.state.bufferPos = 0;
+            ctx.state.bufferSize = input.Length;
+
+            ctx.state.DiscardUnknownFields = false;
+            ctx.state.ExtensionRegistry = null;
+        }
+
         /// <summary>
         /// Creates a ParseContext instance from CodedInputStream.
         /// WARNING: internally this copies the CodedInputStream's state, so after done with the ParseContext,
@@ -104,29 +127,6 @@ namespace Google.Protobuf
             ctx.state.ExtensionRegistry = null;
         }
 
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal static void Initialize(ReadOnlySpan<byte> input, out ParseContext ctx)
-        {
-            Initialize(input, DefaultRecursionLimit, out ctx);
-        }
-
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        internal static void Initialize(ReadOnlySpan<byte> input, int recursionLimit, out ParseContext ctx)
-        {
-            ctx.buffer = input;
-            ctx.state = default;
-            ctx.state.lastTag = 0;
-            ctx.state.recursionDepth = 0;
-            ctx.state.sizeLimit = DefaultSizeLimit;
-            ctx.state.recursionLimit = recursionLimit;
-            ctx.state.currentLimit = int.MaxValue;
-            ctx.state.bufferPos = 0;
-            ctx.state.bufferSize = input.Length;
-
-            ctx.state.DiscardUnknownFields = false;
-            ctx.state.ExtensionRegistry = null;
-        }
-
         /// <summary>
         /// Returns the last tag read, or 0 if no tags have been read or we've read beyond
         /// the end of the input.