Skip to content

Commit a0f325b

Browse files
balazskebalazske
authored
[clang-tidy] Added check 'misc-override-with-different-visibility' (#140086)
1 parent 87e6fd1 commit a0f325b

File tree

11 files changed

+729
-0
lines changed

11 files changed

+729
-0
lines changed

‎clang-tools-extra/clang-tidy/misc/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ add_clang_library(clangTidyMiscModule STATIC
3232
NoRecursionCheck.cpp
3333
NonCopyableObjects.cpp
3434
NonPrivateMemberVariablesInClassesCheck.cpp
35+
OverrideWithDifferentVisibilityCheck.cpp
3536
RedundantExpressionCheck.cpp
3637
StaticAssertCheck.cpp
3738
ThrowByValueCatchByReferenceCheck.cpp

‎clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "NoRecursionCheck.h"
2323
#include "NonCopyableObjects.h"
2424
#include "NonPrivateMemberVariablesInClassesCheck.h"
25+
#include "OverrideWithDifferentVisibilityCheck.h"
2526
#include "RedundantExpressionCheck.h"
2627
#include "StaticAssertCheck.h"
2728
#include "ThrowByValueCatchByReferenceCheck.h"
@@ -81,6 +82,8 @@ class MiscModule : public ClangTidyModule {
8182
"misc-use-anonymous-namespace");
8283
CheckFactories.registerCheck<UseInternalLinkageCheck>(
8384
"misc-use-internal-linkage");
85+
CheckFactories.registerCheck<OverrideWithDifferentVisibilityCheck>(
86+
"misc-override-with-different-visibility");
8487
}
8588
};
8689

‎clang-tools-extra/clang-tidy/misc/OverrideWithDifferentVisibilityCheck.cpp

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
//===--- OverrideWithDifferentVisibilityCheck.cpp - clang-tidy ------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "OverrideWithDifferentVisibilityCheck.h"
10+
#include "../utils/Matchers.h"
11+
#include "../utils/OptionsUtils.h"
12+
#include "clang/ASTMatchers/ASTMatchFinder.h"
13+
14+
using namespace clang::ast_matchers;
15+
using namespace clang;
16+
17+
namespace {
18+
19+
AST_MATCHER(NamedDecl, isOperatorDecl) {
20+
DeclarationName::NameKind const NK = Node.getDeclName().getNameKind();
21+
return NK != DeclarationName::Identifier &&
22+
NK != DeclarationName::CXXConstructorName &&
23+
NK != DeclarationName::CXXDestructorName;
24+
}
25+
26+
} // namespace
27+
28+
namespace clang::tidy {
29+
30+
template <>
31+
struct OptionEnumMapping<
32+
misc::OverrideWithDifferentVisibilityCheck::ChangeKind> {
33+
static llvm::ArrayRef<std::pair<
34+
misc::OverrideWithDifferentVisibilityCheck::ChangeKind, StringRef>>
35+
getEnumMapping() {
36+
static constexpr std::pair<
37+
misc::OverrideWithDifferentVisibilityCheck::ChangeKind, StringRef>
38+
Mapping[] = {
39+
{misc::OverrideWithDifferentVisibilityCheck::ChangeKind::Any,
40+
"any"},
41+
{misc::OverrideWithDifferentVisibilityCheck::ChangeKind::Widening,
42+
"widening"},
43+
{misc::OverrideWithDifferentVisibilityCheck::ChangeKind::Narrowing,
44+
"narrowing"},
45+
};
46+
return {Mapping};
47+
}
48+
};
49+
50+
namespace misc {
51+
52+
OverrideWithDifferentVisibilityCheck::OverrideWithDifferentVisibilityCheck(
53+
StringRef Name, ClangTidyContext *Context)
54+
: ClangTidyCheck(Name, Context),
55+
DetectVisibilityChange(
56+
Options.get("DisallowedVisibilityChange", ChangeKind::Any)),
57+
CheckDestructors(Options.get("CheckDestructors", false)),
58+
CheckOperators(Options.get("CheckOperators", false)),
59+
IgnoredFunctions(utils::options::parseStringList(
60+
Options.get("IgnoredFunctions", ""))) {}
61+
62+
void OverrideWithDifferentVisibilityCheck::storeOptions(
63+
ClangTidyOptions::OptionMap &Opts) {
64+
Options.store(Opts, "DisallowedVisibilityChange", DetectVisibilityChange);
65+
Options.store(Opts, "CheckDestructors", CheckDestructors);
66+
Options.store(Opts, "CheckOperators", CheckOperators);
67+
Options.store(Opts, "IgnoredFunctions",
68+
utils::options::serializeStringList(IgnoredFunctions));
69+
}
70+
71+
void OverrideWithDifferentVisibilityCheck::registerMatchers(
72+
MatchFinder *Finder) {
73+
const auto IgnoredDecl =
74+
namedDecl(matchers::matchesAnyListedName(IgnoredFunctions));
75+
const auto FilterDestructors =
76+
CheckDestructors ? decl() : decl(unless(cxxDestructorDecl()));
77+
const auto FilterOperators =
78+
CheckOperators ? namedDecl() : namedDecl(unless(isOperatorDecl()));
79+
Finder->addMatcher(
80+
cxxMethodDecl(
81+
isVirtual(), FilterDestructors, FilterOperators,
82+
ofClass(
83+
cxxRecordDecl(unless(isExpansionInSystemHeader())).bind("class")),
84+
forEachOverridden(cxxMethodDecl(ofClass(cxxRecordDecl().bind("base")),
85+
unless(IgnoredDecl))
86+
.bind("base_func")))
87+
.bind("func"),
88+
this);
89+
}
90+
91+
void OverrideWithDifferentVisibilityCheck::check(
92+
const MatchFinder::MatchResult &Result) {
93+
const auto *const MatchedFunction =
94+
Result.Nodes.getNodeAs<FunctionDecl>("func");
95+
if (!MatchedFunction->isCanonicalDecl())
96+
return;
97+
98+
const auto *const ParentClass =
99+
Result.Nodes.getNodeAs<CXXRecordDecl>("class");
100+
const auto *const BaseClass = Result.Nodes.getNodeAs<CXXRecordDecl>("base");
101+
CXXBasePaths Paths;
102+
if (!ParentClass->isDerivedFrom(BaseClass, Paths))
103+
return;
104+
105+
const auto *const OverriddenFunction =
106+
Result.Nodes.getNodeAs<FunctionDecl>("base_func");
107+
AccessSpecifier const ActualAccess = MatchedFunction->getAccess();
108+
AccessSpecifier OverriddenAccess = OverriddenFunction->getAccess();
109+
110+
const CXXBaseSpecifier *InheritanceWithStrictVisibility = nullptr;
111+
for (const CXXBasePath &Path : Paths) {
112+
for (const CXXBasePathElement &Elem : Path) {
113+
if (Elem.Base->getAccessSpecifier() > OverriddenAccess) {
114+
OverriddenAccess = Elem.Base->getAccessSpecifier();
115+
InheritanceWithStrictVisibility = Elem.Base;
116+
}
117+
}
118+
}
119+
120+
if (ActualAccess != OverriddenAccess) {
121+
if (DetectVisibilityChange == ChangeKind::Widening &&
122+
ActualAccess > OverriddenAccess)
123+
return;
124+
if (DetectVisibilityChange == ChangeKind::Narrowing &&
125+
ActualAccess < OverriddenAccess)
126+
return;
127+
128+
if (InheritanceWithStrictVisibility) {
129+
diag(MatchedFunction->getLocation(),
130+
"visibility of function %0 is changed from %1 (through %1 "
131+
"inheritance of class %2) to %3")
132+
<< MatchedFunction << OverriddenAccess
133+
<< InheritanceWithStrictVisibility->getType() << ActualAccess;
134+
diag(InheritanceWithStrictVisibility->getBeginLoc(),
135+
"%0 is inherited as %1 here", DiagnosticIDs::Note)
136+
<< InheritanceWithStrictVisibility->getType() << OverriddenAccess;
137+
} else {
138+
diag(MatchedFunction->getLocation(),
139+
"visibility of function %0 is changed from %1 in class %2 to %3")
140+
<< MatchedFunction << OverriddenAccess << BaseClass << ActualAccess;
141+
}
142+
diag(OverriddenFunction->getLocation(), "function declared here as %0",
143+
DiagnosticIDs::Note)
144+
<< OverriddenFunction->getAccess();
145+
}
146+
}
147+
148+
} // namespace misc
149+
150+
} // namespace clang::tidy

‎clang-tools-extra/clang-tidy/misc/OverrideWithDifferentVisibilityCheck.h

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
//===--- OverrideWithDifferentVisibilityCheck.h - clang-tidy --*- C++ -*---===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OVERRIDEWITHDIFFERENTVISIBILITYCHECK_H
10+
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OVERRIDEWITHDIFFERENTVISIBILITYCHECK_H
11+
12+
#include "../ClangTidyCheck.h"
13+
14+
namespace clang::tidy::misc {
15+
16+
/// Finds virtual function overrides with different visibility than the function
17+
/// in the base class.
18+
///
19+
/// For the user-facing documentation see:
20+
/// http://clang.llvm.org/extra/clang-tidy/checks/misc/override-with-different-visibility.html
21+
class OverrideWithDifferentVisibilityCheck : public ClangTidyCheck {
22+
public:
23+
enum class ChangeKind { Any, Widening, Narrowing };
24+
25+
OverrideWithDifferentVisibilityCheck(StringRef Name,
26+
ClangTidyContext *Context);
27+
void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
28+
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
29+
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
30+
bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
31+
return LangOpts.CPlusPlus;
32+
}
33+
34+
private:
35+
ChangeKind DetectVisibilityChange;
36+
bool CheckDestructors;
37+
bool CheckOperators;
38+
std::vector<llvm::StringRef> IgnoredFunctions;
39+
};
40+
41+
} // namespace clang::tidy::misc
42+
43+
#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_OVERRIDEWITHDIFFERENTVISIBILITYCHECK_H

‎clang-tools-extra/docs/ReleaseNotes.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,12 @@ New checks
134134
Checks for uses of MLIR's old/to be deprecated ``OpBuilder::create<T>`` form
135135
and suggests using ``T::create`` instead.
136136

137+
- New :doc:`misc-override-with-different-visibility
138+
<clang-tidy/checks/misc/override-with-different-visibility>` check.
139+
140+
Finds virtual function overrides with different visibility than the function
141+
in the base class.
142+
137143
New check aliases
138144
^^^^^^^^^^^^^^^^^
139145

‎clang-tools-extra/docs/clang-tidy/checks/list.rst

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,7 @@ Clang-Tidy Checks
271271
:doc:`misc-no-recursion <misc/no-recursion>`,
272272
:doc:`misc-non-copyable-objects <misc/non-copyable-objects>`,
273273
:doc:`misc-non-private-member-variables-in-classes <misc/non-private-member-variables-in-classes>`,
274+
:doc:`misc-override-with-different-visibility <misc/override-with-different-visibility>`,
274275
:doc:`misc-redundant-expression <misc/redundant-expression>`, "Yes"
275276
:doc:`misc-static-assert <misc/static-assert>`, "Yes"
276277
:doc:`misc-throw-by-value-catch-by-reference <misc/throw-by-value-catch-by-reference>`,

‎clang-tools-extra/docs/clang-tidy/checks/misc/override-with-different-visibility.rst

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,87 @@
1+
.. title:: clang-tidy - misc-override-with-different-visibility
2+
3+
misc-override-with-different-visibility
4+
=======================================
5+
6+
Finds virtual function overrides with different visibility than the function
7+
in the base class. This includes for example if a virtual function declared as
8+
``private`` is overridden and declared as ``public`` in a subclass. The detected
9+
change is the modification of visibility resulting from keywords ``public``,
10+
``protected``, ``private`` at overridden virtual functions. The check applies to
11+
any normal virtual function and optionally to destructors or operators. Use of
12+
the ``using`` keyword is not considered as visibility change by this check.
13+
14+
15+
.. code-block:: c++
16+
17+
class A {
18+
public:
19+
virtual void f_pub();
20+
private:
21+
virtual void f_priv();
22+
};
23+
24+
class B: public A {
25+
public:
26+
void f_priv(); // warning: changed visibility from private to public
27+
private:
28+
void f_pub(); // warning: changed visibility from public to private
29+
};
30+
31+
class C: private A {
32+
// no warning: f_pub becomes private in this case but this is from the
33+
// private inheritance
34+
};
35+
36+
class D: private A {
37+
public:
38+
void f_pub(); // warning: changed visibility from private to public
39+
// 'f_pub' would have private access but is forced to be
40+
// public
41+
};
42+
43+
If the visibility is changed in this way, it can indicate bad design or
44+
programming error.
45+
46+
If a virtual function is private in a subclass but public in the base class, it
47+
can still be accessed from a pointer to the subclass if the pointer is converted
48+
to the base type. Probably private inheritance can be used instead.
49+
50+
A protected virtual function that is made public in a subclass may have valid
51+
use cases but similar (not exactly same) effect can be achieved with the
52+
``using`` keyword.
53+
54+
Options
55+
-------
56+
57+
.. option:: DisallowedVisibilityChange
58+
59+
Controls what kind of change to the visibility will be detected by the check.
60+
Possible values are `any`, `widening`, `narrowing`. For example the
61+
`widening` option will produce warning only if the visibility is changed
62+
from more restrictive (``private``) to less restrictive (``public``).
63+
Default value is `any`.
64+
65+
.. option:: CheckDestructors
66+
67+
If `true`, the check does apply to destructors too. Otherwise destructors
68+
are ignored by the check.
69+
Default value is `false`.
70+
71+
.. option:: CheckOperators
72+
73+
If `true`, the check does apply to overloaded C++ operators (as virtual
74+
member functions) too. This includes other special member functions (like
75+
conversions) too. This option is probably useful only in rare cases because
76+
operators and conversions are not often virtual functions.
77+
Default value is `false`.
78+
79+
.. option:: IgnoredFunctions
80+
81+
This option can be used to ignore the check at specific functions.
82+
To configure this option, a semicolon-separated list of function names
83+
should be provided. The list can contain regular expressions, in this way it
84+
is possible to select all functions of a specific class (like `MyClass::.*`)
85+
or a specific function of any class (like `my_function` or
86+
`::.*::my_function`). The function names are matched at the base class.
87+
Default value is empty string.

‎clang-tools-extra/test/clang-tidy/checkers/misc/Inputs/override-with-different-visibility/test-system-header.h

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
#pragma clang system_header
2+
3+
namespace sys {
4+
5+
struct Base {
6+
virtual void publicF();
7+
};
8+
9+
struct Derived: public Base {
10+
private:
11+
void publicF() override;
12+
};
13+
14+
}

0 commit comments

Comments
 (0)