Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using default comparer builder option throws StackOverflowException. #25

Closed
amir734jj opened this issue Feb 21, 2019 · 8 comments
Closed
Assignees

Comments

@amir734jj
Copy link
Contributor

Using Default throws StackOverflowException exception but using OrderBy(x => x.Weight) works fine.

public class Component : ComparableBase<Component>
{
    static Component()
    {
        DefaultComparer = ComparerBuilder.For<Component>().Default();
    }
    
    public double AccWeight { get; set; }

    public double Weight { get; set; }
}
@StephenCleary StephenCleary self-assigned this Feb 21, 2019
@StephenCleary
Copy link
Owner

The Default comparer will forward to the IComparable<T> implementation. What behavior were you expecting?

@amir734jj
Copy link
Contributor Author

amir734jj commented Feb 21, 2019

This throws an exception

            var c1 = new Component
            {
                Weight = 0.7,
                AccWeight = 0.2
            };
            
            var c2 = new Component
            {
                Weight = 0.7,
                AccWeight = 0.2
            };

            var result = c1.Equals(c2);

@StephenCleary
Copy link
Owner

Yes, the Default comparer forwards to IComparable<T>, which is implemented by the DefaultComparer, which is just Default, which forwards to IComparable<T>, ... So of course you're going to get a stack overflow exception.

Again, what behavior were you expecting?

@amir734jj
Copy link
Contributor Author

amir734jj commented Feb 21, 2019

I expected it to compare one property at a time and return true.

@amir734jj
Copy link
Contributor Author

amir734jj commented Feb 21, 2019

I may be wrong but I don't think using Equals method should never throw StackOverflowException regardless of implementation. It should return either true or false.

@StephenCleary
Copy link
Owner

@amir734jj: Nito.Comparers does not reflect over all properties of types. That's rarely the desired behavior.

The Default() comparer is behaving appropriately. You would have the same exact problem with the .NET Comparer<T>.Default or EqualityComparer<T>.Default.

@amir734jj
Copy link
Contributor Author

amir734jj commented Feb 21, 2019

I understand. I am passionate about this subject because I spend too much time daily writing and maintaining Equals and GetHashCode methods. We don't necessarily need to use reflection every time. We can just create an Expression and compile it once.

   public class Component
    {
        public double AccumulatedWeight { get; set; }

        public double Weight { get; set; }
        
        public Nested NestedRef { get; set; }
    }
    
    public class Nested
    {
        public double NestedProp { get; set; }
    }
    
    class Program
    {
        static void Main(string[] args)
        {   
            var c1 = new Component
            {
                Weight = 0.7,
                AccumulatedWeight = 0.2,
                NestedRef = new Nested
                {
                    NestedProp = 223
                }
            };
            
            var c2 = new Component
            {
                Weight = 0.7,
                AccumulatedWeight = 0.2,
                NestedRef = new Nested
                {
                    NestedProp = 223
                }
            };

            var result = BuildEquals<Component>()(c1, c2);
            
            Console.WriteLine(result);
        }
        
        public static Func<T, T, bool> BuildEquals<T>()
        {
            return ((Expression<Func<T, T, bool>>) BuildEquals(typeof(T))).Compile();
        }

        public static Expression BuildEquals(Type type)
        {
            bool IsComplexType(Type nestedType)
            {
                return !nestedType.Namespace.StartsWith("System.Collections") && nestedType.IsClass;
            }
            
            var arg1 = Expression.Parameter(type);
            var arg2 = Expression.Parameter(type);
            
            var body = type
                .GetProperties()
                .Select(x =>
                {
                    if (IsComplexType(x.PropertyType))
                    {
                        return (Expression) Expression.Invoke(BuildEquals(x.PropertyType),
                            Expression.Property(arg1, x.Name),
                            Expression.Property(arg2, x.Name));
                    }
                    else
                    {
                        return Expression.Equal(Expression.Property(arg1, x.Name), Expression.Property(arg2, x.Name));
                    }
                })
                .Aggregate((x, y) => Expression.And(x, y));

            return Expression.Lambda(body, arg1, arg2);
        } 

@StephenCleary
Copy link
Owner

StephenCleary commented Feb 21, 2019

You can use a similar approach to BuildEquals along with Nito.Comparers as such:

public static IFullEqualityComparer<T> BuildAllMembersEqualityComparer<T>()
{
    var result = EqualityComparerBuilder.For<T>().Null();
    var type = typeof(T);
    var parameter = Expression.Parameter(type);
    foreach (var prop in type.GetProperties())
    {
        var expression = Expression.Property(parameter, prop.Name);
        dynamic selector = Expression.Lambda(expression, parameter).Compile();
        dynamic childComparer = null;
        if (IsComplexType(prop.PropertyType))
            childComparer = ((MethodInfo)MethodBase.GetCurrentMethod()).MakeGenericMethod(prop.PropertyType).Invoke(null, null);
        result = EqualityComparerExtensions.ThenEquateBy(result, selector, childComparer);
    }
    return result;

    bool IsComplexType(Type nestedType) => !nestedType.Namespace.StartsWith("System.Collections") && nestedType.IsClass;
}

This code takes a similar approach as run-time comparers: starting with Null (everything is equivalent), and adding a ThenEquateBy clause for each member.

Using Nito.Comparers in this way, the same lambda expressions are used to implement both Equals and GetHashCode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants