Page MenuHomePhabricator

[clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`
Needs ReviewPublic

Authored by njames93 on Aug 8 2022, 2:38 AM.

Details

Summary

Adds an option to specify where the const should be placed when emitting fixes.

Diff Detail

Event Timeline

njames93 created this revision.Aug 8 2022, 2:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 2:38 AM
njames93 requested review of this revision.Aug 8 2022, 2:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2022, 2:38 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Mordante added inline comments.
clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
9–10

This is already included in the header.

65

I would suggest to use QualifierAlignment to match the name in clang-format.

We leave formatting decisions in clang-tidy to clang-format and I don't think we should deviate from that policy here without a very clear understanding of when we should relax that restriction. That said, I'm personally not certain we should have such an option (the long-term goal has generally been to integrate clang-format functionality into clang-tidy so there can be an option to just run format after applying fixes in a TU). Is there a compelling reason we should have it?

We leave formatting decisions in clang-tidy to clang-format and I don't think we should deviate from that policy here without a very clear understanding of when we should relax that restriction. That said, I'm personally not certain we should have such an option (the long-term goal has generally been to integrate clang-format functionality into clang-tidy so there can be an option to just run format after applying fixes in a TU). Is there a compelling reason we should have it?

The reason for this is due to the issue that QualifierAlignment is a non whitespace only change and clang-format lists that using it could break some code.
In light of this some users may wish to set the option to QAS_Leave to be sure no code is broken even though they would prefer a specific style.
Therefore having a dedicated option in the check will let those users specify the style, without having to set a clang-format configuration which they aren't content in using.

clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
65

I thought about that, but the clang-format option doesn't just align the const qualifier. It works for all qualifiers.
I am easy on what name we use for the option.

njames93 updated this revision to Diff 452630.Aug 15 2022, 4:54 AM
njames93 marked an inline comment as done.

Remove unneeded include and add Release notes.

We leave formatting decisions in clang-tidy to clang-format and I don't think we should deviate from that policy here without a very clear understanding of when we should relax that restriction. That said, I'm personally not certain we should have such an option (the long-term goal has generally been to integrate clang-format functionality into clang-tidy so there can be an option to just run format after applying fixes in a TU). Is there a compelling reason we should have it?

The reason for this is due to the issue that QualifierAlignment is a non whitespace only change and clang-format lists that using it could break some code.

Yes, and the community decided that risk was reasonable for clang-format.

In light of this some users may wish to set the option to QAS_Leave to be sure no code is broken even though they would prefer a specific style.
Therefore having a dedicated option in the check will let those users specify the style, without having to set a clang-format configuration which they aren't content in using.

When we decided that clang-format was allowed to break code, we also decided that any time it does so on real world code is considered a bug and is something we should work to fix. So I'm not in favor of this change; if clang-tidy uncovers a bug in clang-format, that should be fixed in clang-format rather than worked around in clang-tidy. And if clang-tidy can't trust clang-format to be production quality, that's something we should address with the clang-format folks as a serious concern that needs a plan of action. But I don't think introducing formatting options into clang-tidy is a road we want to go down.

We leave formatting decisions in clang-tidy to clang-format and I don't think we should deviate from that policy here without a very clear understanding of when we should relax that restriction. That said, I'm personally not certain we should have such an option (the long-term goal has generally been to integrate clang-format functionality into clang-tidy so there can be an option to just run format after applying fixes in a TU). Is there a compelling reason we should have it?

The reason for this is due to the issue that QualifierAlignment is a non whitespace only change and clang-format lists that using it could break some code.

Yes, and the community decided that risk was reasonable for clang-format.

In light of this some users may wish to set the option to QAS_Leave to be sure no code is broken even though they would prefer a specific style.
Therefore having a dedicated option in the check will let those users specify the style, without having to set a clang-format configuration which they aren't content in using.

When we decided that clang-format was allowed to break code, we also decided that any time it does so on real world code is considered a bug and is something we should work to fix. So I'm not in favor of this change; if clang-tidy uncovers a bug in clang-format, that should be fixed in clang-format rather than worked around in clang-tidy. And if clang-tidy can't trust clang-format to be production quality, that's something we should address with the clang-format folks as a serious concern that needs a plan of action. But I don't think introducing formatting options into clang-tidy is a road we want to go down.

I suppose another way to look at it is: clang-format options that are considered dangerous (comes with a warning as QualifierAlignment does in https://clang.llvm.org/docs/ClangFormatStyleOptions.html) are things clang-tidy checks with fixes could have options for. However, that gets awkward when clang-format finds a way to remove that warning; then clang-tidy is left supporting formatting options that it shouldn't have (but removing those options can break people's .clang-tidy config files), so I'm not certain I like that approach any better.

but removing those options can break people's .clang-tidy config files

Aren't there deprecation mechanisms? I think trying to be backwards compatible across all possible versions can lead to suboptimal solutions and make the tool harder to use and hard/slow to adapt to the needs of the community.

but removing those options can break people's .clang-tidy config files

Aren't there deprecation mechanisms? I think trying to be backwards compatible across all possible versions can lead to suboptimal solutions and make the tool harder to use and hard/slow to adapt to the needs of the community.

Nothing stops us from deprecating config options with some nice diagnostic behavior, but again, this pushes the work off onto the user to maintain their scripts and gives them an inconsistent interface to clang-tidy where some checks get formatting options and other checks do not.

but removing those options can break people's .clang-tidy config files

Aren't there deprecation mechanisms? I think trying to be backwards compatible across all possible versions can lead to suboptimal solutions and make the tool harder to use and hard/slow to adapt to the needs of the community.

Nothing stops us from deprecating config options with some nice diagnostic behavior, but again, this pushes the work off onto the user to maintain their scripts and gives them an inconsistent interface to clang-tidy where some checks get formatting options and other checks do not.

Yes, I'd agree that the interface becomes inconsistent. Best would be a unified tool that does both fix and formatting as you mentioned earlier.

FWIW, there are already other checks that have formatting settings, for example:
https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/pro-bounds-constant-array-index.html#cmdoption-arg-includestyle

We leave formatting decisions in clang-tidy to clang-format and I don't think we should deviate from that policy here without a very clear understanding of when we should relax that restriction. That said, I'm personally not certain we should have such an option (the long-term goal has generally been to integrate clang-format functionality into clang-tidy so there can be an option to just run format after applying fixes in a TU). Is there a compelling reason we should have it?

The reason for me to work on this feature is the fact that clang-format doesn't work reliable for changing the location of the const. After using clang-tidy and clang-format some consts were still one the right hand side. I didn't do a deep investigation, but it seems to affect classes in namespaces. So some occurrences of std::string were affected, but not all. So in the current state I can't use this clang-tidy fix.

Since clang-format doesn't fully parse the code I wonder whether all cases can be properly fixed. On the other hand clang-tidy has all information available.

Would integrating clang-format in clang-tidy mean clang-format will use the full information of the AST?

clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
65

Fair point.

clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
149

Do you want to incorporate the D131859 here too or do you prefer that I make a separate review for that?

We leave formatting decisions in clang-tidy to clang-format and I don't think we should deviate from that policy here without a very clear understanding of when we should relax that restriction. That said, I'm personally not certain we should have such an option (the long-term goal has generally been to integrate clang-format functionality into clang-tidy so there can be an option to just run format after applying fixes in a TU). Is there a compelling reason we should have it?

The reason for me to work on this feature is the fact that clang-format doesn't work reliable for changing the location of the const. After using clang-tidy and clang-format some consts were still one the right hand side. I didn't do a deep investigation, but it seems to affect classes in namespaces. So some occurrences of std::string were affected, but not all. So in the current state I can't use this clang-tidy fix.

CC @MyDeveloperDay and @owenpan for awareness.

Since clang-format doesn't fully parse the code I wonder whether all cases can be properly fixed. On the other hand clang-tidy has all information available.

Mostly. clang-tidy will miss everything in a preprocessor conditional block, as an example of what it can't handle but clang-format can.

Would integrating clang-format in clang-tidy mean clang-format will use the full information of the AST?

I wouldn't imagine that to be the case (that'd be a pretty big change to the architecture of clang-format; basically a rewrite). I would expect it to be integrated more as a library to call into to perform the post-fix formatting to just specific ranges of code. However, it is interesting to think about the fact that such a library could potentially accept an optional AST node representing the root of the AST represented by the text as a way to help it disambiguate situations it couldn't otherwise figure out.

Those docs are pretty terrible because I have no idea what an "llvm-style" include even *is*.

IIRC, we added this because we added the include sorter to clang-tidy and at the time, it was deemed unsafe to add to clang-format because of the likelihood of breaking code. IMO, it's on the borderline as to whether we should have added it or not -- it's the same situation I was worried about above where we add a feature to clang-tidy and then clang-format later gets the ability to perform the formatting and so the two gradually drift out of sync. Then again, it seems our checks have drifted out of sync with what IncludeSorter supports because there's a third option for google Objective C formatting style (whatever that is).

during the original implementation of the fix-util I had the plan to provide an option for east-vs-west const, but during the discussions we came to the conclusion that clang-format should do this (the patches were already pending at that point in time).

if this option works, i think its ok to provide it. having this check on during development is maybe just annoying when the const always pops up at the "wrong" side.

but i think we should provide more test cases here, especially for the edge cases where const _must_ be on the right side, because it would otherwise apply to something else then intended. that was the original reason why i sticked to east-const, because thats always correct.

JonasToth added inline comments.Sep 1 2022, 12:42 AM
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp
43

could you please provide more test cases with pointers in combination with

  • macros
  • typedefs
  • template parameters
  • auto *

I think we should be thorough here. The users might not have all details of where const applies in their forehead and big transformations of code-bases should definitely not break the code.