IRTypeCheck reports error for inlined constructor

[shiqicao]

trafficstars

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.

[NTillmann]

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...

[shiqicao]

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

[shiqicao]

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;
            }

[NTillmann]

"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: https://github.com/facebook/redex/blob/2c9d897e6060ceeadfce89b1613098733016568f/service/method-inliner/ConstructorAnalysis.h#L24

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.

[NTillmann]

For your example, both kinds of checks should fail (and the inlining shouldn't happen).

[NTillmann]

This was recently overhauled in Redex, adding support for relaxed constructor inlining. Feel free to re-open if still is still causing issues.