views:

95

answers:

3

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?

+5  A: 

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);
    }
}
Jon Skeet
Will upvote the answer once I get votes back
Graphain
Interesting point you allude to Jon - if my understanding is right, you would have to recompile your code that referenced the `ThirdParty` DLL before their method would be called, correct? So the OP is not entirely accurate in that simply updating the ThirdParty DLL would not be enough - you'd have to have rebuilt too (which is where your compiler warning would save the day).
Dan Puzey
@Dan: The problem is that the compiler doesn't give any warnings. It just happily makes your code call the third party library instead of the extension method
simendsjo
@Dan: Yes, you need to rebuild before you'll see this problem. But that's a fairly common case :)
Jon Skeet
I appreciate the issue - and yes, it's common that you'd rebuild, but that does change the shape of the problem somewhat. I suspect that the pragmatic solution would be to ensure that you have unit tests against your extension methods to prove that they work when built. Alternatively, in terms of generating build-time warnings, this is something that a custom FxCop rule could detect.
Dan Puzey
Why should the compiler warn you when you shoot yourself in the foot?If you're an API developer, C# provides the sealed keyword for your pleasure, but there is simply no reason for the compiler or the language to hold your customer's hand when they go and break your class.
David Souther
@David: The point is that *someone else* is breaking your class. For example, suppose I have a `CopyTo` extension method on `Stream`. That's entirely reasonable in .NET 3.5 - but useless in .NET 4.0. My method will no longer be called. Now, are you saying I should examine *every single new method* introduced in .NET 4.0 to see if it's going to break my extension methods? *My* code hasn't changed... but its behaviour has. `sealed` is irrelevant here. In what way have I shot myself in the foot?
Jon Skeet
The problem is how do you turn *off* the warning if the behaviour is desirable or if you don't care? There's no obvious way to do so. Furthermore: how is this different than putting a method on a derived class that is a better match than a method on a base class? We don't warn about that either. Should we? Seems like there would be a lot of spurious warnings.
Eric Lippert
@Eric: I think turning off the warning would just be a case of `#pragma` as for any other warning (at the declaration site). It's brutal, but it would be an unlikely thing to ignore, IMO :) The "better match" question is a more interesting one... but I'm not sure it's quite the same situation, because that's *you* adding the method after the one that's already one the base class. That's your choice. In this case, it's a change in the someone else's class which is changing the behaviour of your code. That's why it's harder to pick up. Or maybe I'm missing something about your scenario.
Jon Skeet
@Eric: If you like, I'd be happy to take this up by email where we have a bit more room to play... and then edit a summary into my answer here when we've come to a conclusion.
Jon Skeet
@Eric - I would turn off the warning by deleting the recently obsolesced extension method (if the behavior is the same), or refactoring it with a new name (if the behavior is different). That may sound drastic, but after all it's an extension method can no longer be called as an extension method. That's a pretty serious mark against keeping the old code as-is.
Jeffrey L Whitledge
@Jeffrey: What if the offending extension method is in a library that *you don't own* but you want to keep using *other* extension methods in that library? If you own all the code then the problem is easy to solve; it's those scenarios where you are using multiple third-party libraries that don't work well together that is the problem. We don't want to produce situations where you always get warnings pointing out flaws in code that you didn't write and cannot change.
Eric Lippert
@Eric: In this hypothetical situation, are we still *building* this code that we don't own? If that's the case, don't we have the same problem with *any* warning? For example, suppose we get the "add new or override" warning... aren't we in exactly the same situation?
Jon Skeet
A: 

One possible way to reduce the chance of this happening is to include the abbreviation Ext at the end of the extension method name. I know, this is very ugly, but it reduces the chances of this happening.

MyMethod_Ext

or

MyMethodExt
Daniel Dyson
Yeah, quite ugly. It's nearly as ugly as calling the extension class itself: ThirdPartyExtensions.MyMethod(instance)
simendsjo
Yeah, quite, quite ugly. I just thought I would put it out there as a possibilty. The chances of this happening in the first place a quite low. This just reduces the chances evn further. I like Jon Skeet's idea of a utility to check for these possibilities.
Daniel Dyson
+1  A: 

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.

Stephen Cleary