Replaced string.Compare with Buffer.EndsWith to fix #39140 #39853

violetshine · 2020-07-23T19:02:03Z

Replaced string.Compare with Buffer.EndsWith/StartsWith in both
StringSegment.EndsWith and StringSegment.StartsWith.
string.Compare caused issues in StringSegment.EndsWith when the Buffer
is only one character long.
Replaced string.Compare in StringSegment.StartsWith as well to make
both StartsWith and EndsWith function similarly.

EDIT: As @GrabYourPitchforks already said in that issue, both of those shouldn't be using string.Compare.

Fix #39140

dnfadmin · 2020-07-23T19:02:17Z

All CLA requirements met.

GrabYourPitchforks

Please also fix up other uses of string.Compare within this class (such as within the Equals method) and introduce unit tests for them. For example:

bool areEqual = new StringSegment("ae").Equals("\u00C6", StringComparer.InvariantCultureIgnoreCase);
Assert.True(areEqual);

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

violetshine · 2020-07-24T08:17:23Z

I cannot run the tests for some reason, it kept complaining about wrong .NET SDK version (global.json would've wanted "preview 8" which doesn't even exist, current preview is 7). I changed the version to preview 7 and now it errors with System.IO.FileLoadException : Could not load file or assembly 'System.Runtime.CompilerServices.Unsafe, Version=5.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. Strong name validation failed. (Exception from HRESULT: 0x8013141A)

violetshine · 2020-07-24T08:36:04Z

I made the requested changes but I couldn't get the tests to run as I said. I tested this by just calling those functions from my code and they seem to work fine but I don't know if that's enough or is it mandatory to run unit tests.

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs

violetshine · 2020-07-24T18:54:49Z

Removed that if clause, anything else?

GrabYourPitchforks · 2020-07-24T19:23:48Z

Can't really think of anything aside from the unit test updates. To run the unit tests, make sure you have the proper tools (find your OS from https://github.com/dotnet/runtime/tree/master/docs/workflow/requirements) installed on your machine. Then, from the command line:

build -s clr -c Release
build -s libs+libs.tests -rc Release -test

The first line will build System.Private.CoreLib and friends. The second line will build the rest of the libraries (including Microsoft.Extensions.*) and unit tests, then it'll run all of the unit tests.

If you want to use Visual Studio to build & run tests, make sure you also have the latest nightly SDK installed from https://aka.ms/dotnet/net5/dev/Sdk/dotnet-sdk-win-x64.exe. You'll need to at minimum build the CLR + libraries (follow above instructions but leave off the -test flag) before opening the solution file in VS.

Some good examples of tests would be ensuring that new StringSegment("xaex", 1, 2).Equals("\u00C6", StringComparison.InvariantCultureIgnoreCase) returns true, etc. That is, "ae" (2 chars) and "Æ" (1 char) should compare as equal under the InvariantCultureIgnoreCase comparer, even though they're different lengths.

violetshine · 2020-07-27T09:06:38Z

I added that unit test but it + 6 other tests fail. I don't know why, I'm tired of unit testing already.

There shouldn't be any differences with string.Compare way of doing it and .Equals. Or are there?

eerhardt · 2020-07-27T17:17:11Z

Can you also add a test for the scenario reported in #39140?

Console.WriteLine(new StringSegment("/").EndsWith("/", StringComparison.Ordinal));
// prints False !!!

To run the tests, after building the repo (see the libraries build doc), you can do the following:

cd src\libraries\Microsoft.Extensions.Primitives\tests
dotnet build /t:Test

You can find the correct version of dotnet in the root of the repo - $REPO_ROOT\dotnet.cmd.

See https://github.com/dotnet/runtime/blob/master/docs/workflow/testing/libraries/testing.md

violetshine · 2020-07-30T09:54:22Z

I added that.

eerhardt · 2020-07-30T15:32:48Z

The tests are failing with this change. Can you fix them?

    Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_InvariantCultureIgnoreCase [FAIL]
      Assert.True() Failure
      Expected: True
      Actual:   False
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs(612,0): at Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_InvariantCultureIgnoreCase()
    Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualStringSegment(candidate: ) [FAIL]
      Assert.False() Failure
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs(541,0): at Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualStringSegment(StringSegment candidate)
    Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualStringSegment(candidate: ) [FAIL]
      Assert.False() Failure
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs(541,0): at Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualStringSegment(StringSegment candidate)
    Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_StartsWith_Invalid [FAIL]
      Assert.False() Failure
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs(332,0): at Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_StartsWith_Invalid()
    Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualString(candidate: "") [FAIL]
      Assert.False() Failure
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs(568,0): at Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualString(String candidate)
    Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_EndsWith_Invalid [FAIL]
      Assert.False() Failure
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs(284,0): at Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_EndsWith_Invalid()
    Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_EqualsString_Invalid [FAIL]
      Assert.False() Failure
      Expected: False
      Actual:   True
      Stack Trace:
        /_/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs(486,0): at Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_EqualsString_Invalid()
  Finished:    Microsoft.Extensions.Primitives.Tests
=== TEST EXECUTION SUMMARY ===
   Microsoft.Extensions.Primitives.Tests  Total: 460, Errors: 0, Failed: 7, Skipped: 0, Time: 0.800s

violetshine · 2020-08-02T13:00:22Z

I don't even understand how I can fix it. Even GrabYourPitchforks said I should use this.AsSpan.EndsWith/StartsWith and I did that. Are there any differences between String.Compare and EndWith/StartsWith/Equals?

public bool Equals(StringSegment other, StringComparison comparisonType)
{
    return this.AsSpan().Equals(other.AsSpan(), comparisonType);
}

public bool EndsWith(string text, StringComparison comparisonType)
{
    if (text == null)
    {
        ThrowHelper.ThrowArgumentNullException(ExceptionArgument.text);
    }

    return this.AsSpan().EndsWith(text.AsSpan(), comparisonType);
}

public bool StartsWith(string text, StringComparison comparisonType)
{
    if (text == null)
    {
        ThrowHelper.ThrowArgumentNullException(ExceptionArgument.text);
    }

    return this.AsSpan().StartsWith(text.AsSpan(), comparisonType);
}

All of those should do the same thing as the String.Compare versions. Or am I missing something here?

GrabYourPitchforks · 2020-08-02T21:16:54Z

The HasValue checks were removed from the latest code review. They likely need to be re-added in order for the tests to pass.

violetshine · 2020-08-03T19:38:55Z

I added those checks, still 3 test failures.

<test name="Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_InvariantCultureIgnoreCase" type="Microsoft.Extensions.Primitives.StringSegmentTest" method="StringSegment_Equals_InvariantCultureIgnoreCase" time="0.0035022" result="Fail">
  <failure exception-type="Xunit.Sdk.TrueException">
    <message><![CDATA[Assert.True() Failure\r\nExpected: True\r\nActual:   False]]></message>
    <stack-trace><![CDATA[   at Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_InvariantCultureIgnoreCase() in D:\Workspace\dotnet\runtime\src\libraries\Microsoft.Extensions.Primitives\tests\StringSegmentTest.cs:line 612]]></stack-trace> 
  </failure>
</test>

<test name="Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualStringSegment(candidate: )" type="Microsoft.Extensions.Primitives.StringSegmentTest" method="DefaultStringSegment_DoesNotEqualStringSegment" time="0.0005672" result="Fail">
  <failure exception-type="Xunit.Sdk.FalseException">
    <message><![CDATA[Assert.False() Failure\r\nExpected: False\r\nActual:   True]]></message>
    <stack-trace><![CDATA[   at Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualStringSegment(StringSegment candidate) in D:\Workspace\dotnet\runtime\src\libraries\Microsoft.Extensions.Primitives\tests\StringSegmentTest.cs:line 541]]></stack-trace>
  </failure>
</test>

<test name="Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualStringSegment(candidate: )" type="Microsoft.Extensions.Primitives.StringSegmentTest" method="DefaultStringSegment_DoesNotEqualStringSegment" time="0.0002969" result="Fail">
  <failure exception-type="Xunit.Sdk.FalseException">
    <message><![CDATA[Assert.False() Failure\r\nExpected: False\r\nActual:   True]]></message>
    <stack-trace><![CDATA[   at Microsoft.Extensions.Primitives.StringSegmentTest.DefaultStringSegment_DoesNotEqualStringSegment(StringSegment candidate) in D:\Workspace\dotnet\runtime\src\libraries\Microsoft.Extensions.Primitives\tests\StringSegmentTest.cs:line 541]]></stack-trace>
  </failure>
</test>

GrabYourPitchforks · 2020-08-03T20:58:33Z

One test is failing because this line:
https://github.com/dotnet/runtime/blob/c7f8a4711fc7bf2ab9397d24bed188bdd427b633/src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs#L229

Should instead read:

if (!HasValue)

Also, for the test you added at below:
https://github.com/dotnet/runtime/blob/c7f8a4711fc7bf2ab9397d24bed188bdd427b633/src/libraries/Microsoft.Extensions.Primitives/tests/StringSegmentTest.cs#L602-L613

Please add the equivalent that calls segment.Equals(new StringSegment("\u00C6"), ...). That will help catch future regressions in this area.

GrabYourPitchforks · 2020-08-03T21:06:01Z

Looks like some of the calls to string.Compare might have to remain. Specifically, the code paths in StringSegment.Equals and StringSegment.Compare may still need to go through string.Compare because of the way it handles null strings. However, the code paths in StringSegment.StartsWith and StringSegment.EndsWith should continue to use the spanified versions that the PR now has.

Important: The calls to string.Compare should not be guarded with if (this.Length != other.Length) checks.

Replaced string.Compare in Microsoft.Extensions.Primitives.StringSegment string.Compare caused issues in StringSegment.EndsWith when the Buffer is only one character long. Fix #39140

violetshine · 2020-08-04T07:47:59Z

I made the requested changes.

GrabYourPitchforks · 2020-08-04T16:59:09Z

@DKittenz Are you still not able to run tests locally? There's a decent number of CI failures that local runs would have caught.

My comment on the string.Compare code paths (used by Compare and Equals) were that all early-exit length checks should be removed before the call to string.Compare. That is, the lines if (this.Length != other.Length) and int actualLength = Math.Min(this.Length, other.Length) and similar should be nixed.

violetshine · 2020-08-04T17:45:32Z

I got the tests running now, I will be pushing the final commit when I get those tests to pass.

GrabYourPitchforks · 2020-08-04T17:46:51Z

For reference, the reason I said we need to remove the length checks is that two strings of different lengths might compare as equal under certain comparers. That was the basis of the last part of my comment at #39853 (comment).

If the length checks remain (or Math.Min(a.Length, b.Length) or similar), it would cause these two strings to compare as inequal because the check would have been short-circuited, even though the underlying globalization infrastructure might have said "turns out they're equal after all!"

violetshine · 2020-08-04T18:37:37Z

public bool Equals(StringSegment other, StringComparison comparisonType)
{
    return string.Compare(Buffer, Offset, other.Buffer, other.Offset, other.Length, comparisonType) == 0;
}

This is what the .Equals function looks like (I removed the length checks), still yielding 3 test errors regarding .Equals.

<test name="Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_StringSegment_Valid(candidate: ell, comparison: Ordinal, expectedResult: False)" type="Microsoft.Extensions.Primitives.StringSegmentTest" method="StringSegment_Equals_StringSegment_Valid" time="0.0055716" result="Fail">
	<failure exception-type="Xunit.Sdk.EqualException">
		<message><![CDATA[Assert.Equal() Failure\r\nExpected: False\r\nActual:   True]]></message>
		<stack-trace><![CDATA[   at Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_StringSegment_Valid(StringSegment candidate, StringComparison comparison, Boolean expectedResult) in D:\Workspace\dotnet\runtime\src\libraries\Microsoft.Extensions.Primitives\tests\StringSegmentTest.cs:line 599]]></stack-trace>
	</failure>
</test>

<test name="Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_InvariantCultureIgnoreCase" type="Microsoft.Extensions.Primitives.StringSegmentTest" method="StringSegment_Equals_InvariantCultureIgnoreCase" time="0.0010391" result="Fail">
	<failure exception-type="Xunit.Sdk.TrueException">
		<message><![CDATA[Assert.True() Failure\r\nExpected: True\r\nActual:   False]]></message>
		<stack-trace><![CDATA[   at Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_InvariantCultureIgnoreCase() in D:\Workspace\dotnet\runtime\src\libraries\Microsoft.Extensions.Primitives\tests\StringSegmentTest.cs:line 612]]></stack-trace>
	</failure>
</test>

<test name="Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_InvariantCultureIgnoreCase_StringSegment" type="Microsoft.Extensions.Primitives.StringSegmentTest" method="StringSegment_Equals_InvariantCultureIgnoreCase_StringSegment" time="0.0005599" result="Fail">
	<failure exception-type="Xunit.Sdk.TrueException">
		<message><![CDATA[Assert.True() Failure\r\nExpected: True\r\nActual:   False]]></message>
		<stack-trace><![CDATA[   at Microsoft.Extensions.Primitives.StringSegmentTest.StringSegment_Equals_InvariantCultureIgnoreCase_StringSegment() in D:\Workspace\dotnet\runtime\src\libraries\Microsoft.Extensions.Primitives\tests\StringSegmentTest.cs:line 625]]></stack-trace>
	</failure>
</test>

ghost · 2020-09-30T16:04:15Z

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

GrabYourPitchforks · 2020-10-27T17:37:39Z

@DKittenz Are you still interested in driving this? Let us know what you're thinking or whether you'd like us to take this over - no worries either way!

violetshine · 2020-10-28T12:12:42Z

I can still do it, I just never got the tests to pass.

GrabYourPitchforks · 2020-10-28T17:38:17Z

If you're having trouble getting the tests to run on your box, let us know what errors you're seeing and we'll see what we can do. If the tests are running but not passing, hopefully that's an indication we have good test coverage of this area? I'll take the optimistic view. 😀

violetshine · 2020-10-28T19:36:07Z

It's the second one. I got the tests to run after a bit of tinkering around but they don't pass. There is always that one string somewhere that doesn't work with the changes I make. I'll see what I can do to it.

violetshine · 2020-10-30T13:04:50Z

Nevermind, I can't do this right now because of my schedule.

I really need to focus on a project of mine with one of my friends.

maryamariyan · 2020-11-20T17:21:49Z

@DKittenz thanks for the progress. Closing this PR for now. We could assign you back later on the issue if you decide to later resume changes here.

Dotnet-GitSync-Bot added the area-Meta label Jul 23, 2020

GrabYourPitchforks requested changes Jul 23, 2020

View reviewed changes

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs Outdated Show resolved Hide resolved

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs Outdated Show resolved Hide resolved

GrabYourPitchforks requested changes Jul 24, 2020

View reviewed changes

src/libraries/Microsoft.Extensions.Primitives/src/StringSegment.cs Outdated Show resolved Hide resolved

Replaced string.Compare

c503a31

Replaced string.Compare in Microsoft.Extensions.Primitives.StringSegment string.Compare caused issues in StringSegment.EndsWith when the Buffer is only one character long. Fix #39140

gfoidl mentioned this pull request Aug 15, 2020

[Perf -13%] Microsoft.Extensions.Primitives.StringSegmentBenchmark.EndsWith #40866

Closed

maryamariyan added area-Extensions-Primitives and removed area-Meta labels Sep 30, 2020

maryamariyan requested review from eerhardt and maryamariyan October 7, 2020 16:23

maryamariyan closed this Nov 20, 2020

ghost locked as resolved and limited conversation to collaborators Dec 20, 2020

Replaced string.Compare with Buffer.EndsWith to fix #39140 #39853

Replaced string.Compare with Buffer.EndsWith to fix #39140 #39853

Uh oh!

Conversation

violetshine commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfadmin commented Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GrabYourPitchforks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

violetshine commented Jul 24, 2020

Uh oh!

violetshine commented Jul 24, 2020

Uh oh!

Uh oh!

violetshine commented Jul 24, 2020

Uh oh!

GrabYourPitchforks commented Jul 24, 2020

Uh oh!

violetshine commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt commented Jul 27, 2020

Uh oh!

violetshine commented Jul 30, 2020

Uh oh!

eerhardt commented Jul 30, 2020

Uh oh!

violetshine commented Aug 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GrabYourPitchforks commented Aug 2, 2020

Uh oh!

violetshine commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GrabYourPitchforks commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GrabYourPitchforks commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

violetshine commented Aug 4, 2020

Uh oh!

GrabYourPitchforks commented Aug 4, 2020

Uh oh!

violetshine commented Aug 4, 2020

Uh oh!

GrabYourPitchforks commented Aug 4, 2020

Uh oh!

violetshine commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 30, 2020

Uh oh!

GrabYourPitchforks commented Oct 27, 2020

Uh oh!

violetshine commented Oct 28, 2020

Uh oh!

GrabYourPitchforks commented Oct 28, 2020

Uh oh!

violetshine commented Oct 28, 2020

Uh oh!

violetshine commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maryamariyan commented Nov 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

violetshine commented Jul 23, 2020 •

edited

Loading

dnfadmin commented Jul 23, 2020 •

edited

Loading

violetshine commented Jul 27, 2020 •

edited

Loading

violetshine commented Aug 2, 2020 •

edited

Loading

violetshine commented Aug 3, 2020 •

edited

Loading

GrabYourPitchforks commented Aug 3, 2020 •

edited

Loading

GrabYourPitchforks commented Aug 3, 2020 •

edited

Loading

violetshine commented Aug 4, 2020 •

edited

Loading

violetshine commented Oct 30, 2020 •

edited

Loading