search

rjmurillo / EffectiveCSharp.Analyzers

Many of the recommendations in the book Effective C#: 50 Specific Ways to Improve Your C# can be validated by Roslyn-based analyzers and code fixes.
MIT License
2 stars 1 forks source link

ECS0600 False positives #73

Closed MattKotsenas closed 2 days ago

MattKotsenas commented 4 days ago

ECS0600 triggers in this situation:

internal static class WellKnownTypeNames
{
    internal const string MockName = "Mock"; // _not_ ECS0600
    internal const string MockBehavior = "MockBehavior"; // ECS0600

I think it should not. It appears to be triggering because field name and value are the same. However, the MockBehavior = nameof(MockBehavior) should not be promoted as a best practice, because changing the name of the variable would silently also change the value, which may not be clear when refactoring.

Note that in the docs the heading for this rule is also "ECS0006", which doesn't match the file name.

rjmurillo commented 2 days ago

Thanks for your issue report. I've corrected the documentation issue

rjmurillo commented 2 days ago

As for the false positive: I disagree that this is a false positive. The naming convention of the two cases are different, the first adding the "Name" suffix to the variable, while the second is the name of the member. Changing the first to be internal const string Mock = "Mock"; would also trigger ECS0600.

From "Item 6: Avoid String-ly Typed APIs"

We also use many different libraries that rely on names and string identifiers to work with our data. ... Many developers have seen this basic usage and do correctly use nameof in those simple cases where the name of a local variable must be used in some API. ... The payoff of using the nameof operator in ... locations is that any changes or updates in the name of the symbol will be correctly reflected in the name of the variable. Static analysis tools can also find mistakes and mismatches when the name of an argument is used in the wrong location.

I interpret the intent of Item 6 in this case that the nameof operator should be used, but key off of the Moq type so that when the symbol in Moq changes, so does our type name. However, it's not really possible to do that here.

There are other issues also: Item 2: Prefer readonly over const via #26

From "Item 2: Prefer readonly to const"

C# has two different versions of constants: compile-time constants and runtime constants. They have very different behaviors, and using the wrong one will cost you. You should prefer runtime constants to compile-time constants. Compile-time constants are slightly faster, but far less flexible, than runtime constants. Reserve the compile-time constants for when performance is critical and the value of the constant will never change between releases. ... The most important distinction is that readonly values are resolved at runtime. The IL generated when you reference a readonly constant references the readonly variable, not the value. This difference has far-reaching implications for maintenance over time. Compile-time constants generate the same IL as though you used the [constants] in your code, even across assemblies: A constant in one assembly is still replaced with the value when used in another assembly.

While it is unlikely that the type will change, it can and there's little practical benefit of it being a const versus a static readonly. I've opened https://github.com/rjmurillo/moq.analyzers/pull/236 to correct the root issues identified