Repository: rust-lang/rust-bindgen
Pull request #2809 (2024-04-17 23:55:05)

Remove which dependency
The which dependency was introduced alongside the ability for bindgen to call rustfmt: https://github.com/rust-lang/rust-bindgen/pull/905

Theres been some dancing around the need for this dependency, with a feature added to disable it back in 2019 https://github.com/rust-lang/rust-bindgen/pull/1615

But I dont believe its actually required at all and in this PR I propose that we can just remove it entirely.

My understanding is that `which` will check the PATH for any binaries matching the passed name.
However if we were to just attempt to execute that name directly the OS would perform the exact same operation, checking the PATH for any binaries matching the name.
So we should just directly execute `rustfmt`, unless its overridden by the `RUSTFMT` env var.

If my understanding is incorrect, lets instead document why which is needed.

pvdrz: I agree with you, I don't see why we need `which` considering that `Command` already does the whole `$PATH` resolution itself. It could be just there for legacy reasons but there could be some reason why people still need it so I'm pinging @emilio in case he knows why.

Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 4fa9dbcf..dcf6240a 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -40,7 +40,6 @@ dependencies = [
  "rustc-hash",
  "shlex",
  "syn 2.0.18",
- "which",
 ]
 
 [[package]]
@@ -661,17 +660,6 @@ version = "0.11.0+wasi-snapshot-preview1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "9c8d87e72b64a3b4db28d11ce29237c246188f4f51057d65a7eab63b7987e423"
 
-[[package]]
-name = "which"
-version = "4.4.0"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2441c784c52b289a054b7201fc93253e288f094e2f4be9058343127c4226a269"
-dependencies = [
- "either",
- "libc",
- "once_cell",
-]
-
 [[package]]
 name = "winapi"
 version = "0.3.9"
diff --git a/bindgen-cli/Cargo.toml b/bindgen-cli/Cargo.toml
index 24066c4c..40b4b4cb 100644
--- a/bindgen-cli/Cargo.toml
+++ b/bindgen-cli/Cargo.toml
@@ -28,12 +28,10 @@ log = { version = "0.4", optional = true }
 shlex = "1"
 
 [features]
-default = ["logging", "runtime", "which-rustfmt"]
+default = ["logging", "runtime"]
 logging = ["bindgen/logging", "dep:env_logger", "dep:log"]
 static = ["bindgen/static"]
 runtime = ["bindgen/runtime"]
-# Dynamically discover a `rustfmt` binary using the `which` crate
-which-rustfmt = ["bindgen/which-rustfmt"]
 prettyplease = ["bindgen/prettyplease"]
 
 ## The following features are for internal use and they shouldn't be used if
diff --git a/bindgen/Cargo.toml b/bindgen/Cargo.toml
index 95293daf..4e3ad81b 100644
--- a/bindgen/Cargo.toml
+++ b/bindgen/Cargo.toml
@@ -40,15 +40,14 @@ regex = { version = "1.5.3", default-features = false, features = ["std", "unico
 rustc-hash = "1.0.1"
 shlex = "1"
 syn = { version = "2.0", features = ["full", "extra-traits", "visit-mut"] }
-which = { version = "4.2.1", optional = true, default-features = false }
 
 [features]
-default = ["logging", "prettyplease", "runtime", "which-rustfmt"]
+default = ["logging", "prettyplease", "runtime"]
 logging = ["dep:log"]
 static = ["clang-sys/static"]
 runtime = ["clang-sys/runtime"]
-# Dynamically discover a `rustfmt` binary using the `which` crate
-which-rustfmt = ["dep:which"]
+# This feature is no longer used for anything and should be removed in bindgen 0.70
+which-rustfmt = []
 experimental = ["dep:annotate-snippets"]
 
 ## The following features are for internal use and they shouldn't be used if
diff --git a/bindgen/codegen/dyngen.rs b/bindgen/codegen/dyngen.rs
index b26c0b18..4b2749ec 100644
--- a/bindgen/codegen/dyngen.rs
+++ b/bindgen/codegen/dyngen.rs
@@ -1,7 +1,7 @@
 use crate::codegen;
 use crate::ir::context::BindgenContext;
 use crate::ir::function::ClangAbi;
-use proc_macro2::Ident;
+use proc_macro2::{Ident, TokenStream};
 
 /// Used to build the output tokens for dynamic bindings.
 #[derive(Default)]
@@ -122,7 +122,7 @@ impl DynamicItems {
     }
 
     #[allow(clippy::too_many_arguments)]
-    pub(crate) fn push(
+    pub(crate) fn push_func(
         &mut self,
         ident: Ident,
         abi: ClangAbi,
@@ -196,4 +196,47 @@ impl DynamicItems {
             #ident
         });
     }
+
+    pub fn push_var(
+        &mut self,
+        ident: Ident,
+        ty: TokenStream,
+        is_required: bool,
+    ) {
+        let member = if is_required {
+            quote! { *mut #ty }
+        } else {
+            quote! { Result<*mut #ty, ::libloading::Error> }
+        };
+
+        self.struct_members.push(quote! {
+            pub #ident: #member,
+        });
+
+        let deref = if is_required {
+            quote! { self.#ident }
+        } else {
+            quote! { *self.#ident.as_ref().expect("Expected variable, got error.") }
+        };
+        self.struct_implementation.push(quote! {
+            pub unsafe fn #ident (&self) -> *mut #ty {
+                #deref
+            }
+        });
+
+        let ident_str = codegen::helpers::ast_ty::cstr_expr(ident.to_string());
+        self.constructor_inits.push(if is_required {
+            quote! {
+                let #ident = __library.get::<*mut #ty>(#ident_str).map(|sym| *sym)?;
+            }
+        } else {
+            quote! {
+                let #ident = __library.get::<*mut #ty>(#ident_str).map(|sym| *sym);
+            }
+        });
+
+        self.init_fields.push(quote! {
+            #ident
+        });
+    }
 }
diff --git a/bindgen/codegen/mod.rs b/bindgen/codegen/mod.rs
index 070d9dec..0770275d 100644
--- a/bindgen/codegen/mod.rs
+++ b/bindgen/codegen/mod.rs
@@ -53,7 +53,7 @@ use crate::ir::ty::{Type, TypeKind};
 use crate::ir::var::Var;
 
 use proc_macro2::{Ident, Span};
-use quote::TokenStreamExt;
+use quote::{ToTokens, TokenStreamExt};
 
 use crate::{Entry, HashMap, HashSet};
 use std::borrow::Cow;
@@ -799,7 +799,17 @@ impl CodeGenerator for Var {
                 }
             );
 
-            result.push(tokens);
+            if ctx.options().dynamic_library_name.is_some() {
+                result.dynamic_items().push_var(
+                    canonical_ident,
+                    self.ty()
+                        .to_rust_ty_or_opaque(ctx, &())
+                        .into_token_stream(),
+                    ctx.options().dynamic_link_require_all,
+                );
+            } else {
+                result.push(tokens);
+            }
         }
     }
 }
@@ -4576,7 +4586,7 @@ impl CodeGenerator for Function {
             let args_identifiers =
                 utils::fnsig_argument_identifiers(ctx, signature);
             let ret_ty = utils::fnsig_return_ty(ctx, signature);
-            result.dynamic_items().push(
+            result.dynamic_items().push_func(
                 ident,
                 abi,
                 signature.is_variadic(),
diff --git a/bindgen/ir/comp.rs b/bindgen/ir/comp.rs
index f6c9e629..13a8184f 100644
--- a/bindgen/ir/comp.rs
+++ b/bindgen/ir/comp.rs
@@ -1449,7 +1449,7 @@ impl CompInfo {
                 }
                 CXCursor_TemplateTypeParameter => {
                     let param = Item::type_param(None, cur, ctx).expect(
-                        "Item::type_param should't fail when pointing \
+                        "Item::type_param shouldn't fail when pointing \
                          at a TemplateTypeParameter",
                     );
                     ci.template_params.push(param);
diff --git a/bindgen/lib.rs b/bindgen/lib.rs
index c2df8a8a..84dc1111 100644
--- a/bindgen/lib.rs
+++ b/bindgen/lib.rs
@@ -983,14 +983,6 @@ impl Bindings {
         if let Ok(rustfmt) = env::var("RUSTFMT") {
             return Ok(Cow::Owned(rustfmt.into()));
         }
-        #[cfg(feature = "which-rustfmt")]
-        match which::which("rustfmt") {
-            Ok(p) => Ok(Cow::Owned(p)),
-            Err(e) => {
-                Err(io::Error::new(io::ErrorKind::Other, format!("{}", e)))
-            }
-        }
-        #[cfg(not(feature = "which-rustfmt"))]
         // No rustfmt binary was specified, so assume that the binary is called
         // "rustfmt" and that it is in the user's PATH.
         Ok(Cow::Owned("rustfmt".into()))


Test patch:
diff --git a/bindgen-tests/Cargo.toml b/bindgen-tests/Cargo.toml
index 1c06c3f1..a253b349 100644
--- a/bindgen-tests/Cargo.toml
+++ b/bindgen-tests/Cargo.toml
@@ -19,7 +19,6 @@ owo-colors = "3.5.0"
 logging = ["bindgen/logging"]
 static = ["bindgen/static"]
 runtime = ["bindgen/runtime"]
-which-rustfmt = ["bindgen/which-rustfmt"]
 
 __testing_only_extra_assertions = ["bindgen/__testing_only_extra_assertions"]
 __testing_only_libclang_9 = ["bindgen/__testing_only_libclang_9"]
diff --git a/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_required.rs b/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_required.rs
new file mode 100644
index 00000000..a96efbe5
--- /dev/null
+++ b/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_required.rs
@@ -0,0 +1,32 @@
+#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
+pub struct TestLib {
+    __library: ::libloading::Library,
+    pub foo: *mut ::std::os::raw::c_int,
+    pub baz: *mut *mut ::std::os::raw::c_int,
+}
+impl TestLib {
+    pub unsafe fn new<P>(path: P) -> Result<Self, ::libloading::Error>
+    where
+        P: AsRef<::std::ffi::OsStr>,
+    {
+        let library = ::libloading::Library::new(path)?;
+        Self::from_library(library)
+    }
+    pub unsafe fn from_library<L>(library: L) -> Result<Self, ::libloading::Error>
+    where
+        L: Into<::libloading::Library>,
+    {
+        let __library = library.into();
+        let foo = __library.get::<*mut ::std::os::raw::c_int>(b"foo\0").map(|sym| *sym)?;
+        let baz = __library
+            .get::<*mut *mut ::std::os::raw::c_int>(b"baz\0")
+            .map(|sym| *sym)?;
+        Ok(TestLib { __library, foo, baz })
+    }
+    pub unsafe fn foo(&self) -> *mut ::std::os::raw::c_int {
+        self.foo
+    }
+    pub unsafe fn baz(&self) -> *mut *mut ::std::os::raw::c_int {
+        self.baz
+    }
+}
diff --git a/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_simple.rs b/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_simple.rs
new file mode 100644
index 00000000..ced70dbd
--- /dev/null
+++ b/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_simple.rs
@@ -0,0 +1,32 @@
+#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
+pub struct TestLib {
+    __library: ::libloading::Library,
+    pub foo: Result<*mut ::std::os::raw::c_int, ::libloading::Error>,
+    pub baz: Result<*mut *mut ::std::os::raw::c_int, ::libloading::Error>,
+}
+impl TestLib {
+    pub unsafe fn new<P>(path: P) -> Result<Self, ::libloading::Error>
+    where
+        P: AsRef<::std::ffi::OsStr>,
+    {
+        let library = ::libloading::Library::new(path)?;
+        Self::from_library(library)
+    }
+    pub unsafe fn from_library<L>(library: L) -> Result<Self, ::libloading::Error>
+    where
+        L: Into<::libloading::Library>,
+    {
+        let __library = library.into();
+        let foo = __library.get::<*mut ::std::os::raw::c_int>(b"foo\0").map(|sym| *sym);
+        let baz = __library
+            .get::<*mut *mut ::std::os::raw::c_int>(b"baz\0")
+            .map(|sym| *sym);
+        Ok(TestLib { __library, foo, baz })
+    }
+    pub unsafe fn foo(&self) -> *mut ::std::os::raw::c_int {
+        *self.foo.as_ref().expect("Expected variable, got error.")
+    }
+    pub unsafe fn baz(&self) -> *mut *mut ::std::os::raw::c_int {
+        *self.baz.as_ref().expect("Expected variable, got error.")
+    }
+}
diff --git a/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_with_allowlist.rs b/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_with_allowlist.rs
new file mode 100644
index 00000000..3f29e738
--- /dev/null
+++ b/bindgen-tests/tests/expectations/tests/dynamic_loading_variable_with_allowlist.rs
@@ -0,0 +1,30 @@
+#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
+pub struct TestLib {
+    __library: ::libloading::Library,
+    pub foo: Result<*mut ::std::os::raw::c_int, ::libloading::Error>,
+    pub bar: Result<*mut ::std::os::raw::c_int, ::libloading::Error>,
+}
+impl TestLib {
+    pub unsafe fn new<P>(path: P) -> Result<Self, ::libloading::Error>
+    where
+        P: AsRef<::std::ffi::OsStr>,
+    {
+        let library = ::libloading::Library::new(path)?;
+        Self::from_library(library)
+    }
+    pub unsafe fn from_library<L>(library: L) -> Result<Self, ::libloading::Error>
+    where
+        L: Into<::libloading::Library>,
+    {
+        let __library = library.into();
+        let foo = __library.get::<*mut ::std::os::raw::c_int>(b"foo\0").map(|sym| *sym);
+        let bar = __library.get::<*mut ::std::os::raw::c_int>(b"bar\0").map(|sym| *sym);
+        Ok(TestLib { __library, foo, bar })
+    }
+    pub unsafe fn foo(&self) -> *mut ::std::os::raw::c_int {
+        *self.foo.as_ref().expect("Expected variable, got error.")
+    }
+    pub unsafe fn bar(&self) -> *mut ::std::os::raw::c_int {
+        *self.bar.as_ref().expect("Expected variable, got error.")
+    }
+}
diff --git a/bindgen-tests/tests/headers/constructors.hpp b/bindgen-tests/tests/headers/constructors.hpp
index d4174889..2f8d57b0 100644
--- a/bindgen-tests/tests/headers/constructors.hpp
+++ b/bindgen-tests/tests/headers/constructors.hpp
@@ -1,6 +1,6 @@
 
 class TestOverload {
-  // This one shouldnt' be generated.
+  // This one shouldn't be generated.
   TestOverload();
 public:
   TestOverload(int);
diff --git a/bindgen-tests/tests/headers/constructors_1_33.hpp b/bindgen-tests/tests/headers/constructors_1_33.hpp
index 7c6262d4..e275f890 100644
--- a/bindgen-tests/tests/headers/constructors_1_33.hpp
+++ b/bindgen-tests/tests/headers/constructors_1_33.hpp
@@ -1,7 +1,7 @@
 // bindgen-flags: --rust-target 1.33
 
 class TestOverload {
-  // This one shouldnt' be generated.
+  // This one shouldn't be generated.
   TestOverload();
 public:
   /// Calling this should use `mem::unintialized()` and not `MaybeUninit()` as only rust 1.36 includes that.
diff --git a/bindgen-tests/tests/headers/dynamic_loading_variable_required.h b/bindgen-tests/tests/headers/dynamic_loading_variable_required.h
new file mode 100644
index 00000000..8e811bb3
--- /dev/null
+++ b/bindgen-tests/tests/headers/dynamic_loading_variable_required.h
@@ -0,0 +1,4 @@
+// bindgen-flags: --dynamic-loading TestLib --dynamic-link-require-all
+
+int foo;
+int *baz;
\ No newline at end of file
diff --git a/bindgen-tests/tests/headers/dynamic_loading_variable_simple.h b/bindgen-tests/tests/headers/dynamic_loading_variable_simple.h
new file mode 100644
index 00000000..5ecad752
--- /dev/null
+++ b/bindgen-tests/tests/headers/dynamic_loading_variable_simple.h
@@ -0,0 +1,4 @@
+// bindgen-flags: --dynamic-loading TestLib
+
+int foo;
+int *baz;
\ No newline at end of file
diff --git a/bindgen-tests/tests/headers/dynamic_loading_variable_with_allowlist.hpp b/bindgen-tests/tests/headers/dynamic_loading_variable_with_allowlist.hpp
new file mode 100644
index 00000000..5e273030
--- /dev/null
+++ b/bindgen-tests/tests/headers/dynamic_loading_variable_with_allowlist.hpp
@@ -0,0 +1,5 @@
+// bindgen-flags: --dynamic-loading TestLib --allowlist-var foo --allowlist-var bar
+
+int foo;
+int bar;
+int baz; // should not be allowed
\ No newline at end of file