SA1005 does not allow indentation of comments

[AArnott]

I don't know how the original StyleCop rule behaved, so this may or may not be by design. But even if it's by design, I'd like to challenge it to allow for a comment like this:

        // PERF: This has shown up on ETL traces as inefficient and expensive
        //       WithPart should call WithParts instead, and WithParts should
        //       execute a more efficient batch operation.

Currently, the rule complains that the second and third lines do not begin with "a space".
This certainly warrants discussion around exactly what the refined rule might be.

[sharwell]

I don't have an answer for you yet, but I have some additional notes on this situation.

1. I had to fix a bunch of cases like this in 461f510.
2. Neither Visual Studio nor StyleCopAnalyzers enforces the indentation for the comment you show above. Its possible the maintainability of your code could be improved by simply removing the indentation from the 2^nd and 3^rd lines. This might not cause a readability problem, especially if someone implements a tagger extension to change the syntax highlighting of comment "tags", including but not limited to TODO: and PERF:.
3. Even if you choose to remove indentation in your case, it could still be helpful to users if we implement an additional code fix for consecutive single-line comments which trigger SA1005 which converts the group to a single multi-line comment.

[AArnott]

Well, perhaps if we had an SX1005 rule, it would be to require at least one space, but allow more than one.

[sharwell]

Do you happen to know what the reference version of StyleCop does for this case?

@vweijsters @pdelvo I believe you both have it installed for testing these things?

[pdelvo]

Btw you can "install" StyleCop by installing the StyleCop.MsBuild nuget package. No system wide installation needed.
StyleCop does not report any diagnostics on a comment like that

[sharwell]

@pdelvo I'm moving this to a bug based on that.

[AArnott]

Thanks, @pdelvo and @sharwell

[oatkins]

The StyleCop source code is documented with these comments that don't seem to be handled by the implementation here:

• If the token length is less then two, this is not a valid comment so just ignore it.
• The first character in the comment must be a space, except for the following four cases:
  1. The comment may start with three or more slashes: ///whatever
  2. The command may start with a backwards slash: //\whatever
  3. The comment may start with a dash if there are at last two dashes: //--
  4. The character after the second slash may be a newline character.
• The comment starts with more than one space. This is only a violation if this is the first single-line comment in a row. If there is another single-line comment directly above this one with no blank line between them, this is not a violation.

[pdelvo]

We usually implement these rules the way the (bad) stylecop documentation describes them. This is why there are these differences. Comments starting with /// or //// are already ignored by this rule and the other stuff should be trivial to implement as well. Do you know why //\ and //-- are ignored?

[oatkins]

@pdelvo Working from the documentation sounds reasonable, though I do have some reservations with this approach. Lots of us finding this project are no doubt interested because of having a large code base that uses StyleCop - so it's important that the migrated rules match the behaviour of the "original" as closely as possible.

I've no idea why //\ is ignored. I wonder if it's because patterns like //----------- and ////// are sometimes used as "dividers" in code, though the latter isn't very pretty!

The reason why I started looking into this rule is that our standard file header causes a violation. The original StyleCop SA1005 doesn't check the file header, because that's covered by more specific rules instead.

I've started experimenting with adjusting the code more closely to match the SC implementation. I don't know how far I'll get with this, mind you! In general, if the actual SC behaviour is different from what is documented, how do you usually decide which behaviour to migrate?

[sharwell]

Just stopping in to say welcome to the project @oatkins and thanks for taking a closer look at this issue. 👍

[pdelvo]

We try to do what makes the most sense to us.I personally think we should implement the following:

• If the token length is less then two, this is not a valid comment so just ignore it.
• he first character in the comment must be a space, except for the following four cases: i.The comment may start with three or more slashes: ///whatever
  • The comment may start with a dash if there are at last two dashes: //--
  • The character after the second slash may be a newline character.
• The comment starts with more than one space. This is only a violation if this is the first single-line comment in a row. If there is another single-line comment directly above this one with no blank line between them, this is not a violation.

@oatkins What do you think? What does your file header look like? Would there still be a problem if we would change the rules like that?

[AArnott]

@pdelvo My large codebases that are migrating have similar headers to @oatkins by the sounds of it. Here is a sample of mine:

//-----------------------------------------------------------------------
// <copyright file="HandsOffService.cs" company="Microsoft">
//     Copyright (c) Microsoft Corporation.  All rights reserved.
// </copyright>
//-----------------------------------------------------------------------

But I think the best route would be for this rule to exclude any validation of header comments (which I suppose just means the first block of comments at the top of the file).

As for your proposed alteration, the rules look roughly the same as what @oatkins listed earlier. Can you highlight the differences for us?

[pdelvo]

I removed the //\ rule because I dont like it.
With the PR @oatkins proposed your file headers should not be reported any more because it allows comments to start with //-. Do you still think we should exclude file headers completely?

[AArnott]

In that case, it isn't important enough for me to push for at this point. But if the old implementation ignores headers and the new doesn't, it sounds like a new issue might be filed in the future by someone who hits a compact issue. But I don't mind waiting for that to maybe happen before we put in more work.

[oatkins]

Interestingly, SC's parser has a FileHeader type which means that this was an easy exception to write before. I don't think Roslyn has a file header concept, so it would entail trying to work out what delimits the file header in SC's rules.

[pdelvo]

No we have to implement the file header stuff from scratch. The file header diagnostics are going to be a lot of work and this exception might be a freebie when we implement SA1633-SA1641.