Fix closure untupling/retupling

[avanhatt]

Description of changes:

RMC currently fails to verify some nested boxed closures, such as this:

    let f: Box<Box<dyn FnOnce(f32, i32)>> = Box::new(Box::new(|x, y| { assert!(x == 1.0); assert!(y == 2) }));
    f(1.0, 2);

The problem is that we were checking whether to untuple closure arguments based on whether the function's type kind was a Closure, but this misses some cases (where the trait points to a defined function, or with vtable shims) where the type is a FnDef. The right way to check for untupling is via a check to the function's ABI. However, in that case, we also need to retuple arguments based on the MIR's spread_arg flag. See this Zulip for more discussion.

The trickiness here is that we need to insert both the tuple and its individual components into the symbol table, and then write the components out to the tuple at the start of a function. To do this, I added a new naming convention for the untupled args, avoiding naming conflicts based on the local index. Local index math determines what gets retupled when necessary.

Here is an example of the code we now generate (with renaming for clarity):

struct Unit call_once_vtable_shim(struct [boxed_closure.rs:8:49: 11:6] *var_1, float spread_2, int spread_3)
{
  struct (f32, i32) var_2={ .0=spread_2, .1=spread_3 }; // new: retuple since the body requires it
  struct Unit var_0;

bb0:
  ;
  boxed_closure_main(*var_1, var_2.0, var_2.1);

bb1:
  ;
  return var_0;
}

Resolved issues:

Resolves #240

Call-outs:

ignore_var_ty is mystifying, and there are some intrinsic cases where skipping FnDef there elides the environment argument of a call through a tuple. I don't like this, and we should probably eventually remove ignore_var_ty in favor of elides arguments only for specific, documented reasons (like the being Zero Sized Types that rust elides).

Testing:

• How is this change tested?

Existing tests, including a fixme added in my last PR.

• Is this a refactor change?

Somewhat.

Checklist

• Each commit message has a non-empty body, explaining why the change was made
• Methods or procedures are documented
• Regression or unit tests are included, or existing tests cover the modified code
• My PR is restricted to a single feature or bugfix

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.

[adpaco-aws]

Hi @avanhatt , can you please add the example to the description?

[adpaco-aws]

Great work! A few minor comments and questions.

[adpaco-aws]

Why do we need this check?

[avanhatt]

The spread arg is added in the prelude, so we shouldn't re-add it to the table here. But I added a different, more explicit check and comment instead, thanks!

[adpaco-aws]

Do we need to hard-code this?

[avanhatt]

I think so, the name spread is not special, we just need to name it something different from var and I chose this to try and help convey the purpose.

[adpaco-aws]

LGTM! Do you mind updating manually and fixing the typo below?