[Mono-dev] Bugzilla Bug 475860, or, "the wrong patch for the right job"

[David Mitchell]

I'm investigating http://bugzilla.novell.com/show_bug.cgi?id=475860; I've
come up with a patch that addresses the issue, but I'm certain that it's the
wrong way to solve the problem. Here's the patch:

---
Index: ecore.cs
===================================================================
--- ecore.cs	(revision 126868)
+++ ecore.cs	(working copy)
@@ -4027,6 +4027,12 @@
 						Methods [j++] = m;
 				}
 				nmethods = j;
+
+				if (nmethods == 0 && candidate_overrides != null) {
+					foreach (MethodBase m in candidate_overrides)
+						Methods [j++] = m;
+					nmethods = j;
+				}
 			}
 
 			//
@@ -4291,11 +4297,11 @@
 			// If the method is a virtual function, pick an override closer to the
LHS type.
 			//
 			if (!IsBase && best_candidate.IsVirtual) {
-				if (TypeManager.IsOverride (best_candidate))
+/*				if (TypeManager.IsOverride (best_candidate))
 					throw new InternalErrorException (
-						"Should not happen.  An 'override' method took part in overload
resolution: " + best_candidate);
+						"Should not happen.  An 'override' method took part in overload
resolution: " + best_candidate);*/
 
-				if (candidate_overrides != null) {
+				if (!TypeManager.IsOverride (best_candidate) && candidate_overrides !=
null) {
 					Type[] gen_args = null;
 					bool gen_override = false;
 					if (TypeManager.IsGenericMethod (best_candidate))
---

The reason that the code listed in the Bugzilla bug fails to compile is that
at the time of overload resolution, the method table for the family of
Reference classes has not been completely initialized. Because of this, when
TypeManager.TryGetBaseDefinition is called, it fails, and no candidates are
added to the list of method overloads.

The patch included above addresses this problem by allowing overrides to be
added to the list of candidates if no other candidates have been found.
While this allows the example code to compile and even produces correct
results at runtime, I'm pretty sure that the most appropriate way to solve
this problem would be to instead make sure that the method table is
appropriately populated before we even get to this code (also, a comparison
of the IL produced by gmcs and the IL produced by csc reveals that csc
produces virtual calls to Reference::CompareTo(), while gmcs produces
virtual calls to ReferencePoint::CompareTo() and
ReferenceRange::CompareTo()).

I'm afraid that fixing this bug will require a deeper knowledge of the
compiler than I currently possess. Can anyone suggest a more appropriate way
of solving this problem?

--Dave

PS-I've extended the example code attached to the Bugzilla a bit to improve
my ability to test it. Here's a copy of my current version (which can be
compiled/run as a program):

using System;

namespace MonoBug
{
	public abstract class Reference :
		IEquatable<Reference>,
		IComparable<Reference>,
		IComparable<ReferencePoint>,
		IComparable<ReferenceRange>
	{
		public abstract bool Equals(Reference other);
		public int CompareTo(Reference other)
		{
			if (other == null)
				return 1;
			var otherRange = other as ReferenceRange;
			if (otherRange != null)
				return CompareTo(otherRange);
			else
				return CompareTo((ReferencePoint)other);
		}

		public abstract int CompareTo(ReferencePoint other);
		public abstract int CompareTo(ReferenceRange other);
	}

	public class ReferencePoint : Reference, IEquatable<ReferencePoint>
	{
		public ReferencePoint(int value)
		{
			_value = value;
		}

		public override bool Equals(Reference other)
		{
			return Equals(other as ReferencePoint);
		}

		public bool Equals(ReferencePoint other)
		{
			if (other == null)
				return false;
			return _value == other._value;
		}

		public override int CompareTo(ReferencePoint other)
		{
			return _value.CompareTo(other._value);
		}

		public override int CompareTo(ReferenceRange other)
		{
			if (other == null)
				return 1;

			// compare to start (if equal, a point is always before a range)
			int nCompare = CompareTo(other.Start);
			if (nCompare == 0)
				nCompare = -1;
			return nCompare;
		}

		int _value;
	}

	public class ReferenceRange : Reference, IEquatable<ReferenceRange>
	{
		public ReferenceRange(int start, int end)
		{
			_start = new ReferencePoint(start);
			_end = new ReferencePoint(end);
		}

		public override bool Equals(Reference other)
		{
			return Equals(other as ReferenceRange);
		}

		public bool Equals(ReferenceRange other)
		{
			return Start.Equals(other.Start) && End.Equals(other.End);
		}

		public override int CompareTo(ReferencePoint other)
		{
			if (other == null)
				return 1;
			return -other.CompareTo(this);
		}

		public override int CompareTo(ReferenceRange other)
		{
			if (other == null)
				return 1;

			int nCompare = Start.CompareTo(other.Start);
			if (nCompare == 0)
				nCompare = End.CompareTo(other.End);

			return nCompare;
		}

		public ReferencePoint Start { get { return _start; } }
		public ReferencePoint End { get { return _end; } }

		ReferencePoint _start;
		ReferencePoint _end;
	}

	public static class Program
	{
		public static void Main(string[] arguments)
		{
			var points = new [] {
				new ReferencePoint(0),
				new ReferencePoint(1),
				new ReferencePoint(2),
			};

			var ranges = new[] {
				new ReferenceRange(0, 1),
				new ReferenceRange(1, 2),
				new ReferenceRange(0, 2),
			};

			Console.WriteLine("Comparing points to points:");
			for (int i = 0; i < points.Length; i++)
			{
				for (int j = i; j < points.Length; j++)
				{
					Console.WriteLine("({0}, {1}) {2}", i, j,
points[i].CompareTo(points[j]));
					Console.WriteLine("({0}, {1}) {2}", j, i,
points[j].CompareTo(points[i]));
				}
			}
			Console.WriteLine();

			Console.WriteLine("Comparing ranges to ranges:");
			for (int i = 0; i < ranges.Length; i++)
			{
				for (int j = i; j < points.Length; j++)
				{
					Console.WriteLine("({0}, {1}) {2}", i, j,
ranges[i].CompareTo(ranges[j]));
					Console.WriteLine("({0}, {1}) {2}", j, i,
ranges[j].CompareTo(ranges[i]));
				}
			}
			Console.WriteLine();

			Console.WriteLine("Comparing points to ranges (and vice versa):");
			for (int i = 0; i < points.Length; i++)
			{
				for (int j = 0; j < ranges.Length; j++)
				{
					Console.WriteLine("(point {0}, range {1}) {2}", i, j,
points[i].CompareTo(ranges[j]));
					Console.WriteLine("(range {0}, point {1}) {2}", j, i,
ranges[j].CompareTo(points[i]));
				}
			}
		}
	}
}
-- 
View this message in context: http://www.nabble.com/Bugzilla-Bug-475860%2C-or%2C-%22the-wrong-patch-for-the-right-job%22-tp22007653p22007653.html
Sent from the Mono - Dev mailing list archive at Nabble.com.