-
Notifications
You must be signed in to change notification settings - Fork 1.5k
C++ Interop: Fix access calculation to handle private member of base classes correctly #6238
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.
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
Conversation
Always take into account both the lookup access specifier and the lexical access specifier. When set, lookup access specifier takes precedence. When not set, we have two use cases: 1. This is not a record member, so no access is specified at all. Treat this as public. 2. This is a private record member of a base class. Treat this as private.
// CHECK:STDERR: fail_import_overload_set_public_base_class_call_non_public.carbon:[[@LINE+5]]:3: error: cannot access protected member `foo` of type `Cpp.PublicDerived` [ClassInvalidMemberAccess] | ||
// CHECK:STDERR: Cpp.PublicDerived.foo(1); | ||
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~ | ||
// CHECK:STDERR: fail_import_overload_set_public_base_class_call_non_public.carbon: note: declared here [ClassMemberDeclaration] | ||
// CHECK:STDERR: | ||
Cpp.PublicDerived.foo(1); | ||
// CHECK:STDERR: fail_import_overload_set_public_base_class_call_non_public.carbon:[[@LINE+5]]:3: error: cannot access private member `foo` of type `Cpp.PublicDerived` [ClassInvalidMemberAccess] | ||
// CHECK:STDERR: Cpp.PublicDerived.foo(1, 2); | ||
// CHECK:STDERR: ^~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
// CHECK:STDERR: fail_import_overload_set_public_base_class_call_non_public.carbon: note: declared here [ClassMemberDeclaration] | ||
// CHECK:STDERR: | ||
Cpp.PublicDerived.foo(1, 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where you're doing multiple calls to foo
, do you think it'd be helpful to comment about what's distinct about each call? The name "foo" and the parameters "1" don't carry meaning, and the diagnostic doesn't point out the different overrides, so I worry that outside the context of a PR it'll be hard to understand.
A different approach might be to create types that make it clearer the difference between each overload (e.g., struct PrivateCall {};
) so that you would see it in the signature (e.g. Cpp.PublicDerived.foo(PrivateCall.PrivateCall());
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (took the different approach you suggested).
Thanks!
toolchain/check/cpp/import.cpp
Outdated
case clang::AS_private: | ||
return SemIR::AccessKind::Private; | ||
} | ||
return DeduceClangAccess(iterator.getAccess(), iterator->getAccess()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference between .getAccess
and ->getAccess
seems subtle, perhaps document what's occurring here where you do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer relevant here anymore.
Added a comment in CalculateEffectiveAccess()
.
toolchain/check/cpp/import.cpp
Outdated
} | ||
SemIR::AccessKind access_kind = MapAccess(overload_set.begin()); | ||
for (auto it = overload_set.begin() + 1; | ||
it != overload_set.end() && access_kind != SemIR::AccessKind::Public; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd typically expect a for
loop to focus on comparing the value it's iterating over (it
), with other comparisons inside the loop. What made you choose to do this comparison as part of the for loop parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the old code it made sense to me to avoid a break.
Moved the optimization to an explicit condition with a break.
toolchain/check/cpp/access.h
Outdated
|
||
// Deduces the effective access kind from the given lookup and lexical access | ||
// specifiers. | ||
auto DeduceClangAccess(clang::AccessSpecifier lookup_access, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've generally been saying "Deduce" in the toolchain specifically for generic deduction. Also, skimming through the cpp headers, don't they typically say "Cpp" instead of "Clang"?
Perhaps there's a better term here, because it's just doing a combination of the two parameters. Had you considered something like "ConvertCppAccess"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
toolchain/check/cpp/access.h
Outdated
auto DeduceClangAccess(clang::AccessSpecifier lookup_access, | ||
clang::AccessSpecifier lexical_access) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like both callers could pass in the access specifiers as a DeclAccessPair
. Had you considered taking that here? That might even let you just remove MapAccess
from import.cpp
, since there's it.getPair()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
toolchain/check/cpp/import.cpp
Outdated
SemIR::AccessKind access_kind = MapAccess(overload_set.begin()); | ||
for (auto it = overload_set.begin() + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had you considered initializing access_kind
to Private
, so that you wouldn't need this begin()
offset and could only call MapAccess
from inside the loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
foo(1, 2); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test that does just a little more access control layering, such as:
// --- layered_inheritance_test.carbon | |
library "[[@TEST_NAME]]"; | |
import Cpp inline ''' | |
class C { | |
public: | |
static auto Public() -> void; | |
protected: | |
static auto Protected() -> void; | |
private: | |
static auto Private() -> void; | |
}; | |
class D : private C {}; | |
class E : public D {}; | |
'''; | |
class F { | |
extend base: Cpp.E; | |
fn G[self: Self]() { | |
Self.Public(); | |
Self.Protected(); | |
Self.Private(); | |
} | |
} |
Also, are non-static methods intended to work? If so, perhaps it'd be good to test those too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more coverage that include what you propose here.
Thanks!
toolchain/check/cpp/access.cpp
Outdated
if (lexical_access != clang::AS_none) { | ||
// When a base class private member is accessed through a derived class, the | ||
// lookup access would be set to `AS_none`. | ||
CARBON_CHECK(lexical_access == clang::AS_private); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this CHECK correct? I've suggested a test which I think may hit it.
If it's not correct, perhaps this function could be replaced with something like:
auto DeduceClangAccess(clang::AccessSpecifier lookup_access,
clang::AccessSpecifier lexical_access)
-> SemIR::AccessKind {
// Lookup access takes precedence.
switch (lookup_access != clang::AS_none ? lookup_access : lexical_access) {
case clang::AS_public:
return SemIR::AccessKind::Public;
case clang::AS_protected:
return SemIR::AccessKind::Protected;
case clang::AS_private:
return SemIR::AccessKind::Private;
case clang::AS_none:
// This is not a record member.
return SemIR::AccessKind::Public;
}
}
That also is pretty short, and if you're not expecting many other access functions, perhaps could be inlined into the header to remove access.cpp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, while equivalent [ed: for current tests, the suggested] tests might also suggest this still should have some TODOs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this check was incorrect.
I've added more tests based on your suggestions and adjusted the logic to make it correct for these cases.
@zygoloid Would you be okay finishing review here, once it's updated? |
…e condition with a break.
… like other tests.
…otected` and `private` overloads.
Updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor things, but otherwise LG.
// members it means we lost access along the inheritance path. Otherwise | ||
// it means there's no access associated with this function so we treat it | ||
// as public. | ||
return access_pair->isCXXClassMember() ? clang::AS_private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see some justification here for why it's correct to map "none" to "private". I think what this comes down to is:
The difference between "private" and "none" only matters when the access check is performed within a friend or member of the naming class. Because the naming class is a C++ class, and we don't yet have a mechanism for a C++ class to befriend a Carbon class, we can safely map "none" to "private" for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've included that information in the comment.
Can you clarify or provide a pointer for my understanding and for posterity how should private
and none
be treated in a friend or member of the naming class?
toolchain/check/cpp/access.h
Outdated
|
||
// Calculates the effective access kind from the given (declaration, lookup | ||
// access) pair. | ||
auto ConvertCppAccess(clang::DeclAccessPair access_pair) -> SemIR::AccessKind; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert*
is typically used to name a language-level type conversion function. Maybe MapCppAccess
would work better, following how we name the Map*Type
functions in cpp/import.cpp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Always take into account both the lookup access specifier and the declaration. When set, lookup access specifier takes precedence. When not set, we have two use cases:
Also, deduplicate access mapping between import and overload resolution.
Background: #6221 (comment)
Part of #5859.