[clang-tidy] Add new cert-dcl21-cpp check.

[xazax.hun]

Summary

This check flags postfix operator++ and operator-- declarations, where the return type is not a const value type. It also attempt to fix the error.

This rule is described by cert: https://www.securecoding.cert.org/confluence/display/cplusplus/DCL21-CPP.+Overloaded+postfix+increment+and+decrement+operators+should+return+a+const+object

[JonasToth]

clang-tidy/cert/PostfixOperatorCheck.cpp

28

could the ,this); be on this line? seems odd.

44

Location may be const

48

ReturnType may be const

50

typo. s/opeators/operators/, s/return/returns/

51

just out of curiosity, why does -> work here? In line 47 the QualType ReturnType is not a pointer.

57

here . is used instead -> iam confused :D

70

same confusion like above.
this seems to be an else if. If not, what happens in the else path of the bigger if? is it unreachable or a case that would just be ignored.

73

for clarity, this could be an else path to the if in line 69.
The if could check on the negation as well, and fix only in that case. (kinda swap the logic). I think that would be clearer.

docs/ReleaseNotes.rst

63

i think the use of operator++/-- would be better, like in the doc page.

[Eugene.Zelenko]

We have misc-unconventional-assign-operator and google-runtime-operator checks already. Form user perspective, I think will be good idea to have single check for different operators.

docs/ReleaseNotes.rst

63

Either it should be operator returns, or operators return

[aaron.ballman]

Thank you for working on this check!

clang-tidy/cert/CERTTidyModule.cpp

22

Please keep the list of includes alphabetized.

clang-tidy/cert/CMakeLists.txt

8

Please keep the list of source files alphabetized.

clang-tidy/cert/PostfixOperatorCheck.cpp

37

Better to use isInstance() instead of !isStatic().

44

Please don't use auto here as the type is not spelled out in the initializer. I don't think there's a need to mark it as a const, however (we don't usually mark non-pointer/reference local variables as const).

48

Again, we don't typically mark this sort of thing as const.

51

QualType is a wrapper around a Type * and overloads operator->().

53

You can drop the comma in the diagnostic text.

70

I'm not certain I understand the concern here. We do not use an else after a return (per the coding conventions).

73

Instead of using "constant type", I would use "constant object type", and reword it to be more similar to the previous diagnostic. In fact, you could even combine the two diagnostics into one with a %select, because this diagnostic should also receive the same fixit as the above one.

"overloaded %0 returns a %select{reference|non-constant object}1, instead of a constant object type"

docs/clang-tidy/checks/cert-dcl21-cpp.rst

7

Drop the comma.

8

where -> if

const type -> const object

Should also mention this check flags if the return type is a reference, because many people aren't going to know that a reference isn't technically an object.

10

rule -> recommendation

(it's not a CERT rule, it's a CERT recommendation).

11

DCL21-CPP, not FLP21-CPP

[xazax.hun]

• Fixes according to the review comments.

[xazax.hun]

• Fixed include order.

[alexfh]

clang-tidy/cert/PostfixOperatorCheck.cpp

28

Just let clang-format do its job, no need for manual tuning.

Finder->addMatcher(functionDecl(anyOf(hasOverloadedOperatorName("++"),
                                      hasOverloadedOperatorName("--")))
                       .bind("decl"),
                   this);

In this specific case this being on a separate line is reasonable, since the first argument of addMatcher spans multiple lines and it's more difficult to spot the second argument when it's placed on the same line as .bind(...).

test/clang-tidy/cert-dcl21-cpp.cpp

2

As usual, please add tests with macros and templates with multiple instantiations. When diagnostics in macros are ignored, the tests should demonstrate this as well.

[aaron.ballman]

clang-tidy/cert/CMakeLists.txt

8

This is still in the wrong position in the list.

clang-tidy/cert/PostfixOperatorCheck.cpp

73

Is there a reason you didn't combine the two diagnostics?

[xazax.hun]

• Fix alphabetical ordering of files in cmake.
• Let clang-format do its job.

clang-tidy/cert/PostfixOperatorCheck.cpp

73

Since the fixits are also different for the two cases, I think it is cleaner to keep the diag separate.

[aaron.ballman]

test/clang-tidy/cert-dcl21-cpp.cpp

2

In addition to these tests, I'd like to see a test for the following:

struct S {};
typedef S& SRef;

SRef operator++(SRef, int);

struct T {
  typedef T& TRef;
  
  TRef operator++(int);
};

struct U {
  typedef const U& ConstURef;
  
  ConstURef& operator++(int); // There's an extra ref shoved on here for fun.
};

struct V {
  V *operator++(int);
  V *const operator--(int);
};

[xazax.hun]

• Added more test cases and make them pass.

[xazax.hun]

test/clang-tidy/cert-dcl21-cpp.cpp

2

I added those test cases.

I choose not to warn about primitive and pointer types since it makes no sense to make them const. Do you agree?

Unfortunately, the current fixits might use the underlying type instead of the typedef. Is that a blocker for this to be commited?
In same cases it is not possible to preserve the typedef, e.g. when the underlying type is a reference. (Or we need to insert a std::remove_reference meta call).

In one case the const is not added. This is because the underlying type is already const. Rewriting the fixit is not trivial, because it is not easy to get the source range for the whole return type. Unfortunately, getReturnTypeSourceRange ignores the qualifiers.

[aaron.ballman]

test/clang-tidy/cert-dcl21-cpp.cpp

2

I choose not to warn about primitive and pointer types since it makes no sense to make them const. Do you agree?

I think that's reasonable behavior, yes.

Unfortunately, the current fixits might use the underlying type instead of the typedef. Is that a blocker for this to be commited?

That's what I was wondering about. You may want to simply disable the fixit if the underlying type is a typedef (or alias, etc).

In same cases it is not possible to preserve the typedef, e.g. when the underlying type is a reference. (Or we need to insert a std::remove_reference meta call).

Yes, I think it's a complex problem that doesn't need to be solved right now.

In one case the const is not added. This is because the underlying type is already const. Rewriting the fixit is not trivial, because it is not easy to get the source range for the whole return type. Unfortunately, getReturnTypeSourceRange ignores the qualifiers.

That is unfortunate, but I don't think it's a deal-breaker, either.

[xazax.hun]

• Do not do fixits for type aliases.

[xazax.hun]

test/clang-tidy/cert-dcl21-cpp.cpp

2

Good idea, done.

[aaron.ballman]

LGTM!

clang-tidy/cert/CERTTidyModule.cpp

 #include "../misc/MoveConstructorInitCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
 #include "CommandProcessorCheck.h"
 #include "DontModifyStdNamespaceCheck.h"
 #include "FloatLoopCounter.h"
 #include "LimitedRandomnessCheck.h"
+#include "PostfixOperatorCheck.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cert {
 
 class CERTModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     // C++ checkers
     // DCL
+    CheckFactories.registerCheck<PostfixOperatorCheck>(
+        "cert-dcl21-cpp");
     CheckFactories.registerCheck<VariadicFunctionDefCheck>("cert-dcl50-cpp");
     CheckFactories.registerCheck<misc::NewDeleteOverloadsCheck>(
         "cert-dcl54-cpp");
     CheckFactories.registerCheck<DontModifyStdNamespaceCheck>(
         "cert-dcl58-cpp");
     CheckFactories.registerCheck<google::build::UnnamedNamespaceInHeaderCheck>(
         "cert-dcl59-cpp");
     // OOP

clang-tidy/cert/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCERTModule
   CERTTidyModule.cpp
   CommandProcessorCheck.cpp
   DontModifyStdNamespaceCheck.cpp
   FloatLoopCounter.cpp
   LimitedRandomnessCheck.cpp
+  PostfixOperatorCheck.cpp
   SetLongJmpCheck.cpp
   StaticObjectExceptionCheck.cpp
   StrToNumCheck.cpp
   ThrownExceptionTypeCheck.cpp
   VariadicFunctionDefCheck.cpp
 
   LINK_LIBS
   clangAnalysis

clang-tidy/cert/PostfixOperatorCheck.h

+//===--- PostfixOperatorCheck.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_CERT_POSTFIX_OPERATOR_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_POSTFIX_OPERATOR_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+/// Checks if the overloaded postfix ++ and -- operator return a constant
+/// object.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cert-postfix-operator.html
+class PostfixOperatorCheck : public ClangTidyCheck {
+public:
+  PostfixOperatorCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CERT_POSTFIX_OPERATOR_H

clang-tidy/cert/PostfixOperatorCheck.cpp

+//===--- PostfixOperatorCheck.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 "PostfixOperatorCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cert {
+
+void PostfixOperatorCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+    return;
+
+  Finder->addMatcher(functionDecl(anyOf(hasOverloadedOperatorName("++"),
+                                        hasOverloadedOperatorName("--")))
+                         .bind("decl"),
+                     this);
+}
+
+void PostfixOperatorCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *FuncDecl = Result.Nodes.getNodeAs<FunctionDecl>("decl");
+
+  bool HasThis = false;
+  if (const auto *MethodDecl = dyn_cast<CXXMethodDecl>(FuncDecl))
+    HasThis = MethodDecl->isInstance();
+
+  // Check if the operator is a postfix one.
+  if (FuncDecl->getNumParams() != (HasThis ? 1 : 2))
+    return;
+
+  SourceRange ReturnRange = FuncDecl->getReturnTypeSourceRange();
+  SourceLocation Location = ReturnRange.getBegin();
+  if (!Location.isValid())
+    return;
+
+  QualType ReturnType = FuncDecl->getReturnType();
+
+  // Warn when the operators return a reference.
+  if (ReturnType->isReferenceType()) {
+    auto Diag = diag(Location, "overloaded %0 returns a reference instead of a "
+                               "constant object type")
+                << FuncDecl;
+
+    if (Location.isMacroID())
+      return;
+
+    QualType ReplaceType =
+        ReturnType.getNonReferenceType().getLocalUnqualifiedType();
+    // The getReturnTypeSourceRange omits the qualifiers. We do not want to
+    // duplicate the const.
+    if (!ReturnType->getPointeeType().isConstQualified())
+      ReplaceType.addConst();
+
+    Diag << FixItHint::CreateReplacement(
+        ReturnRange,
+        ReplaceType.getAsString(Result.Context->getPrintingPolicy()) + " ");
+
+    return;
+  }
+
+  if (ReturnType.isConstQualified() || ReturnType->isBuiltinType() ||
+      ReturnType->isPointerType())
+    return;
+
+  auto Diag =
+      diag(Location, "overloaded %0 returns a non-constant object instead of a "
+                     "constant object type")
+      << FuncDecl;
+
+  if (!Location.isMacroID())
+    Diag << FixItHint::CreateInsertion(Location, "const ");
+}
+
+} // namespace cert
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
+- New `cert-dcl21-cpp
+  <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl21-cpp.html>`_ check
+
+  Checks if the overloaded postfix ``operator++/--`` returns a constant object.
+
 - New `cert-dcl58-cpp
   <http://clang.llvm.org/extra/clang-tidy/checks/cert-dcl58-cpp.html>`_ check
 
   Finds modification of the ``std`` or ``posix`` namespace.
 
 - Improved `cppcoreguidelines-no-malloc
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-no-malloc.html>`_ check
 

docs/clang-tidy/checks/cert-dcl21-cpp.rst

+.. title:: clang-tidy - cert-dcl21-cpp
+
+cert-dcl21-cpp
+==============
+
+This check flags postfix ``operator++`` and ``operator--`` declarations
+if the return type is not a const object. This also warns if the return type
+is a reference type.
+
+This check corresponds to the CERT C++ Coding Standard recommendation
+`DCL21-CPP. Overloaded postfix increment and decrement operators should return a const object
+<https://www.securecoding.cert.org/confluence/display/cplusplus/DCL21-CPP.+Overloaded+postfix+increment+and+decrement+operators+should+return+a+const+object>`_.

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    boost-use-to-string
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
+   cert-dcl21-cpp
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl58-cpp
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-env33-c
    cert-err09-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err09-cpp>
    cert-err34-c
    cert-err52-cpp

test/clang-tidy/cert-dcl21-cpp.cpp

+// RUN: %check_clang_tidy %s cert-dcl21-cpp %t
+
+class A {};
+
+A operator++(A &, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator++' returns a non-constant object instead of a constant object type [cert-dcl21-cpp]
+// CHECK-FIXES: {{^}}const A operator++(A &, int);
+
+A operator--(A &, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator--' returns a no
+// CHECK-FIXES: {{^}}const A operator--(A &, int);
+
+class B {};
+
+B &operator++(B &);
+const B operator++(B &, int);
+
+B &operator--(B &);
+const B operator--(B &, int);
+
+
+class D {
+D &operator++();
+const D operator++(int);
+
+D &operator--();
+const D operator--(int);
+};
+
+class C {
+C operator++(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator++' returns a no
+// CHECK-FIXES: {{^}}const C operator++(int);
+
+C operator--(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator--' returns a no
+// CHECK-FIXES: {{^}}const C operator--(int);
+};
+
+class E {};
+
+E &operator++(E &, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator++' returns a reference instead of a constant object type [cert-dcl21-cpp]
+// CHECK-FIXES: {{^}}const E operator++(E &, int);
+
+E &operator--(E &, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator--' returns a re
+// CHECK-FIXES: {{^}}const E operator--(E &, int);
+
+class G {
+G &operator++(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator++' returns a re
+// CHECK-FIXES: {{^}}const G operator++(int);
+
+G &operator--(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator--' returns a re
+// CHECK-FIXES: {{^}}const G operator--(int);
+};
+
+class F {};
+
+const F &operator++(F &, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: overloaded 'operator++' returns a re
+// CHECK-FIXES: {{^}}const F operator++(F &, int);
+
+const F &operator--(F &, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: overloaded 'operator--' returns a re
+// CHECK-FIXES: {{^}}const F operator--(F &, int);
+
+class H {
+const H &operator++(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: overloaded 'operator++' returns a re
+// CHECK-FIXES: {{^}}const H operator++(int);
+
+const H &operator--(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: overloaded 'operator--' returns a re
+// CHECK-FIXES: {{^}}const H operator--(int);
+};
+
+
+#define FROM_MACRO P&
+class P {
+const FROM_MACRO operator++(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: overloaded 'operator++' returns a re
+// CHECK-FIXES: {{^}}const FROM_MACRO operator++(int);
+};
+
+
+template<typename T>
+class Q {
+const Q &operator++(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: overloaded 'operator++' returns a re
+// CHECK-FIXES: {{^}}const Q<T> operator++(int);
+
+const Q &operator--(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: overloaded 'operator--' returns a re
+// CHECK-FIXES: {{^}}const Q<T> operator--(int);
+};
+
+void foobar() {
+  Q<int> a;
+  Q<float> b;
+  (void)a;
+  (void)b;
+}
+
+struct S {};
+typedef S& SRef;
+
+SRef operator++(SRef, int);
+// CHECK-MESSAGES: :[[@LINE-1]]:1: warning: overloaded 'operator++' returns a re
+// CHECK-FIXES: {{^}}const S  operator++(SRef, int);
+
+struct T {
+  typedef T& TRef;
+  
+  TRef operator++(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: overloaded 'operator++' returns a re
+// CHECK-FIXES: {{^}}  const T  operator++(int);
+};
+
+struct U {
+  typedef const U& ConstURef;
+  
+  ConstURef& operator++(int);
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: overloaded 'operator++' returns a re
+// FIXME: Insert const for the return type.
+// CHECK-FIXES: {{^}}  U  operator++(int);
+};
+
+struct V {
+  V *operator++(int);
+  V *const operator--(int);
+};
+