Implement SA1315

[pdelvo]

Fixes #1949.

[codecov-io]

Current coverage is 93.40%

Merging #2015 into master will increase coverage by +0.08% as of e71d7b3

@@            master   #2015   diff @@
======================================
  Files          561     564     +3
  Stmts        35455   35636   +181
  Branches      2272    2282    +10
  Methods                          
======================================
+ Hit          33089   33286   +197
- Partial        749     757     +8
+ Missed        1617    1593    -24

Review entire Coverage Diff as of e71d7b3

Powered by Codecov. Updated on successful CI builds.

[sharwell]

I think this needs to be SA1315 (see #1925).

[vweijsters]

This seems a copy-paste error.

[vweijsters]

💭 It would be nice to have a few test cases that would cover multiple levels of inheritance. Like:

public class TopLevelBaseClass
{
    public void Method(int baseArg);
}

public class IntermediateBaseClass : TopLevelBaseClass
{
}

public class TestClass : IntermediateBaseClass
{
  public void override Method(int arg);
}

[vweijsters]

❓ How would the scenario above work when the IntermediateBaseClass redefines the parameter names with an override (like: public override Method(int renamedArg))?

[vweijsters]

💭 I think a test is when a sub class redefines a method declaration with the new keyword (which should not trigger a diagnostic) would be nice.

[vweijsters]

❓ Is it possible that context.Symbol is null because the method declaration is incomplete? -> If so, the statement above needs a null check as well.

[pdelvo]

I don't think so. It should work like it does with syntax node actions. I register for a method symbol and I should get one.

[vweijsters]

❓ The code fix currently can offer multiple rename options based on the original definitions found. Is this a good way address this? I would expect that the root declaration will determine the name and that there will be no alternatives.

[sharwell]

❓ The code fix currently can offer multiple rename options based on the original definitions found. Is this a good way address this? I would expect that the root declaration will determine the name and that there will be no alternatives.

The following code can produce this:

interface IInterface
{
  void Method(string p1);
}

class BaseClass
{
  public abstract void Method(string p2);
}

class Derived : BaseClass, IInterface
{
  public override void Method(string p)
  {
  }
}

The parameter p needs to be p1 to match the interface, but p2 to match the base class. If any code fix is offered in this case, it should be renaming it to p2, since the correct way to fix the method with respect to the interface is add a new method:

void IInterface.Method(string p1)
{
  this.Method(p1);
}

In the case of multiple overloads in a chain of base classes, the analyzer and code fix should only consider the method which would be called using the base. syntax. That method could have a SuppressWarnings attribute applied to it for this diagnostic, and we don't want all methods which override it to have to repeat that suppression.

[pdelvo]

What if there are multiple interfaces with parameter names that do not match?

[ghost]

@pdelvo @sharwell Would you consider also submitting this code to the dotnet/roslyn-analyzers repo as the implementation of rule CA1725, ParameterNamesShouldMatchBaseDeclaration? This is issue dotnet/roslyn-analyzers#457.

@srivatsn FYI

[vweijsters]

@lgolding @srivatsn If this is still wanted, I'll be happy to start working on it this week. I checked with @pdelvo and he is OK with it.

[ghost]

Hi Vincent,

We'd be delighted if you would contribute your work on this analyzer SA1315 to the roslyn-analyzers repo as rule CA1725.

The roslyn-analyzers repo includes skeleton code for all the analyzers we haven't yet implemented. The skeleton code for the CA1725 analyzer is here, and the skeleton code for the fixer is here.

If you search the roslyn-analyzers repo for "CA1725" you'll see that for both the analyzer and the fixer, there is a base class, and then there are derived classes for C# and VB versions. In your case, you have written a symbol analyzer, which works for both languages, so you would simply remove the C# and VB analyzers, declare the "core" analyzer to handle both C# and VB, and paste your implementation in there. I see that your fixer works at the syntax level, so you would paste your implementation into the C# version. You are welcome to contribute a VB fixer, but you certainly don't have to. At the moment, we are concentrating on analyzers and deferring work on fixers until we have a sufficient set of analyzers completed.

Feel free to email me with any questions (email in my profile).

Best regards,
Larry

[pdelvo]

Do we still want this analyzer in stylecop analyzers?

[vweijsters]

Good question, I have no real preference.
But if we do want to keep it, we need to line this one up with the one in roslyn-analyzers. I made several small modifications there.

[pdelvo]

I close this for now. @vweijsters maybe you can create a new PR with the changes you made