transpile: const macros: do not resolve expr type

[fw-immunant]

• Fixes Missing casts in const macro translation #853.

Still needs tests.

[kkysen]

Oh, I didn't realize you were working on this, too (I was as well). It's okay, though; the fix looks simple. As for the tests, I have some already in #1304.

[kkysen]

I don't think this works. It doesn't fix test_silk_int16_MIN, the test I added for the buggy case from #853.

[fw-immunant]

After cherry-picking:
d002704
6e4ebd4
498f130
f8d0e9e

onto this branch and running c2rust-transpile --overwrite-existing --translate-const-macros c2rust-transpile/tests/snapshots/macros.c I see that test translated as follows:

pub const silk_int16_MIN: std::ffi::c_short = 0x8000 as std::ffi::c_int
    as std::ffi::c_short;
#[no_mangle]
pub unsafe extern "C" fn test_silk_int16_MIN() -> std::ffi::c_int {
    let mut _null: std::ffi::c_char = (*::core::mem::transmute::<
        &[u8; 1],
        &[std::ffi::c_char; 1],
    >(b"\0"))[(silk_int16_MIN as std::ffi::c_int + 0x8000 as std::ffi::c_int) as usize];
    return silk_int16_MIN as std::ffi::c_int;
}

That looks sane to me (the const is -32768). Am I missing something?

[kkysen]

Hmm, that is correct. I just wasn't seeing that in the snapshot. If you rebase onto that branch and check in the updated snapshot, it should be all good. It just wasn't working for me for some reason.

[kkysen]

@fw-immunant, can I rebase this?

[kkysen]

What's the difference between this and the above

                let ty = self.ast_context[id]
                    .kind
                    .get_type()
                    .ok_or_else(|| format_err!("Invalid expression type"))?;

?

[fw-immunant]

Oh, good catch--there really isn't one. I guess this can be dropped; all we needed to do was remove the unnecessary resolve_expr_type_id.

[kkysen]

@fw-immunant, I'm not sure this is the right approach. It makes the translation much worse (see the snapshot diff).

[fw-immunant]

Here's my reasoning around this change:

resolve_expr_type_id is documented to "Resolve true expression type, iterating through any casts and variable references.". From what I can tell, it does this correctly, though one has to consider any outer casts not to be real parts of the expression. So for an expression like (double)(char)500 it will give back the expression and type of 500.

We call this in recreate_const_macro_from_expansions, presumably to have a better chance of looking at not the surrounding casts in context but instead at the type of the macro expansion itself. But the expression pointed at by this is not necessarily going to have the same value as the original one; 500 != (char)500. So it isn't valid to rewrite all uses of a macro with the expression that resolve_expr_type_id gives back.

If we avoid calling resolve_expr_type_id, we stick with an expression that has the correct value, but it may have a type unduly influenced by implicit casts in its context, e.g. what we see in the changed snapshot where the true macro is translated as a double due to its usage in a floating-point arithmetic expression in MIXED_ARITHMETIC LITERAL_INT.

I'll look into whether there's some way to determine which casts are caused by the expansion context as opposed to being part of the macro itself, as the former as the only ones we should strip off to generate the translation for the macro body.

[kkysen]

I'll look into whether there's some way to determine which casts are caused by the expansion context as opposed to being part of the macro itself, as the former as the only ones we should strip off to generate the translation for the macro body.

If we're able to do that, that sounds like it would be good. Pulling in all the contextual casts not written in the macro itself, though, like this does, just seems like it results in a much worse translation, although I understand the reasoning for it.