Misplaced const-qualification with typedef types

[aaron.ballman]

Summary

Sometimes it's easy for developers to not notice that the type they are working with is a typedef type to a pointer and add a const-qualifier that applies to the typedef type rather than the underlying pointer type. e.g.,

typedef int *int_ptr;
void func(const int_ptr ip);

This results in ip having the type int * const rather than const int *.

This patch adds a new clang-tidy check to diagnose such constructs to alert the developer of the difference in types. It does not diagnose if the underlying pointer type points to a const type or is a function type because the const-qualifier on the typedef is more likely to be intentional.

[sbenza]

I think this would be more interesting with macros.
Eg triggering in code like this:

#define FOO(type, op) const type& X = op()
FOO(int*, bar);

clang-tidy/misc/MisplacedConstCheck.cpp

33

guessAlternateQualification (lower case)

test/clang-tidy/misc-misplaced-const.c

21

If we are guessing that 'const' goes to the int, shouldn't we guess that volatile also goes to the int?

21

Only keep the first instance with the full message.
The rest can be collapsed with {{.*}} to try to fit in 80 cols.

test/clang-tidy/misc-misplaced-const.cpp

21

I assume this check does not trigger for 'const X&' where X is a pointer.
Please add a test for that.

[aaron.ballman]

Thank you for the quick turn-around!

test/clang-tidy/misc-misplaced-const.c

21

Actually, that test was purposely showing that we do *not* treat all cv-qualifiers the same. volatile and restrict have sufficiently complex semantics that I would be wary about pointing out the type differences as being potentially problematic. Typically, if you are using those qualifiers, you probably know more about what you want the code to do and whether you want the *pointer* to have the qualifier instead of the pointee.

21

I'll collapse the ones after the volatile test.

test/clang-tidy/misc-misplaced-const.cpp

21

Do you want:

template <typename Ty>
struct S {
  const Ty *i;
  const Ty &i2;
};

template struct S<int>;
template struct S<ip>; // ok
template struct S<cip>;
template struct S<int *>; // ok

Or something else?

[hokein]

test/clang-tidy/misc-misplaced-const.c

18

Will the check show the warning on ip const i3 = 0;? Technically, const ip i3 = 0 is exactly the same with ip const i3 = 0;.

[aaron.ballman]

test/clang-tidy/misc-misplaced-const.c

18

Yes, it will, but I should add a test case for it just the same.

[aaron.ballman]

Updated based on review feedback:

• Added another two test cases (one for templates and one for const binding to the right of the typedef type).
• Updated test cases to be less verbose in message checking
• Corrected case for a static function

[aaron.ballman]

In D21036#450106, @sbenza wrote:

I think this would be more interesting with macros.
Eg triggering in code like this:

#define FOO(type, op) const type& X = op()
FOO(int*, bar);

Sorry, I originally missed this comment.

That might be a reasonable addition to this check, but strikes me as a future enhancement. Are you looking for the patch to cover this case initially, or is it fine for a follow-up?

[sbenza]

In D21036#450935, @aaron.ballman wrote:

In D21036#450106, @sbenza wrote:

I think this would be more interesting with macros.
Eg triggering in code like this:

#define FOO(type, op) const type& X = op()
FOO(int*, bar);

Sorry, I originally missed this comment.

That might be a reasonable addition to this check, but strikes me as a future enhancement. Are you looking for the patch to cover this case initially, or is it fine for a follow-up?

Yes, a follow up is ok. I was just making sure this is considered.

[sbenza]

clang-tidy/misc/MisplacedConstCheck.cpp

23

allOf() is unnecessary

27

Should we ignore pointer to members?
If not, we should add a test for those.

[aaron.ballman]

Corrected more review feedback.

clang-tidy/misc/MisplacedConstCheck.cpp

23

Good catch, that was a holdover from when I was using clang-query.

27

Pointer to members are not pointer types (they're a MemberPointerType which derives from Type, not PointerType), so they are ignored already. I think that's reasonable because we'd want the pointer to member function types to behave the same as function types. I'll add an explicit test case.

[sbenza]

clang-tidy/misc/MisplacedConstCheck.cpp

23

You shouldn't need it for clang-query either.

[aaron.ballman]

clang-tidy/misc/MisplacedConstCheck.cpp

23

Oddly enough, I did. What's more, there's a bug with hasType that I didn't have time to track down. It seems hasType(allOf(...)) always fails but allOf(hasType(...), hasType(...)) works. :-(

[aaron.ballman]

Committed in r272025.

[hokein]

@aaron.ballman, you forgot to add the check in docs/ReleaseNotes.rst.

clang-tidy/misc/CMakeLists.txt

 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
+  MisplacedConstCheck.cpp
   UnconventionalAssignOperatorCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DanglingHandleCheck.cpp

clang-tidy/misc/MiscTidyModule.cpp

 #include "../ClangTidyModuleRegistry.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
+#include "MisplacedConstCheck.h"
 #include "UnconventionalAssignOperatorCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DanglingHandleCheck.h"

     CheckFactories.registerCheck<ArgumentCommentCheck>("misc-argument-comment");
     CheckFactories.registerCheck<AssertSideEffectCheck>(
         "misc-assert-side-effect");
+    CheckFactories.registerCheck<MisplacedConstCheck>(
+        "misc-misplaced-const");
     CheckFactories.registerCheck<UnconventionalAssignOperatorCheck>(
         "misc-unconventional-assign-operator");
     CheckFactories.registerCheck<BoolPointerImplicitConversionCheck>(

clang-tidy/misc/MisplacedConstCheck.h

+//===--- MisplacedConstCheck.h - clang-tidy----------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISPLACED_CONST_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISPLACED_CONST_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This check diagnoses when a const qualifier is applied to a typedef to a
+/// pointer type rather than to the pointee.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-misplaced-const.html
+class MisplacedConstCheck : public ClangTidyCheck {
+public:
+  MisplacedConstCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISPLACED_CONST_H

clang-tidy/misc/MisplacedConstCheck.cpp

+//===--- MisplacedConstCheck.cpp - clang-tidy------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "MisplacedConstCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void MisplacedConstCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      valueDecl(hasType(isConstQualified()),
+                hasType(typedefType(hasDeclaration(
+                    typedefDecl(hasType(pointerType(unless(pointee(
+                                    anyOf(isConstQualified(),
+                                          ignoringParens(functionType())))))))
+                        .bind("typedef")))))
+          .bind("decl"),
+      this);
+}
+
+static QualType guessAlternateQualification(ASTContext &Context, QualType QT) {
+  // We're given a QualType from a typedef where the qualifiers apply to the
+  // pointer instead of the pointee. Strip the const qualifier from the pointer
+  // type and add it to the pointee instead.
+  if (!QT->isPointerType())
+    return QT;
+
+  Qualifiers Quals = QT.getLocalQualifiers();
+  Quals.removeConst();
+
+  QualType NewQT = Context.getPointerType(
+      QualType(QT->getPointeeType().getTypePtr(), Qualifiers::Const));
+  return NewQT.withCVRQualifiers(Quals.getCVRQualifiers());
+}
+
+void MisplacedConstCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Var = Result.Nodes.getNodeAs<ValueDecl>("decl");
+  const auto *Typedef = Result.Nodes.getNodeAs<TypedefDecl>("typedef");
+  ASTContext &Ctx = *Result.Context;
+  QualType CanQT = Var->getType().getCanonicalType();
+
+  diag(Var->getLocation(), "%0 declared with a const-qualified typedef type; "
+                           "results in the type being '%1' instead of '%2'")
+      << Var << CanQT.getAsString(Ctx.getPrintingPolicy())
+      << guessAlternateQualification(Ctx, CanQT)
+             .getAsString(Ctx.getPrintingPolicy());
+  diag(Typedef->getLocation(), "typedef declared here", DiagnosticIDs::Note);
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/list.rst

 =========================
 
 .. toctree::
-
    boost-use-to-string
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl50-cpp

    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
+   misc-misplaced-const
    misc-misplaced-widening-cast
    misc-move-const-arg
    misc-move-constructor-init

docs/clang-tidy/checks/misc-misplaced-const.rst

+.. title:: clang-tidy - misc-misplaced-const
+
+misc-misplaced-const
+====================
+
+This check diagnoses when a const qualifier is applied to a typedef to a pointer
+type rather than to the pointee, because such constructs are often misleading to
+developers because the const applies to the pointer rather than the pointee.
+
+For instance, in the following code, the resulting type is `int *` const rather
+than `const int *`:
+
+.. code:: c++
+
+  typedef int *int_ptr;
+  void f(const int_ptr ptr);
+
+The check does not diagnose when the underlying typedef type is a pointer to a
+const type or a function pointer type. This is because the const qualifier is
+less likely to be mistaken because it would be redundant (or disallowed) on the
+underlying pointee type.

test/clang-tidy/misc-misplaced-const.c

+// RUN: %check_clang_tidy %s misc-misplaced-const %t
+
+typedef int plain_i;
+typedef int *ip;
+typedef const int *cip;
+
+typedef void (*func_ptr)(void);
+
+void func(void) {
+  // ok
+  const int *i0 = 0;
+  const plain_i *i1 = 0;
+  const cip i2 = 0; // const applies to both pointer and pointee.
+
+  // Not ok
+  const ip i3 = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'i3' declared with a const-qualified typedef type; results in the type being 'int *const' instead of 'const int *'
+
+  ip const i4 = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'i4' declared with a const-qualified
+
+  const volatile ip i5 = 0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'i5' declared with a const-qualified typedef type; results in the type being 'int *const volatile' instead of 'const int *volatile'
+}
+
+void func2(const plain_i *i1,
+           const cip i2,
+           const ip i3,
+           // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: 'i3' declared with a const-qualified
+           const int *i4) {
+}
+
+struct S {
+  const int *i0;
+  const plain_i *i1;
+  const cip i2;
+  const ip i3;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: 'i3' declared with a const-qualified
+};
+
+// Function pointers should not be diagnosed because a function
+// pointer type can never be const.
+void func3(const func_ptr fp) {
+  const func_ptr fp2 = fp;
+}

test/clang-tidy/misc-misplaced-const.cpp

+// RUN: %check_clang_tidy %s misc-misplaced-const %t
+
+typedef int plain_i;
+typedef int *ip;
+typedef const int *cip;
+
+void func() {
+  if (const int *i = 0)
+    ;
+  if (const plain_i *i = 0)
+    ;
+  if (const cip i = 0)
+    ;
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: 'i' declared with a const-qualified typedef type; results in the type being 'int *const' instead of 'const int *'
+  if (const ip i = 0)
+    ;
+}
+
+template <typename Ty>
+struct S {
+  const Ty *i;
+  const Ty &i2;
+};
+
+template struct S<int>;
+template struct S<ip>; // ok
+template struct S<cip>;
+template struct S<int *>; // ok
+
+struct T {
+  typedef void (T::*PMF)();
+
+  void f() {
+    const PMF val = &T::f; // ok
+  }
+};