[WIP] Add FxCop to the build

[Numpsy]

Add FxCop to the build to see what issues it finds.

Marked as WIP because it creates over a thousand warnings in the build.

Some of them look worth looking at (e.g. warnings about not disposing of things before they go out of scope), but a lot of them are things like passing string literals to exceptions rather than using resources, which are possibly not really useful. (so could be turned off)

(Note: #445 is something that was found by the analyzer)

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

[piksel]

Good initiative. I basically did this and turned it off again because of the spamming, but we should set up a ignore list, including CA1303 (until we get localization).

[Numpsy]

Is it ok to use the editorconfig approach to control the analyzer settings? (It seems to be the MS recommended way now in VS 2019, but I can't say offhand if that works in 2017)

[Numpsy]

I take the way that the number of warnings in the CI build didn't go down after updating editorconfig to mean the answer to my previous question is 'no' :-(

In any case, I sent #450 to fix some of the produced warnings.

[Numpsy]

@piksel I left this using the 2.9 version of FxCop as the release notes for the v3 version say it requires VS 2019 and I thought that may not be wanted, but maybe that's not an issue now the tests are targetted at Core 3.1 and need that anyway?

[piksel]

Yeah, I don't see any problems with requiring VS 2019 for FxCop. If a need for this to change emerges then we could just disable or change this later, but for now I don't see any reason to use anything earlier than 2019.

[Numpsy]

hmm, I don't know how this change could cause that one test failure.

[piksel]

No, that's probably due to something unexpected in the CI. Especially since it worked on the same platform, different config, and same config, different platform(s).

[codecov[bot]]

Codecov Report

Merging #449 (7418872) into master (34879be) will decrease coverage by 0.11%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   73.35%   73.23%   -0.12%     
==========================================
  Files          68       68              
  Lines        8721     8721              
==========================================
- Hits         6397     6387      -10     
- Misses       2324     2334      +10     

Impacted Files	Coverage Δ
...ICSharpCode.SharpZipLib/BZip2/BZip2OutputStream.cs	78.40% <0.00%> (-1.13%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 34879be...7418872. Read the comment docs.

[piksel]

I'm thinking about how to best get this merged. The best thing to do is probably to just create a bulk commit/PR with most of the fixes done. This will render other PRs in need of rebasing, but at least it would be more of an one-time event.

[Numpsy]

I think a pretty large proportion of the remaining warnings were about checking public params for being null, and at the time I might have thought that #501 would deal with some of those and not looked in much detail. I'll have a look, and see what proportion are 'non-functionality' ones.

[Numpsy]

Theres also the question at this point on if it should use NetAnalyzers instead of FxCop (I can see about a PR that fixes many warnings at once in either case though))