Add Microsoft static code analyzers

[Evangelink]

Fixes #2760

I have used the "default" sets of rules tweaked a bit based on some of the inputs from your resharper PR. This PR may already be a little too big so feel free to let me know if you prefer to split it.

Some split idea:

1. Per analyzer
2. Infra then 1 PR per "rule"

EDIT:
There are a couple of diagnostics being raised as info as they are awaiting for a decision:

• CA1816: Dispose. In a couple of places in the code the Dispose pattern is not implemented correctly. I could implement it but there are some open questions: can some classes be marked as sealed (in this case there is no need for the pattern). There is one case where Dispose is virtual, I don't know if I can make the change (causing a potential API break, if that's part of what you consider to be the API of your library).

• CA2219: Do not raise from finally. Is it fine for you to make a change (flags) and throw outside?

• CA1016: Mark assemblies with assembly version.

• CA2211: Non constant fields should not be visible

• CA1018: Specify attribute usage

• CA1806: unused HResult. There is a call to SetError in a Dispose and the result is ignored. Ignoring HResult can lead to weird behaviors but in the case of a Dispose we may consider that to be ok. I don't know enough the code to be able to make such call.

[asbjornu]

Besides the failing dotnet format test and the remaining _ in the codebase, this is quite wonderful. Thank you!

The only thing I can think of to add, perhaps in a followup pull request, is null guards in all constructors.

[arturcic]

I'd prefer to keep the curly braces here as it's more readable and aligned

[Evangelink]

Sorry I missed this formatting issue. Just to be sure what's your preference between

var versionCalculator = GetBaseVersionCalculator(contextBuilder =>
    contextBuilder.OverrideServices(services =>
    {
        services.RemoveAll<IVersionStrategy>();
        services.AddSingleton<IVersionStrategy>(new V1Strategy(DateTimeOffset.Now));
        services.AddSingleton<IVersionStrategy>(new V2Strategy(dateTimeOffset));
    }));

var versionCalculator = GetBaseVersionCalculator(contextBuilder =>
{
    contextBuilder.OverrideServices(services =>
    {
        services.RemoveAll<IVersionStrategy>();
        services.AddSingleton<IVersionStrategy>(new V1Strategy(DateTimeOffset.Now));
        services.AddSingleton<IVersionStrategy>(new V2Strategy(dateTimeOffset));
    }
}));

In case you go with choice 2, we will probably have to either reduce the severity, prevent preference of expression body for lambdas or use pragam disable for such places.

[arturcic]

option 1 seems fine if the second expression is on new line and aligned as in the sample

[arturcic]

Besides the failing dotnet format test and the remaining _ in the codebase, this is quite wonderful. Thank you!

The only thing I can think of to add, perhaps in a followup pull request, is null guards in all constructors.

I do agree with removing _.

There are 2 more things to add.

• I'm not a big fan of too long lines so in a follow up PR I think we need to restrict the length of lines to make it more readable
• I noticed in couple of places there are var => var2 => statements. I would prefer to have the second pair with curly braces as it looks much more readable.

And as I suggested, please add the nuget packages to Directory.build.props instead of the csprojs

[Evangelink]

There is no analyzer to help with this kind of formatting. I will make the changes manually, just let me know your preference between:

1. Always break after =>
2. Always break before => (that's the one I usually go for but that's purely personal)
3. Break only on long line.

[arturcic]

3

[Evangelink]

The test seems to be working fine on my machine (VS + command line). Could you make a test on yours and let me know if that works or fails?

[asbjornu]

All tests green. LGTM!

[Evangelink]

@asbjornu Merge done.

[Evangelink]

Thank you for the multiple build relaunch @asbjornu

@arturcic Let us know if you feel we should make more changes here. I am more than happy to provide a follow-up PR (or commit) with any extra changes.

[arturcic]

@Evangelink If you don't mind I'll check it tomorrow, having day off today

[Evangelink]

Hey sure! Enjoy your day off and speak to you soon.

[arturcic]

@Evangelink can you rebase on top of main and solve all conflicts, then we can merge it

[Evangelink]

@arturcic Sure! Will work on it now. Any preference between rebase + force push and merge main?

[arturcic]

I usually rebase as it's cleaner in terms of history. As long as the branch is not merged into the main I always sync the PR branch with main by rebase-ing

[arturcic]

@Evangelink can you fix formatting? https://github.com/GitTools/GitVersion/runs/3135376706?check_suite_focus=true

[Evangelink]

@arturcic My bad! I will fix that today.

[asbjornu]

@arturcic, all green! Do you approve? :)

[mergify]

Thank you @Evangelink for your contribution!