Extension methods overridden by class gives no warning

Question

I had a discussion in another thread, and found out that class methods takes precedence over extension methods with the same name and parameters. This is good as extension methods won't hijack methods, but assume you have added some extension methods to a third party library:

public class ThirdParty
{
}

public static class ThirdPartyExtensions
{
    public static void MyMethod(this ThirdParty test)
    {
        Console.WriteLine("My extension method");
    }
}

Works as expected: ThirdParty.MyMethod -> "My extension method"

But then ThirdParty updates it's library and adds a method exactly like your extension method:

public class ThirdParty
{
    public void MyMethod()
    {
        Console.WriteLine("Third party method");
    }
}

public static class ThirdPartyExtensions
{
    public static void MyMethod(this ThirdParty test)
    {
        Console.WriteLine("My extension method");
    }
}

ThirdPart.MyMethod -> "Third party method"

Now suddenly code will behave different at runtime as the third party method has "hijacked" your extension method! The compiler doesn't give any warnings.

Is there a way to enable such warnings or otherwise avoid this?

Answer 1

Nope - this is a known downside of extension methods, and something to be very careful about. Personally I wish that the C# compiler would warn you if you declared an extension method which would never be called other than via the normal static route (ExtensionClassName.MethodName(target, ...)).

It probably wouldn't be too hard to write a little tool to examine all the extension methods in an assembly and issue warnings that way. It probably wouldn't need to be very precise to start with: just warning if there's already a method with the same name (without worrying about parameter types) would be a good start.

EDIT: Okay... here's a very crude tool to at least give a starting point. It appears to work at least to some extent with generic types - but it's not trying to do anything with parameter types or names... partly because that becomes tricky with parameter arrays. It also loads assemblies "fully" instead of with reflection only, which would be nicer - I tried the "proper" route, but ran into some problems which weren't immediately trivial to resolve, so fell back to the quick and dirty route :)

Anyway, hopefully it'll be useful to someone, somewhere.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;

public class ExtensionCollisionDetector
{
    private static void Main(string[] args)
    {
        if (args.Length == 0)
        {
            Console.WriteLine
                ("Usage: ExtensionCollisionDetector <assembly file> [...]");
            return;
        }
        foreach (string file in args)
        {
            Console.WriteLine("Testing {0}...", file);
            DetectCollisions(file);
        }
    }

    private static void DetectCollisions(string file)
    {
        try
        {
            Assembly assembly = Assembly.LoadFrom(file);
            foreach (var method in FindExtensionMethods(assembly))
            {
                DetectCollisions(method);
            }
        }
        catch (Exception e)
        {
            // Yes, I know catching exception is generally bad. But hey,
            // "something's" gone wrong. It's not going to do any harm to
            // just go onto the next file.
            Console.WriteLine("Error detecting collisions: {0}", e.Message);
        }
    }

    private static IEnumerable<MethodBase> FindExtensionMethods
        (Assembly assembly)
    {
        return from type in assembly.GetTypes()
               from method in type.GetMethods(BindingFlags.Static |
                                              BindingFlags.Public |
                                              BindingFlags.NonPublic)
               where method.IsDefined(typeof(ExtensionAttribute), false)
               select method;
    }


    private static void DetectCollisions(MethodBase method)
    {
        Console.WriteLine("  Testing {0}.{1}", 
                          method.DeclaringType.Name, method.Name);
        Type extendedType = method.GetParameters()[0].ParameterType;
        foreach (var type in GetTypeAndAncestors(extendedType).Distinct())
        {
            foreach (var collision in DetectCollidingMethods(method, type))
            {
                Console.WriteLine("    Possible collision in {0}: {1}",
                                  collision.DeclaringType.Name, collision);
            }
        }
    }

    private static IEnumerable<Type> GetTypeAndAncestors(Type type)
    {
        yield return type;
        if (type.BaseType != null)
        {
            // I want yield foreach!
            foreach (var t in GetTypeAndAncestors(type.BaseType))
            {
                yield return t;
            }
        }
        foreach (var t in type.GetInterfaces()
                              .SelectMany(iface => GetTypeAndAncestors(iface)))
        {
            yield return t;
        }        
    }

    private static IEnumerable<MethodBase>
        DetectCollidingMethods(MethodBase extensionMethod, Type type)
    {
        // Very, very crude to start with
        return type.GetMethods(BindingFlags.Instance |
                               BindingFlags.Public |
                               BindingFlags.NonPublic)
                   .Where(candidate => candidate.Name == extensionMethod.Name);
    }
}

Answer 2

I like Jon's answer, but there's another approach similar to Daniel's. If you have a lot of extension methods, you can define a "namespace" of sorts. This works best if you have a stable interface to work from (i.e., if you knew IThirdParty wasn't going to change). In your case, however, you'd need a wrapper class.

I've done this to add methods for treating strings as file paths. I defined a FileSystemPath type that wraps a string and provides properties and methods such as IsAbsolute and ChangeExtension.

When defining an "extension namespace," you need to provide a way to enter it and a way to leave it, as such:

// Enter my special namespace
public static MyThirdParty AsMyThirdParty(this ThirdParty source) { ... }

// Leave my special namespace
public static ThirdParty AsThirdParty(this MyThirdParty source) { ... }

The method to "leave" the "namespace" may work better as an instance method instead of an extension method. My FileSystemPath just has an implicit conversion to string, but this does not work in all cases.

If you want MyThirdParty to have all the currently-defined members of ThirdParty as well as the extension methods (but not future-defined members of ThirdParty), then you'd have to forward member implementations to the wrapped ThirdParty object. This can be tedious, but tools like ReSharper can do it semi-automatically.

Final Note: the "As" prefix on entering/leaving the namespace is a sort of unspoken guideline. LINQ uses this system (e.g., AsEnumerable, AsQueryable, AsParallel leave the current "namespace" and enter another one).

I wrote a blog post early this year about what I call "extension-based types." There are more pitfalls than just the inability to override instance methods. However, it is a very interesting concept.