Enable more style checking analyzers

[marek-safar]

There are several analyzers that check for code style that runtime repo could benefit from if enabled. They are not too pedantic and could help to make the large codebase more readable.

For example

• SA1021: Negative sign should be preceded by a space #94827
• SA1107: Code should not contain multiple statements on one line
• SA1119: A C# statement contains parenthesis which are unnecessary and should be removed.
• SA1123: Region should not be located within a code element
• SA1128: Put constructor initializers on their own line
• SA1130: Use lambda syntax
• SA1136: Enum values should be on separate lines #94098
• SA1507: Code should not contain multiple blank lines in a row

[ghost]

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

There are several analyzers that check for code style that runtime repo could benefit from if enabled. They are not too pedantic and could help to make the large codebase more readable.

For example

• SA1021: Negative sign should be preceded by a space
• SA1107: Code should not contain multiple statements on one line
• SA1119: A C# statement contains parenthesis which are unnecessary and should be removed.
• SA1123: Region should not be located within a code element
• SA1128: Put constructor initializers on their own line
• SA1130: Use lambda syntax
• SA1136: Enum values should be on separate lines
• SA1507: Code should not contain multiple blank lines in a row

Author:	marek-safar
Assignees:	-
Labels:	area-Meta
Milestone:	-

[ericstj]

@stephentoub thoughts on this? Seems reasonable, but not for 8.0 at this point. Let's move this to 9.0 and do it early.

[stephentoub]

Most of these look fine. Someone can try (one at a time, please), and we can look at the impact to see if there's anything unexpected we don't like.

[danmoseley]

This is labeled analyzer but there's no analyzer work here, right? isn't it just a regular work item in area-meta?
Then we can change to check boxes and encourage community help.

[ghost]

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

---

There are several analyzers that check for code style that runtime repo could benefit from if enabled. They are not too pedantic and could help to make the large codebase more readable.

For example

• SA1021: Negative sign should be preceded by a space
• SA1107: Code should not contain multiple statements on one line
• SA1119: A C# statement contains parenthesis which are unnecessary and should be removed.
• SA1123: Region should not be located within a code element
• SA1128: Put constructor initializers on their own line
• SA1130: Use lambda syntax
• SA1136: Enum values should be on separate lines
• SA1507: Code should not contain multiple blank lines in a row

Author:	marek-safar
Assignees:	-
Labels:	area-Infrastructure-libraries, code-analyzer
Milestone:	9.0.0

[ghost]

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

---

There are several analyzers that check for code style that runtime repo could benefit from if enabled. They are not too pedantic and could help to make the large codebase more readable.

For example

• SA1021: Negative sign should be preceded by a space
• SA1107: Code should not contain multiple statements on one line
• SA1119: A C# statement contains parenthesis which are unnecessary and should be removed.
• SA1123: Region should not be located within a code element
• SA1128: Put constructor initializers on their own line
• SA1130: Use lambda syntax
• SA1136: Enum values should be on separate lines
• SA1507: Code should not contain multiple blank lines in a row

Author:	marek-safar
Assignees:	-
Labels:	area-Meta, help wanted, code-analyzer
Milestone:	9.0.0

[ViktorHofer]

@jeffschwMSFT I moved this back to area-Meta as there's no infrastructure work necessary for this.

[danmoseley]

if anyone wants to look at these, start by editing eng\CodeAnalysis.src.globalconfig and/or eng\CodeAnalysis.test.globalconfig then open things up in VS and build.

[artl93]

@danmoseley - do we typically prefer these as warnings or errors? I can see value in both.
From an implementation standpoint, should we just remove the configuration from our .props files, or set the value explicitly do make the configuration self-documenting, i.e., we're doing this on purpose? (See WIP: #94100)

[danmoseley]

@artl93 i don't know what current thinking is in this repo - @stephentoub can say.

In general we've discussed that (a) we don't like PR validation to fail merely because of a trivial style issue leaving us with no test results and (b) we like to be able to code locally without the build failing merely because of trivial style issues in work that's not finished. The @jaredpar approach I believe is for them to be warnings locally and in some but not all PR validation flavors. Not sure what thinking is in this repo.

[jaredpar]

The way we've approach code style warning is:

1. On local dev boxes
   1. Do not run them by default on command line
   2. Do run them in VS but at info level, not warning
2. On CI
   1. For any leg that feeds into unit tests do not run style checking. In fact we disable all analyzers here, do not promote warnings to errors, etc ...
   2. Have a specific leg that builds the code with style checking enabled, /warnAsError, all possible analyzers, etc ...

The rationale for this setup is ...

The goal of CI should be to provide the maximum amount of information possible, in the fastest amount of time, for any given change. As such do not run code style checks on the paths that feed to unit tests. It's potentially introducing warnings that risk promoting to errors and stopping tests from running (as well as impacting build time). There is no reason a trailing space should stop a unit test from even running. This is also why the same jobs disable /warnAsError, analyzers, etc ... Test legs should get unit test results as often and fast as possible. In parallel have a dedicated job that builds with all code style warnings, all analyzers, /warnAsError, etc ... that serves as the style enforcement check.

As for local developer setup there was a lot of back and forth about the tradeoffs for running style checks locally. There is a measurable perf hit for some rules, particularly when dealing with repos the size of roslyn / runtime. After discussion we decided most devs were more comfortable with having info level diagnostics only in VS, but nothing in the command line. 99% of time VS is just going to auto-format it the correct way any who. Devs who want to have it run locally can specify an environment variable that enables all that checking on their machine (very small minority do this)

Another way to think about all this is a sure fire way to upset developers enough to reach for their pitchforks is ...:

1. If they send a PR, go to lunch, come back and find that zero tests ran because they had an extra blank line, trailing space after a paren, etc ...
2. If they are inner loop debugging and F5 fails after a long build cause of a trailing space somewhere.

Hope that helps.

[artl93]

Thanks! That totally makes sense. How would this practically translate to changes I would need to make the to globalgonfig file, as I am working on in #94100? Would I just be removing the line in eng/CodeAnalysis.src.globalconfig, and then leaving eng/CodeAnalysis.test.globalconfig alone?

[artl93]

One more thing - (again, displaying my ignorance) I'm on a Mac, so I use VS on Windows by exception, not as a rule (though, that's likely to change). Is there a way to see all the potential errors my global change will cause across all projects? Looking at the repo and docs, it seems I'd have to load them individually. FWIW - is there a way to get this same error information to show on the command line, or (for bonus points) in VS Code?

[stephentoub]

i don't know what current thinking is in this repo - @stephentoub can say.

Thus far in runtime, we've taken a different approach from what Jared outlines: all analyzer rules we've explicitly decided to get clean against are enabled as warnings (which are then promoted by default via warnings-as-errors) and run as such everywhere, command-line, in VS, and in CI. If a developer doesn't want them as errors locally, they disable warnings-as-errors.

[stephentoub]

Would I just be removing the line in eng/CodeAnalysis.src.globalconfig, and then leaving eng/CodeAnalysis.test.globalconfig alone?

We've tried to be explicit about every analyzer, so you wouldn't remove a line from the globalconfig, you'd just change the severity on the line that's already there (or if for some reason we're missing a line for the analyzer in question, add it).

[stephentoub]

Is there a way to see all the potential errors my global change will cause across all projects?

You can just build from root but with warnings-as-errors disabled, e.g.

./build.sh clr+libs -rc release -warnaserrors 0

[jaredpar]

all analyzer rules we've explicitly decided to get clean against are enabled as warnings (which are then promoted by default via warnings-as-errors) and run as such everywhere, command-line, in VS, and in CI.

Do you all do this on test legs too? When we were designing our system the only item that had universal agreement was to turn off analyzers and /warnaserror for builds that feed into test legs (and virtually nothing has universal agreement with style conversations). The rationale being that getting test feedback was very valuable even if there are style errors. Basically when you come back from lunch and see all tests pass but style failed cause there was an extra space that was much more valuable than having no tests run at all. That mentality ended up driving a lot of our decision process.