Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECS0600 False positives #73

Closed
MattKotsenas opened this issue Oct 22, 2024 · 2 comments
Closed

ECS0600 False positives #73

MattKotsenas opened this issue Oct 22, 2024 · 2 comments
Assignees
Labels
analyzers bug Something isn't working documentation Improvements or additions to documentation .NET triage

Comments

@MattKotsenas
Copy link

MattKotsenas commented Oct 22, 2024

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 rjmurillo added .NET triage bug Something isn't working analyzers labels Oct 22, 2024
@rjmurillo rjmurillo self-assigned this Oct 22, 2024
@rjmurillo rjmurillo added documentation Improvements or additions to documentation triage and removed triage labels Oct 22, 2024
@rjmurillo rjmurillo reopened this Oct 23, 2024
@rjmurillo
Copy link
Owner

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

@rjmurillo
Copy link
Owner

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 rjmurillo/moq.analyzers#236 to correct the root issues identified

@rjmurillo rjmurillo closed this as not planned Won't fix, can't repro, duplicate, stale Oct 23, 2024
rjmurillo added a commit to rjmurillo/moq.analyzers that referenced this issue Oct 23, 2024
…CS0600 (#236)

Effective C# rules 0200 and 0600 have violations in
`src/Common/WellKnownTypeNames.cs`. This change updates the members in
`WellKnownTypes` type and their usages

Related to
rjmurillo/EffectiveCSharp.Analyzers#73, see
item for discussion

- Updated constant declarations to static read-only fields for improved
flexibility as referenced in
[ECS0200](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/main/docs/rules/ECS0200.md)
and removed suppression.
- Updated naming conventions for `WellKnownTypes` to avoid triggering
[ECS0600](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/main/docs/rules/ECS0600.md)
- Updated control flow in
`src/Analyzers/ConstructorArgumentsShouldMatchAnalyzer.cs` to move from
switch, which requires compile time constants, to a more traditional
if/else
- Updated names in the former `WellKnownTypeNames` type (now
`WellKnownMoqNames`) with improved names and added documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
analyzers bug Something isn't working documentation Improvements or additions to documentation .NET triage
Projects
None yet
Development

No branches or pull requests

2 participants