refactor: compare foreign function declarations by resolved type

[ahomescu]

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

Some relevant output from Claude:

    Regression test: `test_reorganize_foreign_fn_type_identity`
    (`tests/snapshots/reorganize_foreign_fn_type_identity.rs`).
    Each translation unit has its own `buf`, and the two differ in field visibility,
    so the transform correctly keeps them apart and renames the second to `buf_1`.
    Both units then declare `fill(b: *mut buf)` — spelled identically, but naming a
    different type in each unit.
    
    `find_foreign_item` compares the two with `compatible_fn_prototypes`.
    A foreign item has no body and so no typeck results to resolve against,
    and the comparison falls through to a purely syntactic check that sees `*mut buf`
    against `*mut buf` and reports a match. The second declaration is dropped
    and both call sites are pointed at the first, so the caller written against
    `buf_1` now passes it to a function taking `*mut buf`:
    
        error[E0308]: mismatched types

    Comparing foreign functions by resolved type routed the comparison through
    `structural_eq_tys_impl`, whose array arm lets a zero-length array match an
    array of any length. That leniency is there for an `extern` array
    declaration, which C lets omit the length that the definition gives, and
    which we translate to a zero length; it does not hold inside a function
    signature. `*mut [c_int; 0]` and `*mut [c_int; 4]` are distinct Rust types
    with no coercion between them, so two declarations that differ only there
    are not interchangeable at a call site.
    
    `test_reorganize_foreign_fn_rename` regressed on exactly that: its two
    `compute` declarations were collapsed into one and `b_call` was rewritten
    to call a function it cannot pass its argument to.
    
    `TypeCompare` now carries an `exact_array_lens` flag, off by default so
    that the `extern` array declaration it exists for keeps matching its
    definition, and on for `compatible_fn_sigs` and `compatible_fn_prototypes`.
    An array length reached through a signature, including one nested in a
    pointed-to struct, has to match exactly.

    `find_foreign_item` decided whether two foreign functions were the same by
    handing their `FnDecl`s to `compatible_fn_prototypes`. That resolves each
    parameter through `opt_node_type`, but a foreign item has no body and so no
    typeck results. Two declarations from different modules whose parameters were
    merely spelled alike were reported identical, one was dropped, and its
    callers were rewritten to a function taking a different type.
    
    `compatible_foreign_fns` now compares function signatures with `compatible_fn_sigs`.
    `match_exports` already compared foreign functions against external ones
    this way; this brings the crate-local comparison in line with it. The
    syntactic path is kept as a fallback for a signature that will not resolve,
    which is also why the arity check added earlier still matters.
    
    Structurally identical types from different modules still compare equal, so
    this only separates declarations that genuinely differ.

[fw-immunant]

The commit message for compare foreign function declarations by resolved type is exhaustingly long, but doesn't answer questions like:

• when will the signature fail to resolve, resulting in syntactic comparison?
• what are the specific E0308/E0603 errors referenced?

[fw-immunant]

I think this is functionally reasonable, modulo various comments. That said, the newly-added test does appear to be failing. I'd also prefer if commit messages generated by an LLM were demarcated as quotation (if included at all) rather than appearing like a message from the committer.

[fw-immunant]

A doc comment explaining the purpose of this type would be good to have, as the name TypeCompare is not entirely clear about how the data stored here relates to comparing types--is it a pass over source code which compares types, guidance for which types to compare, a specification of what to compare about types, the contextual information required to compare types, etc.

In practice it seems like it's the latter, and now being augmented with the penultimate.

[ahomescu]

That seems independent of the current change, TypeCompare is a really old type. I'll make a separate PR for that.

[ahomescu]

Re this line: I'm not sure why we needed the new derive, so I deleted it.

Update: I found out why we needed it, it's because with_exact_array_lens takes a reference but produces a new object.

[fw-immunant]

Is there any way we could avoid translating incomplete array types to zero-length arrays, e.g. by translating to a pointer to a DST or such? Our test coverage of what we currently do here seems pretty minimal (e.g. our c2rust-transpile/tests/snapshots/incomplete_arrays.c snapshot generates no code), but in practice I don't see how Rust code using elements from a zero-length array (even if its actual definition is foreign) can be sound.

[ahomescu]

We might need to do different things for each of the following cases:

• extern array, e.g. extern int foo[];. I'm not sure we can use a DST here, since a pointer or reference to it becomes a fat pointer.
• Function argument: void foo(int arg[]);. This is an array-to-pointer decay, so replacing it with int *arg might be fine.
• Flexible array member at the end of a structure. I think this is the only one that could be a DST, but we would have to detect it.

I'm not sure the transpiler handles these cases differently, I'll have to go check.

[ahomescu]

I need to investigate what happened here with all the rebasing, because the test and the fix don't seem to match anymore.

[ahomescu]

when will the signature fail to resolve, resulting in syntactic comparison?

There are now more details in the comment that cover this case.

what are the specific E0308/E0603 errors referenced?

Is that relevant? Anyway, E0308 is Expected type did not match the received type. and E0603 is A private item was used outside its scope. but I think the latter is gone now.

[ahomescu]

This is a fix for a pre-existing array issue that is now exposed by the more accurate signature comparison. I'm starting to think it should be its own separate PR, but in either case it needs to lands first. Thoughts?