New analyzer: Do not mark ref/out parameter with OptionalAttribute

[Evangelink]

Marking a ref or out parameter with OptionalAttribute is confusing and will lead to compilation failure if called without the argument.

This analyzer is pretty unlikely to catch issues as OptionalAttribute seems to be mainly used on vbnet.

public class C
{
    public void M1([Optional] ref int i) {}

    public void M2()
    {
        M1(); // CS7036
    }
}

[Evangelink]

@mavasani Could you confirm you are fine with this rule?

[mavasani]

@Evangelink This seems more in line with the IDE rules in Roslyn to remove unnecessary code. For example, @Youssef1313 recently added rule to fade and recommend removing unnecessary ByVal modifier for VB.

[Evangelink]

Well in this case this isn't an unecessary code as could be the Out attribute in vbnet but it is clearly a bug and as mentioned in my previous comment this will lead to a runtime bug.

[Youssef1313]

As the OptionalAttribute resides in System.Runtime.InteropServices, it obviously should have something to do with interoperability. We need to make sure that this can't break any C++/CLI code calling this .NET method.

I'm also a bit confused by this sentence:

This analyzer is pretty unlikely to catch issues as OptionalAttribute seems to be mainly used on vbnet.

Does this suggest that this analyzer should be C# only? or it's meant to be VB only? and why? or you meant something else with it?

[Evangelink]

I'm also a bit confused by this sentence:

This analyzer is pretty unlikely to catch issues as OptionalAttribute seems to be mainly used on vbnet.

Does this suggest that this analyzer should be C# only? or it's meant to be VB only? and why? or you meant something else with it?

I meant that the only few cases where I have seen people using this attribute is on vbnet codebase (and I can count the numbers on one hand) so I think that there will be only few times this analyzer will be handy. Besides if you do use this attribute in C# or VB.NET and you don't pass the argument you will have a compilation issue. Still this is a non-sense to combine ref and Optional because they have contradictory meaning.

[jmarolf]

Design Meeting Conclusion:

• Move this to the roslyn-analyzers repo

[sharwell]

@jmarolf I thought the meeting conclusion was to see if the compiler wanted this in a warning wave, and move it if they said no. Have we verified that it's not going to be in the compiler?

[jmarolf]

@sharwell you are correct I mis-interpreted my notes. Moving back to roslyn

[jmarolf]

@333fred would you be OK asking the LDM if this is something that they would accept as part of a new warning wave?

[333fred]

Sure, I'll send an email.