ArgumentOutOfRangeException in CompositeText.RemoveSplitLineBreaksAndEmptySegments

[mavasani]

Version Used: 4.14.0

Regression Information: I verified that the issue does not repro with the same code against MS.CA version 4.10.0. Investigating a bit, I see that the method CompositeText.RemoveSplitLineBreaksAndEmptySegments was added with this commit last August: 8e98fc9#diff-f7aa04037254dd4688389954443cbad14c756cc9edcdb5536ce52c8904b1252f
This confirms that this is a recent regression from this commit

Steps to Reproduce:
Unzip and Run the attached console app:

ConsoleApp1.zip

• It works fine with MS.CA 4.10.0
• ArgumentOutOfRangeException with MS.CA 4.14.0

Unhandled exception. System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'span')
   at Microsoft.CodeAnalysis.Text.SubText..ctor(SourceText text, TextSpan span)
   at Microsoft.CodeAnalysis.Text.SubText.GetSubText(TextSpan span)
   at Microsoft.CodeAnalysis.Text.CompositeText.RemoveSplitLineBreaksAndEmptySegments(ArrayBuilder`1 segments)
   at Microsoft.CodeAnalysis.Text.CompositeText.ToSourceText(ArrayBuilder`1 segments, SourceText original, Boolean adjustSegments)
   at Microsoft.CodeAnalysis.Text.SourceText.WithChanges(IEnumerable`1 changes)
   at Microsoft.CodeAnalysis.Text.SourceText.WithChanges(TextChange[] changes)
   at Program.<Main>$(String[] args) in Q:\source\ConsoleApp1\ConsoleApp2\Program.cs:line 11

Q:\source\ConsoleApp1\ConsoleApp2\bin\Debug\net8.0\ConsoleApp2.exe (process 183496) exited with code -532462766 (0xe0434352).
Press any key to close this window . . .

[mavasani]

Tagging @ToddGrun @CyrusNajmabadi Moving from Roslyn 4.10.0 to 4.14.0 is leading to large number of test failures in our repo.

Todd, if you could take a look and see if the bug is in our code or Roslyn code with 8e98fc9#diff-f7aa04037254dd4688389954443cbad14c756cc9edcdb5536ce52c8904b1252f, it will be helpful. If in our code, we can fix it up and unblock the failing nightly tests. If not, we will have to either skip the failing tests or find some other workaround. We are more concerned about any cascading product issues that might be hiding under the covers.

[mavasani]

Confirmed that the app works fine even on 4.11.0 but the exception occurs starting 4.12.0

[mavasani]

Copilot tells me:

The most probable cause is:

curSegment.Length == 1 → new TextSpan(1, 0)`, which fails because there's no character at position 1.

roslyn/src/Compilers/Core/Portable/Text/CompositeText.cs

Lines 206 to 237 in f8eb8e5

	private static void RemoveSplitLineBreaksAndEmptySegments(ArrayBuilder<SourceText> segments)
	{
	if (segments.Count > 1)
	{
	// Remove empty segments before checking for split line breaks
	segments.RemoveAll(static (s, _, _) => s.Length == 0, arg: 0);
	var splitLineBreakFound = false;
	for (int i = 1; i < segments.Count; i++)
	{
	var prevSegment = segments[i - 1];
	var curSegment = segments[i];
	if (prevSegment.Length > 0 && prevSegment[^1] == '\r' && curSegment[0] == '\n')
	{
	splitLineBreakFound = true;
	segments[i - 1] = prevSegment.GetSubText(new TextSpan(0, prevSegment.Length - 1));
	segments.Insert(i, SourceText.From("\r\n"));
	segments[i + 1] = curSegment.GetSubText(new TextSpan(1, curSegment.Length - 1));
	i++;
	}
	}
	if (splitLineBreakFound)
	{
	// If a split line break was present, ensure there aren't any empty lines again
	// due to the sourcetexts created from the GetSubText calls.
	segments.RemoveAll(static (s, _, _) => s.Length == 0, arg: 0);
	}
	}
	}

[mavasani]

I am able to work around this issue by making the below code change on our side:

Original code:

                    var oldText = lastParsedInfo.Item1;
                    var oldTree = lastParsedInfo.Item2;
                    var diff = StringDiffUtils.StringDiff.ComputeStringDiff(oldText, programString);
                    var change = new TextChange(TextSpan.FromBounds(diff.StartLocation, diff.EndLocation), diff.NewText);
                    var oldSourceText = oldTree.GetText(CancellationToken.None);
                    var newSourceText = oldSourceText.WithChanges(change);

                    newTree = oldTree.WithChangedText(newSourceText);

New Code:

                    var oldTree = lastParsedInfo.Item2;
                    var newText = SourceText.From(programString, oldTree.Encoding);
                    newTree = oldTree.WithChangedText(newText);

I think the new code is simpler and better, as WithChangedText already does the diff computation and uses incremental parsing. I will need to confirm if this leads to any performance regressions or not. If not, I think we should be unblocked on this issue, but regardless this issue is still a Roslyn regression that must be addressed.

roslyn/src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.cs

Lines 529 to 537 in f8eb8e5

	/// <summary>
	/// Creates a new syntax based off this tree using a new source text.
	/// </summary>
	/// <remarks>
	/// If the new source text is a minor change from the current source text an incremental parse will occur
	/// reusing most of the current syntax tree internal data. Otherwise, a full parse will occur using the new
	/// source text.
	/// </remarks>
	public override SyntaxTree WithChangedText(SourceText newText)

[ToddGrun]

Going to take a look today. This looks extremely helpful, thanks a ton!

[mavasani]

I am able to work around this issue by making the below code change on our side:

Original code:

                var oldText = lastParsedInfo.Item1;
                var oldTree = lastParsedInfo.Item2;
                var diff = StringDiffUtils.StringDiff.ComputeStringDiff(oldText, programString);
                var change = new TextChange(TextSpan.FromBounds(diff.StartLocation, diff.EndLocation), diff.NewText);
                var oldSourceText = oldTree.GetText(CancellationToken.None);
                var newSourceText = oldSourceText.WithChanges(change);

                newTree = oldTree.WithChangedText(newSourceText);

New Code:

                var oldTree = lastParsedInfo.Item2;
                var newText = SourceText.From(programString, oldTree.Encoding);
                newTree = oldTree.WithChangedText(newText);

I think the new code is simpler and better, as WithChangedText already does the diff computation and uses incremental parsing. I will need to confirm if this leads to any performance regressions or not. If not, I think we should be unblocked on this issue, but regardless this issue is still a Roslyn regression that must be addressed.

roslyn/src/Compilers/CSharp/Portable/Syntax/CSharpSyntaxTree.cs

Lines 529 to 537 in f8eb8e5

///

/// Creates a new syntax based off this tree using a new source text.
///

///
/// If the new source text is a minor change from the current source text an incremental parse will occur
/// reusing most of the current syntax tree internal data. Otherwise, a full parse will occur using the new
/// source text.
///
public override SyntaxTree WithChangedText(SourceText newText)

Thanks @ToddGrun. What are your thoughts on the workaround I mentioned above? We are little concerned that the new code above might cause re parsing more often than the original code. Also tagging @CyrusNajmabadi @jaredpar for their views

[CyrusNajmabadi]

My current thoughts are that we shuold just fix the issue, and port it back as appropriate as part of servicing. Can you wait on the fix? or do you have to have a workaround now?

[mavasani]

We are going to put the workaround to unblock the unit tests and the scenarios that broke with this regression. Once the Roslyn fix is available (either in 4.15.0 or a hotfix patch), we can revert the product change.