C++ Interop: Add test coverage for access control of static/instance data/function members #6199
Conversation
…data/function members This demonstrates two bugs: 1. It seems like we wrongly treat private static data members the same way we treat protected and allow access to them from within derived classes member functions. 2. We wrongly don't allow calling instance member functions of a base C++ class using a derived class as self (no implicit upcast). This doesn't seem related to access control, but prevents us from testing some access control use cases.
| // CHECK:STDERR: let static_x: i32 = d.static_x; | ||
| // CHECK:STDERR: ^~~~~~~~~~ | ||
| // CHECK:STDERR: | ||
| let static_x: i32 = d.static_x; |
There was a problem hiding this comment.
Does Carbon allow accessing C++ static members like this (as C++ does), or do we need to write static_data?
How about we write static_data here like we did in other tests, and maybe Derived.static_data and Cpp.C.static_data as well?
And we could add another test that just checks the form instance.static_x?
There was a problem hiding this comment.
Replaced to Derived.static_data and Cpp.C.static_data.
I might be missing something because just writing static_data would not find that name here.
| class Derived { | ||
| extend base: Cpp.C; | ||
| fn Foo() -> i32 { | ||
| return static_x; |
There was a problem hiding this comment.
| return static_x; | |
| // TODO: C.static_x is private, so this should fail. | |
| return static_x; |
| // CHECK:STDERR: fail_import_function_member_private.carbon:[[@LINE+4]]:3: error: member name `S` not found in `Cpp` [MemberNameNotFoundInInstScope] | ||
| // CHECK:STDERR: Cpp.S.static_foo(); | ||
| // CHECK:STDERR: ^~~~~ | ||
| // CHECK:STDERR: | ||
| Cpp.S.static_foo(); |
There was a problem hiding this comment.
Looks like this meant to be Cpp.C? Maybe we also want a Derived.static_fn() as mentioned above.
There was a problem hiding this comment.
Yes, sorry about that.
Added.
| // CHECK:STDERR: ^~~~~~~~~~~~~~~~~ | ||
| // CHECK:STDERR: fail_import_function_member_private.carbon: note: declared here [ClassMemberDeclaration] | ||
| // CHECK:STDERR: | ||
| self.instance_foo(); |
There was a problem hiding this comment.
What is this testing that the c->instance_foo() below isn't testing? Both are calling a method on C from within the scope of Derived. AFAICT the only difference is pointer vs value, which isn't something this test seems to need to differentiate?
There was a problem hiding this comment.
c->instance_foo() below isn't calling a method on C from within the scope of Derived, it's calling it from within the scope of the global F, which isn't in the scope of Derived.
I might be missing something here.
| class Derived { | ||
| extend base: Cpp.C; | ||
| fn Foo[self: Self]() -> (i32, i32) { | ||
| return (self.instance_x, static_x); |
There was a problem hiding this comment.
Do we want to also test Derived.static_data and maybe Cpp.C.static_data too?
FWIW we don't need to use a return statement here, which will make things simpler I think.
| int instance_x; | ||
| static int static_x; |
There was a problem hiding this comment.
| int instance_x; | |
| static int static_x; | |
| int instance_data; | |
| static int static_data; |
|
|
||
| class Derived { | ||
| extend base: Cpp.C; | ||
| fn Foo[self: Self]() -> i32 { |
There was a problem hiding this comment.
Could we combine this with F() below? self and d are the same things?
There was a problem hiding this comment.
I've changed the below to Derived and not d when accessing the static variable.
For the instance, if instance_data was protected then it would be behave differently, so I think we want both checks.
| class Derived { | ||
| extend base: Cpp.C; | ||
| fn Foo[self: Self]() { | ||
| static_foo(); |
There was a problem hiding this comment.
Maybe also Derived.static_fn() and Cpp.C.static_fn()?
| auto instance_foo() -> void; | ||
| static auto static_foo() -> void; |
There was a problem hiding this comment.
| auto instance_foo() -> void; | |
| static auto static_foo() -> void; | |
| auto instance_fn() -> void; | |
| static auto static_fn() -> void; |
… the derived class using qualified names.
… class directly outside of the derived class scope.
2 participants
This demonstrates two issues:
Part of #5859.