Improve extension method overload resolution: Use generic type constraints to exclude current namespace's incompatible extension method in favor of imported namespace's compatible one

[HappyNomad]

I've had an extension method with this signature for ages:

public static IEnumerable<Type> Ancestors( this Type startNode ) => null;

I just added another with the same name but a different signature and in a different namespace:

public static IEnumerable<T> Ancestors<T>( this T startNode )
	where T : Node
	=> null;

Now I get compiler errors when calling the older one:

The type 'System.Type' cannot be used as type parameter 'T' in the generic type or method 'NodeExtensions.Ancestors(T)'. There is no implicit reference conversion from 'System.Type' to 'Node'

The compiler is trying to bind the older calls to the new extension method. The error goes away when I do one of the following:
a) Eliminate the new extension method's type parameter.
b) Move the older extension method into the same namespace as everything else.
c) Move the new extension method into a completely different namespace.

For my use-case, neither option 'a' nor 'b' is acceptable. Option 'c' is not ideal either, but it will suffice. Even so, this quirky dependency on the namespace implies a compiler bug to me. Is this in fact a language issue, or should I post this at https://github.com/dotnet/roslyn/issues?

Here's the complete example:

using System;
using System.Collections.Generic;
using Test.Reflection;

namespace Test
{
	public class Node { }

	public class SomeTest
	{
		public SomeTest()
		{
			typeof( string ).Ancestors();
			new Node().Ancestors();
		}
	}

	public static class NodeExtensions
	{
		public static IEnumerable<T> Ancestors<T>( this T startNode )
			where T : Node
			=> null;
	}
}

namespace Test.Reflection
{
	public static class ReflectionExtensions
	{
		public static IEnumerable<Type> Ancestors( this Type startNode )
			=> null;
	}
}

[HaloFour]

#98 will allow overload resolution to consider generic type constraints, looks like it's slated for C# 7.3.

[HappyNomad]

@HaloFour Nice but if it's not yet supported then why does it currently work in particular namespace configurations?

[HaloFour]

@HappyNomad

Looks like a quirk in how extension methods are resolved. If the compiler can find a compatible extension method in the current namespace it doesn't appear to start looking in imported namespaces. The compiler isn't using generic type constraints to exclude the one found extension method so it tries to bind to NodeExtensions which fails.

This isn't limited to generics, either. If you have a compatible extension method for any type in the current namespace the compiler won't try to find better extension methods in other namespaces, e.g.:

namespace A {
    using B;
    
    public class Foo { }
    public class Bar : Foo { }
    
    public class C {
        void M() {
            var bar = new Bar();
            bar.X(); // calls FooExtensions.X(bar)
        }
    }
    
    public static class FooExtensions {
        public static void X(this Foo foo) { }
    }
}


namespace B {
    using A;
    
    public static class BarExtensions {
        public static void X(this Bar bar) { }
    }
}

Perhaps another candidate for the same "bestest betterness" proposal since that deals with improving overload resolution and I imagine that applies to extension methods as well.

[CoolDadTx]

I just ran into this as well and would love to know when this is going to get fixed. With extension methods being used for everything and the fact that Intellisense is reporting the "correct" overload while the compiler is choosing the wrong overload this is really confusing.

In my case we have a core logging library that has a log method that accepts a string and trace level. We also have extension methods for some common cases like begin/end activity. In some of our services we create additional extension methods to customize the logging messages once but the compiler chooses the "in namespace" extensions over the core extensions and causes a stack overflow.

[CyrusNajmabadi]

I just ran into this as well and would love to know when this is going to get fixed.

There is no 'champion' for this issue. So the current answer is: unknown.

[CyrusNajmabadi]

but the compiler chooses the "in namespace" extensions over the core extensions and causes a stack overflow.

This will be an issue no matter what. It found a match an picked it. And that behavior could certainly not change in the future (as that would be a breaking change).

[CoolDadTx]

No I don't believe it should be an issue. Overload resolution is supposed to prefer no type conversions of arguments over conversion. If there is an exact match overload then it chooses that in all cases. That is the scenario I have.

void Foo ( string );
void Foo ( object );

Foo("Hello");  //Calls Foo(string)

This is correct overload resolution. If you move Foo(string) to a separate namespace and then add a using so it is in scope the compiler should bring it into scope and therefore at resolution time it should match Foo(string) but it doesn't. It matches Foo(object) because that is in the "current" namespace. That is incorrect and what @HaloFour mentions. The compiler seems to be download overload resolution on only the current namespace objects first and if it doesn't find any matches then it moves out to the others rather than looking for the best match across all overloads in scope.

Interestingly Intellisense is picking the right overload when you look at it.

[HaloFour]

The fact that the compiler is already picking an "overload" means that a change has the potential to break existing code. The big question is how does the spec describe how extension methods are resolved and if there is a preference when it comes to namespaces. If the compiler is behaving as the spec describes then the story is basically closed. If the compiler isn't behaving as the spec describes then there may be more of a conversation, but given that this behavior is already a part of the compiler it's more likely that the spec would be modified to meet what the compiler actually does than vice versa. It's also worthwhile to compare the current behavior to previous versions of the compiler (particularly pre-Roslyn, C# 5.0 and earlier) to see if they behaved differently as this might be considered a regression if that is the case.

[CyrusNajmabadi]

No I don't believe it should be an issue.

It is. You have code that is compiling today with a specific meaning. The compiler cannot change that meaning in the future because it means that literally recompiling the same code would result in different behavior. that's teh very definition of a breaking change :)

[CyrusNajmabadi]

Interestingly Intellisense is picking the right overload when you look at it.

that's a bug in intellisense then. can you give me a full repro? if so, i'll file the issue against dotnet/rosln for you.

[CoolDadTx]

@CyrusNajmabadi

that's teh very definition of a breaking change

Yes but when it is a compiler (regression) bug it severe enough to warrant changing then it is necessary. The compiler has "broken" behavior before in these cases. As @HaloFour mentioned the team would need to look at the spec but my understanding of how importing namespaces work indicates this to be a bug. Unfortunately I don't know if it is a regression bug or not. If it is a regression bug then fixing it would probably trump behavioral change. If it didn't work that way before then the spec would need to be updated to add clarification. Irrelevant it is not for us to determine whether it should be fixed or not but rather the compiler team so I'll leave it at that.

"that's a bug in intellisense then. can you give me a full repro?"

Refer to my original post. It replicates the issue. The Intellisense list pulls in all overloads from all imported namespaces and therefore as soon as you type the first argument as a string it matches to that overload even though at runtime it will actually use the object version instead.

[CyrusNajmabadi]

Yes but when it is a compiler (regression) bug

What regression? Has the behavior you're asking for ever worked this way for extension methods? If not, it's not a regression.

Can you give a piece of code with extension methods that the compiler used to compile in one way, but now compiles differently?

Thanks!

[CyrusNajmabadi]

Refer to my original post. It replicates the issue

Can you provide a code sample. Thanks!

[CoolDadTx]

What regression? Has the behavior you're asking for ever worked this way for extension methods? If not, it's not a regression.
I wasn't referring to this issue. I said that the compiler has broken compatibility in the past when a regression bug has been found and that as @HaloFour mentioned they'll have to determine whether this is a regression bug or not. Whether it is a regression bug or not will help determine whether a breaking change is appropriate or not.

I feel at this point that we're just discussing a mute point on this though so I'll refrain from further comments on whether this is breaking, regresion or whatever and wait for the compiler team to let us know.

[HaloFour]

Per 7.6.5.2 of the C# specification this behavior seems to be intentional:

The search for C proceeds as follows:

• Starting with the closest enclosing namespace declaration, continuing with each enclosing namespace declaration, and ending with the containing compilation unit, successive attempts are made to find a candidate set of extension methods:
  • If the given namespace or compilation unit directly contains non-generic type declarations Ci with eligible extension methods Mj, then the set of those extension methods is the candidate set.
  • If namespaces imported by using namespace directives in the given namespace or compilation unit directly contain non-generic type declarations Ci with eligible extension methods Mj, then the set of those extension methods is the candidate set.

Once the compiler has scanned the current namespace it has a candidate set of extension methods.

[CyrusNajmabadi]

and wait for the compiler team to let us know.

AFAICT there is no regression here. Even testing the old native compiler produces the same results.

[HappyNomad]

Has this issue been fixed yet? (Not sure why it's been converted to a question.)

[CyrusNajmabadi]

There is no actual issue identified. You seem to be describing a situation where hte compiler is operating as per hte language specification. If you want the compiler to be changed, an actual proposal needs to be made to the actual language specification. Discussions are our preferred way to allow you to converse on the topic and potentially lead toward an actual proposal. Absent a proposal here, no changes will be made here given that this seems to all be working as per spec.

[HappyNomad]

I did indeed identify an issue in my original post - it's pretty clear if you care to read over it again. But you're saying that I must instead submit a proposal to the language specification. Where and how do I do that?

[CyrusNajmabadi]

I did indeed identify an issue in my original post

I'm not seeing an issue. You said: > this quirky dependency on the namespace implies a compiler bug to me.

There is no compiler bug here. This is how the language is spec'ed it :)

But you're saying that I must instead submit a proposal to the language specification. Where and how do I do that?

The discussion can help you come up with a set of semantics on how you want this to work. So this discussion is hte right place. Once it's at a reasonable point where it seems like something can fit, someone may potentially want to champion this work and help come up with a true proposal here on this :)

[CyrusNajmabadi]

Currently though, i'm not seeing an actual explanation on how you want this to work. It doesn't suffice to say "the language is wrong here", we need to discuss what the actual rules would be that a compiler would need to follow. Those rules need to be pretyt clear, and cannot introduce breaks in existing code.

[HaloFour]

Don't know how to convert this into spec-ese but what if ...

1. the compiler identifies overload candidates for an extension method in the current namespace, but ...
2. none of those overload candidates can be applied to the call due to generic constraints, then ...
3. the compiler considers extension method candidates from imported namespaces.

That wouldn't result in any breaks as any affected code currently won't compile.