Skip to content

cleanup AST builder functionality #1350

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 28, 2025
+34 −162
Merged

cleanup AST builder functionality #1350

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f0c777
transpile: cleanup AST builder functionality
bungcip Aug 24, 2025
7a093a4
transpile: refactor `comma_separated` to use `itertools::intersperse`
bungcip Sep 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions c2rust-ast-builder/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,6 @@ keywords.workspace = true
categories.workspace = true

[dependencies]
itertools = "0.10.0"
proc-macro2 = { version = "1.0", features = ["span-locations"]}
syn = { version = "2.0", features = ["full", "extra-traits", "printing", "clone-impls"]}
155 changes: 14 additions & 141 deletions c2rust-ast-builder/src/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use std::str;

use itertools::intersperse;
use proc_macro2::{Literal, Punct, Spacing, Span, TokenStream, TokenTree};
use std::default::Default;
use std::iter::FromIterator;
Expand Down Expand Up @@ -161,6 +162,16 @@ fn punct_box<T, P: Default>(x: Vec<Box<T>>) -> Punctuated<T, P> {
Punctuated::from_iter(x.into_iter().map(|x| *x))
}

fn comma_separated<I, T>(items: I) -> TokenStream
where
I: Iterator<Item = T>,
T: ToTokens + Clone,
{
let items = items.map(|items| items.to_token_stream());
let comma = TokenTree::Punct(Punct::new(',', Spacing::Alone)).into_token_stream();
intersperse(items, comma).collect()
}

pub trait Make<T> {
fn make(self, mk: &Builder) -> T;
}
Expand Down Expand Up @@ -208,28 +219,6 @@ impl<'a> Make<Path> for &'a str {
}
}

impl<'a> Make<Visibility> for &'a str {
fn make(self, mk_: &Builder) -> Visibility {
match self {
"pub" => Visibility::Public(Token![pub](mk_.span)),
"priv" | "" | "inherit" => Visibility::Inherited,
"pub(crate)" => Visibility::Restricted(VisRestricted {
pub_token: Token![pub](mk_.span),
paren_token: token::Paren(mk_.span),
in_token: None,
path: Box::new(mk().path("crate")),
}),
"pub(super)" => Visibility::Restricted(VisRestricted {
pub_token: Token![pub](mk_.span),
paren_token: token::Paren(mk_.span),
in_token: None,
path: Box::new(mk().path("super")),
}),
_ => panic!("unrecognized string for Visibility: {:?}", self),
}
}
}

impl<'a> Make<Abi> for &'a str {
fn make(self, mk: &Builder) -> Abi {
Abi {
Expand All @@ -252,47 +241,6 @@ impl Make<Extern> for Abi {
}
}

impl<'a> Make<Mutability> for &'a str {
fn make(self, _mk: &Builder) -> Mutability {
match self {
"" | "imm" | "immut" | "immutable" => Mutability::Immutable,
"mut" | "mutable" => Mutability::Mutable,
_ => panic!("unrecognized string for Mutability: {:?}", self),
}
}
}

impl<'a> Make<Unsafety> for &'a str {
fn make(self, _mk: &Builder) -> Unsafety {
match self {
"" | "safe" | "normal" => Unsafety::Normal,
"unsafe" => Unsafety::Unsafe,
_ => panic!("unrecognized string for Unsafety: {:?}", self),
}
}
}

impl<'a> Make<Constness> for &'a str {
fn make(self, _mk: &Builder) -> Constness {
match self {
"" | "normal" | "not-const" => Constness::NotConst,
"const" => Constness::Const,
_ => panic!("unrecognized string for Constness: {:?}", self),
}
}
}

impl<'a> Make<UnOp> for &'a str {
fn make(self, _mk: &Builder) -> UnOp {
match self {
"deref" | "*" => UnOp::Deref(Default::default()),
"not" | "!" => UnOp::Not(Default::default()),
"neg" | "-" => UnOp::Neg(Default::default()),
Comment on lines -288 to -290
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure getting rid of all of these is helpful. "!" is pretty easy to read and understand (although things like "not" and the other non-standard spellings for things seem bad). It has the downside of not being as easily searchable since it's not tied to a symbol like UnOp::Not, but it's also easier to read and write in a bunch of ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think is more readable for all unary_expr() usage in transpile to just use mk().deref_expr(), mk().neg_expr(), and mk().not_expr(). I can create follow up PR for the refactor

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the original motivation for this was to isolate the actual types of the syntax library from the code using the builder API as much as possible. This was nice when we changed from using the rustc syntax tree to syn. But I don't think we anticipate any such switch in the future, and I don't think we desperately need to keep this coupling loose anymore. I'm slightly in favor of the not_expr()/etc. refactor you suggest, but I don't think using UnOp enum values in core translator code is verboten at this point.

_ => panic!("unrecognized string for UnOp: {:?}", self),
}
}
}

impl<I: Make<Ident>> Make<Lifetime> for I {
fn make(self, mk: &Builder) -> Lifetime {
Lifetime {
Expand Down Expand Up @@ -330,82 +278,21 @@ impl Make<TokenStream> for Vec<TokenTree> {
}
}

impl Make<TokenStream> for Vec<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

With an iterator-based fn comma_separated, you can simplify this to:

impl Make<TokenStream> for Vec<&str> {
    fn make(self, _mk: &Builder) -> TokenStream {
        comma_separated(self.iter().map(|&s| Ident::new(s, Span::call_site())))
    }
}

impl Make<TokenStream> for Vec<u64> {
    fn make(self, _mk: &Builder) -> TokenStream {
        comma_separated(self.iter().map(|&s| Literal::u64_unsuffixed(s)))
    }
}

impl Make<TokenStream> for Vec<Meta> {
    fn make(self, _mk: &Builder) -> TokenStream {
        comma_separated(self.iter())
    }
}

fn make(self, _mk: &Builder) -> TokenStream {
let mut tokens = vec![];
let mut first = true;

for s in self {
if !first {
tokens.push(TokenTree::Punct(Punct::new(',', Spacing::Alone)));
} else {
first = false;
}

tokens.push(TokenTree::Ident(Ident::new(&s, Span::call_site())));
}

tokens.into_iter().collect::<TokenStream>()
}
}

impl Make<TokenStream> for Vec<&str> {
fn make(self, _mk: &Builder) -> TokenStream {
let mut tokens = vec![];
let mut first = true;

for s in self {
if !first {
tokens.push(TokenTree::Punct(Punct::new(',', Spacing::Alone)));
} else {
first = false;
}

tokens.push(TokenTree::Ident(Ident::new(s, Span::call_site())));
}

tokens.into_iter().collect::<TokenStream>()
comma_separated(self.iter().map(|&s| Ident::new(s, Span::call_site())))
}
}

impl Make<TokenStream> for Vec<u64> {
fn make(self, _mk: &Builder) -> TokenStream {
let mut tokens = vec![];
let mut first = true;

for s in self {
if !first {
tokens.push(TokenTree::Punct(Punct::new(',', Spacing::Alone)));
} else {
first = false;
}

tokens.push(TokenTree::Literal(Literal::u64_unsuffixed(s)));
}

tokens.into_iter().collect::<TokenStream>()
comma_separated(self.iter().map(|&s| Literal::u64_unsuffixed(s)))
}
}

impl Make<TokenStream> for Vec<Meta> {
fn make(self, _mk: &Builder) -> TokenStream {
let mut tokens = TokenStream::new();

let mut first = true;

for meta in self {
if !first {
let tt = TokenTree::Punct(Punct::new(',', Spacing::Alone));

tokens.extend(vec![tt]);
} else {
first = false;
}

meta.to_tokens(&mut tokens);
}

tokens
comma_separated(self.iter())
}
}

Expand All @@ -415,12 +302,6 @@ impl Make<PathArguments> for AngleBracketedGenericArguments {
}
}

impl Make<PathArguments> for ParenthesizedGenericArguments {
fn make(self, _mk: &Builder) -> PathArguments {
PathArguments::Parenthesized(self)
}
}

impl Make<GenericArgument> for Box<Type> {
fn make(self, _mk: &Builder) -> GenericArgument {
GenericArgument::Type(*self)
Expand Down Expand Up @@ -661,14 +542,6 @@ impl Builder {
}
}

pub fn parenthesized_args(self, tys: Vec<Box<Type>>) -> ParenthesizedGenericArguments {
ParenthesizedGenericArguments {
paren_token: token::Paren(self.span),
inputs: punct_box(tys),
output: ReturnType::Default,
}
}

pub fn angle_bracketed_args<A>(self, args: Vec<A>) -> AngleBracketedGenericArguments
where
A: Make<GenericArgument>,
Expand Down
5 changes: 3 additions & 2 deletions c2rust-transpile/src/cfg/structures.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,12 +667,13 @@ impl StructureState {
/// * Negating something of the form `!<expr>` produces `<expr>`
///
fn not(bool_expr: &Expr) -> Box<Expr> {
use syn::UnOp;
Copy link
Contributor

Choose a reason for hiding this comment

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

If this can be hoisted to the top-level, that'd be preferable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we can create a new module, lets call it transform, which contain all helper function that transform a rust AST to another rust AST. so we can put helper function like this not() and unparen() in there. However i prefer doing that in another PR rather than this

match *bool_expr {
Expr::Unary(ExprUnary {
op: syn::UnOp::Not(_),
op: UnOp::Not(_),
ref expr,
..
}) => Box::new(unparen(expr).clone()),
_ => mk().unary_expr("!", Box::new(bool_expr.clone())),
_ => mk().unary_expr(UnOp::Not(Default::default()), Box::new(bool_expr.clone())),
}
}
4 changes: 2 additions & 2 deletions c2rust-transpile/src/translator/literals.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ impl<'c> Translation<'c> {
// Fallback for characters outside of the valid Unicode range
if (val as i32) < 0 {
mk().unary_expr(
"-",
UnOp::Neg(Default::default()),
mk().lit_expr(
mk().int_lit((val as i32).unsigned_abs() as u128, "i32"),
),
Expand Down Expand Up @@ -170,7 +170,7 @@ impl<'c> Translation<'c> {
Ok(WithStmts::new_unsafe_val(transmute_expr(
mk().array_ty(u8_ty, width_lit),
self.convert_type(ty.ctype)?,
mk().unary_expr("*", mk().lit_expr(val)),
mk().unary_expr(UnOp::Deref(Default::default()), mk().lit_expr(val)),
)))
}
}
Expand Down
30 changes: 13 additions & 17 deletions c2rust-transpile/src/translator/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1008,7 +1008,7 @@ fn make_submodule(
items.push(mk().extern_("C").foreign_items(foreign_items));
}

let module_builder = mk().vis("pub");
let module_builder = mk().pub_();
let module_builder = if reorganize_definitions {
let file_path_str = file_path.map_or(mod_name.as_str(), |path| {
path.to_str().expect("Found invalid unicode")
Expand Down Expand Up @@ -1929,16 +1929,15 @@ impl<'c> Translation<'c> {
.expect("Variables should already be renamed");
let ConvertedVariable { ty, mutbl, init: _ } =
self.convert_variable(ctx.static_(), None, typ)?;
// When putting extern statics into submodules, they need to be public to be accessible
let visibility = if self.tcfg.reorganize_definitions {
"pub"
} else {
""
};
let mut extern_item = mk_linkage(true, &new_name, ident)
.span(span)
.set_mutbl(mutbl)
.vis(visibility);
.set_mutbl(mutbl);

// When putting extern statics into submodules, they need to be public to be accessible
if self.tcfg.reorganize_definitions {
extern_item = extern_item.pub_();
};

if has_thread_duration {
extern_item = extern_item.single_attr("thread_local");
}
Expand Down Expand Up @@ -2407,16 +2406,13 @@ impl<'c> Translation<'c> {
))
} else {
// Translating an extern function declaration
let mut mk_ = mk_linkage(true, new_name, name).span(span);

// When putting extern fns into submodules, they need to be public to be accessible
let visibility = if self.tcfg.reorganize_definitions {
"pub"
} else {
""
if self.tcfg.reorganize_definitions {
mk_ = mk_.pub_();
};

let mut mk_ = mk_linkage(true, new_name, name).span(span).vis(visibility);

for attr in attrs {
mk_ = match attr {
c_ast::Attribute::Alias(aliasee) => mk_.str_attr("link_name", aliasee),
Expand Down Expand Up @@ -2703,7 +2699,7 @@ impl<'c> Translation<'c> {

if self.ast_context.is_va_list(typ.ctype) {
// translate `va_list` variables to `VaListImpl`s and omit the initializer.
let pat_mut = mk().set_mutbl("mut").ident_pat(rust_name);
let pat_mut = mk().mutbl().ident_pat(rust_name);
let ty = {
let path = vec!["core", "ffi", "VaListImpl"];
mk().path_ty(mk().abs_path(path))
Expand Down Expand Up @@ -2747,7 +2743,7 @@ impl<'c> Translation<'c> {
zeroed.to_pure_expr()
}
.expect("Expected decl initializer to not have any statements");
let pat_mut = mk().set_mutbl("mut").ident_pat(rust_name.clone());
let pat_mut = mk().mutbl().ident_pat(rust_name.clone());
let local_mut = mk().local(pat_mut, Some(ty.clone()), Some(zeroed));
if has_self_reference {
let assign = mk().assign_expr(mk().ident_expr(rust_name), init);
Expand Down