swift
[cxx-interop] Import non-public inherited members
#79348
Merged

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

j-hui
j-hui147 days ago

This patch is follow-up work from #78942 and concurrent with #79093.
It imports non-public members, which were previously not being imported.

The access level of each inherited base member is computed according to
the inheritance rules in C++, and accounts for public/protected/private
inheritance. Base members that are inaccessible from the derived class
(either because they are private, or due to nested private inheritance)
are also imported into Swift but marked as unavailable, to ensure that
(1) trying to access these members produces a reasonable error message
rather than claiming that the inaccessible member doesn't exist, and
(2) ambiguous member lookups aren't erroneously disambiguated by virtue
of one being public and the other private.

Once #79093 (SWIFT_PRIVATE_FILEID) is merged in, these inherited members
will be available within Swift extensions that have been blessed by the
SWIFT_PRIVATE_FILEID annotation.

rdar://137764620

j-hui [cxx-interop] Import non-public inherited members
0f8b97aa
j-hui Add tests for importing non-public inherited members
1f853c0f
j-hui Check for null pointers
54382491
j-hui Fix some test cases
18ab1b4c
j-hui Handle inaccessible private members
8eec9098
j-hui Formatting
9c390930
j-hui Merge remote-tracking branch 'origin/main' into import-inherited-members
a5ac1680
j-hui Add hash_value() to ClangInheritanceInfo
6a9a2919
j-hui Import inaccessible private base class members but make them unavailable
917bdd1c
j-hui j-hui requested a review from zoecarver zoecarver 147 days ago
j-hui j-hui requested a review from hyp hyp 147 days ago
j-hui j-hui requested a review from egorzhdan egorzhdan 147 days ago
j-hui j-hui requested a review from Xazax-hun Xazax-hun 147 days ago
j-hui j-hui requested a review from beccadax beccadax 147 days ago
j-hui j-hui requested a review from ian-twilightcoder ian-twilightcoder 147 days ago
j-hui j-hui requested a review from hborla hborla 147 days ago
j-hui j-hui requested a review from slavapestov slavapestov 147 days ago
j-hui j-hui requested a review from xedin xedin 147 days ago
j-hui
j-hui147 days ago

@swift-ci Please test

Xazax-hun
Xazax-hun commented on 2025-02-13
Xazax-hun146 days ago

Are there any tests where a field with the same name appears in multiple bases? We can disambiguate accessing those in C++ by using qualified lookup but it is not possible in Swift where in case of structs we put everything in the same type. So I think we might want to skip fields (and methods?) that can cause ambiguities.

(This is different from the case where the members in the derived class shadows the members in the base.)

j-hui
j-hui146 days ago

Ambiguous lookups need follow-up work. Last time I tried it out, it gave me a very unhelpful error message, because the way the diagnostics are implemented don't seem to consider that kind of ambiguity.

That said, I still think we should import those ambiguous members, because it lets us report the same error from Swift as in C++ (even if that isn't quite implemented right now). This is the same reason why I am importing inaccessible private base members; without doing so, we get "missing field" errors that don't actually point to the underlying problem.

To work around an ambiguous member lookup, users can add a shim method in C++ where they perform the fully qualified lookup there.

j-hui
j-hui146 days ago

I will add a test case that has ambiguous members and make sure it does not affect unambiguous member lookup.

j-hui Adjust module interface test case for using base members to acccount …
17d2be9b
j-hui Correct silly typo in header guard
3ca3fde7
j-hui Add test case for shadowing of non-public members
8652f1b4
j-hui Formatting
6bb3409f
j-hui
j-hui146 days ago

@Xazax-hun I added a test case that checks ambiguous lookups, and it misbehaves somewhat (specifically, lookups that should be ambiguous do not seem to be).

There is also a pretty big discrepancy in how ClangImporter.cpp::ClangRecordMemberLookup and ImportDecl.cpp::loadAllMembersOfRecordDecl() deal with ambiguities, which causes the module interface suggest a different behavior that what we observe when type-checking various lookups.

To avoid blowing up the size of this PR, I would like to revisit this issue in follow-up work.

rdar://144843878

j-hui
j-hui146 days ago

@swift-ci Please test

j-hui j-hui requested a review from Xazax-hun Xazax-hun 146 days ago
Xazax-hun
Xazax-hun145 days ago

To avoid blowing up the size of this PR, I would like to revisit this issue in follow-up work.

Sounds good!

Xazax-hun
Xazax-hun approved these changes on 2025-02-14
Xazax-hun145 days ago

Overall looks good to me, some nits/questions inline.

Conversation is marked as resolved
Show resolved
include/swift/ClangImporter/ClangImporter.h
772/// Information used to compute the access level of inherited C++ members.
773class ClangInheritanceInfo {
774 clang::AccessSpecifier inheritedAccess;
775
bool nestedPrivate;
Xazax-hun145 days ago👍 1

Could you elaborate on what nestedPrivate means and why does it need special tracking?

Xazax-hun142 days ago

Thanks for adding the comment! I am wondering why we need this additional piece of information. Wouldn't the cumulative access above already have all the information we need?

j-hui139 days ago

Consider this example:

struct C1 { public: void foo(void) {} }
struct C2 : private C1 {}
struct C3 : C2 {}

The member foo() has three accessibility levels here:

  • C1::foo() is public, just like we declared it to be
  • C2::foo() is private, because C2 inherits from C1 privately, so it can only be accessed in members and friends of C2
  • C3::foo() is inaccessible, because foo() is private in C2. This is the case whether C3 inherits from C2 publicly, privately, or, er, "protectedly."

If we only naively track the cumulative access, foo() will be incorrectly deemed private, rather than inaccessible (which I implement by marking the imported member unavailable).

In essence, I am modeling an access level beneath private. I could have encoded this as std::optional<clang::AccessSpecifier>, or overloaded clang::AS_none to mean "more private than private" in this particular case, but the former leads is a bit noisy and the latter can be misleading and error prone (because normally clang::AS_none maps to Swift public in importer::convertClangAccess()). I felt the clearest way to express this logic was to have a separately named field (though perhaps it wasn't clear enough here; do you have any advice on what I should do here?)

Xazax-hun139 days ago

Thanks for the example, it is really insightful! Consider:

struct C1 {}
struct C2 : C1 { private: void foo(void) {} }
struct C3 : C2 {}

I'd argue this case is in some sense isomorphic to your example when we consider the C3 : C2 inheritance. If foo is private in C2, it will be inaccessible in C3. And it does not matter why foo is private. Is it because of the private inheritance? Is it because it was declared as private? The result is the same. So there is no memory how we ended up here. And I am not sure if I'd consider this case "nested" in any way. And in case something is inaccessible, AccessSpecifier is meaningless. This makes me think that the right representation here makes non-sensical states (like nestedPrivate == true && AccessSpecifier == public) unrepresentable. This makes me think maybe std::optional<clang::AccessSpecifier> is a better representation. What do you think?

j-hui138 days ago

The case you cite is a bit different, because the access specifier applies to the member rather than the inheritance (what this struct tracks).

That said, your point about making invalid states unrepresentable is well taken. I'll change this to use std::optional!

Xazax-hun138 days ago

access specifier applies to the member rather than the inheritance (what this struct tracks).

Ah, I see! Sorry for the confusion.

Conversation is marked as resolved
Show resolved
include/swift/ClangImporter/ClangImporterRequests.h
139145
140146 ClangRecordMemberLookupDescriptor(NominalTypeDecl *recordDecl, DeclName name)
141 : recordDecl(recordDecl), inheritingDecl(recordDecl), name(name) {
147 : recordDecl(recordDecl), inheritingDecl(recordDecl), name(name),
148
inheritance(ClangInheritanceInfo()) {
Xazax-hun145 days ago👍 1

Nit: I wonder if you actually don't need to spell ClangInheritanceInfo out explicitly here but just inheritance() would work.

Conversation is marked as resolved
Show resolved
lib/ClangImporter/ClangImporter.cpp
8361 ClangInheritanceInfo info;
8362
8363 auto *derivedRecord =
8364
llvm::cast_if_present<clang::CXXRecordDecl>(usingDecl->getDeclContext());
Xazax-hun145 days ago

Do you need the llvm:: for the cast? If you do, I wonder if we should change this as we already access isa and similar cast functions without having to spell the namespace out.

j-hui145 days ago

Ah I had forgotten that Swift has declarations like using llvm::isa and using llvm::dyn_cast_or_null. I just switched to using the latter.

I had originally used the former because the documentation for llvm::dyn_cast_or_null suggests that it is deprecated in favor of llvm::cast_if_present.

j-hui Address review feedback
606cc1da
j-hui j-hui requested a review from fahadnayyar fahadnayyar 145 days ago
j-hui
j-hui145 days ago

@swift-ci Please test

j-hui Get rid of confusing and error-prone ClangInheritanceInfo::forUsingDe…
fccb2660
j-hui
j-hui143 days ago

@swift-ci Please test

j-hui Merge remote-tracking branch 'origin/main' into import-inherited-members
742aad94
j-hui Add 'using' tests with more emphasis on access levels and inheritance
e18734a8
j-hui Add protected inheritance test case to using-base-members
0d2fac59
j-hui Add some test cases to using-base-members test case
ee61104c
j-hui
j-hui139 days ago

@swift-ci Please test

j-hui Comment out tests for some broken operators
34359c55
j-hui Eliminate nestedPrivate field from ClangInheritanceInfo
e93cf5c2
j-hui Remove stray tee %.output
d99a9e8b
j-hui
j-hui138 days ago

@swift-ci Please smoke test

Xazax-hun
Xazax-hun approved these changes on 2025-02-21
Xazax-hun138 days ago

Thanks!

j-hui Account for multiple pointees in conformToCxxOptionalIfNeeded()
b70cff99
j-hui Use private inheritance for using-base-members
8f7cf9cc
j-hui Formatting
cd87c7e9
j-hui
j-hui138 days ago

@swift-ci Please test

j-hui Adjust access tests
27960ee1
j-hui
j-hui138 days ago

@swift-ci please test

j-hui REVERTME: logging for Windows CI
bdd8ea4b
j-hui
j-hui137 days ago

@swift-ci please smoke test windows platform

j-hui Revert "REVERTME: logging for Windows CI"
07af2d43
j-hui Add hack that fixes MSVC std::optional
046c7df2
j-hui Revert "Account for multiple pointees in conformToCxxOptionalIfNeeded()"
04e79ddb
j-hui Formatting
f4da02ad
j-hui
j-hui135 days ago

@swift-ci please test

j-hui Fix test from reordering
53502db7
j-hui
j-hui135 days ago

@swift-ci Please test

j-hui j-hui enabled auto-merge (squash) 135 days ago
j-hui j-hui merged 66c2e2c5 into main 135 days ago
j-hui j-hui added c++ interop
j-hui j-hui added c++ to swift

Login to write a write a comment.

Login via GitHub

Assignees
No one assigned
Labels
Milestone