amazon-ion / ion-dotnet

A .NET implementation of Amazon Ion.
https://amazon-ion.github.io/ion-docs/
Other
56 stars 31 forks source link

Error when MoveNext over a struct or list with text reader #148

Closed CoderNate closed 1 year ago

CoderNate commented 1 year ago

With input text like [{}], the following throws an exception:

var reader = new UserTextReader("[{}]");
Assert.AreEqual(IonType.List, reader.MoveNext());
reader.StepIn();
Assert.AreEqual(IonType.Struct, reader.MoveNext());

// Try to skip over the struct instead of stepping in but this errors
Assert.AreEqual(IonType.None, reader.MoveNext());

reader.StepOut(); // Step out of list
Assert.AreEqual(IonType.None, reader.MoveNext());

This is the exception:

System.FormatException: Illegal character: expected '}' character but encountered ']'
  Stack Trace:
      at Amazon.IonDotnet.Internals.Text.RawTextReader.ValidateClosingSymbol(Int32 token) in /home/nate/repos/ion-dotnet/Amazon.IonDotnet/Internals/Text/RawTextReader.cs:line 768
   at Amazon.IonDotnet.Internals.Text.RawTextReader.ParseNext() in /home/nate/repos/ion-dotnet/Amazon.IonDotnet/Internals/Text/RawTextReader.cs:line 732
   at Amazon.IonDotnet.Internals.Text.RawTextReader.HasNext() in /home/nate/repos/ion-dotnet/Amazon.IonDotnet/Internals/Text/RawTextReader.cs:line 285
   at Amazon.IonDotnet.Internals.Text.UserTextReader.HasNext() in /home/nate/repos/ion-dotnet/Amazon.IonDotnet/Internals/Text/UserTextReader.cs:line 75
   at Amazon.IonDotnet.Internals.Text.RawTextReader.MoveNext() in /home/nate/repos/ion-dotnet/Amazon.IonDotnet/Internals/Text/RawTextReader.cs:line 132
   at Amazon.IonDotnet.Tests.Internals.TextReaderTest.StructInList() in /home/nate/repos/ion-dotnet/Amazon.IonDotnet.Tests/Internals/TextReaderTest.cs:line 302
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

The call to MoveNext that moves us onto the struct looks like: MoveNext -> HasNext -> HasNext -> ParseNext which runs the ActionStartList case and pushes TokenCloseBrace onto the expectedContainerClosingSymbol stack. Then the next MoveNext where we try to skip over the struct instead of into it looks like: MoveNext -> HasNext -> HasNext -> FinishValue -> scanner.FinishToken -> SkipToEnd -> SkipOverList -> SkipOverContainer. That eats the closing brace. Then, still inside HasNext, it calls ParseNext which calls ValidateClosingSymbol which throws the exception because the closing brace was already eaten and it finds the closing square character for the end of the list instead.

It seems like the simplest fix is to not push the closing brace onto the expectedContainerClosingSymbol stack when we're just trying to skip the struct. Push the closing brace inside of StepIn instead.

popematt commented 1 year ago

Thanks for the detailed analysis of the problem and for submitting a PR to fix it. We'll take a look at it right away.