[clang-tools-extra] 9425cf9 - [clang-tidy] Fix crash and invalid fix-its in bugprone-crtp-constructor-accessibility (#232163)
via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 25 07:54:19 PDT 2026
Author: Moonn
Date: 2026-10-25T10:54:14-04:00
New Revision: 9425cf999dc5e0c2319c6b4b4132ab6532890c74
URL: https://github.com/llvm/llvm-project/commit/9425cf999dc5e0c2319c6b4b4132ab6532890c74
DIFF: https://github.com/llvm/llvm-project/commit/9425cf999dc5e0c2319c6b4b4132ab6532890c74.diff
LOG: [clang-tidy] Fix crash and invalid fix-its in bugprone-crtp-constructor-accessibility (#232163)
The fix-it puts `private:` at `getBeginLoc().getLocWithOffset(-1)` and
the old access specifier at `getEndLoc().getLocWithOffset(1)`. Stepping
back one character is what breaks:
- When the constructor starts with a macro, as in the issue, -1 leaves
the macro's SLocEntry. If the entry before it is a file, assertion
builds crash in `isMacroArgExpansion`. If it's a macro argument, nothing
asserts and `--fix` writes `private:` into that argument, so `ID(1)`
becomes `ID(1private:`.
- Without macros, `private:` gets glued onto whatever comes before the
constructor: `public:CRTP() {}` becomes `publicprivate:`, an empty
`CONSTEXPR` macro becomes `CONSTEXPRprivate:` (7 of the 12 `private:`
insertions in Boost's `date_time` at `-std=c++11`), and an unindented
constructor after a `// comment` line gets it inside the comment.
Crash repro:
```cpp
#define CONSTEXPR constexpr
template <typename T>
class CRTP {
public:
CONSTEXPR CRTP() {}
};
class A : CRTP<A> {};
```
Now `private:` goes right before the first token and the old specifier
right after the last one, mapped out of macros with
`Lexer::isAtStartOfMacroExpansion` and `Lexer::getLocForEndOfToken`. If
either end is in the middle of a macro expansion there's no fix-it, and
no `friend` fix-it either, so `--fix` doesn't add `friend T;` to a
constructor it left public. Mapping only the start isn't enough:
`edit::Commit` would then put the end insertion inside a macro argument.
In the common case the only visible change is that `private:` now goes
right before the constructor instead of one character earlier.
The macro tests have their own file because the crash needs a file entry
right before the first expansion. In the existing test file, the `>>`
split in `CRTP<A<T>>` creates an expansion entry and hides it.
Fixes #145093
I used an AI assistant while working on this, and reviewed and tested
the change myself.
Added:
clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp
Modified:
clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
clang-tools-extra/docs/ReleaseNotes.md
clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
index 13d9750234b0d4..87a3e830701bec 100644
--- a/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/CrtpConstructorAccessibilityCheck.cpp
@@ -67,22 +67,29 @@ getDerivedParameter(const ClassTemplateSpecializationDecl *CRTP,
static std::vector<FixItHint>
hintMakeCtorPrivate(const CXXConstructorDecl *Ctor,
const std::string &OriginalAccess) {
- std::vector<FixItHint> Hints;
-
- Hints.emplace_back(FixItHint::CreateInsertion(
- Ctor->getBeginLoc().getLocWithOffset(-1), "private:\n"));
-
const ASTContext &ASTCtx = Ctor->getASTContext();
- const SourceLocation CtorEndLoc =
+ const SourceManager &SM = ASTCtx.getSourceManager();
+ const LangOptions &LangOpts = ASTCtx.getLangOpts();
+
+ // Insert right before the first token and after the last one, mapped out of
+ // macros. Give up if either end is in the middle of a macro expansion.
+ SourceLocation CtorBeginLoc = Ctor->getBeginLoc();
+ if (CtorBeginLoc.isMacroID() &&
+ !Lexer::isAtStartOfMacroExpansion(CtorBeginLoc, SM, LangOpts,
+ &CtorBeginLoc))
+ return {};
+
+ const SourceLocation CtorEndLoc = Lexer::getLocForEndOfToken(
Ctor->isExplicitlyDefaulted()
- ? utils::lexer::findNextTerminator(Ctor->getEndLoc(),
- ASTCtx.getSourceManager(),
- ASTCtx.getLangOpts())
- : Ctor->getEndLoc();
- Hints.emplace_back(FixItHint::CreateInsertion(
- CtorEndLoc.getLocWithOffset(1), '\n' + OriginalAccess + ':' + '\n'));
-
- return Hints;
+ ? utils::lexer::findNextTerminator(Ctor->getEndLoc(), SM, LangOpts)
+ : Ctor->getEndLoc(),
+ 0, SM, LangOpts);
+ if (CtorEndLoc.isInvalid())
+ return {};
+
+ return {FixItHint::CreateInsertion(CtorBeginLoc, "private:\n"),
+ FixItHint::CreateInsertion(CtorEndLoc,
+ '\n' + OriginalAccess + ':' + '\n')};
}
void CrtpConstructorAccessibilityCheck::registerMatchers(MatchFinder *Finder) {
@@ -159,15 +166,15 @@ void CrtpConstructorAccessibilityCheck::check(
const bool IsPublic = Ctor->getAccess() == AS_public;
const std::string Access = IsPublic ? "public" : "protected";
+ const std::vector<FixItHint> CtorHints = hintMakeCtorPrivate(Ctor, Access);
WithFriendHintIfNeeded(
diag(Ctor->getLocation(),
"%0 constructor allows the CRTP to be %select{inherited "
"from|constructed}1 as a regular template class; consider making "
"it private%select{| and declaring the derived class as friend}2")
- << Access << IsPublic << NeedsFriend
- << hintMakeCtorPrivate(Ctor, Access),
- NeedsFriend);
+ << Access << IsPublic << NeedsFriend << CtorHints,
+ NeedsFriend && !CtorHints.empty());
}
}
diff --git a/clang-tools-extra/docs/ReleaseNotes.md b/clang-tools-extra/docs/ReleaseNotes.md
index 037df4cd0937c4..310b084c04eb04 100644
--- a/clang-tools-extra/docs/ReleaseNotes.md
+++ b/clang-tools-extra/docs/ReleaseNotes.md
@@ -189,6 +189,11 @@ infrastructure are described first, followed by tool-specific sections.
assignments inside GNU statement expressions unless they form the value of
the statement expression.
+- Fixed a crash in {doc}`bugprone-crtp-constructor-accessibility
+ <clang-tidy/checks/bugprone/crtp-constructor-accessibility>` when a
+ constructor begins with a macro, and fix-its that inserted `private:` into
+ the preceding token, comment or macro argument.
+
- Improved {doc}`bugprone-easily-swappable-parameters
<clang-tidy/checks/bugprone/easily-swappable-parameters>` check by no longer
emitting empty notes for type aliases when {option}`ModelImplicitConversions`
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp
new file mode 100644
index 00000000000000..94539bc9d06cdc
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility-macro.cpp
@@ -0,0 +1,40 @@
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-crtp-constructor-accessibility %t -- -- -fno-delayed-template-parsing
+
+// CONSTEXPR must stay the first expansion in this file: the crash needs a file
+// entry right before it, and a macro or a split '>>' above would hide it.
+#define CONSTEXPR constexpr
+#define EMPTY_BODY {}
+
+namespace ctor_begins_and_ends_with_macro {
+template <typename T>
+class CRTP {
+public:
+ CONSTEXPR CRTP() EMPTY_BODY
+ // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]
+ // CHECK-FIXES: private:
+ // CHECK-FIXES-NEXT: CONSTEXPR CRTP() EMPTY_BODY
+ // CHECK-FIXES-NEXT: public:
+ // CHECK-FIXES: friend T;
+};
+
+class A : CRTP<A> {};
+} // namespace ctor_begins_and_ends_with_macro
+
+#define MEMBER_THEN(X) int I; X
+#define THEN_MEMBER(X) X int J;
+
+namespace ctor_in_middle_of_macro {
+template <typename T>
+class CRTP {
+public:
+ MEMBER_THEN(CRTP() {})
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]
+ // CHECK-FIXES: MEMBER_THEN(CRTP() {})
+ THEN_MEMBER(CRTP(int) {})
+ // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]
+ // CHECK-FIXES: THEN_MEMBER(CRTP(int) {})
+ // CHECK-FIXES-NOT: friend T;
+};
+
+class A : CRTP<A> {};
+} // namespace ctor_in_middle_of_macro
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
index 44cfcd136f2387..a9a677b443afae 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/crtp-constructor-accessibility.cpp
@@ -284,3 +284,17 @@ class CRTP
class A : CRTP<A> {};
} // namespace public_copy_protected_move_constructor_deleted
+
+namespace ctor_right_after_access_specifier {
+template <typename T>
+class CRTP {
+public:CRTP() {}
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: public constructor allows the CRTP to be constructed as a regular template class; consider making it private and declaring the derived class as friend [bugprone-crtp-constructor-accessibility]
+ // CHECK-FIXES: public:private:
+ // CHECK-FIXES-NEXT: CRTP() {}
+ // CHECK-FIXES-NEXT: public:
+ // CHECK-FIXES: friend T;
+};
+
+class A : CRTP<A> {};
+} // namespace ctor_right_after_access_specifier
More information about the cfe-commits
mailing list