Re-enable --reorganize-definitions flag

c2rust-transpile/src/lib.rs

@@ -464,8 +464,43 @@ fn get_extra_args_macos() -> Vec<String> {
     args
 }
 
-fn invoke_refactor(_build_dir: &Path) -> Result<(), Error> {
-    Ok(())
+fn invoke_refactor(build_dir: &Path) -> Result<(), Error> {
+    // Make sure the crate builds cleanly
+    let status = process::Command::new("cargo")
+        .args(&["check"])
+        .env("RUSTFLAGS", "-Awarnings")
+        .current_dir(build_dir)
+        .status()?;
+    if !status.success() {
+        return Err(failure::format_err!("Crate does not compile."));
+    }
+
+    // Assumes the subcommand executable is in the same directory as this program.
+    let cmd_path = std::env::current_exe().expect("Cannot get current executable path");
+    let mut cmd_path = cmd_path.as_path().canonicalize().unwrap();
+    cmd_path.pop(); // remove current executable
+    cmd_path.push(format!("c2rust-refactor"));
+    assert!(cmd_path.exists(), "{:?} is missing", cmd_path);
+    let args = [
+        "--cargo",
+        "--rewrite-mode",
+        "inplace",
+        "rename_unnamed",
+        ";",
+        "reorganize_definitions",
+    ];
+    let status = process::Command::new(cmd_path.into_os_string())
+        .args(&args)
+        .current_dir(build_dir)
+        .status()?;
+    if status.success() {
+        Ok(())
+    } else {
+        Err(failure::format_err!(
+            "Refactoring failed. Please fix errors above and re-run:\n    c2rust refactor {}",
+            args.join(" "),
+        ))
+    }
 }
 
 fn reorganize_definitions(

c2rust-transpile/src/rust_ast/item_store.rs

@@ -78,6 +78,24 @@ impl PathedMultiImports {
             })
             .collect()
     }
+
+    /// Remove all imports covered by the other `PathedMultiImports`.
+    pub fn remove(&mut self, other: &PathedMultiImports) {
+        for (k, v) in &mut self.0 {
+            // We don't consider attributes, just subtract leaf sets.
+            let other_items = other
+                .0
+                .get(k)
+                .map(|imports| {
+                    imports
+                        .leaves
+                        .iter()
+                        .collect::<std::collections::HashSet<_>>()
+                })
+                .unwrap_or_default();
+            v.leaves.retain(|leaf| !other_items.contains(leaf));
+        }
+    }
 }
 
 #[derive(Debug, Default)]

c2rust-transpile/src/translator/literals.rs

@@ -334,6 +334,10 @@ impl<'c> Translation<'c> {
                     .borrow()
                     .resolve_decl_name(union_id)
                     .unwrap();
+                if let Some(cur_file) = self.cur_file.get() {
+                    log::debug!("in file {cur_file} importing union {union_name}, id {union_id:?}");
+                    self.add_import(cur_file, union_id, &union_name);
+                }
                 match self.ast_context.index(union_field_id).kind {
                     CDeclKind::Field { typ: field_ty, .. } => {
                         let val = if ids.is_empty() {

c2rust-transpile/src/translator/mod.rs

@@ -18,7 +18,7 @@ use syn::{
     ForeignItemStatic, ForeignItemType, Ident, Item, ItemConst, ItemEnum, ItemExternCrate, ItemFn,
     ItemForeignMod, ItemImpl, ItemMacro, ItemMod, ItemStatic, ItemStruct, ItemTrait,
     ItemTraitAlias, ItemType, ItemUnion, ItemUse, Lit, MacroDelimiter, PathSegment, ReturnType,
-    Stmt, Type, TypeTuple, Visibility,
+    Stmt, Type, TypeTuple, UseTree, Visibility,
 };
 use syn::{BinOp, UnOp}; // To override `c_ast::{BinOp,UnOp}` from glob import.
 
@@ -630,7 +630,7 @@ pub fn translate(
         }
 
         {
-            let convert_type = |decl_id: CDeclId, decl: &CDecl| {
+            let export_type = |decl_id: CDeclId, decl: &CDecl| {
                 let decl_file_id = t.ast_context.file_id(decl);
                 if t.tcfg.reorganize_definitions {
                     t.cur_file.set(decl_file_id);
@@ -664,6 +664,14 @@ pub fn translate(
                 if t.tcfg.reorganize_definitions
                     && decl_file_id.map_or(false, |id| id != t.main_file)
                 {
+                    let name: Option<&String> = t
+                        .ast_context
+                        .get_decl(&decl_id)
+                        .and_then(|x| x.kind.get_name());
+                    log::debug!(
+                        "emitting submodule imports for exported type {}",
+                        name.unwrap_or(&"unknown".to_owned())
+                    );
                     t.generate_submodule_imports(decl_id, decl_file_id);
                 }
             };
@@ -684,7 +692,7 @@ pub fn translate(
                     _ => false,
                 };
                 if needs_export {
-                    convert_type(decl_id, decl);
+                    export_type(decl_id, decl);
                 }
             }
         }
@@ -771,6 +779,7 @@ pub fn translate(
             };
 
             if t.tcfg.reorganize_definitions && decl_file_id.map_or(false, |id| id != t.main_file) {
+                log::debug!("emitting any imports needed by {:?}", decl.kind.get_name());
                 t.generate_submodule_imports(*top_id, decl_file_id);
             }
         }
@@ -870,14 +879,17 @@ pub fn translate(
 
             all_items.extend(mod_items);
 
+            // Before consuming `uses`, remove them from the uses from submodules to avoid duplicates.
+            let (_, _, mut new_uses) = new_uses.drain();
+            new_uses.remove(&uses);
+
             // This could have been merged in with items below; however, it's more idiomatic to have
             // imports near the top of the file than randomly scattered about. Also, there is probably
             // no reason to have comments associated with imports so it doesn't need to go through
             // the above comment store process
             all_items.extend(uses.into_items());
 
             // Print new uses from submodules
-            let (_, _, new_uses) = new_uses.drain();
             all_items.extend(new_uses.into_items());
 
             if !foreign_items.is_empty() {
@@ -907,6 +919,42 @@ pub fn translate(
     }
 }
 
+/// Represent the set of names made visible by a `use`: either a set of specific names, or a glob.
+enum IdentsOrGlob<'a> {
+    Idents(Vec<&'a Ident>),
+    Glob,
+}
+
+impl<'a> IdentsOrGlob<'a> {
+    fn join(self, other: Self) -> Self {
+        use IdentsOrGlob::*;
+        match (self, other) {
+            (Glob, _) => Glob,
+            (_, Glob) => Glob,
+            (Idents(mut own), Idents(other)) => Idents({
+                own.extend(other.into_iter());
+                own
+            }),
+        }
+    }
+}
+
+/// Extract the set of names made visible by a `use`.
+fn use_idents<'a>(i: &'a UseTree) -> IdentsOrGlob<'a> {
+    match i {
+        UseTree::Path(up) => use_idents(&up.tree),
+        UseTree::Name(un) => IdentsOrGlob::Idents(vec![&un.ident]),
+        UseTree::Rename(ur) => IdentsOrGlob::Idents(vec![&ur.rename]),
+        UseTree::Glob(_ugl) => IdentsOrGlob::Glob,
+        UseTree::Group(ugr) => ugr
+            .items
+            .iter()
+            .map(|tree| use_idents(tree))
+            .reduce(IdentsOrGlob::join)
+            .unwrap_or(IdentsOrGlob::Idents(vec![])),
+    }
+}
+
 fn item_ident(i: &Item) -> Option<&Ident> {
     use Item::*;
     Some(match i {
@@ -986,6 +1034,8 @@ fn foreign_item_ident_vis(fi: &ForeignItem) -> Option<(&Ident, Visibility)> {
     })
 }
 
+/// Create a submodule containing the items in `item_store`. Populates `use_item_store` with the set
+/// of `use`s to import the submodule's exports.
 fn make_submodule(
     ast_context: &TypedAstContext,
     item_store: &mut ItemStore,
@@ -1000,34 +1050,67 @@ fn make_submodule(
         .get_file_include_line_number(file_id)
         .unwrap_or(0);
     let mod_name = clean_path(mod_names, file_path);
+    let use_path = || vec!["self".into(), mod_name.clone()];
 
     for item in items.iter() {
         let ident_name = match item_ident(item) {
             Some(i) => i.to_string(),
             None => continue,
         };
-        let use_path = vec!["self".into(), mod_name.clone()];
 
         let vis = match item_vis(item) {
             Some(Visibility::Public(_)) => mk().pub_(),
             Some(_) => mk(),
             None => continue,
         };
 
-        use_item_store.add_use_with_attr(false, use_path, &ident_name, vis);
+        use_item_store.add_use_with_attr(false, use_path(), &ident_name, vis);
     }
 
     for foreign_item in foreign_items.iter() {
         let ident_name = match foreign_item_ident_vis(foreign_item) {
             Some((ident, _vis)) => ident.to_string(),
             None => continue,
         };
-        let use_path = vec!["self".into(), mod_name.clone()];
 
-        use_item_store.add_use(false, use_path, &ident_name);
+        use_item_store.add_use(false, use_path(), &ident_name);
     }
 
+    // Consumers will `use` reexported items at their exported locations.
     for item in uses.into_items() {
+        if let Item::Use(ItemUse {
+            vis: Visibility::Public(_),
+            tree,
+            ..
+        }) = &*item
+        {
+            match use_idents(tree) {
+                IdentsOrGlob::Idents(idents) => {
+                    for ident in idents {
+                        fn is_simd_type_name(name: &str) -> bool {
+                            const SIMD_TYPE_NAMES: &[&str] = &[
+                                "__m128i", "__m128", "__m128d", "__m64", "__m256", "__m256d",
+                                "__m256i",
+                            ];
+                            SIMD_TYPE_NAMES.contains(&name) || name.starts_with("_mm_")
+                        }
+                        let name = &*ident.to_string();
+                        if is_simd_type_name(name) {
+                            // Import vector type/operation names from the stdlib, as we also generate
+                            // other uses for them from that location and can't easily reason about
+                            // the ultimate target of reexported names when avoiding duplicate imports
+                            // (which are verboten).
+                            simd::add_arch_use(use_item_store, "x86", name);
+                            simd::add_arch_use(use_item_store, "x86_64", name);
+                        } else {
+                            // Add a `use` for `self::this_module::exported_name`.
+                            use_item_store.add_use(false, use_path(), name);
+                        }
+                    }
+                }
+                IdentsOrGlob::Glob => {}
+            }
+        }
         items.push(item);
     }
 
@@ -4913,10 +4996,11 @@ impl<'c> Translation<'c> {
             self.import_type(type_id, file_id);
         }
 
+        // Caching skips critical side effect of `import_type` call.
         // Look up the decl in the cache and return what we find (if we find anything)
-        if let Some(init) = self.zero_inits.borrow().get(&decl_id) {
+        /*if let Some(init) = self.zero_inits.borrow().get(&decl_id) {
             return Ok(init.clone());
-        }
+        }*/
 
         let name_decl_id = match self.ast_context.index(type_id).kind {
             CTypeKind::Typedef(decl_id) => decl_id,

c2rust-transpile/src/translator/simd.rs

@@ -67,7 +67,7 @@ static SIMD_X86_64_ONLY: &[&str] = &[
     "_mm_crc32_u64",
 ];
 
-fn add_arch_use(store: &mut ItemStore, arch_name: &str, item_name: &str) {
+pub fn add_arch_use(store: &mut ItemStore, arch_name: &str, item_name: &str) {
     store.add_use_with_attr(
         true,
         vec!["core".into(), "arch".into(), arch_name.into()],

c2rust-transpile/src/translator/structs.rs

@@ -398,6 +398,10 @@ impl<'a> Translation<'a> {
         field_expr_ids: &[CExprId],
     ) -> TranslationResult<WithStmts<Box<Expr>>> {
         let name = self.resolve_decl_inner_name(struct_id);
+        if let Some(cur_file) = self.cur_file.get() {
+            log::debug!("in file {cur_file} importing struct {name}, id {struct_id:?}");
+            self.add_import(cur_file, struct_id, &name);
+        }
 
         let (field_decl_ids, platform_byte_size) = match self.ast_context.index(struct_id).kind {
             CDeclKind::Struct {

c2rust/Cargo.toml

@@ -21,10 +21,12 @@ log = "0.4"
 regex = "1.3"
 shlex = "1.3"
 c2rust-transpile = { version = "0.21.0", path = "../c2rust-transpile" }
+c2rust-refactor = { version = "0.21.0", path = "../c2rust-refactor" }
 
 [build-dependencies]
 c2rust-build-paths = { path = "../c2rust-build-paths", version = "0.21.0" }
 
 [features]
 # Force static linking of LLVM
 llvm-static = ["c2rust-transpile/llvm-static"]
+profile = ["c2rust-refactor/profile"]

c2rust-refactor/src/bin/c2rust-refactor.rs → c2rust/src/bin/c2rust-refactor.rs

@@ -1,5 +1,12 @@
-use clap::{load_yaml, App, ArgMatches};
-use log::info;
+extern crate env_logger;
+#[macro_use]
+extern crate log;
+#[macro_use]
+extern crate clap;
+extern crate c2rust_refactor;
+extern crate shlex;
+
+use clap::{App, ArgMatches};
 use std::fs::File;
 use std::io::Read;
 use std::process;

c2rust/src/bin/c2rust-transpile.rs

@@ -124,9 +124,17 @@ struct Args {
     #[clap(long)]
     reduce_type_annotations: bool,
 
-    /// Output file in such a way that the refactoring tool can deduplicate code
-    #[clap(short = 'r', long)]
-    reorganize_definitions: bool,
+    /// Disable `--reorganize-definitions`
+    #[clap(long = "no-reorganize-definitions", hidden = true, action = clap::ArgAction::SetFalse)]
+    reorganize_definitions: bool, // NB, this *is* the right field; this flag sets it to *false*.
+
+    /// Output file in such a way that the refactoring tool can deduplicate code (enabled by default; disable with `--no-reorganize-definitions`)
+    #[clap(
+        short = 'r',
+        long = "reorganize-definitions",
+        conflicts_with = "reorganize-definitions"
+    )]
+    _no_reorganize_definitions: bool, // Field is unused, but flag mutually excludes the negative sense above.
 
     /// Extra arguments to pass to clang frontend during parsing the input C file
     #[clap(multiple = true, last(true))]

c2rust/src/main.rs

@@ -60,7 +60,7 @@ impl SubCommand {
     /// Get all known [`SubCommand`]s.  These have no [`SubCommand::path`].
     /// Even if the subcommand executables aren't there, we can still suggest them.
     pub fn known() -> impl Iterator<Item = Self> {
-        ["transpile", "instrument", "pdg", "analyze"]
+        ["transpile", "refactor", "instrument", "pdg", "analyze"]
             .into_iter()
             .map(|name| Self {
                 path: None,

tests/items/src/test_fn_attrs.rs

@@ -6,6 +6,12 @@ pub fn test_fn_attrs() {
     // so instead we're checking the source itself
     let src = include_str!("fn_attrs.rs");
 
+    // Remove the c2rust::src_loc annotation, which is only produced if
+    // --reorganize-definitions is enabled.
+    let mut lines: Vec<&str> = src.lines().collect();
+    lines.retain(|x| !x.contains("#[c2rust::src_loc"));
+    let src = lines.join("\n");
+
     // Some C99 rules for convenience:
     // * In C99, a function defined inline will never, and a function defined extern inline
     //   will always, emit an externally visible function.
@@ -57,8 +63,7 @@ pub fn test_fn_attrs() {
 
     if cfg!(not(target_os = "macos")) {
         // aliased_fn is aliased to the inline_extern function
-        assert!(src.contains(
-            "extern \"C\" {\n    #[link_name = \"inline_extern\"]\n    fn aliased_fn();"
-        ));
+        let aliased_fn_syntax = |public| format!("extern \"C\" {{\n    #[link_name = \"inline_extern\"]\n    {}fn aliased_fn();", public);
+        assert!(src.contains(&aliased_fn_syntax("")) || src.contains(&aliased_fn_syntax("pub ")));
     }
 }