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

[njames93]

Summary

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

[Mordante]

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.

[aaron.ballman]

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?

[njames93]

In D131386#3722749, @aaron.ballman wrote:

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]

Remove unneeded include and add Release notes.

[aaron.ballman]

In D131386#3722822, @njames93 wrote:

In D131386#3722749, @aaron.ballman wrote:

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.

[aaron.ballman]

In D131386#3722849, @aaron.ballman wrote:

In D131386#3722822, @njames93 wrote:

In D131386#3722749, @aaron.ballman wrote:

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.

[carlosgalvezp]

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.

[aaron.ballman]

In D131386#3723144, @carlosgalvezp wrote:

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.

[carlosgalvezp]

In D131386#3723164, @aaron.ballman wrote:

In D131386#3723144, @carlosgalvezp wrote:

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

[Mordante]

In D131386#3722749, @aaron.ballman wrote:

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?

[aaron.ballman]

In D131386#3723490, @Mordante wrote:

In D131386#3722749, @aaron.ballman wrote:

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.

In D131386#3723254, @carlosgalvezp wrote:

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

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).

[JonasToth]

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]

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.

[MyDeveloperDay]

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

Its true we made the decision if clang-format couldn't work it out we'd bail out and NOT make the change. We felt it was better to be correct than change all the const's

but if you could show us the examples it might help us work through those cases.

clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h

 //===--- ConstCorrectnessCheck.h - clang-tidy -------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONSTCORRECTNESSCHECK_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONSTCORRECTNESSCHECK_H
 
 #include "../ClangTidyCheck.h"
+#include "../utils/FixItHintUtils.h"
 #include "clang/Analysis/Analyses/ExprMutationAnalyzer.h"
 #include "llvm/ADT/DenseSet.h"
 
 namespace clang {
 namespace tidy {
 
 namespace misc {
 
private:
 
   const bool AnalyzeValues;
   const bool AnalyzeReferences;
   const bool WarnPointersAsValues;
 
   const bool TransformValues;
   const bool TransformReferences;
   const bool TransformPointersAsValues;
+
+  const utils::fixit::QualifierPolicy ConstAlignment;
 };
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONSTCORRECTNESSCHECK_H

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

 //===--- ConstCorrectnessCheck.cpp - clang-tidy -----------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "ConstCorrectnessCheck.h"
-#include "../utils/FixItHintUtils.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 
 #include <iostream>
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
+
+template <> struct OptionEnumMapping<utils::fixit::QualifierPolicy> {
+  static ArrayRef<std::pair<utils::fixit::QualifierPolicy, StringRef>>
+  getEnumMapping() {
+    static constexpr std::pair<utils::fixit::QualifierPolicy, StringRef>
+        Mapping[] = {
+            {utils::fixit::QualifierPolicy::Left, "left"},
+            {utils::fixit::QualifierPolicy::Right, "right"},
+            // Let people use east/west spelling.
+            {utils::fixit::QualifierPolicy::Left, "west"},
+            {utils::fixit::QualifierPolicy::Right, "east"},
+        };
+    return Mapping;
+  }
+};
+
 namespace misc {
 
 namespace {
 // FIXME: This matcher exists in some other code-review as well.
 // It should probably move to ASTMatchers.
 AST_MATCHER(VarDecl, isLocal) { return Node.isLocalVarDecl(); }
 AST_MATCHER_P(DeclStmt, containsAnyDeclaration,
               ast_matchers::internal::Matcher<Decl>, InnerMatcher) {
ConstCorrectnessCheck::ConstCorrectnessCheck(StringRef Name,
                                              ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       AnalyzeValues(Options.get("AnalyzeValues", true)),
       AnalyzeReferences(Options.get("AnalyzeReferences", true)),
       WarnPointersAsValues(Options.get("WarnPointersAsValues", false)),
       TransformValues(Options.get("TransformValues", true)),
       TransformReferences(Options.get("TransformReferences", true)),
       TransformPointersAsValues(
-          Options.get("TransformPointersAsValues", false)) {
+          Options.get("TransformPointersAsValues", false)),
+      ConstAlignment(
+          Options.get("ConstAlignment", utils::fixit::QualifierPolicy::Right)) {
   if (AnalyzeValues == false && AnalyzeReferences == false)
     this->configurationDiag(
         "The check 'misc-const-correctness' will not "
         "perform any analysis because both 'AnalyzeValues' and "
         "'AnalyzeReferences' are false.");
 }
 
 void ConstCorrectnessCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "AnalyzeValues", AnalyzeValues);
   Options.store(Opts, "AnalyzeReferences", AnalyzeReferences);
   Options.store(Opts, "WarnPointersAsValues", WarnPointersAsValues);
 
   Options.store(Opts, "TransformValues", TransformValues);
   Options.store(Opts, "TransformReferences", TransformReferences);
   Options.store(Opts, "TransformPointersAsValues", TransformPointersAsValues);
+
+  Options.store(Opts, "ConstAlignment", ConstAlignment);
 }
 
 void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
   const auto ConstType = hasType(isConstQualified());
   const auto ConstReference = hasType(references(isConstQualified()));
   const auto RValueReference = hasType(
       referenceType(anyOf(rValueReferenceType(), unless(isSpelledAsLValue()))));
 
void ConstCorrectnessCheck::check(const MatchFinder::MatchResult &Result) {
   // a transformation might effect the other variables as well and be incorrect.
   if (VarDeclStmt == nullptr || !VarDeclStmt->isSingleDecl())
     return;
 
   using namespace utils::fixit;
   if (VC == VariableCategory::Value && TransformValues) {
     Diag << addQualifierToVarDecl(*Variable, *Result.Context,
                                   DeclSpec::TQ_const, QualifierTarget::Value,
-                                  QualifierPolicy::Right);
+                                  ConstAlignment);
     // FIXME: Add '{}' for default initialization if no user-defined default
     // constructor exists and there is no initializer.
     return;
   }
 
   if (VC == VariableCategory::Reference && TransformReferences) {
     Diag << addQualifierToVarDecl(*Variable, *Result.Context,
                                   DeclSpec::TQ_const, QualifierTarget::Value,
-                                  QualifierPolicy::Right);
+                                  ConstAlignment);
     return;
   }
 
   if (VC == VariableCategory::Pointer) {
     if (WarnPointersAsValues && TransformPointersAsValues) {
       Diag << addQualifierToVarDecl(*Variable, *Result.Context,
                                     DeclSpec::TQ_const, QualifierTarget::Value,
-                                    QualifierPolicy::Right);
+                                    ConstAlignment);
     }
     return;
   }
 }
 
 void ConstCorrectnessCheck::registerScope(const CompoundStmt *LocalScope,
                                           ASTContext *Context) {
   auto &Analyzer = ScopesCache[LocalScope];
   if (!Analyzer)
     Analyzer = std::make_unique<ExprMutationAnalyzer>(*LocalScope, *Context);
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 - Improved :doc:`bugprone-signal-handler
   <clang-tidy/checks/bugprone/signal-handler>` check. Partial
   support for C++14 signal handler rules was added. Bug report generation was
   improved.
 
+- :doc:`misc-const-correctness <clang-tidy/checks/misc/const-correctness>`
+  check now supports a ``ConstAlignment`` option to control the location of the
+  ``const`` keyword when emitting fixes.
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

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

.. code-block:: c++
     int *ptr_value = &value;
 
     int result = 100 * (*ptr_value);
     // This modification of the pointee is still allowed and not analyzed/diagnosed.
     *ptr_value = 0;
 
     // The following pointer may not become a 'int *const'.
     int *changing_pointee = &value;
     changing_pointee = &result;
 
+.. option:: ConstAlignment 
+
+  Controls where to insert ``const`` when creating fix-it hints, defaults to ``right``.
+
+  - ``left``:
+
+   .. code-block:: c++ 
+
+     const int I = 0;
+     const int &IRef = I;
+     int *const IPtr = &I;
+
+
+  - ``right``:
+
+   .. code-block:: c++
+      
+     int const I = 0;
+     int const &IRef = I;
+     int *const IPtr = &I;
+
+  ``east`` and ``west`` aliases are also supported.

clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp

+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",WEST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: west, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",WEST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: left, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",EAST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: east, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+// RUN: %check_clang_tidy %s misc-const-correctness %t -check-suffixes=",EAST" \
+// RUN:   -config="{CheckOptions: { \
+// RUN:   misc-const-correctness.ConstAlignment: right, \
+// RUN:   misc-const-correctness.WarnPointersAsValues: true, \
+// RUN:   misc-const-correctness.TransformPointersAsValues: true \
+// RUN:   }}"
+
+void foo() {
+  int I = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'I' of type 'int' can be declared 'const'
+  // CHECK-FIXES-WEST: const int I = 0;
+  // CHECK-FIXES-EAST: int const I = 0;
+}
+
+void foo(int I) {
+  int & IRef = I;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'IRef' of type 'int &' can be declared 'const'
+  // CHECK-FIXES-WEST: const int & IRef = I;
+  // Having the double space and the const attached the the `&` is unfortunate,
+  // however it shouldn't matter for users as fixes get clang-formatted.
+  // CHECK-FIXES-EAST: int  const& IRef = I;
+
+  // When decorating a pointer with const, the const always goes to the right 
+  // of the `*`.
+  int *IPtr = &I;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'IPtr' of type 'int *' can be declared 'const'
+  // CHECK-FIXES: int *const IPtr = &I;
+}