Warn if there exists an instance method with the same name as an extension method

[alrz]

class C 
{
  public void M() {}
}

static class X
{
  // this could never be called as an instance invocation, remove `this` modifier
  public static void M(this C @this) {}
}

Related to warning waves.

[sharwell]

💭 Most cases where I've seen this arise involved dealing with compatibility across multiple releases, or subtle differences in the accessibility of the methods. It seems like a warning that would be difficult to get "just right", and at that point I'm not sure how much it would be responsible for better code as a result.

[alrz]

I'm not sure how much it would be responsible for better code as a result.

It would prevent one to accidentally write such methods, potentially due to false expectation, and thereafter giving the false signal to the reader that this can be used as an instance invocation, while that might not the case and the code is actually unreachable. You should literally navigate the whole class definition to make sure - if you had a clue. I expect this would be one of those warnings and lights up in the codebase without you even expecting it.

PS: could be an IDE analyzer - I still don't have a clear vision on warning waves vs. IDE analyzers and that what should go to either of those categories.

[jcouv]

Linking to warning wave issue

[benaadams]

Example of the unreachable code https://github.com/dotnet/roslyn/pull/47501

[CyrusNajmabadi]

this could never be called as an instance invocation, remove

This seems hard to prove. However, it might be possible in trivial cases.

It may also be the case that this method is there got source compatibility and is being compiled to go with other versions of a lot that don't have the instance method.

I would be very cautious with this rule.

[alrz]

This seems hard to prove

I think a mix of overload+shadowing singnature comparison could be used which excludes the first arg.

It may also be the case that this method is there got source compatibility

if this is ever an exception user can suppress it probably with a comment explaining the reason.

[alrz]

Also this might light up if a method is added to a library and the client happen to have an extension with the same signature which is not any different than shadowing warning and is generally useful.

[RikkiGibson]

It seems like the really problematic scenario looks like this:

graph TD;
td["Type declaration"]-->_{IVT}-->us["Usage site"];
ed["Extension declaration"]-->us["Usage site"];

Where "Extension declaration" cannot access internal members of "Type declaration", but "Usage site" can access those internal members because of an InternalsVisibleToAttribute.

I would suggest the following flow to handle this.

graph TD;
ivt["Does the original type give IVT to anything?"]-->y{Yes}-->ivty["Pretend the extension declaration can access the internals of the original type."];
ivt-->n{No}-->ivtn["Treat the extension declaration as having its actual level of accessibility to the original type."];
ivty-->callitself["Can the extension method call itself in reduced form by passing its own parameters as arguments?"];
ivtn-->callitself;
callitself-->y1{Yes}-->nowarning["No warning"];
callitself-->n1{No}-->warning["warning: this extension method is superseded by instance member ..."];

Where "calling itself by passing its own parameters as arguments" looks like

public class Underlying
{
    public int Extension(int input) => throw null!;
}

static class Ext
{
    public static int Extension(this Underlying underlying, int input)
    {
        // does this call the containing method 'Ext.Extension' or an instance method?
        return underlying.Extension(input);
    }
}

Consider using a different error code if the shadowing can only occur through an IVT.

I don't know how this would best be simulated from the context of an analyzer. But that's what I'd use as a starting point.