[clang-tidy] Add duplicated access specifier readability check (PR25403)

[m4tx]

Summary

This finds redundant access specifier declarations inside classes, structs, and unions.

https://bugs.llvm.org/show_bug.cgi?id=25403

[Eugene.Zelenko]

Please close PR25403 after commit.

docs/ReleaseNotes.rst

70

Please use alphabetical order for new checks.

[m4tx]

Use alphabetical order in ReleaseNotes.rst

[lebedev.ri]

Please add tests with preprocessor (#if ...) that will show that it ignores disabled code. e.g.:

class ProbablyValid {
private:
  int a;
#if defined(ZZ)
public:
  int b;
#endif
private:
  int c;
protected:
  int d;
public:
  int e;
};

[m4tx]

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

[riccibruno]

In D55793#1333691, @m4tx wrote:

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

Thanks! CXXRecordDecl is already huge and so adding iterators for a single checker is in my opinion not a good idea (especially if the alternative is actually less code).
Would it make sense to also issue a diagnostic where the first access specifier is redundant (ie public in a struct, and private in a class) ?

[Eugene.Zelenko]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

37

Type is not obvious here, so please don't use auto.

[aaron.ballman]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

22

You should only register this matcher in C++ mode.

32

Please switch to const AccessSpecDecl *. Also, that should be LastAccessDecl per the naming conventions.

34

Why NS -- that seems like a strange naming choice.

37

Also, decl doesn't match our naming conventions -- change to ASDecl?

[m4tx]

Fix variable names, add macro expansion checking as well as macro and inner class tests.

[m4tx]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

34

Sorry, this small piece of code was copied from another place and I forgot to change the variable name. I switched to AS, hopefully that's more meaningful here.

[m4tx]

In D55793#1333723, @riccibruno wrote:

In D55793#1333691, @m4tx wrote:

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

Thanks! CXXRecordDecl is already huge and so adding iterators for a single checker is in my opinion not a good idea (especially if the alternative is actually less code).
Would it make sense to also issue a diagnostic where the first access specifier is redundant (ie public in a struct, and private in a class) ?

Yes, I was thinking about the same thing! However, I believe that some people may find this kind of "redundant" access specs actually more readable, so maybe it makes sense to add a check option for this to allow users to disable it?

[m4tx]

In D55793#1333661, @lebedev.ri wrote:

Please add tests with preprocessor (#if ...) that will show that it ignores disabled code. e.g.:

class ProbablyValid {
private:
  int a;
#if defined(ZZ)
public:
  int b;
#endif
private:
  int c;
protected:
  int d;
public:
  int e;
};

Is this actually possible? It seems that macros are ran through the preprocessor before one can fiddle with them in clang-tidy. In other words, int b is not at all present in the AST. However, I added a code to detect macro expansions, so duplicated access specifiers are ignored if at least one of them comes from a macro. If there is a way to cover your case as well, please let me know, because even after looking at the code of other checks I haven't found out a solution for this.

[lebedev.ri]

In D55793#1335249, @m4tx wrote:

In D55793#1333661, @lebedev.ri wrote:

Please add tests with preprocessor (#if ...) that will show that it ignores disabled code. e.g.:

class ProbablyValid {
private:
  int a;
#if defined(ZZ)
public:
  int b;
#endif
private:
  int c;
protected:
  int d;
public:
  int e;
};

Is this actually possible?
It seems that macros are ran through the preprocessor before one can fiddle with them in clang-tidy.
In other words, int b is not at all present in the AST.

.. and by "ignores" i meant that it will be diagnosing this code, since it did not know anything about the code within the preprocessor-disabled section.

However, I added a code to detect macro expansions, so duplicated access specifiers are ignored if at least one of them comes from a macro. If there is a way to cover your case as well, please let me know, because even after looking at the code of other checks I haven't found out a solution for this.

[riccibruno]

Yes, I was thinking about the same thing! However, I believe that some people may find this kind of "redundant" access specs actually more readable, so maybe it makes sense to add a check option for this > to allow users to disable it?

Perhaps ? I will leave someone with more experience here to comment.

[aaron.ballman]

In D55793#1335243, @m4tx wrote:

In D55793#1333723, @riccibruno wrote:

In D55793#1333691, @m4tx wrote:

Don't use CXXRecordDecl::accessSpecs(), use unique comments in tests.

Thanks! CXXRecordDecl is already huge and so adding iterators for a single checker is in my opinion not a good idea (especially if the alternative is actually less code).
Would it make sense to also issue a diagnostic where the first access specifier is redundant (ie public in a struct, and private in a class) ?

Yes, I was thinking about the same thing! However, I believe that some people may find this kind of "redundant" access specs actually more readable, so maybe it makes sense to add a check option for this to allow users to disable it?

I'm on the fence about whether this needs to be an option or not. Perhaps having some data on the rate of diagnosis would be helpful here -- can you try adding the feature and running it over some large code bases to see if the implicit access spec warning triggers a lot more than expected? Also, I kind of think this check should be named readability-redundant-access-specifiers instead of duplicated if it's going to consider implicit access specifiers (since the user doesn't write those, "duplicate" may be surprising).

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

32

Still need to switch where the const is.

44

Add a full stop at the end of the sentence.

52

There is no %0 in the diagnostic string, so I'm not certain what this argument is trying to print out. Did you mean ASDecl->getSourceRange() for the underlining?

[m4tx]

In D55793#1335274, @lebedev.ri wrote:

In D55793#1335249, @m4tx wrote:

In D55793#1333661, @lebedev.ri wrote:

Please add tests with preprocessor (#if ...) that will show that it ignores disabled code. e.g.:

class ProbablyValid {
private:
  int a;
#if defined(ZZ)
public:
  int b;
#endif
private:
  int c;
protected:
  int d;
public:
  int e;
};

Is this actually possible?
It seems that macros are ran through the preprocessor before one can fiddle with them in clang-tidy.
In other words, int b is not at all present in the AST.

.. and by "ignores" i meant that it will be diagnosing this code, since it did not know anything about the code within the preprocessor-disabled section.

However, I added a code to detect macro expansions, so duplicated access specifiers are ignored if at least one of them comes from a macro. If there is a way to cover your case as well, please let me know, because even after looking at the code of other checks I haven't found out a solution for this.

Ok, thanks for the clarification. I've added the test in the latest diff!

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

52

Sorry, this is another line I forgot to remove. Thanks for pointing this out!

By the way, does the underlining work in clang-tidy's diag() function? I see it is used outside the project, but here only FixItHints seem to generate underline in the generated messages.

[aaron.ballman]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

52

That's a good question that I don't actually know the answer to. :-D I believe it still works -- you can try it out by passing a SourceRange to the diag() call, like:

diag(LastASDecl->getLocation(), "duplicated access specifier") << SomeSourceRange;

You should see the range from SomeSourceRange underlined.

[JonasToth]

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

52

I believe, if you pass in a token or tokenrange or so, the underlying exists. I thought i got suprised by something like that once, not sure though.

[m4tx]

I've added the implicit access specifier check and run it on some bigger codebases. My findings are as below:

• Dolphin: 6 triggers across 856 record types
• OpenCV: 31 triggers across 3370 record types
• Inkscape: 39 triggers across 846 record types
• LLVM (Core + Clang + Clang Tools Extra): 128 triggers across 9214 record types
• Blender: 948 triggers across 6264 record types

Since the check was triggered in every big project I've tested, I've decided to add an option (disabled by default) to enable the implicit access specifier check. It is included in my latest Diff, along with the rename to readability-redundant-access-specifiers.

[m4tx]

Updated the diff for the latest (master) version, used GitHub monorepo for the changes

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst

34 ↗

(On Diff #195408)

Please use double back-ticks for language keywords, like private anc class.

47 ↗

(On Diff #195408)

Please use single back-ticks for option names.

[m4tx]

Updated the backticks in the documentation.

[aaron.ballman]

clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp

35–36 ↗

(On Diff #195409)

I have a slight preference for using assignment operators here rather than explicit constructor calls.

54 ↗

(On Diff #195409)

This is a bit terse, how about: redundant access specifier has the same accessibility as the implicit access specifier?

69 ↗

(On Diff #195409)

This is a bit terse, how about: redundant access specifier has the same accessibility as the previous access specifier?

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst

6 ↗

(On Diff #195409)

structs and unions -> structs, and unions

One thing the docs leave unclear is which access specifiers you're talking about. Currently, the check only cares about the access specifiers for fields, but it seems reasonable that the check could also be talking about access specifiers on base class specifiers. e.g.,

struct Base {
  int a, b;
};

class C : private Base { // The 'private' here is redundant.
};

You should probably clarify this in the docs. Implementing this functionality may or may not be useful, but if you want to implement it, you could do it in a follow-up patch.

33 ↗

(On Diff #195409)

give warning -> diagnose

34 ↗

(On Diff #195409)

May also want to put public inside struct or union as well.

48 ↗

(On Diff #195409)

since -> because

clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

71–72 ↗

(On Diff #195409)

I think that diagnosing here is unfortunate. If the user defines ZZ, then the access specifier is no longer redundant. However, it may not be easy for you to handle this case when ZZ is not defined because the access specifier will have been removed by the preprocessor.

[m4tx]

Updated per @aaron.ballman comments

[m4tx]

clang-tools-extra/clang-tidy/readability/RedundantAccessSpecifiersCheck.cpp

35–36 ↗

(On Diff #195409)

This is not possible here as specific_decl_iterator has the copy constructor marked as explicit.

54 ↗

(On Diff #195409)

Sounds good, changed this in the latest revision.

69 ↗

(On Diff #195409)

As above.

clang-tools-extra/docs/clang-tidy/checks/readability-redundant-access-specifiers.rst

6 ↗

(On Diff #195409)

This is actually included in the documentation (*member* access specifier), but I've added explicit "field and method" clarification.

clang-tools-extra/test/clang-tidy/readability-redundant-access-specifiers.cpp

71–72 ↗

(On Diff #195409)

I agree, but I think that we cannot do anything reasonable here. The code is removed by the preprocessor, and I believe the only behavior that would make sense would be to completely suppress the warnings if there is a preprocessor directive between the last access specifier and the current one. However, if I'm not mistaken, there is no way in clang-tidy to detect that.

[m4tx]

Remove the accidentally added patch file

[aaron.ballman]

LGTM!

[JonasToth]

Given this revision is accepted and seems ready to go: Do you need someone to commit on your behalf?

[m4tx]

@JonasToth thanks for asking! Yes, since I do not have commit access, I need someone to do the commit for me.

[aaron.ballman]

In D55793#1535895, @m4tx wrote:

@JonasToth thanks for asking! Yes, since I do not have commit access, I need someone to do the commit for me.

I'm sorry for the terribly long delay on getting this committed -- it fell off my radar for a bit. When I try to apply the branch to trunk, I get merge conflicts. Can you rebase it on ToT, please?

[twardakm]

Rebase with master

[twardakm]

In D55793#1571534, @aaron.ballman wrote:

In D55793#1535895, @m4tx wrote:

@JonasToth thanks for asking! Yes, since I do not have commit access, I need someone to do the commit for me.

I'm sorry for the terribly long delay on getting this committed -- it fell off my radar for a bit. When I try to apply the branch to trunk, I get merge conflicts. Can you rebase it on ToT, please?

I rebased the commit with current master, run the tests and everything looks fine. @JonasToth can you take a look and push the changes?

[aaron.ballman]

I've commit on your behalf in 29dc0b17de6b04afa6110a040053a19b02ca1a87, thank you for the patch!

[Eugene.Zelenko]

@aaron.ballman: Please move Release Notes entry to new checks section (in alphabetical order). Currently it's located in Improvements to include-fixer. Please also close PR.

[aaron.ballman]

In D55793#1727423, @Eugene.Zelenko wrote:

@aaron.ballman: Please move Release Notes entry to new checks section (in alphabetical order). Currently it's located in Improvements to include-fixer. Please also close PR.

Both should be done now, thanks!

[Eugene.Zelenko]

In D55793#1727437, @aaron.ballman wrote:

In D55793#1727423, @Eugene.Zelenko wrote:

@aaron.ballman: Please move Release Notes entry to new checks section (in alphabetical order). Currently it's located in Improvements to include-fixer. Please also close PR.

Both should be done now, thanks!

Thank you for quick fix!

clang-tidy/readability/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyReadabilityModule
   AvoidConstParamsInDecls.cpp
   BracesAroundStatementsCheck.cpp
   ConstReturnTypeCheck.cpp
   ContainerSizeEmptyCheck.cpp
   DeleteNullPointerCheck.cpp
   DeletedDefaultCheck.cpp
+  DuplicatedAccessSpecifiersCheck.cpp
   ElseAfterReturnCheck.cpp
   FunctionSizeCheck.cpp
   IdentifierNamingCheck.cpp
   ImplicitBoolConversionCheck.cpp
   InconsistentDeclarationParameterNameCheck.cpp
   IsolateDeclarationCheck.cpp
   MagicNumbersCheck.cpp
   MisleadingIndentationCheck.cpp

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.h

+//===--- DuplicatedAccessSpecifiersCheck.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_READABILITY_DUPLICATEDACCESSSPECIFIERSCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATEDACCESSSPECIFIERSCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Detects duplicated access specifiers inside classes, structs, and unions.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-duplicated-access-specifiers.html
+class DuplicatedAccessSpecifiersCheck : public ClangTidyCheck {
+public:
+  DuplicatedAccessSpecifiersCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_DUPLICATEDACCESSSPECIFIERSCHECK_H

clang-tidy/readability/DuplicatedAccessSpecifiersCheck.cpp

+//===--- DuplicatedAccessSpecifiersCheck.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 "DuplicatedAccessSpecifiersCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+void DuplicatedAccessSpecifiersCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxRecordDecl(has(accessSpecDecl()))
+          .bind("duplicated-access-specifiers"), this);
+}
+
+void DuplicatedAccessSpecifiersCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl =
+      Result.Nodes.getNodeAs<CXXRecordDecl>("duplicated-access-specifiers");
+
+  AccessSpecDecl const *lastAccessDecl = nullptr;
+  for (auto const *decl : MatchedDecl->accessSpecs()) {
+    if (lastAccessDecl != nullptr &&
+        lastAccessDecl->getAccess() == decl->getAccess()) {
+      diag(decl->getLocation(), "duplicated access specifier")
+          << MatchedDecl
+          << FixItHint::CreateRemoval(decl->getSourceRange());
+      diag(lastAccessDecl->getLocation(), "previously declared here",
+           DiagnosticIDs::Note);
+    } else {
+      lastAccessDecl = decl;
+    }
+  }
+}
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang

clang-tidy/readability/ReadabilityTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidConstParamsInDecls.h"
 #include "BracesAroundStatementsCheck.h"
 #include "ConstReturnTypeCheck.h"
 #include "ContainerSizeEmptyCheck.h"
 #include "DeleteNullPointerCheck.h"
 #include "DeletedDefaultCheck.h"
+#include "DuplicatedAccessSpecifiersCheck.h"
 #include "ElseAfterReturnCheck.h"
 #include "FunctionSizeCheck.h"
 #include "IdentifierNamingCheck.h"
 #include "ImplicitBoolConversionCheck.h"
 #include "InconsistentDeclarationParameterNameCheck.h"
 #include "IsolateDeclarationCheck.h"
 #include "MagicNumbersCheck.h"
 #include "MisleadingIndentationCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ConstReturnTypeCheck>(
         "readability-const-return-type");
     CheckFactories.registerCheck<ContainerSizeEmptyCheck>(
         "readability-container-size-empty");
     CheckFactories.registerCheck<DeleteNullPointerCheck>(
         "readability-delete-null-pointer");
     CheckFactories.registerCheck<DeletedDefaultCheck>(
         "readability-deleted-default");
+    CheckFactories.registerCheck<DuplicatedAccessSpecifiersCheck>(
+        "readability-duplicated-access-specifiers");
     CheckFactories.registerCheck<ElseAfterReturnCheck>(
         "readability-else-after-return");
     CheckFactories.registerCheck<FunctionSizeCheck>(
         "readability-function-size");
     CheckFactories.registerCheck<IdentifierNamingCheck>(
         "readability-identifier-naming");
     CheckFactories.registerCheck<ImplicitBoolConversionCheck>(
         "readability-implicit-bool-conversion");

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 The improvements are...
 
 Improvements to clang-tidy
 --------------------------
 
 - New :doc:`abseil-duration-comparison
   <clang-tidy/checks/abseil-duration-comparison>` check.
 
   Checks for comparisons which should be done in the ``absl::Duration`` domain
   instead of the float of integer domains.
 
 - New :doc:`abseil-duration-division
   <clang-tidy/checks/abseil-duration-division>` check.
 
- New :doc:`readability-isolate-decl
   tries to refactor the code to one statement per declaration.
 
 - New :doc:`readability-const-return-type
   <clang-tidy/checks/readability-const-return-type>` check.
 
   Checks for functions with a ``const``-qualified return type and recommends
   removal of the ``const`` keyword.
 
+- New :doc:`readability-duplicated-access-specifiers
+  <clang-tidy/checks/readability-duplicated-access-specifiers>` check.
+
+  Finds classes, structs, and unions that contain duplicated member
+  access specifiers.
+
 - New :doc:`readability-magic-numbers
   <clang-tidy/checks/readability-magic-numbers>` check.
 
   Detects usage of magic numbers, numbers that are used as literals instead of
   introduced via constants or symbols.
 
 - New :doc:`readability-uppercase-literal-suffix
   <clang-tidy/checks/readability-uppercase-literal-suffix>` check.

docs/clang-tidy/checks/list.rst

.. toctree::
    performance-unnecessary-value-param
    portability-simd-intrinsics
    readability-avoid-const-params-in-decls
    readability-braces-around-statements
    readability-const-return-type
    readability-container-size-empty
    readability-delete-null-pointer
    readability-deleted-default
+   readability-duplicated-access-specifiers
    readability-else-after-return
    readability-function-size
    readability-identifier-naming
    readability-implicit-bool-conversion
    readability-inconsistent-declaration-parameter-name
    readability-isolate-declaration
    readability-magic-numbers
    readability-misleading-indentation

docs/clang-tidy/checks/readability-duplicated-access-specifiers.rst

+.. title:: clang-tidy - readability-duplicated-access-specifiers
+
+readability-duplicated-access-specifiers
+========================================
+
+Finds classes, structs and unions containing duplicated member access specifiers
+that can be removed.
+
+Examples:
+
+.. code-block:: c++
+
+  class Foo {
+  public:
+    int x;
+    int y;
+  public:
+    int z;
+  protected:
+    int a;
+  public:
+    int c;
+  }
+
+In the example above, the second ``public`` declaration can be removed without
+any changes of behavior.

test/clang-tidy/readability-duplicated-access-specifiers.cpp

+// RUN: %check_clang_tidy %s readability-duplicated-access-specifiers %t
+
+class FooPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+struct StructPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+union UnionPublic {
+public:
+  int a;
+public: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooProtected {
+protected:
+  int a;
+protected: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+private:
+  int c;
+};
+
+class FooPrivate {
+private:
+  int a;
+private: // comment-0
+  // CHECK-MESSAGES: :[[@LINE-1]]:1: warning: duplicated access specifier [readability-duplicated-access-specifiers]
+  // CHECK-MESSAGES: :[[@LINE-4]]:1: note: previously declared here
+  // CHECK-FIXES: {{^}}// comment-0{{$}}
+  int b;
+public:
+  int c;
+};
+
+class Valid {
+private:
+  int a;
+public:
+  int b;
+private:
+  int c;
+protected:
+  int d;
+public:
+  int e;
+};