[clang-tidy] Implement command line option for include style and deprecated such check-specific options

[EugeneZelenko]

Will be good idea to implement command line option for include style (like --format-style) and deprecated such check-specific options. Such check-specific options may be contradictory when code base (or particular part of it) must confirm to single style.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: None (EugeneZelenko)

Details

Will be good idea to implement command line option for include style (like `--format-style`) and deprecated such check-specific options. Such check-specific options may be contradictory when code base (or particular part of it) must confirm to single style.

[carlosgalvezp]

Personally I would remove these options altogether. It's not the responsibility of clang-tidy to do formatting. One should always run clang-format after clang-tidy fixes.

[NagyDonat]

It's not the responsibility of clang-tidy to do formatting.

Personally (as one random contributor) I disagree with this: clang-tidy is responsible for formatting the fragments that it inserts in an appropriate style. As an exaggerated example it would be inappropriate if clang-tidy fixits used formatting like foo( bar , baz ) or foo(bar,baz) even if the code works and the unusual whitespace can be fixed by clang-format. Obviously clang-tidy wouldn't produce so severe style violations, but I think it is reasonable to expect that the fixits can follow the style of the codebase (if it is one of the widespread styles).

One should always run clang-format after clang-tidy fixes.

That is a bit cumbersome to set up, especially if the user just want to adjust the style of changes made by clang tidy without touching his own code. (I know that clang-format can be configured and limited to modifying just the changed lines in a diff, but it is still far from the "out of the box" experience and I don't think that it can guarantee perfect behavior.)

I know that many consider it "good practice" to hand over control of formatting to an automated tool like clang-format, but I don't think that this is so universal that clang-tidy may rely on it.

[carlosgalvezp]

in an appropriate style

What would be the appropriate style? There should be one single source of truth for the formatting style, and that's exactly clang-format. Using clang-tidy for formatting is duplicating information and implementation, which inevitably leads to divergence and conflicts. And ultimately one has to run clang-format anyway, because that's the format enforced in CI.

Obviously clang-tidy should produce some reasonable style as you pointed out, but it should not attempt to do everything that clang-format does (in this case, include style). It gets even worse if people then request even more settings to port from clang-format to clang-tidy, since "the door is open".

still far from the "out of the box" experience

I learned about git clang-format when I started working on this project and I was actually surprised how easy it was to setup, I don't recall any pitfalls or issues really. Maybe documentation is lacking?

but I don't think that this is so universal that clang-tidy may rely on it.

IMHO if a project is serious enough to run clang-tidy, then it would be serious enough to run clang-format too (which I believe is a much more basic thing to have).

I also personally believe it's our responsibility to design tools in a way that encourages C++ developers to follow "best practices". If we create "shortcuts" like these we don't encourage that, and we also create big maintenance problems for ourselves, taking our valuable time.

[NagyDonat]

What would be the appropriate style? There should be one single source of truth for the formatting style, and that's exactly clang-format.

Yes, single source of truth is a strong reason for relying on the clang-format config in some way. What I find philosophically inelegant is:

• if clang-tidy produces "unfinished" output and expects the user to invoke a separate tool;
• if there is no clear way to limit the automatic formatting to the code generated by tidy without also reformatting code manually written by the developer.
  For a developer or team, it is often a good idea to voluntarily hand over code formatting to a tool like clang-format, but I think it is overbearing if a tool used for a different purpose (e.g. clang-tidy in this case, but some new languages have pretty opinionated module/package systems) forces this decision.

In my opinion, the ideal behavior would be if clang-tidy automatically invoked clang-format on just the code inserted by clang-tidy without touching any other code. However I see that this is not practical ("just the code inserted" is not well-defined), so I'm suggesting a pragmatic compromise where:

• clang-tidy allows configuring the format of the fixits in a few cases where this is an important convenience for the users (e.g. there are several equally "appropriate" but clashing styles)
• rarer and difficult-to-implement situations are not handled in tidy can be covered by clang-format.

For example, the fact that several checks already have "include style" options suggests that this the users need this feature and use it within their workflow. Removing these options for the sake of theoretical purity would be a step backwards for those users.

I learned about git clang-format when I started working on this project and I was actually surprised how easy it was to setup, I don't recall any pitfalls or issues really. Maybe documentation is lacking?

I'm using also git clang-format on the LLVM repository and that is indeed easy -- I was mostly concerned about the difficulty of adopting clang-format on a project that doesn't use it yet and has its own traditional coding style. (I know that git clang-format can support a gradual transition by only formatting the commit diff, but what if the team doesn't want to change the style?)

IMHO if a project is serious enough to run clang-tidy, then it would be serious enough to run clang-format too (which I believe is a much more basic thing to have).

It's a good point that clang-tidy is a more heavyweight/serious tool, but its benefits are also more concrete. I can imagine a team that adapts clang-tidy as a tool for finding concrete security issues, but cannot justify spending time on clang-format which only provides vague benefits.

---

One more remark: if the formatting style would be hardcoded in clang-tidy, we would run into uncomfortable bikeshedding about which style to hardcode -- and we could end up with multiple clashing styles frozen in different checks based on the preferences of the original authors. (And I think it is unreasonable to ask a contributor who wrote a check with fixits formatted in one widespread style to completely switch to using a different style in the fixits.)

---

Note: I'm just one random opinion in the community, so can understand if you choose a decision that I do not agree with.

[vbvictor]

In my opinion, the ideal behavior would be if clang-tidy automatically invoked clang-format on just the code inserted by clang-tidy without touching any other code.

Just my 2 cents for now on the discussion: clang-tidy has a way to run clang-format on fixes with an option:

--format-style=<string>          - Style for formatting code around applied fixes:
                                       - 'none' (default) turns off formatting
                                       - 'file' (literally 'file', not a placeholder)
                                         uses .clang-format file in the closest parent
                                         directory
                                       - '{ <json> }' specifies options inline, e.g.
                                         -format-style='{BasedOnStyle: llvm, IndentWidth: 8}'
                                       - 'llvm', 'google', 'webkit', 'mozilla'
                                     See clang-format documentation for the up-to-date
                                     information about formatting styles and options.
                                     This option overrides the 'FormatStyle` option in
                                     .clang-tidy file, if any.

And by looking at implementation, it must only format the replacements:

llvm-project/clang-tools-extra/clang-tidy/ClangTidy.cpp

Lines 213 to 228 in b9859d0

	auto Style = format::getStyle(
	*Context.getOptionsForFile(File).FormatStyle, File, "none");
	if (!Style) {
	llvm::errs() << llvm::toString(Style.takeError()) << "\n";
	continue;
	}
	llvm::Expected<tooling::Replacements> Replacements =
	format::cleanupAroundReplacements(
	Code, FileAndReplacements.second.Replaces, *Style);
	if (!Replacements) {
	llvm::errs() << llvm::toString(Replacements.takeError()) << "\n";
	continue;
	}
	if (llvm::Expected<tooling::Replacements> FormattedReplacements =
	format::formatReplacements(Code, *Replacements, *Style)) {
	Replacements = std::move(FormattedReplacements);

[NagyDonat]

Just my 2 cents for now on the discussion: clang-tidy has a way to run clang-format on fixes with an option:

Oh, this looks great!

Then it's fairly reasonable to avoid redundant configuration options within clang-tidy itself – but the documentation of checks should probably refer to the documentation of --format-style. ("To customize the style of the include directive, use ..." + link -- does this command line flag has documentation on the web that can be linked to?)

(Of course we still should not delete formatting config options from tidy without ensuring that clang-format actually has a rule/option for handling that kind of formatting choice.)

[carlosgalvezp]

Ah, I wasn't aware of that option, great! Encouraging people to use that instead and removing the options would be best IMHO.

The reason I bring this up is because clang-format has a fair amount of options for includes, not just one, and it would be unfortunate if people would request such options to be ported to clang-tidy. I'd be fine keeping the current options, and consolidating them is good. Maybe it would feel better to have them only available via config file and not command line.

I also wanted to bring this up because this ticket would mean a breaking change, so we might want to take the opportunity to do everything in one go (as opposed to first harmonizing the check-specific options and then removing the command line option (if people agree this makes sense), two breaking changes).