Port FxCop rule CA1011: ConsiderPassingBaseTypesAsParameters

[ghost]

Title: Consider passing base types as parameters

Description:

When a base type is specified as a parameter in a method declaration, any type that is derived from the base type can be passed as the corresponding argument to the method. If the additional functionality that is provided by the derived parameter type is not required, use of the base type enables wider use of the method.

Dependency: None

Notes:

[natidea]

MSDN: https://msdn.microsoft.com/en-us/library/3hk32yyz.aspx

[333fred]

@dotnet/roslyn-analysis @sharwell @kuhlenh I'd like some input on how correct we think the fixer for this should be. For example:

interface I1 {}
class Foo : I1 {}

class Bar
{
    void Baz(I1 i1) { }
    void Baz(Foo foo)
    {
        // Uses only I1 functionality
    }
}

In this case, if we offer to fix Baz(Foo) so it takes Baz(I1), there will be an error in the resulting code, as there's two identical Baz signatures. Personally, I think the incorrectness is ok in this scenario. For FixAll scenarios of this, there are possibilities that we end up fixing multiple methods such that they clash after both have been fixed, and that's not detectable until we actually have all things that need to be fixed. FXCop didn't have any logic for detecting whether fixing the issue would result in this breaking, so I'm thinking we shouldn't either.

[jamesqo]

I think there are cases in which this fix should not be applied, e.g. when the type encodes information about what the object represents. One example:

interface IResource {
    string Location { get; }
}

class FileResource : IResource {
    // Location represents a file path
}
class HttpResource : IResource {
    // Location represents an HTTP url
}

// Should not be corrected to IResource
string GetContent(FileResource res) {
    return File.ReadAllText(res.Location);
}

[333fred]

That's not something we're able to detect from the analyzer, though. If the user has a legitimate reason to not use the base class/interface, they can suppress the warning.

[jamesqo]

@333fred

they can suppress the warning.

I don't think there should be a warning in the first place. The term slipped my mind before, but types of this kind are actually so common that they have a term for them-- "marker interfaces" (e.g. interfaces with no members). Enabling this analyzer would issue a warning every time a marker interface would passed to a method, potentially resulting in a huge number of warnings for large codebases/people just disabling the analyzer entirely.

I think Info would be more appropriate.

[sharwell]

Enabling this analyzer would issue a warning every time a marker interface would passed to a method, potentially resulting in a huge number of warnings for large codebases/people just disabling the analyzer entirely.

This diagnostic should only be reported when the method only uses members that are defined in the base type. Under these rules marker interfaces would typically not trigger this.

[sharwell]

In this case, if we offer to fix Baz(Foo) so it takes Baz(I1), there will be an error in the resulting code, as there's two identical Baz signatures. Personally, I think the incorrectness is ok in this scenario.

We should avoid reporting a diagnostic if another overload exists that already takes the base type.

[333fred]

@sharwell yes, that was on my list of things to test.

[333fred]

@sharwell I think I misunderstood your commend when I originally replied. How would you suggest dealing with multiple methods that can be simplified to the same base-type in the fix-all scenario? Personally, I think that if the user has a legitimate reason to not use the base type (and probably merge this method and the conflicting implementation) then they should suppress the warning and indicate why in their code.

[Evangelink]

We shall be careful with how we implement this rule. I have seen quite a lot of wrong behaviors from people following this rule from ReSharper (especially around IEnumerable<T>).

Also when implementing this rule by myself, I have noticed that when implementing analyzers most of the time I was creating private methods taking a specific node type in parameter and the rule was suggesting to use SyntaxNode instead which is ok from the point of view of the APIs I was using but wrong from the point of view of the shape.

[carlossanlop]

FYI - the proposal has been approved: dotnet/runtime#61916 (comment)