dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.02k stars 4.03k forks source link

IDE0009 should not be reported inside nameof #21877

Closed nvmkpk closed 7 years ago

nvmkpk commented 7 years ago

Adding 'this.' does not produce compiler error but it feels wrong to have it in this case. If qualification is really necessary then we should use '<class name>.' instead.

sharwell commented 7 years ago

If qualification is really necessary then we should use '.' instead.

Can you clarify this with an example? I'm not sure I followed.

nvmkpk commented 7 years ago

Updated inline.

sharwell commented 7 years ago

In my opinion, the proposed change would have three primary drawbacks:

  1. It makes the logic surrounding instance and static member qualification more complicated by selectively treating instance members as static when they are used in certain contexts.
  2. It makes the code less intuitive to the reader, by suggesting that some members are static members when in reality they are instance members.
  3. It adds "noise" for the reader, since containing type names are often much longer than 'this'.

In addition, it would raise a bunch of code style questions we can otherwise avoid (specifically related to enforcement of the practice and interaction with other analyzers like Simplify Type Name).

At this point I would be in favor of keeping the current implementation as-is.

sharwell commented 7 years ago

:bulb: If you are interested in applying the described code style to your project, one option is implementing a custom analyzer and code fix for your project. The StyleCop Analyzers project is open source and contains rule SA1101, which you could likely extract to another analyzer project and then modify to meet your needs.

nvmkpk commented 7 years ago

Those who understand what nameof operator is would already understand not to infer static-ness of what is inside it. But my request is to not require 'this' in that context. I am not saying anything authoritative about requiring '<class name>.'.

sharwell commented 7 years ago

This change would still require a new configuration option, and further complicate logic in an area which has already been known to cause problems in the past (e.g. #21528). In addition to the maintenance and documentation overhead of a new option, it makes code simplification efforts like #21846 more complicated (or impossible), and increases the chances of a regression bug appearing if a feature ever touches this, such as dotnet/csharplang#702 or dotnet/csharplang#848.

When it comes to specific reward, the set of users who would benefit from the feature are users who meet both of the following conditions:

  1. Users who prefer to qualify references to instance members with this.
  2. Users who want to make an exception to the first case when inside a nameof

In my experiences over the past two years with StyleCop Analyzers (an external open source analyzers project), the set of users who fall into this category are exceptionally uncommon. The best chance of getting a feature to do exactly what you want in the shortest time possible would be modification of an existing analyzer to target the precise logic desired for the project. :smile:

:memo: Note that I am not saying anything about whether this is a good style or a bad style. I've never used it myself so I'm in no position to judge. :smile:

dpoeschl commented 7 years ago

@nvmkpk Thanks for the feedback. I agree with @sharwell here, that the cost/benefit analysis doesn't favor the implementation of this option in the product. Definitely consider your own analyzer/fixer, perhaps simply adapted from the ones in Roslyn itself.