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 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
