CA1716 warns for vb.net keyword in c#

[bqstony]

Analyzer package

Example: [Microsoft.CodeQuality.Analyzers]

Package Version

v2.6.2

Diagnostic ID

Example: CA1716

Repro steps

1. Create a abstract class in C# with method that attribute named step (class library with .net 4.7.1 in VS15.8.7)

public abstract class myBase
    {
        public virtual void MyStep(CustomActionStep step)
        {
           // Do somthing
        }
    }

Expected behavior

Because it is c# and step is a vb.net keyword, it should not be a Problem

Actual behavior

Warns me

[jmarolf]

From the rule description:

This rule checks against keywords in the following languages:

• Visual Basic
• C#
• C++/CLI

Case-insensitive comparison is used for Visual Basic keywords, and case-sensitive comparison is used for the other languages.

The point of CA1716 is to let you know if a library is not going to be consumable by other .NET languages. The C# compiler already tells you if you use C# keywords as names so if CA1716 is not applicable to you I would turn it off.

[zspitz]

But an argument named with a keyword from a different language won't prevent the method from being called in that language. Specifically, even though Step is a keyword in case-insensitive VB.NET, but the method can be called without any modification:

Dim foo As myBase
Dim bar As CustomActionStep
foo.MyStep(bar)

Calling with named arguments would also not be a problem:

foo.MyStep(step:=bar)

This is also implied in the wording of the rule:

Identifiers for namespaces, types, and virtual and interface members should not match keywords that are defined by languages that target the common language runtime.

no mention of parameters.

Why is there a warning on parameter names?

[Evangelink]

Are we supposed to allow step only or to assess which of the names declared in the black list could be allowed for parameters?

[sharwell]

@bqstony I would recommend disabling this rule in your specific project. Even if you have a mixed-language scenario, the languages have ways to escape keywords. We already changed the default severity of this rule to Hidden inside the repository.

[Evangelink]

@sharwell based on your previous comment I assume that there isn't any work expected here, is it?

[mavasani]

@Evangelink Sam's comment indicates that this is not a high priority rule and is known to have some false positives/negatives, so it is fine for users to turn off the rule. However, we still ship this rule so should fix any bugs found in its implementation. Comment https://github.com/dotnet/roslyn-analyzers/issues/1858#issuecomment-444102623 indicates there is a bug in the rule implementation, as language specific keywords should not be flagged in parameter names.

[Evangelink]

@bqstony @zspitz Are you able to reproduce the issue using the latest version? I have added a unit test (linked PR) and I don't have any issue.

[zspitz]

I'm still seeing this. But I want to qualify: the following doesn't trigger the warning:

public abstract class myBase {
    public void MyStep(CustomActionStep step) { }
}

because as I noted above, method calls generally don't need the parameter name, so the parameter name can be a VB.NET language keyword without a problem. @Evangelink I think this is what is happening in the unit test.

The warning is reported only if the method is marked virtual, which means that any inheriting class will have a method with the same signature; if the implementing class is written in VB.NET, the signature will have to be escaped in some way as @sharwell noted.

I think this analyzer is working as expected.

[Evangelink]

Indeed I was missing the virtual, I can now reproduce the error.

[Evangelink]

At the risk of sounding stupid, the rule documentation states:

Identifiers for namespaces, types, and virtual and interface members should not match keywords that are defined by languages that target the common language runtime.

There is no mention about parameters. @mavasani I am not sure whether your comment:

Comment #1858 (comment) indicates there is a bug in the rule implementation, as language specific keywords should not be flagged in parameter names.

was to say let's stop analyzing parameters or to say let's exclude language specific keywords in which case sorry but I don't get what do we want to exclude.

I made some tests and using the escape symbol there is indeed no problem to have the interop working:

public class CSharpProgram
{
    public static void CSharpMethod(string AddHandler, string step)
    {
        VBProgram.VBMethod(@bool: "", @abstract: ""); // Valid call
    }   
}

Public Module VBProgram
    Public Sub VBMethod(bool As String, abstract As String)
        CSharpProgram.CSharpMethod([AddHandler]:="", [step]:="") ' Valid call
    End Sub
End Module

[zspitz]

I suppose technically, identifiers for ... virtual and interface members could be read to include identifiers specified in the virtual or interface member signature, i.e. parameters.

[Evangelink]

@mavasani so what about introducing an option to allow analysis of parameters and turn it off by default. This way by default there is a behavior matching the documentation and which seems to be the closest to a practical use while allowing people who want to use the legacy fxcop behavior?

[mavasani]

@Evangelink A user configurable option seems reasonable. How about we introduce a new option, say analyzed_symbol_kinds, and default it's value for this rule to comma separate list of all symbol kinds that it supports by default? Note we already have an option whose allowed values are enum field names of a Roslyn public enum: https://github.com/dotnet/roslyn-analyzers/blob/master/docs/Analyzer%20Configuration.md#analyzed-output-kinds. We should probably implement this option in a similar fashion.