transpile: expand --translate-const-macros conservative to a lot more CExprKinds

[kkysen]

• Fixes Problematic macro translation with --translate-const-macros #803.
• Supersedes Prohibit function call macro to const translation #810.

We do this by recursively checking whether an expression is const. This is generally done in a conservative manner, modulo the ExplicitCast bug (#853), non-const sizeof(VLA)s, and f128's non-const methods (#1262). For #853, this does generate incorrect code even on conservative, but I'll work on fixing that next.

Also, because we now allow certain operations that are unsafe, like ptr arithmetic, we wrap all consts in an unsafe block. This is similar to how all fns we generate are marked unsafe fns even if they don't contain any unsafe operations within them. We can improve this, but for now, this works and is correct.

Also, the output was being non-deterministic due to the usage of HashMaps for macro maps like macro_invocations, so I changed these to IndexMaps that are order-preserving.

[fw-immunant]

What variation in output did we see due to nondeterministic iteration? Was it reordered output definitions or something more subtle/messy? Definitely worthwhile to make our output deterministic here, I'm just curious how this manifested.

[kkysen]

What variation in output did we see due to nondeterministic iteration? Was it reordered output definitions or something more subtle/messy? Definitely worthwhile to make our output deterministic here, I'm just curious how this manifested.

It affected the order of macro expansions that we folded over, which ended up meaning sometimes a macro expansion would be replaced by the macro definition, but not always.

[kkysen]

@fw-immunant, note that this does achieve a lot of what #290 does. It's similar to the portable types, stuff. There, we'll now emit a uint32_t type alias instead of a u32 directly, and this will emit a true_0 const instead of a true literal directly. Both could then be refactored pretty easily. I'm not exactly sure how #290 works, but the advantage here is that it matches where the C used the true and false macros, and if C uses 0 and non-zero directly, then it keeps that behavior. And then I'm not sure how #290 handles ints vs _Bools either.

We can also resurrect #295 to use r#true raw identifiers instead if that's helpful.

[fw-immunant]

There definitely is some overlap in spirit but I think it's probably worthwhile to address this from both sides--this PR will clean up some uses of #define symbols, but it seems like we're still mostly translating them as integers.

Do you know what might help us translate LITERAL_BOOL as const LITERAL_BOOL: bool = ...;? It seems like recreate_const_macro_from_expansions might be able to realize that all uses are boolifying it.

[kkysen]

Do you know what might help us translate LITERAL_BOOL as const LITERAL_BOOL: bool = ...;? It seems like recreate_const_macro_from_expansions might be able to realize that all uses are boolifying it.

I'm not sure, but I'll look into it.

[fw-immunant]

One thing I'd like to include here is documentation (just a comment probably) relating the conservative/pessimistic analysis in is_const_expr with the less-conservative (aka unsound) analysis used by --translate-const-macros=experimental: in the latter case, we simply call ctx.set_const(true) and translate the expression. This has the effect of making various checks of (effectively) the form if ctx.is_const() { return error } trigger at various points in translating the expression where we recognize it to be incompatible with const. I don't think we need to merge the is_const_expr approach with this one, as the latter is fail-open (and does so in practice, e.g. in the translation of function call expressions which results in #803). But we should note for readers that there are effectively two implementations of this logic, and if/when we attempt to improve the experimental mode it would be by adding more logic of this form.

[fw-immunant]

First 3 commits look good, but please add the comment/documentation I mention relating is_const_expr to ctx.is_const.

4th commit should have some test coverage before merging.

[kkysen]

4th commit should have some test coverage before merging.

Done in a6ce89e.

[kkysen]

This has the effect of making various checks of (effectively) the form if ctx.is_const() { return error } trigger at various points in translating the expression where we recognize it to be incompatible with const. I don't think we need to merge the is_const_expr approach with this one, as the latter is fail-open (and does so in practice, e.g. in the translation of function call expressions which results in #803).

So you're saying the existing approach of checking if ctx.is_const() is incorrect, right?

[fw-immunant]

This has the effect of making various checks of (effectively) the form if ctx.is_const() { return error } trigger at various points in translating the expression where we recognize it to be incompatible with const. I don't think we need to merge the is_const_expr approach with this one, as the latter is fail-open (and does so in practice, e.g. in the translation of function call expressions which results in #803).

So you're saying the existing approach of checking if ctx.is_const() is incorrect, right?

It's currently known to be incorrect, e.g. it doesn't check function calls for constness, resulting in #803. But it's not fundamentally wrong to implement constness checking that way if we covered every possible case during translation, and for an implementation that wants to cover all cases I think it's probably the right way to implement it--we don't want a separate full AST traversal for each analysis because they would need to stay synchronized with respect to the particular way we translate every idiom so they can reason about the Rust we're generating. For now I think it's fine to have two parallel approaches (a conservative one and an aspirationally precise one) as long as we document what's going on.

[kkysen]

It's currently known to be incorrect, e.g. it doesn't check function calls for constness, resulting in #803. But it's not fundamentally wrong to implement constness checking that way if we covered every possible case during translation, and for an implementation that wants to cover all cases I think it's probably the right way to implement it--we don't want a separate full AST traversal for each analysis because they would need to stay synchronized with respect to the particular way we translate every idiom so they can reason about the Rust we're generating. For now I think it's fine to have two parallel approaches (a conservative one and an aspirationally precise one) as long as we document what's going on.

Fixed in 604c58a.

[fw-immunant]

The comments in the last commit need some fixes (see my inline comments at 604c58a), but that can be addressed in a follow up. These changes look good, but we should avoid making all translations of const macros unsafe now that we're doing them by default. The goal of the conservative const macro translation effort is to generate fully idiomatic code for cases simple enough for us to do so, so we should avoid regressing translation of the simplest cases like #define FOO 5.

So this can merge into kkysen/const-macros-unsafe-block if you like, but I'd rather not merge that branch into master while it adds unsafe to the simplest const macro translations. Merging fw/const-less-unsafe into this branch (or into kkysen/const-macros-unsafe-block after this merges into it) would be my suggested route to that.

The stacked PR workflow is confusing but hopefully the above makes sense.

[kkysen]

These changes look good, but we should avoid making all translations of const macros unsafe now that we're doing them by default. The goal of the conservative const macro translation effort is to generate fully idiomatic code for cases simple enough for us to do so, so we should avoid regressing translation of the simplest cases like #define FOO 5.

This discussion kind of got split over multiple PRs, so I just wanted to link to the other parts here in case you miss it: #1335 (review), #1339 (review).

So this can merge into kkysen/const-macros-unsafe-block if you like, but I'd rather not merge that branch into master while it adds unsafe to the simplest const macro translations. Merging fw/const-less-unsafe into this branch (or into kkysen/const-macros-unsafe-block after this merges into it) would be my suggested route to that.

The stacked PR workflow is confusing but hopefully the above makes sense.

I'm not sure what the point of avoiding a temporary change to master is. We're not publishing a separate version in the meantime. From what I understand, the goal is not to keep things perfect all the time, but to make steady improvements quickly. We also haven't published --translate-const-macros conservative, so I don't see how changing what that emits to be a regression. So I'd much prefer to merge these PRs in order.

[kkysen]

The comments in the last commit need some fixes (see my inline comments at 604c58a), but that can be addressed in a follow up.

Fixed in 81551c0..ecddc95.