From 737702034caf23aea3a2579794935797db5a1d4c Mon Sep 17 00:00:00 2001 From: Ilya Maykov Date: Fri, 26 Sep 2025 15:25:33 -0700 Subject: [PATCH] codegen: Deduplicate derive traits added by add_derives() parse callback Fixes #3286 --- .../add_derives_callback/header_add_derives.h | 3 + .../add_derives_callback/mod.rs | 106 ++++++++++++++++++ bindgen-tests/tests/parse_callbacks/mod.rs | 1 + bindgen/codegen/mod.rs | 40 ++++++- 4 files changed, 146 insertions(+), 4 deletions(-) create mode 100644 bindgen-tests/tests/parse_callbacks/add_derives_callback/header_add_derives.h create mode 100644 bindgen-tests/tests/parse_callbacks/add_derives_callback/mod.rs diff --git a/bindgen-tests/tests/parse_callbacks/add_derives_callback/header_add_derives.h b/bindgen-tests/tests/parse_callbacks/add_derives_callback/header_add_derives.h new file mode 100644 index 0000000000..8cc64a8806 --- /dev/null +++ b/bindgen-tests/tests/parse_callbacks/add_derives_callback/header_add_derives.h @@ -0,0 +1,3 @@ +struct SimpleStruct { + int a; +}; diff --git a/bindgen-tests/tests/parse_callbacks/add_derives_callback/mod.rs b/bindgen-tests/tests/parse_callbacks/add_derives_callback/mod.rs new file mode 100644 index 0000000000..50e9cbd814 --- /dev/null +++ b/bindgen-tests/tests/parse_callbacks/add_derives_callback/mod.rs @@ -0,0 +1,106 @@ +#[cfg(test)] +mod tests { + use bindgen::callbacks::{DeriveInfo, ParseCallbacks}; + use bindgen::{Bindings, Builder}; + use std::path::{Path, PathBuf}; + + #[derive(Debug)] + struct AddDerivesCallback(Vec); + + impl AddDerivesCallback { + fn new(derives: &[&str]) -> Self { + Self(derives.iter().map(|s| (*s).to_string()).collect()) + } + } + + impl ParseCallbacks for AddDerivesCallback { + fn add_derives(&self, _info: &DeriveInfo<'_>) -> Vec { + self.0.clone() + } + } + + struct WriteAdapter<'a>(&'a mut Vec); + + impl std::io::Write for WriteAdapter<'_> { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.0.extend_from_slice(buf); + Ok(buf.len()) + } + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + } + + fn write_bindings_to_string(bindings: &Bindings) -> String { + let mut output = Vec::::new(); + bindings + .write(Box::new(WriteAdapter(&mut output))) + .unwrap_or_else(|e| { + panic!("Failed to write generated bindings: {e}") + }); + String::from_utf8(output).unwrap_or_else(|e| { + panic!("Failed to convert generated bindings to string: {e}") + }) + } + + fn make_builder(header_path: &Path, add_derives: &[&str]) -> Builder { + Builder::default() + .header(header_path.display().to_string()) + .derive_debug(true) + .derive_copy(false) + .derive_default(false) + .derive_partialeq(false) + .derive_eq(false) + .derive_partialord(false) + .derive_ord(false) + .derive_hash(false) + .parse_callbacks(Box::new(AddDerivesCallback::new(add_derives))) + } + + /// Tests that adding a derive trait that's already derived automatically + /// does not result in a duplicate derive trait (which would not compile). + #[test] + fn test_add_derives_callback_dedupe() { + let crate_dir = + PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap()); + let header_path = crate_dir.join( + "tests/parse_callbacks/add_derives_callback/header_add_derives.h", + ); + + let builder = make_builder(&header_path, &["Debug"]); + let bindings = builder + .generate() + .unwrap_or_else(|e| panic!("Failed to generate bindings: {e}")); + let output = write_bindings_to_string(&bindings); + let output_without_spaces = output.replace(' ', ""); + assert!( + output_without_spaces.contains("#[derive(Debug)]") && + !output_without_spaces.contains("#[derive(Debug,Debug)]"), + "Unexpected bindgen output:\n{}", + output.as_str() + ); + } + + /// Tests that adding a derive trait that's not already derived automatically + /// adds it to the end of the derive list. + #[test] + fn test_add_derives_callback() { + let crate_dir = + PathBuf::from(std::env::var("CARGO_MANIFEST_DIR").unwrap()); + let header_path = crate_dir.join( + "tests/parse_callbacks/add_derives_callback/header_add_derives.h", + ); + + let builder = make_builder(&header_path, &["Default"]); + let bindings = builder + .generate() + .unwrap_or_else(|e| panic!("Failed to generate bindings: {e}")); + let output = write_bindings_to_string(&bindings); + let output_without_spaces = output.replace(' ', ""); + assert!( + output_without_spaces.contains("#[derive(Debug,Default)]"), + "Unexpected bindgen output:\n{}", + output.as_str() + ); + } +} diff --git a/bindgen-tests/tests/parse_callbacks/mod.rs b/bindgen-tests/tests/parse_callbacks/mod.rs index 5fe8d90d4c..02d7fe8316 100644 --- a/bindgen-tests/tests/parse_callbacks/mod.rs +++ b/bindgen-tests/tests/parse_callbacks/mod.rs @@ -1,3 +1,4 @@ +mod add_derives_callback; mod item_discovery_callback; use bindgen::callbacks::*; diff --git a/bindgen/codegen/mod.rs b/bindgen/codegen/mod.rs index 1dc108f62a..458e3f4cf5 100644 --- a/bindgen/codegen/mod.rs +++ b/bindgen/codegen/mod.rs @@ -199,6 +199,24 @@ fn derives_of_item( derivable_traits } +/// Appends the contents of the `custom_derives` iterator to the `derives` vector, +/// ignoring duplicates and preserving order. +fn append_custom_derives<'a, I>(derives: &mut Vec<&'a str>, custom_derives: I) +where + I: Iterator, +{ + // Use a HashSet to track already seen elements. + let mut seen: HashSet<&'a str> = Default::default(); + seen.extend(derives.iter().copied()); + + // Add the custom derives to the derives vector, ignoring duplicates. + for custom_derive in custom_derives { + if seen.insert(custom_derive) { + derives.push(custom_derive); + } + } +} + impl From for Vec<&'static str> { fn from(derivable_traits: DerivableTraits) -> Vec<&'static str> { [ @@ -1043,8 +1061,12 @@ impl CodeGenerator for Type { }) }); // In most cases this will be a no-op, since custom_derives will be empty. - derives - .extend(custom_derives.iter().map(|s| s.as_str())); + if !custom_derives.is_empty() { + append_custom_derives( + &mut derives, + custom_derives.iter().map(|s| s.as_str()), + ); + } attributes.push(attributes::derives(&derives)); let custom_attributes = @@ -2475,7 +2497,12 @@ impl CodeGenerator for CompInfo { }) }); // In most cases this will be a no-op, since custom_derives will be empty. - derives.extend(custom_derives.iter().map(|s| s.as_str())); + if !custom_derives.is_empty() { + append_custom_derives( + &mut derives, + custom_derives.iter().map(|s| s.as_str()), + ); + } if !derives.is_empty() { attributes.push(attributes::derives(&derives)); @@ -3678,7 +3705,12 @@ impl CodeGenerator for Enum { }) }); // In most cases this will be a no-op, since custom_derives will be empty. - derives.extend(custom_derives.iter().map(|s| s.as_str())); + if !custom_derives.is_empty() { + append_custom_derives( + &mut derives, + custom_derives.iter().map(|s| s.as_str()), + ); + } attrs.extend( item.annotations()