CA1822 should ignore attributed methods

[JohanLarsson]

[BenchmarkDotNet.Attributes.Benchmark]
public void M()
{
    ...
}

Converting to static breaks the benchmark.

[Evangelink]

Ignoring all members marked with an attribute is definitely going to kill lots of False Positive but it will also hide a lot of real issues. Adding a whitelist of allowed attributes that will prevent the issue to be raised won't solve all the cases because we can't add company custom attributes to this list. Although it might be possible to add some configuration to the rule, where the user could list the full name of all specific attributes to ignore.

@mavasani I don't know if there is a global decision toward FP and FN ratio for this analyzer. I tend to think that excluding all attributes would be removing a lot of real issues and so isn't the best way forward (but that's only my view).

[JohanLarsson]

Out of curiosity do you have a couple of example where you want the fix to warn for attributed methods?

Nullable annotations is one example I can come up with.

[mavasani]

I agree with @Evangelink - bailing out on all methods with attributes is certainly not a good solution as there does not seem to be any association between the attribute and why the method needs access to instance members. This is a case where I feel a source suppression with a proper justification comment acts as a good documentation, and would actually be more valuable then changing this rule. This prevents any future developer working on this code base to accidentally convert the method into a static method, only to find that this breaks the runtime semantics.

[mavasani]

@Evangelink I also agree that if we were to implement this, we would do it by exposing editorconfig option for users to specify if they want bail out on specific attributes OR all attributes. I believe we should wait for more user requests before working on this.

[Evangelink]

@JohanLarsson I don't have an extensive list but I was mainly thinking about 2 big cases:

1/ Specific code that will use DllImport or MethodImpl attributes.

2/ Mainstream codes relying for example on resharper attributes like NotNull or CanBeNull. Or personally, I like to use the Conditional attribute to add some debug checks that I don't want to have in prod (and those methods are almost always static).

[JohanLarsson]

NUnit handles [Test] when the method is made static.

I don't use attributes much but BDN and WCF attributes means changing to static is not ideal. In my view an attribute on a method signals that it is special in some way and that reflection code may rely on it.

[jnm2]

It would be nicer if the BDN/WCF attributes themselves could be annotated with an attribute saying that the target must be static or must be an instance member.

[Evangelink]

@jnm2 It could be really nice to have member attributes able to specify whether they need an instance or not.

@JohanLarsson You are right with saying that we cannot know upfront if the code using the attribute will rely on accessing the actual instance or could work with a static access. That's why I was suggesting to be able to provide a whitelist mechanism for users.

[Evangelink]

@mavasani What do you want to do for this ticket?