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

Infinite recursion in OneOfBase.GetHashCode() and Equals() #28

Closed
DanielSchuessler opened this issue Aug 24, 2018 · 7 comments
Closed

Comments

@DanielSchuessler
Copy link

If OneOfBase is used with the case classes being subclasses of the OneOfBase subclass, as in the readme:

    public abstract class PaymentResult : OneOfBase<PaymentResult.Success, PaymentResult.Declined, PaymentStatus.Failed>
    {
        public class Success : PaymentResult { }  
        public class Declined : PaymentResult { }
        public class Failed  : PaymentResult { public string Reason { get; set; } }
    }

and GetHashCode is not overridden in the case classes, OneOfBase<...>.GetHashCode will just call itself, resulting in a stack overflow. (If it is overriden, it works, but the base impl becomes pointless.). Same problem with Equals.

Not sure offhand what would be the best solution, especially without breaking the use case where the case types are not subclasses of the OneOfBase, and are possibly value types. And without making things harder for users who don't care about equality comparison (e.g. by adding a type constraint like IEquatableCaseClass).

@mcintyre321
Copy link
Owner

mcintyre321 commented Aug 31, 2018

Good find... this is a possible solution, although it involves changing the sig of OneOfBase:

public class OneOfBase<TThis, T0, T1, T2> : IOneOf where TThis : OneOfBase<TThis, T0, T1, T2>
{
	
	
	//...
	
	public override int GetHashCode()
	{
		unchecked
		{
			int hashCode;
			switch (_index)
			{
				case 0:
					hashCode = this.GetType() == typeof(TThis) ? (_value0?.GetHashCode() ?? 0) : base.GetHashCode();
					break;
				case 1:
					hashCode = this.GetType() == typeof(TThis) ? _value1?.GetHashCode() ?? 0 : base.GetHashCode();
					break;
				case 2:
					hashCode = this.GetType() == typeof(TThis) ? _value2?.GetHashCode() ?? 0 : base.GetHashCode();
					break;
				default:
					hashCode = 0;
					break;
			}
			return (hashCode * 397) ^ _index;
		}
	}
}

void Main()
{
        PaymentResult s = new PaymentResult.Success();
	s.GetHashCode().Dump();
}
public abstract class PaymentResult : OneOfBase<PaymentResult, PaymentResult.Success, PaymentResult.Declined, PaymentResult.Failed>
{
	public class Success : PaymentResult { }
	public class Declined : PaymentResult { }
	public class Failed : PaymentResult { public string Reason { get; set; } }
}

@DanielSchuessler
Copy link
Author

DanielSchuessler commented Aug 31, 2018

this.GetType() == typeof(TThis) would always be false because TThis equals PaymentResult.

How about this:

public class OneOfBase<T0, T1, T2> : IOneOf
{
  //...

  public override int GetHashCode()
  {
    unchecked
    {
      int hashCode;
      switch (_index)
      {
        case 0:
          hashCode = _value0 is OneOfBase<T0, T1, T2> ? base.GetHashCode() : (_value0?.GetHashCode() ?? 0);
          break;
        // ...
      }
      return (hashCode * 397) ^ _index;
    }
  }
}

maybe slightly faster (to be benchmarked):

public class OneOfBase<T0, T1, T2> : IOneOf
{
  static readonly bool isT0SubclassOfThis = typeof(OneOfBase<T0,T1,T2>).IsAssignableFrom(typeof(T0));

  //...

  public override int GetHashCode()
  {
    unchecked
    {
      int hashCode;
      switch (_index)
      {
        case 0:
          hashCode = isT0SubclassOfThis ? base.GetHashCode() : (_value0?.GetHashCode() ?? 0);
          break;
        // ...
      }
      return (hashCode * 397) ^ _index;
    }
  }
}

@mcintyre321
Copy link
Owner

mcintyre321 commented May 3, 2019

I think this can probably be fixed using a similar object.ReferenceEquals approach to: #48 (comment)

Will do that if I get some time to work on the library

@dzarda
Copy link

dzarda commented Apr 6, 2020

Do we have any more insight on this? It's proven to be a significant issue.

@mcintyre321
Copy link
Owner

Nope - I haven't got a lot of time to work on it.

However, the workaround is pretty simple - don't inherit from the OneOfBase (use a separate base type if you need), and add implicit cast operators to your case Types to get the same syntactical effect as inheriting.

@dzarda
Copy link

dzarda commented Apr 7, 2020

We've been using OneOfBase happily while overriding all derived Equals() methods. Trouble comes when this is left out...

@mcintyre321
Copy link
Owner

The latest version of OneOf does OneOfBase slightly differently (so some minor code fixes are needed), but avoids this issue.

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

3 participants