[clang-tidy] Add check for detecting declarations with multiple names

[omtcyfz]

Summary

C++ Core Guidelines Section "Expressions and statements" Rule 10 suggests to split declarations with multiple names into multiple single declarations. See the following example.

Example, bad.

std::vector<int> velocities(10, 0), numbers(other_numbers),
                          inputs(input.begin(), input.end());

Example, good.

std::vector<int> velocities(10, 0);
std::vector<int> numbers(other_numbers);
std::vector<int> inputs(input.begin(), input.end());

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-nameres-name-oneaes10-declare-one-name-only-per-declaration

Currently stalled, removed everyone from subscribers and reviewers for convenience.

[Eugene.Zelenko]

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

[Eugene.Zelenko]

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

26

Namespace is inconsistent with following ones. It I'm not mistaken, // namespace is prevailing form in CLang-tidy code.

[malcolm.parsons]

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

30

Can declCountIsGreaterThan(1) be written as unless(declCountIs(1))?

[omtcyfz]

Address couple comments.

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

26

You're right. That's interesting though, because LLVM Coding Guidelines suggest end namespace mah_name.

30

Sure it can. I just thought declCountIsGreaterThan might be useful at some point.

[Eugene.Zelenko]

docs/ReleaseNotes.rst

62 ↗

(On Diff #72881)

Please add one sentence description. Probably Checks for declarations with multiple names. should be enough.

[omtcyfz]

Minor improvements.

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

30

Actually, for the sake of simplicity I'll use unless(declCountIs(1)), when some other part of code would need similar stuff we'd add it. For now it's only one use case, so I should be fine.

Thank you very much for the suggestion!

docs/ReleaseNotes.rst

62 ↗

(On Diff #72881)

Aw, yes, totally forgot about that. Thank you very much!

[ioeric]

The code looks about right, but I am not the right person to decide whether checks go into clang-tidy.

[omtcyfz]

In D25024#557033, @ioeric wrote:

The code looks about right, but I am not the right person to decide whether checks go into clang-tidy.

Okay, thank you!

[aaron.ballman]

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

40

Can you also register this check under the name cert-dcl04-c as well? It fits the behavior of that recommendation too (aside from the exceptions in the recommendation, but those can be handled later). This will also require adding a .rst file for that check name and having it redirect appropriately.

https://www.securecoding.cert.org/confluence/display/c/DCL04-C.+Do+not+declare+more+than+one+variable+per+declaration

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

28

s/diagnostics/fixit ?

39

Diagnostics do not start with a capital letter. Also, this diagnostic is not really accurate. Consider void f(int x, int y); -- that's a single declaration, but it declares multiple names. Perhaps: do not declare more than one variable per declaration since this should really be focusing on variable declarations rather than parameters, template parameter lists, etc

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp

17

Please add tests that show the exceptions in the C++ Core Guidelines are properly handled. Also, I'd like to see tests with other named declarations, such as typedefs, template parameter lists, for loop initializers, etc.

[malcolm.parsons]

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp

9

The guideline says "Flag non-function arguments with multiple declarators involving declarator operators (e.g., int* p, q;)".

There are no declarator operators in this test, so there should be no warning.

17

and structured bindings (no warning).

[omtcyfz]

Addressed bunch of comments.

[omtcyfz]

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

39

Diagnostics do not start with a capital letter.

Clang SA diags do, actually. Though I can totally see the reason: consistency is important since it's clang-tidy check.

Consider void f(int x, int y); -- that's a single declaration, but it declares multiple names. Perhaps: do not declare more than one variable per declaration since this should really be focusing on variable declarations rather than parameters, template parameter lists, etc

Fixed, thank you for the note!

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp

9

The guideline says

Reason: One-declaration-per line increases readability and avoids mistakes related to the C/C++ grammar. It also leaves room for a more descriptive end-of-line comment.

Exception: a function declaration can contain several function argument declarations.

I'm not sure why what you copied is written in "Enforcement" section, but I do not think that is how it should be handled. I am concerned not only about that specific case and I see no reason to cut off cases already presented in this test.

[malcolm.parsons]

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp

9

"mistakes related to the C/C++ grammar" only occur when declarator operators are involved. e.g. in int* p, q a reader might incorrectly think that q was a pointer.

I see reasons not to warn about cases like
for (auto i = c.begin(), e = someExpensiveFn(); i != e; i++)
for (int i = 0, j = someExpensiveFn(); i < j; i++);
because the alternatives increase variable scope, or for
int x = 42, y = 43;
because it's not difficult to read.

As we disagree on this, can it be made configurable?

[aaron.ballman]

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp

9

We usually try to ensure that the check matches the behavior required by the normative wording of coding rules we follow. Based on that, the C++ core guideline rule only cares about declarator operators while the CERT recommendation kind of does not care about them. If you think the C++ core guideline enforcement is wrong, you can file a bug against that project to see if the editors agree, but I think the check should match the documented behavior from the guidelines. FWIW, I'm happy to work on the CERT semantics if you don't want to deal with those beyond what you've already done (or you can tackle the semantic differences if you want).

[omtcyfz]

Opened an issue in CppCoreGuidelines Github repo.

[malcolm.parsons]

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp

9

The CERT recommendation does care about declarator operators:

"Multiple, simple variable declarations can be declared on the same line given that there are no initializations. A simple variable declaration is one that is not a pointer or array."

Declaration	CERT	CppCoreGuidelines	Me
int i;	GOOD	GOOD	GOOD
int i = 1;	GOOD	GOOD	GOOD
int *p;	GOOD	GOOD	GOOD
int *p, q;	BAD	BAD	BAD
int i, j;	GOOD	GOOD	GOOD
int i, j = 1;	BAD	GOOD	BAD
int i = 1, j = 1;	BAD	GOOD	GOOD
int i = 1, j;	BAD	GOOD	BAD
int *i, *j;	BAD	BAD	BAD
for (int i = 0, j = size; i != j; i++)	GOOD	GOOD	GOOD
for (int *p = a, *q = a+size; p != q; p++)	GOOD	BAD	GOOD

I agree with CERT for most cases.

17

and global variables.

[alexfh]

https://reviews.llvm.org/D27621 seems to have a more up-to-date version of this patch.

[omtcyfz]

No longer relevant.

clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
+  OneNamePerDeclarationCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
   ProTypeCstyleCastCheck.cpp
   ProTypeMemberInitCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "InterfacesGlobalInitCheck.h"
+#include "OneNamePerDeclarationCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
 #include "SpecialMemberFunctionsCheck.h"
 #include "SlicingCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
+    CheckFactories.registerCheck<OneNamePerDeclarationCheck>(
+        "cppcoreguidelines-one-name-per-declaration");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsConstantArrayIndexCheck>(
         "cppcoreguidelines-pro-bounds-constant-array-index");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
         "cppcoreguidelines-pro-bounds-pointer-arithmetic");
     CheckFactories.registerCheck<ProTypeConstCastCheck>(
         "cppcoreguidelines-pro-type-const-cast");

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.h

+//===--- OneNamePerDeclarationCheck.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_CPPCOREGUIDELINES_ONE_NAME_PER_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_ONE_NAME_PER_DECLARATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// This check warns about multiple names in one declaration.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-one-name-per-declaration.html
+class OneNamePerDeclarationCheck : public ClangTidyCheck {
+public:
+  OneNamePerDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_ONE_NAME_PER_DECLARATION_H

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

+//===--- OneNamePerDeclarationCheck.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 "OneNamePerDeclarationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+namespace {
+
+AST_MATCHER_P(DeclStmt, declCountIsGreaterThan, unsigned, N) {
+  return std::distance(Node.decl_begin(), Node.decl_end()) > (ptrdiff_t)N;
+}
+
+} // end anonymous namespace
+
+void OneNamePerDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      declStmt(declCountIsGreaterThan(1)).bind("multipleNameDeclaration"),
+      this);
+}
+
+void OneNamePerDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
+  // FIXME: Also provide diagnostics splitting the declarations. E.g.
+  //
+  // Before:
+  //    int x = 42, y = 43, z = 44;
+  // After:
+  //    int x = 42;
+  //    int y = 43;
+  //    int z = 44;
+  const auto *MultipleNameDeclaration =
+      Result.Nodes.getNodeAs<DeclStmt>("multipleNameDeclaration");
+  diag(MultipleNameDeclaration->getStartLoc(),
+       "Do not declare multiple names per declaration");
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

docs/clang-tidy/checks/cppcoreguidelines-one-name-per-declaration.rst

+.. title:: clang-tidy - cppcoreguidelines-one-name-per-declaration
+
+cppcoreguidelines-one-name-per-declaration
+==========================================
+
+Checks for declarations with multiple names. C++ Core Guidelines suggests to
+split such declarations into multiple declarations each containing one name.
+
+This would improve readability and prevents potential bugs caused by inattention
+and C/C++ syntax specifics.
+
+Example, bad.
+
+.. code-block:: c++
+
+      std::vector<int> velocities(10, 0), numbers(other_numbers),
+                       inputs(input.begin(), input.end());
+
+Example, good.
+
+.. code-block:: c++
+
+      std::vector<int> velocities(10, 0);
+      std::vector<int> numbers(other_numbers);
+      std::vector<int> inputs(input.begin(), input.end());
+
+This rule is part of the "Expressions and statements" profile of the C++ Core
+Guidelines, see
+http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-nameres-name-oneaes10-declare-one-name-only-per-declaration

docs/clang-tidy/checks/list.rst

.. toctree::
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp
    cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-oop11-cpp (redirects to misc-move-constructor-init) <cert-oop11-cpp>
    cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-one-name-per-declaration
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-one-name-per-declaration %t
+
+int main() {
+  {
+    int x = 42;
+  }
+  {
+    int x = 42, y = 43;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Do not declare multiple names per declaration [cppcoreguidelines-one-name-per-declaration]
+  }
+  {
+    int x = 42, y = 43, z = 44;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Do not declare multiple names per declaration
+  }
+  return 0;
+}