DotNetAnalyzers / StyleCopAnalyzers

An implementation of StyleCop rules using the .NET Compiler Platform
MIT License
2.66k stars 508 forks source link

SA1515 needs an exception for expression-bodied members #3550

Open gtbuchanan opened 2 years ago

gtbuchanan commented 2 years ago

SA1515 for single-line comments states:

An exception to this rule occurs when the single-line comment is the first item within its scope.

I would expect this exception to also apply to expression-bodied members. However, SA1515 is triggered in this case:

public void Customize(IFixture fixture) =>
    // Prevent already cancelled tokens
    fixture.Register(() => CancellationToken.None);

I confirmed that the exception works fine when changing the method to have a normal body.

public void Customize(IFixture fixture)
{
    // Prevent already cancelled tokens
    fixture.Register(() => CancellationToken.None);
}

Though, we're currently using the //// workaround instead since we've set csharp_style_expression_bodied_methods = true:error in .editorconfig to enforce expression-bodied methods in situations where it is possible.

Tested with 1.2.0-beta.435.

bjornhellander commented 2 years ago

One could possibly argue that a method that needs comments are better off not using expression bodies in the first place, but it's of course not desirable if you can't get StyleCop to harmonize with editor config settings that looks perfectly reasonable.

Updating SA1515 to allow this as well seems easy enough, so this suggestion seems reasonable to me.

sharwell commented 2 years ago

It seems that:

  1. The block-bodied form would be preferable to the expression-bodied form if the comment is required for clarity of the implementation
  2. The expression-bodied form can still be used by documenting the method instead of trying to document the implementation of the method (i.e. move the comment up one line)

Both of these strategies are already compatible with StyleCop Analyzers, so I'm not sure any change is warranted here.

gtbuchanan commented 2 years ago

@sharwell I agree that 1 would be preferable but it does not play well with IDE0022 and suppression for a short comment is not ideal. 2 is okay but converting to a statement body later might leave the comment ambiguous. Converting to a statement body (ReSharper refactor?) with the comment how it is in the OP will preserve the comment location above the line of code.

If you're opposed to adding an exception for this, can the documentation be updated to mention expression-bodied members are not included in the "first item within its scope" comment (or perhaps change "scope" to "block")? It's more of an annoyance than anything but I can explain the workarounds to my team if I need to.

mu88 commented 3 months ago

I'm also stumbling over this issue and I see it like @gtbuchanan

@sharwell / @bjornhellander : Can you please provide an update about this?

mu88 commented 2 months ago

@sharwell / @bjornhellander : friendly ping :)