Preserve using-declaration access for inherited methods#995
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #995 +/- ##
==========================================
+ Coverage 86.56% 86.62% +0.05%
==========================================
Files 23 23
Lines 5621 5643 +22
==========================================
+ Hits 4866 4888 +22
Misses 755 755
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
| if (auto* USD = llvm::dyn_cast_or_null<UsingShadowDecl>(D)) { | ||
| if (llvm::isa_and_nonnull<CXXMethodDecl>(USD->getTargetDecl())) | ||
| return USD->getAccess() == AS; | ||
| } |
There was a problem hiding this comment.
Why not using UnwrapUsingShadowToFunction?
There was a problem hiding this comment.
This is explained now in the inline comments.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
FYI @vgvassilev : this fix is not strictly needed for the ROOT migration, as the ROOT pythonizations tests have evolved to not hit this edge case anymore. So reviewing this PR is not a priority. |
Looks quite good already -- can we make sure the codecov reports are addressed with tests -- should be ready to go. |
guitargeek
force-pushed
the
cppyy-220
branch
2 times, most recently
from
5086ccf to
d91da57
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| Ctor.Invoke((void*)&object, {}, /*self=*/nullptr); | ||
| ASSERT_TRUE(object); | ||
|
|
||
| int a = 3, b = 7, result = 0; |
There was a problem hiding this comment.
warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]
| int a = 3, b = 7, result = 0; | |
| int a = 3; | |
| int b = 7; | |
| int result = 0; |
| int a = 3, b = 7, result = 0; | ||
| std::array<void*, 2> args = {(void*)&a, (void*)&b}; | ||
| Call.Invoke(&result, {args.data(), /*args_size=*/2}, object); | ||
| EXPECT_EQ(result, a * 100 + b); |
There was a problem hiding this comment.
warning: '*' has higher precedence than '+'; add parentheses to explicitly specify the order of operations [readability-math-missing-parentheses]
EXPECT_EQ(result, a * 100 + b);
^
When `GetClassDecls<CXXMethodDecl>` walked a class's decls and hit a
`UsingShadowDecl` whose target is a method, it pushed the *target*
`CXXMethodDecl` from the base class. The target's `getAccess()` returns
the access in the base (e.g. `protected`); the elevated access carried
by the using-declaration (`public` in the introducing class) was
silently lost. Consumers like CPyCppyy filter on `IsPublicMethod`, so a
republished overload such as
class Base { protected: void foo(int, int); };
class Derived: public Base {
public:
using Base::foo;
void foo(int);
};
would never reach the Python proxy - calling `derived.foo(1, 2)` from
cppyy raised `TypeError: takes at most 1 arguments (2 given)`
(cppyy issue compiler-research/cppyy#220).
Push the `UsingShadowDecl` itself (for the non-constructor case;
constructor-using-shadows still go through `findInheritingConstructor`)
and teach `CheckMethodAccess` to consult `USD->getAccess()`. A small
`UnwrapUsingShadowToFunction` helper unwraps the shadow at the entry
of every API that downcasts a `TCppFunction_t` to `FunctionDecl`
(return type, arg counts/types/names/defaults, signature,
IsConst/Static/Virtual/Constructor/Destructor/Templated/Explicit,
function address, operator arity).
`MakeFunctionCallable` keeps the unwrapped target as the wrapped
function but threads a `relaxAccessControl` flag into `make_wrapper`
when the caller passed a using-shadow: the generated wrapper still
references the target by its original qualified name (e.g.
`((Base*)obj)->Base::foo(...)`), so access control has to be relaxed
during wrapper compilation for it to compile. The runtime call is
well-defined because the using-declaration made the member reachable
through the derived class.
Adds a `FunctionReflection_GetClassMethods_UsingShadowAccess`
regression test covering both the public-promoted case from the issue
and a protected-using control case (which must stay hidden).
Fixes: compiler-research/cppyy#220
|
I'm just fixing the clang tidy warnings |
|
clang-tidy review says "All clean, LGTM! 👍" |
2 participants
When
GetClassDecls<CXXMethodDecl>walked a class's decls and hit aUsingShadowDeclwhose target is a method, it pushed the targetCXXMethodDeclfrom the base class. The target'sgetAccess()returns the access in the base (e.g.protected); the elevated access carried by the using-declaration (publicin the introducing class) was silently lost. Consumers like CPyCppyy filter onIsPublicMethod, so a republished overload such aswould never reach the Python proxy - calling
derived.foo(1, 2)from cppyy raisedTypeError: takes at most 1 arguments (2 given)(cppyy issue compiler-research/cppyy#220).Push the
UsingShadowDeclitself (for the non-constructor case; constructor-using-shadows still go throughfindInheritingConstructor) and teachCheckMethodAccessto consultUSD->getAccess(). A smallUnwrapUsingShadowToFunctionhelper unwraps the shadow at the entry of every API that downcasts aTCppFunction_ttoFunctionDecl(return type, arg counts/types/names/defaults, signature, IsConst/Static/Virtual/Constructor/Destructor/Templated/Explicit, function address, operator arity).MakeFunctionCallablekeeps the unwrapped target as the wrapped function but threads arelaxAccessControlflag intomake_wrapperwhen the caller passed a using-shadow: the generated wrapper still references the target by its original qualified name (e.g.((Base*)obj)->Base::foo(...)), so access control has to be relaxed during wrapper compilation for it to compile. The runtime call is well-defined because the using-declaration made the member reachable through the derived class.Adds a
FunctionReflection_GetClassMethods_UsingShadowAccessregression test covering both the public-promoted case from the issue and a protected-using control case (which must stay hidden).Fixes: compiler-research/cppyy#220