IRTypeCheck reports error for inlined constructor
shiqicao opened this issue · 6 comments
MethodInlinePass inlines init for new-instance, so it moves invoke-direct {p0}, Ljava/lang/Object;-><init>()V to caller. However, IRTypeChecker reports
Variable NEW_INSTANCE Lcom/foo/bar/;initialized with the
wrong type at [0x7f71d521be90] OPCODE: INVOKE_DIRECT v5, Ljava/lang/Object;.<init>:()V in
It seems inliner conflicts IRTypeChecker.
That of course should not happen.
The inliner will indeed inline certain calls, but only when it safe to do so. It performs the checks here https://github.com/facebook/redex/blob/main/service/method-inliner/Inliner.cpp#L403 and here https://github.com/facebook/redex/blob/main/service/method-inliner/Inliner.cpp#L1597
More information about how to reproduce the issue, or the circumstances (is the caller a constructor? Does it contain other related new-instance instructions? is the offending instruction reachable in the CFG?), would be needed...
Inlinee
.class public final Lcom/Bar;
.super ...;
.method public constructor <init>()V
.locals 0
invoke-direct {p0}, Ljava/lang/Object;-><init>()V
return-void
.end method
.end class
Caller(before MethodInlinePass)
.method public static bar ()V;
.locals 3
new-instance v2, Lcom/Bar;
invoke-direct {v2}, Lcom/Bar;-><init>()V
...
.end method
Caller(after MethodInlinePass)
.method public static bar ()V;
.locals 3
new-instance v2, Lcom/Bar;
invoke-direct {v2}, Ljava/lang/Object;-><init>()V
...
.end method
Is the following logic(from inline_callees correct? Should !can_inline_init(...) be can_inline_init(...)?
if (!can_inline_init(callee)) {
if (!method::is_init(caller) ||
caller->get_class() != callee->get_class() ||
!caller->get_code()->editable_cfg_built() ||
!constructor_analysis::can_inline_inits_in_same_class(
caller, callee, insn)) {
return editable_cfg_adapter::LOOP_CONTINUE;
}
}
Should the above be the following?
if (can_inline_init(callee)) {
if (!method::is_init(caller) ||
caller->get_class() != callee->get_class() ||
!caller->get_code()->editable_cfg_built() ||
!constructor_analysis::can_inline_inits_in_same_class(
caller, callee, insn)) {
return editable_cfg_adapter::LOOP_CONTINUE;
}
} else {
return editable_cfg_adapter::LOOP_CONTINUE;
}
"can_inline_init" isn't the best name for what it checks; more fitting might be "can_always_inline_init". The comment here explains what it checks:
Even when an init cannot be inlined in all possible contexts, then there's still a chance that it might be inlinable in a more specific case, namely into another constructor of the same class. That's what's being checked if the more general "can_inline(_always)init" doesn't succeed.
For your example, both kinds of checks should fail (and the inlining shouldn't happen).
This was recently overhauled in Redex, adding support for relaxed constructor inlining. Feel free to re-open if still is still causing issues.