Introduce static code analysis

Piedone commented 3 years ago

Static code analysis is when analyzer programs check our code and pinpoint issues. Visual Studio has a lot of such analyzers built-in, for example, if you see build warnings or other hints, those are also added by analyzers. If you bring up automatic code fixes with Ctrl+. then those are something similar too (analyzer packages commonly include a lot of code fixes for issues they uncover). My suggestion here is to gradually introduce static code analysis to the Orchard codebase, showing violations both in the IDE in development time and enforcing them in CI before merging PRs too. This can ultimately improve the quality of the codebase and help contributors.

Why static code analysis?

Having static code analysis helps immensely to enforce best practices, coding style, and correctness, thus keeping the codebase both tidy and low on bugs (analyzers can find various hard async and security issues too, for example, not just simple code styling ones).

As a developer, when writing the code you get a lot of helpful hints and you're always nudged into doing things in the way the community has decided for the project. You don't have to read about coding conventions because you'll see the first time you write something that's against it. This is very helpful for newcomers and less experienced developers thus it fosters contribution.

As a reviewer of PRs you can focus on the big picture and the important stuff instead of nitpicking whitespace issues or leftover unused fields. You can be sure that all the basic stuff was already fixed, as well as latent bugs when you see a green build. This takes the chore out of reviewing PRs. Last time I reviewed a PR by a novice developer and noticed about 15 issues in the handful of changed files that static code analysis can easily uncover. It was a real pain.

Orchard has an .editorconfig currently, and that's a good start, but we can do a lot more with code analysis.

How to do this?

Our .NET Analyzers project that I've demoed a while ago contains a configuration of various tried and tested, widely used analyzers. We don't need to use that project (especially that it has a few different defaults than used in the OC codebase) but we can do pretty much the same thing:

Use a couple of widely used analyzer packages as well as the .NET SDK's built-in ones like these ones.
Define rules on code styling and correctness.
Manage all rules in a single file for the whole solution.
Show warnings in IDEs.
Enforce rules by failing the build on warnings in CI.

What do developers need to install? What's needed in CI?

Nobody has to install anything! Analyzers are free and open-source and come in NuGet packages (or in the case of .NET code style analysis, with the .NET 5 SDK). If you open the solution you'll get them without doing anything else.

The same is true for command-line builds in CI. too. There only a few command line parameters are needed to treat warnings as errors and enforce all analyzers, see the docs.

That's awesome but where's the catch?

Well, there are multiple things to consider:

Introducing analyzers, unless the codebase is already near-perfect, being written by a very disciplined team, is a pain. There will be thousands of violations first, and while the majority of them will be trivial and possible to fix automatically (whitespace issues, unneeded namespace imports...) there will be plenty to fix by hand too. This is a big project but it can be done gradually.
A lot of files will be touched, likely leading to merge conflicts with existing PRs.
Analyzers take time to run. During development time this can be minimized by only running them for the currently open code files (not during build). CI builds will take a couple of minutes more.

Conclusion

Overall, the effort is great but I think worth it: It's a pain to do in an existing project but it'll immediately start paying dividends for years to come. Maintenance efforts after the initial introduction are low, the analyzer packages sometimes need to be updated.

This is a big one, and can only be done in stages. It took us at Lombiq months to do it fully in a large project of ours which has a fraction of the amount of code than Orchard (though part of it was building the mentioned analyzers project and figuring everything out).

I can lead this effort as I have quite some experience with it already if we agree it's something we want to do. But most possibly it'll take a long time to finish it completely.

hishamco commented 3 years ago

Totally agree, I was thinking in the past to write an analyzer to fix the localizer type in constructor because I have seen many localization issues happen accidently by providing a wrong type

Piedone commented 3 years ago

Now writing Orchard-specific analyzers would make this even more awesome!

Skrypt commented 3 years ago

I like the idea. Though, we recently found that we can simply use Visual Studio to analyze everything and fix the issues based on the .editorconfig file. Running this manually from time to time or adding maybe a separate / manually triggered CI task that would return how many issues are found would be interesting. Unless this would fix the actual issues in the code from the CI tasks then I don't think we should execute this on every CI build though. I've not read in detail what you are suggesting @piedone but that's a good initiative that you should keep moving forward with.

Piedone commented 3 years ago

The warnings VS can show would be included here (.NET code style analysis and .NET code quality analysis from the .NET SDK, as well as warnings of the C# compiler). I'm talking about that and additional analyzers that greatly extend what's in there.

I'll set up a PoC in the coming weeks and we can continue from there.

Skrypt commented 3 years ago

Would/Could that work with VS Code too? 😄

Piedone commented 3 years ago

Yes: The analyzers mostly come from NuGet packages, .NET code quality analysis from the .NET SDK. I guess there are analyzers only available in VS though I'm not aware of any significant ones.

hishamco commented 3 years ago

@Piedone could we add something like OrchardCore.Analyzers in the code base or shall do outside, then we can list the common used one in the docs?

Piedone commented 3 years ago

The configuration can be part of the codebase (in Directory.Build.props, ruleset and editorconfig files). If we'd write our own analyzers then that could would most possibly need to be a separate solution with its own build and publish to NuGet (you can't use analyzers as source).

Piedone commented 2 years ago

Coding style guidelines added here: https://github.com/OrchardCMS/OrchardCore/pull/10724

BenedekFarkas commented 4 months ago

I've started working on it, but got some inconsistent behavior locally regarding the allowed language version, see the PR for the changes: #15366

Lombiq.Analyzers(.OrchardCore) defines 11.0 for LangVersion, so I applied an override in Directory.Build.props (what's in src/OrchardCore.Build/Dependencies.props was not sufficient), but after purging the repo to re-test other changes it stopped working and now I get the same errors as this build: https://github.com/OrchardCMS/OrchardCore/actions/runs/7963934165/job/21740460214?pr=15366

I'm starting to think that the initial (working) result was incorrect for some reason, but in any case, second opinions are welcome.

Piedone commented 4 months ago

I don't know where that goes wrong, but if you use 4.0.1-alpha.0.osoe-751 instead, then no override will be necessary, since it configures C# 12.

BenedekFarkas commented 4 months ago

I posted a reply here, but it's gone (IDK), so: Using that version is OK for now for testing, but I'd rather not have a dependency restrict the LangVersion of the solution.

BTW am I seeing it correctly that all of StyleCop (except for 5 rules were set to "suggestion") was disabled? Seems strange.

Meanwhile, I've got a list of the differences between the .editorconfig of Lombiq.Analyzers and what's currently in OC - there's probably a bunch of entries we won't need anymore, but there will be some that we need to keep. AFAIK the only option Lombiq.Analyzers allows now is to place an .editorconfig in a folder lower in the structure than the root. We've got to two folders (src and test) where this should apply, but we can just put into src and have a symlink to it in test, but I'm not sure yet if I like that solution.

Piedone commented 4 months ago

I would skip on Lombiq.Analyzers, and rather do the same thing in this solution as necessary, turning on rules one by one, each with its own PR.

BenedekFarkas commented 4 months ago

The overall methodology of enabling/disabling analyzers is not an issue when using Lombiq.Analyzers, it's already set up in the PR, so that each rule producing a warning is disabled for now. The issue is with the rest of what was in .editorconfig as in literally the editor configuration (like tab size for example), but I have an idea to test - if it doesn't work, then we can fall back to adding the analyzer packages individually.

BenedekFarkas commented 4 months ago

I rolled back the Lombiq.Analyzers integration and started again, but with the same packages and analyzer rule list as Lombiq.Analyzers. Then set everything to silent that produced a warning (and those were marked with a comment).

Further rules (or batches of rules) can be re-enabled and addressed in subsequent PRs. Two things to watch out for:

Other PRs (independent of these) producing code that conflict with one of the newly added analyzer rules - in these cases we to evaluate whether the rule is useful or not.
Editor configuration coming into conflict with one of the rules being enabled, i.e., the .editorconfig's editor settings enforce a formatting that a new rule doesn't agree with - in this case the rule will remain "silent", but will be marked in a comment why.

Piedone commented 3 months ago

Piedone commented 2 months ago

BTW the root Directory.Build.props file contains a lot of analyzer NoWarns too. These should rather be in a .globalconfig file.

Piedone commented 2 months ago

Related PRs:

Piedone commented 1 month ago

A big reason I'd like OC to utilize static code analysis extensively (see https://github.com/OrchardCMS/OrchardCore/discussions/15800) is because that liberates us from a huge amount of nitpicking and personal preference. As we at Lombiq experienced, after that, there's very little debate about things like coding style: we decide on the practices to follow when introducing code analysis, but after that, the build either passes or fails. We thus wouldn't need to have exchanges like this one (I'm not picking on anybody here, just referring to the most recent one).

Code analysis is good for much more than this, of course, but this one is an easily tangible quality-of-life benefit for the whole team.

OrchardCMS / OrchardCore