@subbuss I am confused though...I do not see any TemporaryLocalVariable handling before or after your patch, but obviously the issue in #4251 was exactly why we started initializing all temp locals eagerly to nil. What am I missing?

@subbuss @headius I think our issue in this specific case is the optarg is missing an init when it is an lvar and then we convert it to a temp. So this fixes the problem but I think the underlying issue is we convert non-nil-corrected lvars into temp vars.

Another argument worth asking is should we worry about keeping this assumption that temp vars are always initialized or should we just add this extra logic and cover our asses?

This patch is incomplete. I need to page in this code into my memory. But, looks like I started doing the initialization for all vars (see buildDataflowVars which gets all vars, not just LocalVariables), and at some point decided to test for inits for only LocalVariables.

So, anywhere there is a check for LocalVariable in the code needs an update to not assume lvar. I can follow up if you want, or I can point you to other places to fix.

applyTransferFunction is the only other place it seems .. since tmps don't cross scope boundaries, identifyUndefinedVarsInClosure is correct to look at lvars.

You can reproduce the error with this snippet as well:

def foo(a)
  if (a)
    b = 1
  end
  p b
end

foo 0

You will find that foo will fail to JIT with the same error. This is because we run the the AddMissingInitsPass at the end to catch all possible scenarios of missing inits (which is why the code should work for tmps and lvars). In this case, the OptimizeDynamicVarsPass converts all the lvars to tmps and the tmps will have a missing init instead of the lvar. So, your patch is on the right track. You just need to fix applyTransferFunction as well.

@subbuss I am agreeing it is better to have this work on all variable types now since we have hit this problem more than once; but if we move missing inits higher up the pass chain your snippet will not be broken anymore.

As a point of general guidance should all passes be made to work in any order as a principle do you think? Or should we just get better about enforcing dependencies?

Both.

1. It is more robust to have a pass work anywhere in the chain, and that is how some should work. I really meant to have AddMissingInitsPass work anywhere ... it is just plain oversight since I have been context switching jruby work across long periods of time and I lose attention. So, some of that can be improved by adding tests and specs.
2. Some passes really can work after the IR is in some pre-determined state (ex: CFG has been built, or call protocol instrs haven't been added, code is in SSA form, etc.). So, there are also dependencies that have to be respected and enforced. In most cases, failed dependencies will fail fast. But, better to enforce them.

So, your patch is on the right track. You just need to fix applyTransferFunction as well.

Great! I'll work on that once we're back from Austin.

I added the fix for applyTransferFunction and re-pushed. I'll rebase as well so we can see it with recent greening on master.

You will hate me, but now that I look at the code again, I was wrong. tmp vars don't leak in from outer scopes. Only lvars do! So, applyTransferFunction was correct as is. So, your previous fix was indeed complete. Sorry, but I seem to mix up this "simple" tmp / lvar distinction every once in a while.

@subbuss Oh, so like the closure var walker later in the file, this should only ever see LocalVariable. I'll remove my change and merge, since CI looks pretty good.

Yes.

Can you please add JIT tests (either as part of the PR or in a followup commit)? The example I added earlier in #4252 (comment) should be good enough.

@subbuss This was already failing a couple JIT tests, but I'll add any cases that aren't covered.