Redex: R8, TypeChecker & no_overwrite_this

[danzimm]

Hi friends 👋

I've been working to upgrade our usage of redex into the newest stable version. As I was upgrading I noticed that r8 is outputting some bytecode that the newer type checker doesn't like. Specifically the type checker fails its "no overwrite this" check in certain methods that r8 deems as "dead". See more details (and an example) over on issue 141054269.

TL;DR, though, it looks like if there's an abstract superclass with a single implementor who has a method which is overridden r8 will insert something of the form throw null, but additionally clobbering the this register:

const p0, 0x0
throw p0

I was wondering a few things:

• Is there an optimization pass we're missing or have misconfigured which will eliminate this code pattern?
• Orthogonally, it looks like the TypeCheckerConfig's check_no_overwrite_this value isn't used anywhere. Would you guys accept a patch upstream which pipes that through to type checker invocations? I see 2 places we can pipe it:
  
  
  
  • When running the verifier between passes (we hardcode false here, so using the default false is probably ok?)
  
  
  • When running the verifier after all optimizations have ran. This one is a little tricker: should the TypeCheckerConfig override the API version check? It seems like generally no, as we don't want verifier issues, but also "sure why not, if you're messing with the config you're taking your own life into your hands"
  
  

Cheers

CC @benjaminRomano

[agampe]

Is there an optimization pass we're missing or have misconfigured which will eliminate this code pattern?

Have you tried ResolveRefs followed by RemoveUnreachable? Maybe that can eliminate it.

@amnn worked on related but distinct optimizations for uninstantiable classes. @thezhangwei is working on type optimizations. Maybe there's something in the making there.

(The RemoveUninstantiable work at some point would do the same as R8, and a comment came up that if we cannot remove the method, we should just make it abstract to save the bytecode overhead. If you can't find an optimization, maybe you want to contribute a pass :-) )

When running the verifier between passes (we hardcode false here, so using the default false is probably ok?)

I think that was explicitly made because Redex optimizations might not keep the invariant until some fix-up point.

When running the verifier after all optimizations have ran.

I think that might have just been an oversight or some refactorings. @Feng23, probably OK to take this, right?