ShouldBe does a sequence comparison for ReadOnlyMemory #610

hrai · 2020-03-27T07:53:50Z

Closes #562

@jnm2 I've created this as there was no update on the linked issue so I assumed no one was working on the fix.

sungam3r · 2020-03-27T17:32:44Z

src/Shouldly/Internals/TypeExtensions.cs

+
+        public static bool IsMemory(this Type type, out Type elementType)
+        {
+            if (type.IsGenericType() && type.GetGenericTypeDefinition()?.FullName == "System.Memory`1")


Suggested change

if (type.IsGenericType() && type.GetGenericTypeDefinition()?.FullName == "System.Memory`1")

if (type.IsGenericType() && type.GetGenericTypeDefinition().FullName == "System.Memory`1")

sungam3r · 2020-03-27T17:42:32Z

A couple of questions.

Is reflection really required here?
Is it possible to do more simple way for handling [Readonly]Memory<byte> as the most frequent case?

hrai · 2020-03-27T23:45:01Z

A couple of questions.

1. Is reflection really required here?

2. Is it possible to do more simple way for handling `[Readonly]Memory<byte>` as the most frequent case?

There's an extension method that we could have used but it's only available in .NET Core and .NET Standard. As shouldly targets .NET Framework, I don't think there's another way.

https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.memorymarshal.toenumerable?view=netcore-3.1

The code blocks were added by @jnm2 as it shows in #562

Perhaps he will have more info.

jnm2 · 2020-03-28T00:37:22Z

Shouldly used to build for .NET Framework 4.0 which meant it couldn't reference https://www.nuget.org/packages/System.Memory in its net40 compile output. Reflection seemed easy and simpler than splitting Shouldly's behavior. (Remember, net40 builds can run on net48 and even netcoreapp3.1.)

Since then, Shouldly has dropped support for net40. That means you can add a package reference rather than reflection.

hrai · 2020-03-28T04:33:02Z

As Memory<> and ReadOnlyMemory<> are generic types, reflection seems to be the only way to cast to these types at runtime. When the equality method is invoked, there's no way to determine what the generic parameter's type is going to be.

This is what I found.
https://stackoverflow.com/questions/7676989/how-to-cast-a-generic-type-at-runtime-in-c-sharp

sungam3r · 2020-03-28T07:40:11Z

I talked about explicit check for [Readonly]Memory<byte> and then cast to IEnumerable for the most frequent case. Otherwise fallback to reflection.

jnm2 · 2020-03-28T13:12:00Z

Ah, and I forgot generics just now, quite right.

@sungam3r It would be indistinguishable to first check for IEnumerable, then fall back to using reflection to check for Memory/ReadOnlyMemory. Those are the essential checks. Before we add additional checks on top of that, we need hard numbers on which scenarios it speeds up and which it slows down.

sungam3r · 2020-03-28T13:45:07Z

I think that all scenarios will get a slowdown due to the fact that IEnumerable is used. All elements of value types are boxed to be used in if (!equalityComparer.Equals(enumeratorX.Current, enumeratorY.Current)).

jnm2 · 2020-03-28T14:15:55Z

@sungam3r With regard to that, this PR doesn't change anything. It's good to keep changes small and focused. Because of how much more complex it makes the code to use generic IEnumerable, I'd also prefer to see hard measurements of real-world projects demonstrating the problem so that we know we're doing something meaningful before starting a PR to avoid boxing via IEnumerable.Current.

jnm2 · 2020-03-28T14:23:07Z

Also, if we were to go the direction of avoiding boxing, it would be both simpler and more general to first have a stage where a non-generic IEnumerable is obtained and then a second stage where generic IEnumerable is detected with a specialized iteration implementation that we invoke using MakeGenericMethod. That way not just Memory<byte>/ReadOnlyMemory<byte> but any memory/enumerable of any value type gets the benefit. And that way, this PR doesn't need to be concerned about it one way or the other.

sungam3r · 2020-03-28T14:28:46Z

I understand all this and do not insist.

jnm2 · 2020-03-28T14:33:04Z

@sungam3r I appreciate that. Thanks for your input. Would you be interested in starting a new issue and doing a benchmark to help us understand how big of an effect we're talking about, or do you want to wait and see if someone starts noticing some pain over it?

sungam3r · 2020-03-28T15:18:13Z

No, I don’t think it matters much.

hrai · 2020-03-29T00:00:00Z

It doesn't look like any more changes are expected in this pr. Can I get the approval then @sungam3r ?

jnm2 · 2020-03-29T00:25:21Z

src/Shouldly.Tests/ShouldBe/EnumerableType/EnumerableOfReadOnlyMemoryScenario.cs

+    should be
+System.ReadOnlyMemory<Byte>[2]
+    but was
+System.ReadOnlyMemory<Byte>[3]


Normally the elements are listed in the error message. Could you see what it would take to enumerate here too?

The method that generates the error method takes in the original and expected objects and passes it to this method -

shouldly/src/Shouldly/Internals/ShouldlyCoreExtensions.cs

Line 22 in 5fc845c

internal static void AssertAwesomely<T>(

We can convert actual and expected Memory items into Enumerables in

ShouldBe() and work with Enumerables

this line while constructing ExpectedActualShouldlyMessage

shouldly/src/Shouldly/Internals/ShouldlyCoreExtensions.cs

Line 39 in 5fc845c

throw new ShouldAssertException(new ExpectedActualShouldlyMessage(originalExpected, originalActual, customMessage, shouldlyMethod).ToString());

Thanks, that helps. It looks like those values are used here:

shouldly/src/Shouldly/MessageGenerators/ShouldBeMessageGenerator.cs

Lines 20 to 21 in 5fc845c

var expected = context.Expected.ToStringAwesomely();

var actualValue = context.Actual.ToStringAwesomely();

And the string is built here:

shouldly/src/Shouldly/Internals/StringHelpers.cs

Lines 42 to 51 in 5fc845c

if (value is IEnumerable)

{

var objects = value.As<IEnumerable>().Cast<object>();

var inspect = "[" + objects.Select(o => o.ToStringAwesomely()).CommaDelimited() + "]";

if (inspect == "[]" && value.ToString() != type.FullName)

{

inspect += " (" + value + ")";

}

return inspect;

}

I'm thinking we should check for the memory types and convert to IEnumerable in ToStringAwesomely for consistency. It sometimes uses the type full name and ToString of the original instance, and we wouldn't want to use the instance returned from MemoryMarshal.ToEnumerable for that.

Pushed a change

jnm2 · 2020-03-29T00:28:58Z

src/Shouldly/Internals/EqualityComparer.cs

+        private static IEnumerable ToEnumerable(object readOnlyMemory, Type elementType)
+        {
+            return (IEnumerable)Type.GetType("System.Runtime.InteropServices.MemoryMarshal, System.Memory")
+                ?.GetMethod("ToEnumerable", BindingFlags.Public | BindingFlags.Static | BindingFlags.DeclaredOnly)


It's best to specify parameter types so that this doesn't crash if an overload is added.

I wanted to use this overload of GetMethod() but it only support .NET Standard 2.0 and above - https://docs.microsoft.com/en-us/dotnet/api/system.type.getmethod?view=netframework-4.8#System_Type_GetMethod_System_String_System_Reflection_BindingFlags_System_Reflection_Binder_System_Type___System_Reflection_ParameterModifier___

Below is the modified code which fails to compile.

return (IEnumerable)Type.GetType("System.Runtime.InteropServices.MemoryMarshal, System.Memory") ?.GetMethod("ToEnumerable", BindingFlags.Public | BindingFlags.Static | BindingFlags.DeclaredOnly, null, new[] { typeof(ReadOnlyMemory<T>) }, null) ?.MakeGenericMethod(elementType) .Invoke(null, new[] { readOnlyMemory });

Ah, that's too bad. We could cross our fingers and hope that an overload of MemoryMarshal.ToEnumerable is never added, or we could use GetMethods+SingleOrDefault like you did above, or we could replace netstandard1.3 with netstandard2.0.

@josephwoodward What are your thoughts?

GetMethods+SingleOrDefault sounds like a small enough change as opposed to upgrading to netstandard2.0

The library may stop supporting older .NET versions which may have unintended consequences.

sungam3r · 2020-03-29T09:21:13Z

src/Shouldly/Internals/EqualityComparer.cs

+                {
+                    var readOnlyMemory = type.GetMethods(BindingFlags.Public | BindingFlags.Static | BindingFlags.DeclaredOnly)
+                        .SingleOrDefault(method =>
+                            method.Name == "op_Implicit"


Also you can check method.IsSpecialName for operators.

…nlymemory Types

jnm2 · 2020-03-31T01:08:21Z

src/Shouldly.Tests/Shouldly.Tests.csproj

Any chance you could reformat back to the original (standard) 2-space csproj indentation, and squash it so the most recent line history for every line in the file doesn't point back to this PR?

jnm2 · 2020-03-31T01:09:50Z

src/Shouldly/Internals/StringHelpers.cs

+            else if (objectType.IsReadOnlyMemory(out genericParameterType))
+            {
+                value = value.ToEnumerable(genericParameterType);
+            }

            if (value is IEnumerable)


What are your thoughts on using TryGetEnumerable from here?

SimonCropp · 2020-07-05T00:52:32Z

thanks peoples

Hangjit Rai added 5 commits March 24, 2020 17:36

MEMORY-ENUMERABLE Updated Enumerable Comparer

987a92e

MEMORY-ENUMERABLE Added Extension Methods

03050a1

MEMORY-ENUMERABLE Updated Nuget Ref

dfa99b0

MEMORY-ENUMERABLE Updated Equality Comparer; Added Tests

db88e72

MEMORY-ENUMERABLE Merge Branch 'master' Into Memory-Enumerable

fdf8737

sungam3r reviewed Mar 27, 2020

View reviewed changes

Hangjit Rai added 2 commits March 28, 2020 10:27

MEMORY-ENUMERABLE Review Comments

a00453c

MEMORY-ENUMERABLE Added Tests For Memory

6437fe7

jnm2 reviewed Mar 29, 2020

View reviewed changes

sungam3r reviewed Mar 29, 2020

View reviewed changes

sungam3r approved these changes Mar 29, 2020

View reviewed changes

MEMORY-ENUMERABLE Updated ToStringAwesomely() To Support Memory/reado…

95bd550

…nlymemory Types

jnm2 approved these changes Mar 31, 2020

View reviewed changes

Hangjit Rai added 2 commits March 31, 2020 15:33

MEMORY-ENUMERABLE Review Comments

a0a8ed2

MEMORY-ENUMERABLE Cleanup

c68be16

sungam3r approved these changes Mar 31, 2020

View reviewed changes

SimonCropp added this to the 4.0.0 milestone Jul 5, 2020

SimonCropp merged commit 2d725cb into shouldly:master Jul 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ShouldBe does a sequence comparison for ReadOnlyMemory #610

ShouldBe does a sequence comparison for ReadOnlyMemory #610

hrai commented Mar 27, 2020

sungam3r Mar 27, 2020

sungam3r commented Mar 27, 2020

hrai commented Mar 27, 2020

jnm2 commented Mar 28, 2020 •

edited

Loading

hrai commented Mar 28, 2020

sungam3r commented Mar 28, 2020

jnm2 commented Mar 28, 2020 •

edited

Loading

sungam3r commented Mar 28, 2020

jnm2 commented Mar 28, 2020 •

edited

Loading

jnm2 commented Mar 28, 2020 •

edited

Loading

sungam3r commented Mar 28, 2020

jnm2 commented Mar 28, 2020

sungam3r commented Mar 28, 2020

hrai commented Mar 29, 2020

jnm2 Mar 29, 2020

hrai Mar 29, 2020

jnm2 Mar 29, 2020 •

edited

Loading

hrai Mar 30, 2020

jnm2 Mar 29, 2020

hrai Mar 29, 2020

hrai Mar 29, 2020

jnm2 Mar 29, 2020

hrai Mar 30, 2020

sungam3r Mar 29, 2020

jnm2 Mar 31, 2020

jnm2 Mar 31, 2020

SimonCropp commented Jul 5, 2020

	if (type.IsGenericType() && type.GetGenericTypeDefinition()?.FullName == "System.Memory`1")
	if (type.IsGenericType() && type.GetGenericTypeDefinition().FullName == "System.Memory`1")

	var expected = context.Expected.ToStringAwesomely();
	var actualValue = context.Actual.ToStringAwesomely();

	if (value is IEnumerable)
	{
	var objects = value.As<IEnumerable>().Cast<object>();
	var inspect = "[" + objects.Select(o => o.ToStringAwesomely()).CommaDelimited() + "]";
	if (inspect == "[]" && value.ToString() != type.FullName)
	{
	inspect += " (" + value + ")";
	}
	return inspect;
	}

ShouldBe does a sequence comparison for ReadOnlyMemory #610

ShouldBe does a sequence comparison for ReadOnlyMemory #610

Conversation

hrai commented Mar 27, 2020

Choose a reason for hiding this comment

sungam3r commented Mar 27, 2020

hrai commented Mar 27, 2020

jnm2 commented Mar 28, 2020 • edited Loading

hrai commented Mar 28, 2020

sungam3r commented Mar 28, 2020

jnm2 commented Mar 28, 2020 • edited Loading

sungam3r commented Mar 28, 2020

jnm2 commented Mar 28, 2020 • edited Loading

jnm2 commented Mar 28, 2020 • edited Loading

sungam3r commented Mar 28, 2020

jnm2 commented Mar 28, 2020

sungam3r commented Mar 28, 2020

hrai commented Mar 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 Mar 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonCropp commented Jul 5, 2020

jnm2 commented Mar 28, 2020 •

edited

Loading

jnm2 commented Mar 28, 2020 •

edited

Loading

jnm2 commented Mar 28, 2020 •

edited

Loading

jnm2 commented Mar 28, 2020 •

edited

Loading

jnm2 Mar 29, 2020 •

edited

Loading