Skip to content

[clang-tidy] Switch misc-confusable-identifiers check to a faster algorithm. #130369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+205 −115
Open

[clang-tidy] Switch misc-confusable-identifiers check to a faster algorithm. #130369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
78c9d5d
Switch misc-confusable-identifiers check to a faster algorithm.
zygoloid Mar 7, 2025
870c0ea
Fix file headers
zygoloid Mar 7, 2025
9dd44fe
clang-format
zygoloid Mar 7, 2025
b7accb2
Fix some bugs.
zygoloid Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
236 changes: 139 additions & 97 deletions clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
//===--- ConfusableIdentifierCheck.cpp -
// clang-tidy--------------------------===//
//===--- ConfusableIdentifierCheck.cpp - clang-tidy -----------------------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
Expand All @@ -9,6 +8,7 @@

#include "ConfusableIdentifierCheck.h"

#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/SmallString.h"
Expand Down Expand Up @@ -89,90 +89,69 @@ static llvm::SmallString<64U> skeleton(StringRef Name) {
return Skeleton;
}

static bool mayShadowImpl(const DeclContext *DC0, const DeclContext *DC1) {
return DC0 && DC0 == DC1;
}

static bool mayShadowImpl(const NamedDecl *ND0, const NamedDecl *ND1) {
return isa<TemplateTypeParmDecl>(ND0) || isa<TemplateTypeParmDecl>(ND1);
}

static bool isMemberOf(const ConfusableIdentifierCheck::ContextInfo *DC0,
const ConfusableIdentifierCheck::ContextInfo *DC1) {
return llvm::is_contained(DC1->Bases, DC0->PrimaryContext);
}

static bool enclosesContext(const ConfusableIdentifierCheck::ContextInfo *DC0,
const ConfusableIdentifierCheck::ContextInfo *DC1) {
if (DC0->PrimaryContext == DC1->PrimaryContext)
return true;

return llvm::is_contained(DC0->PrimaryContexts, DC1->PrimaryContext) ||
llvm::is_contained(DC1->PrimaryContexts, DC0->PrimaryContext);
}

static bool mayShadow(const NamedDecl *ND0,
const ConfusableIdentifierCheck::ContextInfo *DC0,
const NamedDecl *ND1,
const ConfusableIdentifierCheck::ContextInfo *DC1) {

if (!DC0->Bases.empty() && !DC1->Bases.empty()) {
// if any of the declaration is a non-private member of the other
// declaration, it's shadowed by the former

if (ND1->getAccess() != AS_private && isMemberOf(DC1, DC0))
return true;
namespace {
struct Entry {
const NamedDecl *ND;
const Decl *Parent;
bool FromDerivedClass;
};
} // namespace

if (ND0->getAccess() != AS_private && isMemberOf(DC0, DC1))
// Map from a context to the declarations in that context with the current
// skeleton. At most one entry per distinct identifier is tracked. The
// context is usually a `DeclContext`, but can also be a template declaration
// that has no corresponding context, such as an alias template or variable
// template.
using DeclsWithinContextMap =
llvm::DenseMap<const Decl *, llvm::SmallVector<Entry, 1>>;

static bool addToContext(DeclsWithinContextMap &DeclsWithinContext,
const Decl *Context, Entry E) {
auto &Decls = DeclsWithinContext[Context];
if (!Decls.empty() &&
Decls.back().ND->getIdentifier() == E.ND->getIdentifier()) {
// Already have a declaration with this identifier in this context. Don't
// track another one. This means that if an outer name is confusable with an
// inner name, we'll only diagnose the outer name once, pointing at the
// first inner declaration with that name.
if (Decls.back().FromDerivedClass && !E.FromDerivedClass) {
// Prefer the declaration that's not from the derived class, because that
// conflicts with more declarations.
Decls.back() = E;
return true;
}

if (!mayShadowImpl(DC0->NonTransparentContext, DC1->NonTransparentContext) &&
!mayShadowImpl(ND0, ND1))
}
return false;

return enclosesContext(DC0, DC1);
}
Decls.push_back(E);
return true;
}

const ConfusableIdentifierCheck::ContextInfo *
ConfusableIdentifierCheck::getContextInfo(const DeclContext *DC) {
const DeclContext *PrimaryContext = DC->getPrimaryContext();
auto [It, Inserted] = ContextInfos.try_emplace(PrimaryContext);
if (!Inserted)
return &It->second;

ContextInfo &Info = It->second;
Info.PrimaryContext = PrimaryContext;
Info.NonTransparentContext = PrimaryContext;
static void addToEnclosingContexts(DeclsWithinContextMap &DeclsWithinContext,
const Decl *Parent,
const NamedDecl *ND) {
const Decl *Outer = Parent;
while (Outer) {
if (const auto *NS = dyn_cast<NamespaceDecl>(Outer))
Outer = NS->getCanonicalDecl();

if (!addToContext(DeclsWithinContext, Outer, {ND, Parent, false}))
return;

if (const auto *RD = dyn_cast<CXXRecordDecl>(Outer)) {
RD = RD->getDefinition();
if (RD) {
RD->forallBases([&](const CXXRecordDecl *Base) {
addToContext(DeclsWithinContext, Base, {ND, Parent, true});
return true;
});
}
}

while (Info.NonTransparentContext->isTransparentContext()) {
Info.NonTransparentContext = Info.NonTransparentContext->getParent();
if (!Info.NonTransparentContext)
auto *OuterDC = Outer->getDeclContext();
if (!OuterDC)
break;
Outer = cast_or_null<Decl>(OuterDC->getNonTransparentContext());
}

if (Info.NonTransparentContext)
Info.NonTransparentContext =
Info.NonTransparentContext->getPrimaryContext();

while (DC) {
if (!isa<LinkageSpecDecl>(DC) && !isa<ExportDecl>(DC))
Info.PrimaryContexts.push_back(DC->getPrimaryContext());
DC = DC->getParent();
}

if (const auto *RD = dyn_cast<CXXRecordDecl>(PrimaryContext)) {
RD = RD->getDefinition();
if (RD) {
Info.Bases.push_back(RD);
RD->forallBases([&](const CXXRecordDecl *Base) {
Info.Bases.push_back(Base);
return false;
});
}
}

return &Info;
}

void ConfusableIdentifierCheck::check(
Expand All @@ -181,42 +160,105 @@ void ConfusableIdentifierCheck::check(
if (!ND)
return;

IdentifierInfo *NDII = ND->getIdentifier();
addDeclToCheck(ND, cast<Decl>(ND->getDeclContext()
->getNonTransparentContext()));

// Associate template parameters with this declaration of this template.
if (const auto *TD = dyn_cast<TemplateDecl>(ND)) {
for (const NamedDecl *Param : *TD->getTemplateParameters())
addDeclToCheck(Param, TD->getTemplatedDecl());
}

// Associate function parameters with this declaration of this function.
if (const auto *FD = dyn_cast<FunctionDecl>(ND)) {
for (const NamedDecl *Param : FD->parameters())
addDeclToCheck(Param, ND);
}
}

void ConfusableIdentifierCheck::addDeclToCheck(const NamedDecl *ND,
const Decl *Parent) {
const IdentifierInfo *NDII = ND->getIdentifier();
if (!NDII)
return;

StringRef NDName = NDII->getName();
if (NDName.empty())
return;

const ContextInfo *Info = getContextInfo(ND->getDeclContext());
NameToDecls[NDII].push_back({ND, Parent});
}

void ConfusableIdentifierCheck::onEndOfTranslationUnit() {
llvm::StringMap<llvm::SmallVector<const IdentifierInfo *, 1>> SkeletonToNames;
// Compute the skeleton for each identifier.
for (auto &[Ident, Decls] : NameToDecls) {
SkeletonToNames[skeleton(Ident->getName())].push_back(Ident);
}

llvm::SmallVector<Entry> &Mapped = Mapper[skeleton(NDName)];
for (const Entry &E : Mapped) {
if (!mayShadow(ND, Info, E.Declaration, E.Info))
// Visit each skeleton with more than one identifier.
for (auto &[Skel, Idents] : SkeletonToNames) {
if (Idents.size() < 2) {
continue;
}

const IdentifierInfo *ONDII = E.Declaration->getIdentifier();
StringRef ONDName = ONDII->getName();
if (ONDName == NDName)
continue;
// Find the declaration contexts that transitively contain each identifier.
DeclsWithinContextMap DeclsWithinContext;
for (const IdentifierInfo *II : Idents) {
for (auto [ND, Parent] : NameToDecls[II]) {
addToEnclosingContexts(DeclsWithinContext, Parent, ND);
}
}

diag(ND->getLocation(), "%0 is confusable with %1") << ND << E.Declaration;
diag(E.Declaration->getLocation(), "other declaration found here",
DiagnosticIDs::Note);
// Check to see if any declaration is declared in a context that
// transitively contains another declaration with a different identifier but
// the same skeleton.
for (const IdentifierInfo *II : Idents) {
for (auto [OuterND, OuterParent] : NameToDecls[II]) {
for (Entry Inner : DeclsWithinContext[OuterParent]) {
// Don't complain if the identifiers are the same.
if (OuterND->getIdentifier() == Inner.ND->getIdentifier())
continue;

// Don't complain about a derived-class name shadowing a base class
// private member.
if (OuterND->getAccess() == AS_private && Inner.FromDerivedClass)
continue;

// If the declarations are in the same context, only diagnose the
// later one.
if (OuterParent == Inner.Parent &&
Inner.ND->getASTContext()
.getSourceManager()
.isBeforeInTranslationUnit(Inner.ND->getLocation(),
OuterND->getLocation()))
continue;

diag(Inner.ND->getLocation(), "%0 is confusable with %1")
<< Inner.ND << OuterND;
diag(OuterND->getLocation(), "other declaration found here",
DiagnosticIDs::Note);
}
}
}
}

Mapped.push_back({ND, Info});
}

void ConfusableIdentifierCheck::onEndOfTranslationUnit() {
Mapper.clear();
ContextInfos.clear();
NameToDecls.clear();
}

void ConfusableIdentifierCheck::registerMatchers(
ast_matchers::MatchFinder *Finder) {
Finder->addMatcher(ast_matchers::namedDecl().bind("nameddecl"), this);
// Parameter declarations sometimes use the translation unit or some outer
// enclosing context as their `DeclContext`, instead of their parent, so
// we handle them specially in `check`.
auto AnyParamDecl = ast_matchers::anyOf(
ast_matchers::parmVarDecl(), ast_matchers::templateTypeParmDecl(),
ast_matchers::nonTypeTemplateParmDecl(),
ast_matchers::templateTemplateParmDecl());
Finder->addMatcher(
ast_matchers::namedDecl(ast_matchers::unless(AnyParamDecl))
.bind("nameddecl"),
this);
}

} // namespace clang::tidy::misc
25 changes: 7 additions & 18 deletions clang-tools-extra/clang-tidy/misc/ConfusableIdentifierCheck.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
//===--- ConfusableIdentifierCheck.h - clang-tidy
//-------------------------------*- C++ -*-===//
//===--- ConfusableIdentifierCheck.h - clang-tidy ---------------*- C++ -*-===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
Expand All @@ -11,7 +10,7 @@
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_CONFUSABLE_IDENTIFIER_CHECK_H

#include "../ClangTidyCheck.h"
#include <unordered_map>
#include "llvm/ADT/DenseMap.h"

namespace clang::tidy::misc {

Expand All @@ -31,23 +30,13 @@ class ConfusableIdentifierCheck : public ClangTidyCheck {
return TK_IgnoreUnlessSpelledInSource;
}

struct ContextInfo {
const DeclContext *PrimaryContext;
const DeclContext *NonTransparentContext;
llvm::SmallVector<const DeclContext *> PrimaryContexts;
llvm::SmallVector<const CXXRecordDecl *> Bases;
};

private:
struct Entry {
const NamedDecl *Declaration;
const ContextInfo *Info;
};

const ContextInfo *getContextInfo(const DeclContext *DC);
void addDeclToCheck(const NamedDecl *ND, const Decl *Parent);

llvm::StringMap<llvm::SmallVector<Entry>> Mapper;
std::unordered_map<const DeclContext *, ContextInfo> ContextInfos;
llvm::DenseMap<
const IdentifierInfo *,
llvm::SmallVector<std::pair<const NamedDecl *, const Decl *>, 1>>
NameToDecls;
};

} // namespace clang::tidy::misc
Expand Down
Loading
Loading