Skip to content

[cxx-interop] Import non-public inherited members #79101

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

Closed
wants to merge 3 commits into from
Closed

[cxx-interop] Import non-public inherited members #79101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0f8b97a
[cxx-interop] Import non-public inherited members
j-hui Feb 1, 2025
1f853c0
Add tests for importing non-public inherited members
j-hui Feb 2, 2025
8a2dd80
Fix latent bug that causes spurious ambiguous member lookups
j-hui Feb 2, 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
16 changes: 12 additions & 4 deletions include/swift/AST/ClangModuleLoader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,18 @@ class ClangModuleLoader : public ModuleLoader {
/// Imports a clang decl directly, rather than looking up its name.
virtual Decl *importDeclDirectly(const clang::NamedDecl *decl) = 0;

/// Imports a clang decl from a base class, cloning it for \param newContext
/// if it wasn't cloned for this specific context before.
virtual ValueDecl *importBaseMemberDecl(ValueDecl *decl,
DeclContext *newContext) = 0;
/// Clones an imported \param decl from its base class to its derived class
/// \param newContext where it is inherited. Its access level is determined
/// with respect to \param inheritance, which signifies whether \param decl
/// was inherited via C++ public/protected/private inheritance.
///
/// This function uses a cache so that it is idempotent; successive
/// invocations will only generate one cloned ValueDecl (and all return
/// a pointer to it). Returns a NULL pointer upon failure.
virtual ValueDecl *
importBaseMemberDecl(ValueDecl *decl,
DeclContext *newContext,
clang::AccessSpecifier inheritance) = 0;

/// Emits diagnostics for any declarations named name
/// whose direct declaration context is a TU.
Expand Down
3 changes: 2 additions & 1 deletion include/swift/ClangImporter/ClangImporter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,8 @@ class ClangImporter final : public ClangModuleLoader {
Decl *importDeclDirectly(const clang::NamedDecl *decl) override;

ValueDecl *importBaseMemberDecl(ValueDecl *decl,
DeclContext *newContext) override;
DeclContext *newContext,
clang::AccessSpecifier inheritance) override;

/// Emits diagnostics for any declarations named name
/// whose direct declaration context is a TU.
Expand Down
31 changes: 22 additions & 9 deletions include/swift/ClangImporter/ClangImporterRequests.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "swift/Basic/Statistic.h"
#include "swift/ClangImporter/ClangImporter.h"
#include "clang/AST/Type.h"
#include "clang/Basic/Specifiers.h"
#include "llvm/ADT/Hashing.h"
#include "llvm/ADT/TinyPtrVector.h"

Expand Down Expand Up @@ -132,25 +133,33 @@ class CXXNamespaceMemberLookup
};

/// The input type for a record member lookup request.
///
/// These lookups may be requested recursively in the case of inheritance,
/// for which we separately keep track of the derived class where we started
/// looking (startDecl) and the access level for the current inheritance.
struct ClangRecordMemberLookupDescriptor final {
NominalTypeDecl *recordDecl;
NominalTypeDecl *inheritingDecl;
DeclName name;
NominalTypeDecl *recordDecl; // Where we are currently looking
NominalTypeDecl *inheritingDecl; // Where we started looking from
DeclName name; // What we are looking for
clang::AccessSpecifier inheritance; // public/protected/private inheritance
// (clang::AS_none means no inheritance)

ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name)
: recordDecl(recordDecl), inheritingDecl(recordDecl), name(name) {
: recordDecl(recordDecl), inheritingDecl(recordDecl), name(name),
inheritance(clang::AS_none) {
assert(isa<clang::RecordDecl>(recordDecl->getClangDecl()));
}

friend llvm::hash_code
hash_value(const ClangRecordMemberLookupDescriptor &desc) {
return llvm::hash_combine(desc.name, desc.recordDecl, desc.inheritingDecl);
return llvm::hash_combine(desc.name, desc.recordDecl, desc.inheritingDecl, desc.inheritance);
}

friend bool operator==(const ClangRecordMemberLookupDescriptor &lhs,
const ClangRecordMemberLookupDescriptor &rhs) {
return lhs.name == rhs.name && lhs.recordDecl == rhs.recordDecl &&
lhs.inheritingDecl == rhs.inheritingDecl;
lhs.inheritingDecl == rhs.inheritingDecl &&
lhs.inheritance == rhs.inheritance;
}

friend bool operator!=(const ClangRecordMemberLookupDescriptor &lhs,
Expand All @@ -163,11 +172,15 @@ struct ClangRecordMemberLookupDescriptor final {

// This private constructor should only be used in ClangRecordMemberLookup,
// for recursively traversing base classes that inheritingDecl inherites from.
ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name,
NominalTypeDecl *inheritingDecl)
: recordDecl(recordDecl), inheritingDecl(inheritingDecl), name(name) {
ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl,
DeclName name,
NominalTypeDecl *inheritingDecl,
clang::AccessSpecifier inheritance)
: recordDecl(recordDecl), inheritingDecl(inheritingDecl), name(name),
inheritance(inheritance) {
assert(isa<clang::RecordDecl>(recordDecl->getClangDecl()));
assert(isa<clang::CXXRecordDecl>(inheritingDecl->getClangDecl()));
assert(inheritance != clang::AS_none && "");
assert(recordDecl != inheritingDecl &&
"recursive calls should lookup elsewhere");
}
Expand Down
96 changes: 68 additions & 28 deletions lib/ClangImporter/ClangImporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5938,9 +5938,6 @@ makeBaseClassMemberAccessors(DeclContext *declContext,
bodyParams = ParameterList::createEmpty(ctx);
}

assert(baseClassVar->getFormalAccess() == AccessLevel::Public &&
"base class member must be public");

auto getterDecl = AccessorDecl::create(
ctx,
/*FuncLoc=*/SourceLoc(),
Expand All @@ -5953,7 +5950,7 @@ makeBaseClassMemberAccessors(DeclContext *declContext,
: computedType,
declContext);
getterDecl->setIsTransparent(true);
getterDecl->setAccess(AccessLevel::Public);
getterDecl->copyFormalAccessFrom(computedVar);
getterDecl->setBodySynthesizer(useAddress
? synthesizeBaseClassFieldAddressGetterBody
: synthesizeBaseClassFieldGetterBody,
Expand Down Expand Up @@ -5988,7 +5985,7 @@ makeBaseClassMemberAccessors(DeclContext *declContext,
: TupleType::getEmpty(ctx),
declContext);
setterDecl->setIsTransparent(true);
setterDecl->setAccess(AccessLevel::Public);
setterDecl->copyFormalAccessFrom(computedVar);
setterDecl->setBodySynthesizer(useAddress
? synthesizeBaseClassFieldAddressSetterBody
: synthesizeBaseClassFieldSetterBody,
Expand Down Expand Up @@ -6041,8 +6038,17 @@ void cloneImportedAttributes(ValueDecl *fromDecl, ValueDecl* toDecl) {
}
}

static ValueDecl *
cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
static ValueDecl *cloneBaseMemberDecl(ValueDecl *decl,
DeclContext *newContext,
clang::AccessSpecifier inheritance) {
assert(inheritance != clang::AS_none &&
"public/protected/private inheritance was not specified");
static_assert(AccessLevel::Private < AccessLevel::Public &&
"std::min() relies on this ordering");
// Adjust access according to whichever is more restrictive, between what decl
// was declared with (in its base class) or what it is being inherited with.
AccessLevel access = std::min(decl->getFormalAccess(),
importer::convertClangAccess(inheritance));
ASTContext &context = decl->getASTContext();

if (auto fn = dyn_cast<FuncDecl>(decl)) {
Expand All @@ -6068,7 +6074,7 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
fn->getGenericParams(), fn->getParameters(),
fn->getResultInterfaceType(), newContext);
cloneImportedAttributes(decl, out);
out->copyFormalAccessFrom(fn);
out->setAccess(access);
out->setBodySynthesizer(synthesizeBaseClassMethodBody, fn);
out->setSelfAccessKind(fn->getSelfAccessKind());
return out;
Expand All @@ -6086,7 +6092,7 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
subscript->getStaticSpelling(), subscript->getSubscriptLoc(),
subscript->getIndices(), subscript->getNameLoc(), subscript->getElementInterfaceType(),
newContext, subscript->getGenericParams());
out->copyFormalAccessFrom(subscript);
out->setAccess(access);
out->setAccessors(SourceLoc(),
makeBaseClassMemberAccessors(newContext, out, subscript),
SourceLoc());
Expand All @@ -6110,7 +6116,7 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
out->setInterfaceType(var->getInterfaceType());
out->setIsObjC(var->isObjC());
out->setIsDynamic(var->isDynamic());
out->copyFormalAccessFrom(var);
out->setAccess(access);
out->getASTContext().evaluator.cacheOutput(HasStorageRequest{out}, false);
auto accessors = makeBaseClassMemberAccessors(newContext, out, var);
out->setAccessors(SourceLoc(), accessors, SourceLoc());
Expand All @@ -6136,7 +6142,7 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
typeAlias->getName(), typeAlias->getNameLoc(),
typeAlias->getGenericParams(), newContext);
out->setUnderlyingType(typeAlias->getUnderlyingType());
out->copyFormalAccessFrom(typeAlias);
out->setAccess(access);
return out;
}

Expand All @@ -6147,7 +6153,7 @@ cloneBaseMemberDecl(ValueDecl *decl, DeclContext *newContext) {
typeDecl->getLoc(), typeDecl->getLoc(), typeDecl->getName(),
typeDecl->getLoc(), nullptr, newContext);
out->setUnderlyingType(typeDecl->getInterfaceType());
out->copyFormalAccessFrom(typeDecl);
out->setAccess(access);
return out;
}

Expand All @@ -6159,7 +6165,7 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
NominalTypeDecl *recordDecl = desc.recordDecl;
NominalTypeDecl *inheritingDecl = desc.inheritingDecl;
DeclName name = desc.name;

clang::AccessSpecifier inheritance = desc.inheritance;
bool inheritedLookup = recordDecl != inheritingDecl;

auto &ctx = recordDecl->getASTContext();
Expand All @@ -6181,6 +6187,14 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
if (isa<clang::CXXConstructorDecl>(found) && isa<ClassDecl>(recordDecl))
continue;

// Skip this base class member if it is private.
//
// BUG: private base class members should be inherited but inaccessible.
// Skipping them here may affect accurate overload resolution in cases of
// multiple inheritance (which is currently buggy anyway).
if (inheritedLookup && found->getAccess() == clang::AS_private)
continue;

auto imported = clangModuleLoader->importDeclDirectly(found);
if (!imported)
continue;
Expand All @@ -6189,7 +6203,7 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
// by synthesizing getters and setters.
if (inheritedLookup) {
imported = clangModuleLoader->importBaseMemberDecl(
cast<ValueDecl>(imported), inheritingDecl);
cast<ValueDecl>(imported), inheritingDecl, inheritance);
if (!imported)
continue;
}
Expand All @@ -6207,8 +6221,23 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
namedMember->getName().getBaseName() != name)
continue;

auto memberClangDecl = namedMember->getClangDecl();

// Skip this base class member if it isn't from recordDecl; this happens
// when this member was inherited.
if (memberClangDecl != recordDecl->getClangDecl())
continue;

// Skip this base class if this is a case of nested private inheritance.
//
// BUG: private base class members should be inherited but inaccessible.
// Skipping them here may affect accurate overload resolution in cases of
// multiple inheritance (which is currently buggy anyway).
Comment on lines +6231 to +6235
Copy link
Contributor Author

@j-hui j-hui Feb 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (and several other pieces which I've commented with // BUG) is a known bug that I wanted to highlight to reviewers. It is a bug in the sense that the behavior differs from what C++ does (derived.baseMember complains about baseMember being inaccessible/not inherited, rather than being non-existent).

I haven't done so yet but I think the right way to do this is to import those fields with an appropriate @unavailable attribute.

if (memberClangDecl->getAccess() == clang::AS_private)
continue;

if (auto imported = clangModuleLoader->importBaseMemberDecl(
namedMember, inheritingDecl))
namedMember, inheritingDecl, inheritance))
result.push_back(cast<ValueDecl>(imported));
}
}
Expand All @@ -6226,7 +6255,15 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
foundNameArities.insert(getArity(valueDecl));

for (auto base : cxxRecord->bases()) {
if (base.getAccessSpecifier() != clang::AccessSpecifier::AS_public)
clang::AccessSpecifier baseInheritance = base.getAccessSpecifier();
// Skip this base class if this is a case of nested private inheritance.
//
// BUG: members of private base classes should actually be imported but
// inaccessible. Skipping them here may affect accurate overload
// resolution in cases of multiple inheritance (which is currently buggy
// anyway).
if (inheritance != clang::AS_none &&
baseInheritance == clang::AS_private)
continue;

clang::QualType baseType = base.getType();
Expand All @@ -6242,12 +6279,19 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate(
if (cast<ValueDecl>(import)->getName() == name)
continue;

if (inheritance != clang::AS_none)
// For nested inheritance, clamp inheritance to least permissive level
// which is the largest numerical value for clang::AccessSpecifier
baseInheritance = std::max(inheritance, baseInheritance);
static_assert(clang::AS_private > clang::AS_protected &&
clang::AS_protected > clang::AS_public &&
"using std::max() relies on this ordering");

// Add Clang members that are imported lazily.
auto baseResults = evaluateOrDefault(
ctx.evaluator,
ClangRecordMemberLookup(
{cast<NominalTypeDecl>(import), name, inheritingDecl}),
{});
ClangRecordMemberLookup({cast<NominalTypeDecl>(import), name,
inheritingDecl, baseInheritance}), {});

for (auto foundInBase : baseResults) {
// Do not add duplicate entry with the same arity,
Expand Down Expand Up @@ -7507,18 +7551,13 @@ Decl *ClangImporter::importDeclDirectly(const clang::NamedDecl *decl) {
return Impl.importDecl(decl, Impl.CurrentVersion);
}

ValueDecl *ClangImporter::Implementation::importBaseMemberDecl(
ValueDecl *decl, DeclContext *newContext) {
// Do not clone private C++ decls.
if (decl->getFormalAccess() < AccessLevel::Public)
return nullptr;

ValueDecl *ClangImporter::Implementation::importBaseMemberDecl(ValueDecl *decl, DeclContext *newContext, clang::AccessSpecifier inheritance) {
// Make sure we don't clone the decl again for this class, as that would
// result in multiple definitions of the same symbol.
std::pair<ValueDecl *, DeclContext *> key = {decl, newContext};
auto known = clonedBaseMembers.find(key);
if (known == clonedBaseMembers.end()) {
ValueDecl *cloned = cloneBaseMemberDecl(decl, newContext);
ValueDecl *cloned = cloneBaseMemberDecl(decl, newContext, inheritance);
known = clonedBaseMembers.insert({key, cloned}).first;
}

Expand All @@ -7536,8 +7575,9 @@ size_t ClangImporter::Implementation::getImportedBaseMemberDeclArity(
}

ValueDecl *ClangImporter::importBaseMemberDecl(ValueDecl *decl,
DeclContext *newContext) {
return Impl.importBaseMemberDecl(decl, newContext);
DeclContext *newContext,
clang::AccessSpecifier inheritance) {
return Impl.importBaseMemberDecl(decl, newContext, inheritance);
}

void ClangImporter::diagnoseTopLevelValue(const DeclName &name) {
Expand Down
Loading