New chunk should be equal/larger than the one it replaces

[maryamariyan]

Fixes #17205

In PR #16926 we changed the behavior on a non-empty StringBuilder such that it will not always grow the last chunk array length on sb.Length=value operation where value is smaller than current Length.

The goal for this change was to keep Capacity within finite range as we loop multiple times to Clear and Append and Insert into the StringBuilder.

This PR removes the assertion that makes sure we always grow it's last chunk array.

cc: @jkotas @danmosemsft @benaadams

[benaadams]

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

[danmoseley]

this comment is now wrong?

                        // We crossed a chunk boundary when reducing the Length. We must replace this middle-chunk with a new larger chunk,
                        // to ensure the original capacity is preserved.

[maryamariyan]

I'll move this to the if block.

[danmoseley]

// Special case where the capacity we want to keep corresponds exactly to the size of the content.
// Just take ownership of the array.

Not sure the assert is worth having now.

[danmoseley]

Can you check there's a test that goes down this path?

[maryamariyan]

I'll move this comment into the else block

[danmoseley]

Is it worth asserting at the end of the method that Length == value

[maryamariyan]

Added this assert to the end:

Debug.Assert(Length == value, "Something went wrong setting Length.");

[danmoseley]

Can you please add specific to StringBuilder that fails before your change and passes after? We should not rely on other test libraries for coverage necessary to protect our functionality.

[danmoseley]

thanks for fixing so quickly!

[maryamariyan]

Will be adding test to corefx PR dotnet/corefx#28038

[danmoseley]

it's not the original capacity though. it's 120% of it at most. Maybe

// to ensure the capacity we want

[maryamariyan]

updated

[danmoseley]

Needs license header 😸

[danmoseley]

Actually you could add .exe.stackdump to our .gitignore at the root.

[maryamariyan]

oh no! didn't mean to add this :)

[benaadams]

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

[maryamariyan]

About the Use Case:

The code snippet below is taken from failing test: System.Security.Tests.SecureStringTests.GrowAndContract_Small.
The last statement in the code snippet below will exercise the code path in this PR.

            var rand = new Random(42);
            var sb = new StringBuilder(string.Empty);
            for (int i = 0; i < 100; i++)
            {
                char c = (char)('a' + rand.Next(0, 26));
                int addPos = rand.Next(0, sb.Length);
                sb.Insert(addPos, c);
            }
            while (sb.Length > 1)
            {
                int removePos = rand.Next(0, sb.Length);
                sb.Remove(removePos, 1);
            }
            sb.Clear(); //sb.Length = 0;//sb.Remove(0, 1);

This will result in a StringBuilder with with length of 1 and two chunks.
The first chunk has offset=0, length=1, array length=16, and the second chunk has offset=1 and length=0, array length=16.
The Capacity is 17. (=last chunk array size, 16 + last chunk offset, 1).

In this case, it should be OK to reduce the Capacity value to 16 as we set Length to 0 and keep chunk array length the same value, rather than expanding the chunk length to 17 as we were doing prior to the change in PR #16926

[benaadams]

Failures seem are infrastructure:

• Ubuntu x64 Checked Build and Test (Jit - CoreFx) - time out during build
• Tizen - usual
• OSX10.12 x64 - downstream failure

Windows_NT x64 Checked Build and Test (Jit - CoreFx) - passes which was failing on this code before