Skip to content

transpile: Various fixes to type handling for builtins#1860

Open
Rua wants to merge 5 commits into
immunant:masterfrom
Rua:builtin-expected-type
Open

transpile: Various fixes to type handling for builtins#1860
Rua wants to merge 5 commits into
immunant:masterfrom
Rua:builtin-expected-type

Conversation

@Rua

@Rua Rua commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

@Rua Rua force-pushed the builtin-expected-type branch 5 times, most recently from 6ac8aad to 7a907b6 Compare June 18, 2026 12:07
@Rua Rua force-pushed the builtin-expected-type branch 3 times, most recently from 6ec15cf to d7fcb1c Compare July 1, 2026 14:01
@ahomescu ahomescu requested review from Copilot and thedataking and removed request for Copilot July 2, 2026 01:04
@Rua Rua force-pushed the builtin-expected-type branch from d7fcb1c to aaabe91 Compare July 2, 2026 10:00
@Rua Rua marked this pull request as ready for review July 2, 2026 10:01

@fw-immunant fw-immunant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall, but would appreciate a comment where mentioned inline.

Comment thread c2rust-transpile/src/c_ast/mod.rs
@Rua Rua force-pushed the builtin-expected-type branch from aaabe91 to 04f75f7 Compare July 3, 2026 16:10

@fw-immunant fw-immunant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAK on the comment change - now the name of bubble_expr_types and its comment disagree on its primary purpose. It exists to homogeneize types between Clang versions by bubbling up types from child AST nodes to parent ones (and these types are not necessarily ones in PULLBACK_KINDS, e.g. in the case of bit shifts), and the other things it does are unrelated to this function. They should really be done separately, so we can remove the function when we no longer support Clang <16. Translating arbitrary C operations producing one type to Rust ones that producing a different type (based on the operation's identity rather than its children) seems best done in expr translation rather than by altering types here.

We don't necessarily need to change where this logic happens in this PR, but I meant to ask for a comment that documents the tech debt here rather than making it sound like the current confusing arrangement is by design.

@Rua

Rua commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

They should really be done separately, so we can remove the function when we no longer support Clang <16.

Iterating through the whole AST is somewhat expensive, so I figure it's more efficient to fit it all together like this.

Translating arbitrary C operations producing one type to Rust ones that producing a different type (based on the operation's identity rather than its children) seems best done in expr translation rather than by altering types here.

Perhaps, but the difficulty here is that the "bubbling" is done bottom-up while translation operates top-down. For example, if you have an expression like sizeof(Foo) + 1 then you want the override on the sizeof type to propagate to the + expression too. The current code does that, but doing it with a top-down convert_expr call tree is much harder and messier. Though perhaps #1679 could provide a solution there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

C2Rust emits size_t for __builtin_object_size cast to unsigned long, causing Rust type mismatch

2 participants