https://github.com/unterumarmung created https://github.com/llvm/llvm-project/pull/191607
Suppress `modernize-use-default-member-init` diagnostics when moving a constructor initializer into a default member initializer would reference a declaration not visible from the field declaration. Add `IgnoreNonVisibleReferences` to allow preserving the warning without emitting unsafe fix-its, and document the new behavior. Fixes #156412 Assisted by Codex >From e457b89bf9885cdfc8f21c96e9e6a074e1fbd4e4 Mon Sep 17 00:00:00 2001 From: Daniil Dudkin <[email protected]> Date: Sat, 11 Apr 2026 10:01:34 +0300 Subject: [PATCH] [clang-tidy] Avoid unsafe use-default-member-init fixes Suppress modernize-use-default-member-init diagnostics when moving a constructor initializer into a default member initializer would reference a declaration not visible from the field declaration. Add IgnoreNonVisibleReferences to allow preserving the warning without emitting unsafe fix-its, and document the new behavior. --- .../modernize/UseDefaultMemberInitCheck.cpp | 107 ++++++++++-- .../modernize/UseDefaultMemberInitCheck.h | 3 +- clang-tools-extra/docs/ReleaseNotes.rst | 7 + .../modernize/use-default-member-init.rst | 7 + .../non-visible-references.h | 156 ++++++++++++++++++ ...mber-init-non-visible-references-notes.cpp | 15 ++ ...ult-member-init-non-visible-references.cpp | 55 ++++++ 7 files changed, 334 insertions(+), 16 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-default-member-init/non-visible-references.h create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init-non-visible-references-notes.cpp create mode 100644 clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init-non-visible-references.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp index cc6b7bfd4fd5b..6bb90c3646e03 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp @@ -11,7 +11,9 @@ #include "clang/AST/Expr.h" #include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/ASTMatchers/ASTMatchers.h" +#include "clang/Basic/DiagnosticIDs.h" #include "clang/Lex/Lexer.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/TypeSwitch.h" using namespace clang::ast_matchers; @@ -51,6 +53,44 @@ static bool isExprAllowedInMemberInit(const Expr *E) { .Default(false); } +static bool isVisibleFromDefaultMemberInitializer(const NamedDecl *ND, + const FieldDecl *Field, + const SourceManager &SM) { + if (!ND || !Field) + return false; + + const auto *FieldClass = cast<CXXRecordDecl>(Field->getParent()); + for (const DeclContext *DC = ND->getDeclContext(); DC; DC = DC->getParent()) + if (DC->Equals(FieldClass)) + return true; + + const SourceLocation DeclLoc = SM.getExpansionLoc(ND->getLocation()); + const SourceLocation FieldLoc = SM.getExpansionLoc(Field->getLocation()); + if (DeclLoc.isInvalid() || FieldLoc.isInvalid()) + return false; + + return DeclLoc == FieldLoc || SM.isBeforeInTranslationUnit(DeclLoc, FieldLoc); +} + +static const DeclRefExpr *findFirstNonVisibleDeclRef(const Stmt *S, + const FieldDecl *Field, + const SourceManager &SM) { + if (!S) + return nullptr; + + if (const auto *DRE = dyn_cast<DeclRefExpr>(S)) { + if (!isVisibleFromDefaultMemberInitializer(DRE->getDecl(), Field, SM) || + !isVisibleFromDefaultMemberInitializer(DRE->getFoundDecl(), Field, SM)) + return DRE; + } + + for (const Stmt *Child : S->children()) + if (const auto *DRE = findFirstNonVisibleDeclRef(Child, Field, SM)) + return DRE; + + return nullptr; +} + namespace { AST_MATCHER_P(InitListExpr, initCountIs, unsigned, N) { @@ -59,6 +99,15 @@ AST_MATCHER_P(InitListExpr, initCountIs, unsigned, N) { AST_MATCHER(Expr, allowedInitExpr) { return isExprAllowedInMemberInit(&Node); } +AST_MATCHER(CXXCtorInitializer, hasOnlyVisibleReferencedDecls) { + const FieldDecl *Field = Node.getAnyMember(); + if (!Field) + return true; + + const SourceManager &SM = Finder->getASTContext().getSourceManager(); + return findFirstNonVisibleDeclRef(Node.getInit(), Field, SM) == nullptr; +} + } // namespace static StringRef getValueOfValueInit(const QualType InitType) { @@ -237,12 +286,15 @@ UseDefaultMemberInitCheck::UseDefaultMemberInitCheck(StringRef Name, ClangTidyContext *Context) : ClangTidyCheck(Name, Context), UseAssignment(Options.get("UseAssignment", false)), - IgnoreMacros(Options.get("IgnoreMacros", true)) {} + IgnoreMacros(Options.get("IgnoreMacros", true)), + IgnoreNonVisibleReferences( + Options.get("IgnoreNonVisibleReferences", true)) {} void UseDefaultMemberInitCheck::storeOptions( ClangTidyOptions::OptionMap &Opts) { Options.store(Opts, "UseAssignment", UseAssignment); Options.store(Opts, "IgnoreMacros", IgnoreMacros); + Options.store(Opts, "IgnoreNonVisibleReferences", IgnoreNonVisibleReferences); } void UseDefaultMemberInitCheck::registerMatchers(MatchFinder *Finder) { @@ -251,17 +303,27 @@ void UseDefaultMemberInitCheck::registerMatchers(MatchFinder *Finder) { initCountIs(0), hasType(arrayType()))), allowedInitExpr()); + auto CandidateField = forField(unless(anyOf( + getLangOpts().CPlusPlus20 ? unless(anything()) : isBitField(), + hasInClassInitializer(anything()), hasParent(recordDecl(isUnion()))))); + auto VisibleDefaultInit = + cxxCtorInitializer(CandidateField, withInitializer(Init), + hasOnlyVisibleReferencedDecls()) + .bind("default"); + Finder->addMatcher( - cxxConstructorDecl(forEachConstructorInitializer( - cxxCtorInitializer( - forField(unless(anyOf( - getLangOpts().CPlusPlus20 ? unless(anything()) : isBitField(), - hasInClassInitializer(anything()), - hasParent(recordDecl(isUnion()))))), - withInitializer(Init)) - .bind("default"))), + cxxConstructorDecl(forEachConstructorInitializer(VisibleDefaultInit)), this); + if (!IgnoreNonVisibleReferences) { + Finder->addMatcher( + cxxConstructorDecl(forEachConstructorInitializer( + cxxCtorInitializer(CandidateField, withInitializer(Init), + unless(hasOnlyVisibleReferencedDecls())) + .bind("default-without-fix"))), + this); + } + Finder->addMatcher( cxxConstructorDecl(forEachConstructorInitializer( cxxCtorInitializer(forField(hasInClassInitializer(anything())), @@ -273,7 +335,11 @@ void UseDefaultMemberInitCheck::registerMatchers(MatchFinder *Finder) { void UseDefaultMemberInitCheck::check(const MatchFinder::MatchResult &Result) { if (const auto *Default = Result.Nodes.getNodeAs<CXXCtorInitializer>("default")) - checkDefaultInit(Result, Default); + checkDefaultInit(Result, Default, /*EmitFix=*/true); + else if (const auto *DefaultWithoutFix = + Result.Nodes.getNodeAs<CXXCtorInitializer>( + "default-without-fix")) + checkDefaultInit(Result, DefaultWithoutFix, /*EmitFix=*/false); else if (const auto *Existing = Result.Nodes.getNodeAs<CXXCtorInitializer>("existing")) checkExistingInit(Result, Existing); @@ -282,7 +348,8 @@ void UseDefaultMemberInitCheck::check(const MatchFinder::MatchResult &Result) { } void UseDefaultMemberInitCheck::checkDefaultInit( - const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init) { + const MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init, + bool EmitFix) { const FieldDecl *Field = Init->getAnyMember(); // Check whether we have multiple hand-written constructors and bomb out, as @@ -301,6 +368,20 @@ void UseDefaultMemberInitCheck::checkDefaultInit( if (StartLoc.isMacroID() && IgnoreMacros) return; + if (!EmitFix) { + diag(Field->getLocation(), "use default member initializer for %0") + << Field; + diag(Init->getLParenLoc(), + "move the referenced declaration or definition before the field " + "declaration to use a default member initializer", + DiagnosticIDs::Note); + return; + } + + auto Diag = + diag(Field->getLocation(), "use default member initializer for %0") + << Field; + const SourceLocation FieldEnd = Lexer::getLocForEndOfToken(Field->getSourceRange().getEnd(), 0, *Result.SourceManager, getLangOpts()); @@ -318,10 +399,6 @@ void UseDefaultMemberInitCheck::checkDefaultInit( UseAssignment && (!ValueInit || !InitType->isEnumeralType()); const bool NeedsBraces = !CanAssign || isa<ArrayType>(InitType); - auto Diag = - diag(Field->getLocation(), "use default member initializer for %0") - << Field; - if (CanAssign) Diag << FixItHint::CreateInsertion(FieldEnd, " = "); if (NeedsBraces) diff --git a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.h b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.h index f379214308715..870ca1af30cb2 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.h @@ -34,12 +34,13 @@ class UseDefaultMemberInitCheck : public ClangTidyCheck { private: void checkDefaultInit(const ast_matchers::MatchFinder::MatchResult &Result, - const CXXCtorInitializer *Init); + const CXXCtorInitializer *Init, bool EmitFix); void checkExistingInit(const ast_matchers::MatchFinder::MatchResult &Result, const CXXCtorInitializer *Init); const bool UseAssignment; const bool IgnoreMacros; + const bool IgnoreNonVisibleReferences; }; } // namespace clang::tidy::modernize diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 17a5b006d4b8f..6158e4a1109e8 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -380,6 +380,13 @@ Changes in existing checks - Improved :doc:`modernize-redundant-void-arg <clang-tidy/checks/modernize/redundant-void-arg>` check to work in C23. +- Improved :doc:`modernize-use-default-member-init + <clang-tidy/checks/modernize/use-default-member-init>` check by fixing a + false positive when a constructor initializer refers to a declaration that + would not be visible from the inserted default member initializer. The + `IgnoreNonVisibleReferences` option can be set to `false` to warn without + emitting fix-its for these cases. + - Improved :doc:`modernize-use-equals-delete <clang-tidy/checks/modernize/use-equals-delete>` check by only warning on private deleted functions, if they do not have a public overload or are a diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-default-member-init.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-default-member-init.rst index 2d3ed38014937..08721cac7ab5c 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/use-default-member-init.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/use-default-member-init.rst @@ -52,3 +52,10 @@ Options If this option is set to `true` (default is `true`), the check will not warn about members declared inside macros. + +.. option:: IgnoreNonVisibleReferences + + If this option is set to `true` (default is `true`), the check will not warn + about member initializers that refer to declarations that would not be + visible from the default member initializer created by the fix-it. If set to + `false`, the check will warn about these cases without emitting fix-its. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-default-member-init/non-visible-references.h b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-default-member-init/non-visible-references.h new file mode 100644 index 0000000000000..bf06316737787 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/Inputs/use-default-member-init/non-visible-references.h @@ -0,0 +1,156 @@ +#ifndef NON_VISIBLE_REFERENCES_H +#define NON_VISIBLE_REFERENCES_H + +using int32_t = int; + +namespace HeaderValues { +constexpr int Constant = 5; +static int Static = 6; +enum Enum { EnumValue = 7 }; +} // namespace HeaderValues + +#define NON_VISIBLE_INIT_VALUE CPP_MACRO_CONSTANT + +class NonVisibleConstexpr { +public: + NonVisibleConstexpr(); + +private: + int32_t member; + // CHECK-MESSAGES-ALLOW: :[[@LINE-1]]:11: warning: use default member initializer for 'member' +}; + +class NonVisibleStatic { +public: + NonVisibleStatic(); + +private: + int32_t member; + // CHECK-MESSAGES-ALLOW: :[[@LINE-1]]:11: warning: use default member initializer for 'member' +}; + +class NonVisibleEnum { +public: + NonVisibleEnum(); + +private: + int32_t member; + // CHECK-MESSAGES-ALLOW: :[[@LINE-1]]:11: warning: use default member initializer for 'member' +}; + +class NonVisibleNestedCast { +public: + NonVisibleNestedCast(); + +private: + int32_t member; + // CHECK-MESSAGES-ALLOW: :[[@LINE-1]]:11: warning: use default member initializer for 'member' +}; + +class NonVisibleMacroReference { +public: + NonVisibleMacroReference(); + +private: + int32_t member; + // CHECK-MESSAGES-ALLOW: :[[@LINE-1]]:11: warning: use default member initializer for 'member' +}; + +class NonVisibleUsingDeclaration { +public: + NonVisibleUsingDeclaration(); + +private: + int32_t member; + // CHECK-MESSAGES-ALLOW: :[[@LINE-1]]:11: warning: use default member initializer for 'member' +}; + +template <typename T> +class NonVisibleTemplate { +public: + NonVisibleTemplate(); + +private: + int32_t member; + // CHECK-MESSAGES-ALLOW: :[[@LINE-1]]:11: warning: use default member initializer for 'member' +}; + +class HeaderVisibleConstexpr { +public: + HeaderVisibleConstexpr(); + +private: + int member; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'member' + // CHECK-MESSAGES-ALLOW: :[[@LINE-2]]:7: warning: use default member initializer for 'member' + // CHECK-FIXES: int member{HeaderValues::Constant}; + // CHECK-FIXES-ALLOW: int member{HeaderValues::Constant}; +}; + +class HeaderVisibleStatic { +public: + HeaderVisibleStatic(); + +private: + int member; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'member' + // CHECK-MESSAGES-ALLOW: :[[@LINE-2]]:7: warning: use default member initializer for 'member' + // CHECK-FIXES: int member{HeaderValues::Static}; + // CHECK-FIXES-ALLOW: int member{HeaderValues::Static}; +}; + +class HeaderVisibleEnum { +public: + HeaderVisibleEnum(); + +private: + int member; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'member' + // CHECK-MESSAGES-ALLOW: :[[@LINE-2]]:7: warning: use default member initializer for 'member' + // CHECK-FIXES: int member{HeaderValues::EnumValue}; + // CHECK-FIXES-ALLOW: int member{HeaderValues::EnumValue}; +}; + +class SameClassLaterStatic { +public: + SameClassLaterStatic(); + +private: + int member; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'member' + // CHECK-MESSAGES-ALLOW: :[[@LINE-2]]:7: warning: use default member initializer for 'member' + // CHECK-FIXES: int member{ClassConstant}; + // CHECK-FIXES-ALLOW: int member{ClassConstant}; + static constexpr int ClassConstant = 8; +}; + +class SameClassLaterEnum { +public: + SameClassLaterEnum(); + +private: + int member; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'member' + // CHECK-MESSAGES-ALLOW: :[[@LINE-2]]:7: warning: use default member initializer for 'member' + // CHECK-FIXES: int member{ClassEnumValue}; + // CHECK-FIXES-ALLOW: int member{ClassEnumValue}; + enum { ClassEnumValue = 9 }; +}; + +struct BaseWithStatic { + static constexpr int BaseConstant = 10; +}; + +class InheritedStatic : public BaseWithStatic { +public: + InheritedStatic(); + +private: + int member; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'member' + // CHECK-MESSAGES-ALLOW: :[[@LINE-2]]:7: warning: use default member initializer for 'member' + // CHECK-FIXES: int member{BaseConstant}; + // CHECK-FIXES-ALLOW: int member{BaseConstant}; +}; + +#endif // NON_VISIBLE_REFERENCES_H diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init-non-visible-references-notes.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init-non-visible-references-notes.cpp new file mode 100644 index 0000000000000..b902950adf2db --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init-non-visible-references-notes.cpp @@ -0,0 +1,15 @@ +// RUN: %check_clang_tidy -std=c++11-or-later -check-suffix=ALLOW \ +// RUN: %s modernize-use-default-member-init %t -- \ +// RUN: -config="{CheckOptions: {modernize-use-default-member-init.IgnoreNonVisibleReferences: false}}" + +struct NonVisibleNote { + NonVisibleNote(); + int member; + // CHECK-NOTES-ALLOW: :[[@LINE-1]]:7: warning: use default member initializer for 'member' [modernize-use-default-member-init] +}; + +constexpr int LocalConstant = 1; + +NonVisibleNote::NonVisibleNote() : member(LocalConstant) {} +// CHECK-NOTES-ALLOW: :[[@LINE-1]]:42: note: move the referenced declaration or definition before the field declaration to use a default member initializer +// CHECK-FIXES-ALLOW: NonVisibleNote::NonVisibleNote() : member(LocalConstant) {} diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init-non-visible-references.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init-non-visible-references.cpp new file mode 100644 index 0000000000000..eff845fa2b42a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-default-member-init-non-visible-references.cpp @@ -0,0 +1,55 @@ +// RUN: %check_clang_tidy -std=c++11-or-later \ +// RUN: -check-header %S/Inputs/use-default-member-init/non-visible-references.h \ +// RUN: %s modernize-use-default-member-init %t -- -- -I%S/Inputs/use-default-member-init +// RUN: %check_clang_tidy -std=c++11-or-later -check-suffix=ALLOW \ +// RUN: -check-header %S/Inputs/use-default-member-init/non-visible-references.h \ +// RUN: %s modernize-use-default-member-init %t.allow -- \ +// RUN: -config="{CheckOptions: {modernize-use-default-member-init.IgnoreNonVisibleReferences: false}}" -- \ +// RUN: -I%S/Inputs/use-default-member-init + +#include "non-visible-references.h" + +struct MainPositive { + MainPositive() : member(42) {} + int member; + // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: use default member initializer for 'member' [modernize-use-default-member-init] + // CHECK-MESSAGES-ALLOW: :[[@LINE-2]]:7: warning: use default member initializer for 'member' + // CHECK-FIXES: int member{42}; + // CHECK-FIXES-ALLOW: int member{42}; +}; + +namespace { +constexpr double CppConstant = 2.0; +static int CppStatic = 3; +enum { CppEnum = 4 }; +} // namespace + +#define CPP_MACRO_CONSTANT CppConstant + +NonVisibleConstexpr::NonVisibleConstexpr() : member(CppConstant) {} +// CHECK-FIXES-ALLOW: NonVisibleConstexpr::NonVisibleConstexpr() : member(CppConstant) {} +NonVisibleStatic::NonVisibleStatic() : member(CppStatic) {} +// CHECK-FIXES-ALLOW: NonVisibleStatic::NonVisibleStatic() : member(CppStatic) {} +NonVisibleEnum::NonVisibleEnum() : member(CppEnum) {} +// CHECK-FIXES-ALLOW: NonVisibleEnum::NonVisibleEnum() : member(CppEnum) {} +NonVisibleNestedCast::NonVisibleNestedCast() + : member(static_cast<int>(CppConstant + 1.0)) {} +// CHECK-FIXES-ALLOW: : member(static_cast<int>(CppConstant + 1.0)) {} +NonVisibleMacroReference::NonVisibleMacroReference() + : member(NON_VISIBLE_INIT_VALUE) {} +// CHECK-FIXES-ALLOW: : member(NON_VISIBLE_INIT_VALUE) {} +using HeaderValues::Constant; +NonVisibleUsingDeclaration::NonVisibleUsingDeclaration() : member(Constant) {} +// CHECK-FIXES-ALLOW: NonVisibleUsingDeclaration::NonVisibleUsingDeclaration() : member(Constant) {} +template <typename T> +NonVisibleTemplate<T>::NonVisibleTemplate() : member(CppConstant) {} +// CHECK-FIXES-ALLOW: NonVisibleTemplate<T>::NonVisibleTemplate() : member(CppConstant) {} +NonVisibleTemplate<int> NonVisibleTemplateInstance; + +HeaderVisibleConstexpr::HeaderVisibleConstexpr() + : member(HeaderValues::Constant) {} +HeaderVisibleStatic::HeaderVisibleStatic() : member(HeaderValues::Static) {} +HeaderVisibleEnum::HeaderVisibleEnum() : member(HeaderValues::EnumValue) {} +SameClassLaterStatic::SameClassLaterStatic() : member(ClassConstant) {} +SameClassLaterEnum::SameClassLaterEnum() : member(ClassEnumValue) {} +InheritedStatic::InheritedStatic() : member(BaseConstant) {} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
