Consider integrating a style-verification tool into CI

[pabigot]

Zephyr has had for a very long time an uncrustify configuration that is recommended to ensure code conforms to the Coding Style. However there is no QA infrastructure to detect violations of this style.

Part of the plan for coding guidelines includes an enforcement stage. Tools like Coccinelle may be very helpful in ensuring conformance. However, Coccinelle is unaware of coding style, and left to itself will produce changes that are not compliant. See, for example, #21391 where Coccinelle output must be post-processed to eliminate a space from cast expression.

It would be significantly easier to use tools to ensure code guideline conformance if their output could be automatically transformed to restore style conformance. For #21391 uncrustify might have been able to fix the resulting long lines.

Painful though it would be, I propose that steps be taken to run all Zephyr implementation and header files through uncrustify, and that the CI infrastructure be updated to emit warnings when newly committed code is not style-compliant.

There would be several benefits:

• Coding style, in particular use of tabs versus spaces and indentation, would become consistent throughout Zephyr.
• Arguments about whether empty lines are required before/after certain statements would be eliminated by a simple policy: if uncrustify does not change the code, and it isn't explicitly handled in the Coding Style, the code is acceptable.
• Semantic patching and other automated transformations can be safely applied without causing style inconsistency.
• Contributors may be acclimated to style-based expectations, easing the path to enforcing coding guidelines.

[carlescufi]

+1 for this idea, I've actually always thought about this. Should we then remove checkpatch completely or are there some checks that would be missing otherwise?

[pabigot]

Checkpatch verifies things outside coding style (like commit message content) so it would still be necessary.

[nashif]

Thought about this multiple times in the past, it is not straightforward but definitely feasible with some effort needed:

1. We need something that works on changes, ie. commits and not on complete files. Not sure if uncrustify can work this way, clang-format does with clang-format-diff
2. We need to review all the rules we already have in .uncrustify.cfg and .clang-format and tighten them up, there are many nice to haves in there that would result in major changes required. Also, right now both configurations are incompatible, need to fix that.
3. We will need to create a solid baseline and do a full pass on existing code. It does not make sense to require contributors to modify their changes based on input from a linter when the rest of the file is following some other style
4. We need a good way to display feedback and results from the linters in pull request, this is going to be a major challenge and might result in lots of noise.

[chrta]

Just a comment, because i used uncrustify in a pre-commit-hook and on a ci in the past. The pre-commit-hook is something similar to https://github.com/ddddavidmartin/Pre-commit-hooks/blob/master/pre-commit-uncrustify

1. Uncrustify behaves a little bit different with different versions of it. So it would make sense to integrate a specific uncrustify version in the sdk.

2. I created a patch-file that fixes all the uncrustify-issues in the pre-commit-hook. This patch could be applied with one command. This simplified the process a lot and helped that all developer accepted this workflow.

[galak]

dev-review (Mar 26): @nashif we need a tool that is able to deal with a diff, since we shouldn't warn about changes not introduced by the actual PR.

uncrustify config in zephyr is inconsistent with checkpatch, so we need to update the config to be consistent.

We have 3 means today between clang-format, uncrustify, and checkpatch. Need to see if we can get to one tool.

@pabigot will look into clang-format.

[galak]

We might be able to utilize https://github.com/DoozyX/clang-format-lint-action, I haven't looked at it in great detail.

[nashif]

We do need something that can run this:

https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/clang-format-diff.py

[pabigot]

@pabigot will look into clang-format.

My results are in #23823, which it'd be nice to have reviewed and merged. clang-format has potential, but we'd have to develop a style file for it, and possibly change some of Zephyr's rules to accommodate what it can verify.

[pabigot]

As a follow-up: I've confirmed to my satisfaction that clang-format will always insert a space between a ForEach macro used as a loop construct and the opening parenthesis of its argument:

-               SYS_SLIST_FOR_EACH_CONTAINER_SAFE(&data->cb, cb, tmp, node) {
+               SYS_SLIST_FOR_EACH_CONTAINER_SAFE (&data->cb, cb, tmp, node) {

This will generate a diagnostic from checkpatch:

-:1775: WARNING:SPACING: space prohibited between function name and open parenthesis '('
#1775: FILE: drivers/gpio/gpio_intel_apl.c:194:
+               SYS_SLIST_FOR_EACH_CONTAINER_SAFE (&data->cb, cb, tmp, node) {

Options:

• Modify clang-format to make the space insertion optional;
• Accept the change tree-wide and update checkpatch to not emit a diagnostic (viz. because it's not a function name, it's a synthesized control structure);
• Find another style formatting tool.

[cfriedt]

Another good option is clang-format. It's available everywhere that LLVM is available and is quite highly configurable. The fact that it integrates directly with the compiler ensures that it is very accurate. Popular conventions can be used out-of-the-box (Google, Apple, Mozilla), and there are quite a lot of .clang-format options available for custom styles.

There is also already very good git integration with git-clang-format. So setting up pre-commit hooks (automatically formatting code before a commit) and server side hooks (rejecting code that is not correctly formatted) is fairly straight forward.

Also, with the format on / off tags, it's easy to perform workarounds

/* clang-format off */
/*
 * something free-form
 */
/* clang-format on */

But really, in the end, whitespace shouldn't even be a consideration for developers / reviewers. To quote myself, "we don't make money selling whitespace." Also, whitespace can be a fairly personal thing. There really is no "correct" way to format it. Some devs with poor vision might feel more comfortable with more whitespace - e.g. those with astigmatism. It might make other devs' skin crawl.

If it can be "corrected" with a git hook or simple command, essentially for free, and every dev is then able to format the code whatever way they like prior to commit, then the project should really adopt a convention that works everywhere.

[pabigot]

I've updated the issue description to avoid referencing uncrustify; #21392 (comment) is based on the assumption that clang-format is a better underlying tool. But we need a resolution to the fact it breaks checkpatch.

The original driver for my opening this was to have an objective rule for whether a style change was necessary: if the as-submitted code gets throught the style checker without any changes, then variations are personal taste.

[chrta]

@pabigot

Modify clang-format to make the space insertion optional;

This seems to be done now (for foreach macros), see https://reviews.llvm.org/D78869 and in https://clang.llvm.org/docs/ClangFormatStyleOptions.html search for "ControlStatementsExceptForEachMacros" This seems to be included starting with clang-format-11.

[pabigot]

Great, thanks for the pointer. The feature was merged 2020-04-26 to the clang-11 development branch. Based on previous LLVM releases clang-12 should be available in the second half of 2020-09. Once it's out we can make further progress.

[pabigot]

Please see #27058 and give it a try if you have (or can install) a clang-12 pre-release.

[rettichschnidi]

I like the idea, but I think that this will inevitably result is some purely cosmetic commits (a good thing IMHO). Therefore, I'd like to see this project to start maintaining a .git-blame-ignore-revs file so that the git blame option --ignore-revs-file can be used in the future.

[pabigot]

I infer that the rationale for .git-blame-ignore-revs is to list commits that are (supposedly) whitespace only so those changes don't intrude on blame. I like the idea, but I don't believe it would be maintained to a degree that would make it useful.

[JordanYates]

I have had a look into this and have made the following observations:
ControlStatementsExceptForEachMacros is present in clang-format 11.0.0, it fixes the issue identified by @pabigot.
AlignConsecutiveMacros will probably want to use AcrossEmptyLinesAndComments, as this is the default format in a lot of Zephyr code, however this is only present in version 12.0.0, which is released, but not readily available on many platforms.

A larger issue I have identified is how clang-format treats (or mangles) lists of nested values. For example:

#define RAW {1}, {2}, {3}
// ^ goes to v
#define FORMATTED     \
	{ 1 }, { 2 }, \
	{             \
		3     \
	}

This only gets worse with more complicated structures. The Bluetooth subsystem at a minimum uses this pattern to declare things, and none of the default styles (llvm, google, webkit) formatted this example in a sane way.

The formatter also likes moving fields in struct instantiations on to the same line, which I have not identified a way to stop:

struct dummy test = {
    .a = 1, .b = 2, .c = 3,
    .d = 4
};

[ppryga-nordic]

There was another issue related with clang-format and checkpach that was solved: bit fields formatting, In past clang-format had inserted spaces before and after a colon.
It is fixed in version 12 that is already released. There is new config. option BitFieldColonSpacing that can be set to none and no white spaces will be inserted.

//Pre-12 version formatting:
struct foo {
        uint8_t bar_a : 1;
        uint8_t bar_b : 2;
        uint8_t bar_c : 5;
};

I'm interested in update the .clang-format config file to more recent version of the tool. Besides CI it is also useful for contributors. Probably most of IDEs may be configured to format whole files or modification on save. That would increase productivity.

What do you think about enabling all features that are aligned with Coding Style and do not make commits to fail on checkpatch ?
We may leave information about clang-format versions where particular features were introduced to help contributors that use older versions of the tool.

[JordanYates]

What do you think about enabling all features that are aligned with Coding Style and do not make commits to fail on checkpatch ?

We need at least 1 script doing some amount of checking against the coding style, currently this is checkpatch. While clang-format can be run on diffs only, I would guess that we can't switch to a tool which generates (IMO) garbage output for large chunks of the repo (see the nested value issue above).

If we can resolve that, and to a lesser extent the struct instantiation issue, I think it would be easy to sell the benefits to everyone.

[ppryga-nordic]

@JordanYates Is there a list of incompatibility items of clang-format and Coding Style rules? Have you got knowledge about such list?

[ppryga-nordic]

As a follow-up: I've confirmed to my satisfaction that clang-format will always insert a space between a ForEach macro used as a loop construct and the opening parenthesis of its argument:

-               SYS_SLIST_FOR_EACH_CONTAINER_SAFE(&data->cb, cb, tmp, node) {
+               SYS_SLIST_FOR_EACH_CONTAINER_SAFE (&data->cb, cb, tmp, node) {

This will generate a diagnostic from checkpatch:

-:1775: WARNING:SPACING: space prohibited between function name and open parenthesis '('
#1775: FILE: drivers/gpio/gpio_intel_apl.c:194:
+               SYS_SLIST_FOR_EACH_CONTAINER_SAFE (&data->cb, cb, tmp, node) {

Options:

• Modify clang-format to make the space insertion optional;
• Accept the change tree-wide and update checkpatch to not emit a diagnostic (viz. because it's not a function name, it's a synthesized control structure);
• Find another style formatting tool.

@pabigot The issue with foreach macros is solved with: SpaceBeforeParens : 'SBPO_ControlStatementsExceptForEachMacros'.
Thanks to that foreach statements are treated as regular functions and there is no space inserted before opening parentheses.

Sorry... I didn't spot your draft PR where it is already in use.

[ppryga-nordic]

I've ctreated and issue for:

#define RAW {1}, {2}, {3}
// ^ goes to v
#define FORMATTED     \
	{ 1 }, { 2 }, \
	{             \
		3     \
	}

in clang/llvm bug tracked: https://bugs.llvm.org/show_bug.cgi?id=50679
I'll be tracking it and provide a feedback when receive any response.

[0Grit]

Could clang-tidy be used with clang-format to get full coverage? This is stopping me dead in my tracks.

[ppryga-nordic]

Do you mean, use of clang-tidy to solve issues in coding style that are not supported by clang-format itself?

[Thalley]

I've also recently run Intel's open source ControlFlag tool (https://github.com/IntelLabs/control-flag) on the Zephyr codebase, to see if it is worth looking into. It did not find many things (and most of those were false positives it looks like), and unfortunately it takes quite a while to run (about 13h on 8/16 cores for the zephyr directory).

Logs from the run:
thread_0.log
thread_1.log
thread_2.log
thread_3.log

Based on my experience, the tool did not catch enough nor runs fast enough for consideration.

[nashif]

outdated, closing. Please open new issue with updated information if this is still needed.