implement analyzers from runtime

[elachlan]

Fixes #5655
Fixes #4092

[elachlan]

@cdmihai here you go. I copied the files from dotnet/runtime as a base.

[elachlan]

Failure is due to dotnet/runtime using Microsoft.CodeAnalysis.NetAnalyzers. Which is for .NET5. I guess for now we use the Microsoft.CodeQuality.Analyzers package? Unless you think we could add to the nuget.config

<add key="dotnet-eng" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json" />
<add key="dotnet5" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5/nuget/v3/index.json" />
<add key="dotnet5-transport" value="https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet5-transport/nuget/v3/index.json" />

[elachlan]

Build fails because of new rules. I believe this is because warnings cause failures is enabled.

[elachlan]

List of error counts

SA1028 Count | 3039 
SA1121 Count | 1642
CA1805 Count | 159
SA1206 Count | 127
SA1400 Count | 90
CA2007 Count | 85
SA1129 Count | 37
SA1001 Count | 36
SA1115 Count | 23
SA1027 Count | 22
SA1113 Count | 20
SA1212 Count | 14
SA1026 Count | 12
CA1200 Count | 8
CA1802 Count | 8
SA1000 Count | 7
SA1019 Count | 4
SA1404 Count | 4
SA1411 Count | 4
CA1823 Count | 3
CA1507 Count | 2
CA1714 Count | 2
CA1717 Count | 2
SA1205 Count | 2
SA1410 Count | 2
SA1517 Count | 2
SA1617 Count | 2
SA1112 Count | 1

[elachlan]

@Forgind do you think it would make sense to have all the current rules set to Info. We then merge the PR and use the rules for refactoring and slowly change the rules back to warning after they are resolved? It gives a bit more power in that each PR can assess the individual rule and its affect.

[Forgind]

What would the effect of that be? Like as a PR author, at what stage would you see the messages, and would they show up for every file or just the files changed? I'd actually be fine with keeping them as warnings if they only run on files changed in the PR.

[elachlan]

What would the effect of that be? Like as a PR author, at what stage would you see the messages, and would they show up for every file or just the files changed? I'd actually be fine with keeping them as warnings if they only run on files changed in the PR.

At the moment, rules with warnings cause build failures. If we move the rules to Information. They won't cause build failures. They will just show up in the information section of the errors list when you run the Analyzer. With that implemented in master, I'll be able to do a separate PR for each rule that has violations. Then when all those violations are resolved, we set the rule back to warning. Which will cause any violations to cause build failures in Azure.

Its really the only way to do this change. Since the other way would be making sure all the rules are passing in this PR. Which would be way too many changes at once to review sanely.

[Forgind]

They will just show up in the information section of the errors list when you run the Analyzer.

So it sounds like I wouldn't notice unless I try to find them? That sounds safe.

Its really the only way to do this change. Since the other way would be making sure all the rules are passing in this PR. Which would be way too many changes at once to review sanely.

Definitely agree with this part. I had previously thought about leaving it as warnings in this PR so that we can watch our progress towards 0 warnings (as we merge other PRs fixing parts of it) and merge it when everything is green, but your version makes more sense to me, since that will also make it easier for contributors to know how their code should be styled: they can just look at the editorconfig. Additionally, as I mentioned previously, we'll need to look at other dotnet repos and see if there's a common style we should follow and, if not, collectively decide what our style should be before merging this. We'll likely have that meeting at some point next week.

Last, @rainersigwald also mentioned that we have some files that shouldn't be checked, so we'd have to track those down and put empty editorconfigs in those directories.

[cdmihai]

Potentially stupid question, but do these rules come with automated code fixers? If they do, how hard is it to configure our PR CI or some PR bot or GH actions to apply the fixers only on the changed files in each PR, as a new commit to that PR? That would be another way to trickle the changes to make them easier to check. And over time, as the repo accumulates PRs, all the relevant touched code will slowly get fixed.

If not, the solution of putting them on info and rate limiting the fixes to 1-2 a week seems the next best thing.

[elachlan]

@cdmihai here is the issue for StyleCop to run from the command line: DotNetAnalyzers/StyleCopAnalyzers#2571

It would make it possible to implement it in CI/CD. I actually would suggest against that though. I feel like when you commit code, a build failure is more appropriate then the system changing code and committing it.

[elachlan]

@rainersigwald I was told there are a few folders or files we should be ignoring in the code files. Is it possible to get a list from the team?

[elachlan]

Build is now passing. I just need the list of directories that need to be ignored with the .editorconfig override. If we really want to stop changes, we can also have the .editorconfig override files not run any of the Analyzers rules.

[elachlan]

@rainersigwald the only thing left here is the .editorconfig override files. Otherwise it is done.

[Forgind]

Sorry we've been a bit slow with this. Deciding which rules to keep and which we don't want requires a lot of input. As you suggested, we started from dotnet/runtime's rules, since they have comments explaining what each does, and at this point have under ten rules that we're still undecided on. I intend to schedule one last meeting to talk about those soon and have an answer to you within about two weeks. Sorry for the delay!

[Forgind]

Hi @elachlan,

I'm sorry it took so long to get around to this, but there are a lot of rules to look through! We liked that Runtime provided comments, so those are added back here. A couple specific comments:
For CA1031, we aren't opposed in general, but we do like code like:
catch (Exception e) when ErrorUtilities.IsIoRelatedException(e)
So if we'd have to lose that, we voted to not use that rule.
For CA1068, we aren't opposed in spirit, but we shouldn't change our public API surface for no reason, and if that means keeping a CancellationToken as a middle parameter, that's what we have to do. I left it in in the suggested change, but if it causes too much trouble, we may need to take it out.

Last, we didn't look carefully at the rules for changes in your editor like putting { on a new line. We were thinking we'd keep those for now and revisit that if we find them too annoying.

[Forgind]

Changes look good to me!

I'm assuming some of the new rules will fail, and we can change those to info as needed, but I'm happy with how this looks.

[elachlan]

These are the rules that I changed, I suggest the team go over each of them and decide if they are needed. If not, then mark them as "None". Then we take a list of the needed/useful rules and create a MR for each rule fixing the code issues.

CA1032
CA1034
CA1050
CA1200
CA1507
CA1707
CA1714
CA1717
CA1802
CA1805
CA1806
CA1810
CA1814
CA1815
CA1819
CA1823
CA1825
CA1829
CA1834
CA1835
CA1836
CA1837
CA1838
CA2007
CA2008
CA2016
CA2219
CA2241
CA2249
SA1404
SA1625

[Forgind]

I haven't talked about this with others on the team, but I think this is right based on our previous conversations. And if I got one wrong, we can always change it later.

[elachlan]

So is there a list of rules that you want to re-enable once this is merged?

[Forgind]

By re-enable, do you mean from None to Info or from Info to Warning? I was trying to organize it such that None should stay None forever—those are rules we decided we didn't want for some reason—and Info can hopefully eventually change to Warning or Error (depending on severity) as we make improvements. Does that sound reasonable?

[elachlan]

Sounds good. I guess the team will need to make a list of rules that the team would like moved to warning from info that require fixes (if not all).

Then we can work through each rule one by one making fixes in another Issue/PR.

[Forgind]

I talked with two other people on the team this morning, and we're all on board with moving everything currently labelled "info" into warning or error (as appropriate) at some point.

Thanks for your work on this; I think it's good to go! I'll poke someone on my team for approval, then mark it merge-when-branch-open.

[elachlan]

Awesome! Sorry it took so long. I will look at the info to warning rules one by one in separate PRs once this is merged

[benvillalobos]

@Forgind had mentioned that these analyzer warnings come up multiple times, is there any reason why that would be happening?

Also, why is it that only src/Build/Microsoft.Build.csproj needs to opt into this and it carries forward? Does only one project need to opt in, regardless of what project that is?

[elachlan]

@Forgind had mentioned that these analyzer warnings come up multiple times, is there any reason why that would be happening?

Also, why is it that only src/Build/Microsoft.Build.csproj needs to opt into this and it carries forward? Does only one project need to opt in, regardless of what project that is?

I honestly have no idea on either of those points. I don't know enough about the rulesets.

[Forgind]

ladipro suggested offline that the extra warnings are because it builds it once for net framework and once for net core, so I'm not too worried about that. On the other hand, he also pointed out that the warning didn't seem to pop up when making a mistake in another project, so we should probably add it to other projects as well before merging.

[Forgind]

Pushed a change to make the analyzers apply to other assemblies. Not 100% sure it's ok to have the packagereferences always there, but my local version caught a mistake in either Microsoft.Build or Microsoft.Build.Utilities, so I think it's working properly, and in its current state, it built without errors or warnings. I'm happy with it. I'll bring it up by Monday at the latest and see if we can get it merged. 🎉

[elachlan]

Pushed a change to make the analyzers apply to other assemblies. Not 100% sure it's ok to have the packagereferences always there, but my local version caught a mistake in either Microsoft.Build or Microsoft.Build.Utilities, so I think it's working properly, and in its current state, it built without errors or warnings. I'm happy with it. I'll bring it up by Monday at the latest and see if we can get it merged. 🎉

Thanks Forgind! Sorry I didn't get around to making the changes. I have been pretty busy with work/life. This should be a great help in the future to catch code mistakes.

[Forgind]

Thanks @elachlan! This should make it a lot easier to enforce style rules 🙂