From 7b9567394e2ba7d76341746db36133da31e3b682 Mon Sep 17 00:00:00 2001 From: Christian Poveda Ruiz <31802960+pvdrz@users.noreply.github.com> Date: Tue, 3 Oct 2023 16:06:41 -0500 Subject: [PATCH] Let clap handle all CLI parsing errors (#2656) This PR removes some ad-hoc error propagation that the CLI parser had for more sophisticated arguments and just uses `clap::Error` for everything. --- bindgen-cli/options.rs | 168 ++++++++++++++++++++++------------------- 1 file changed, 92 insertions(+), 76 deletions(-) diff --git a/bindgen-cli/options.rs b/bindgen-cli/options.rs index b6ff059da2..d52ccb8e89 100644 --- a/bindgen-cli/options.rs +++ b/bindgen-cli/options.rs @@ -1,13 +1,14 @@ use bindgen::callbacks::TypeKind; use bindgen::{ - builder, AliasVariation, Builder, CodegenConfig, EnumVariation, + builder, Abi, AliasVariation, Builder, CodegenConfig, EnumVariation, FieldVisibilityKind, Formatter, MacroTypeVariation, NonCopyUnionStyle, RegexSet, RustTarget, DEFAULT_ANON_FIELDS_PREFIX, RUST_TARGET_STRINGS, }; +use clap::error::{Error, ErrorKind}; use clap::{CommandFactory, Parser}; use std::fs::File; -use std::io::{self, Error, ErrorKind}; -use std::path::PathBuf; +use std::io; +use std::path::{Path, PathBuf}; use std::process::exit; fn rust_target_help() -> String { @@ -18,7 +19,9 @@ fn rust_target_help() -> String { ) } -fn parse_codegen_config(what_to_generate: &str) -> io::Result { +fn parse_codegen_config( + what_to_generate: &str, +) -> Result { let mut config = CodegenConfig::empty(); for what in what_to_generate.split(',') { match what { @@ -29,9 +32,9 @@ fn parse_codegen_config(what_to_generate: &str) -> io::Result { "constructors" => config.insert(CodegenConfig::CONSTRUCTORS), "destructors" => config.insert(CodegenConfig::DESTRUCTORS), otherwise => { - return Err(Error::new( - ErrorKind::Other, - format!("Unknown generate item: {}", otherwise), + return Err(Error::raw( + ErrorKind::InvalidValue, + format!("Unknown codegen item kind: {}", otherwise), )); } } @@ -40,20 +43,64 @@ fn parse_codegen_config(what_to_generate: &str) -> io::Result { Ok(config) } +fn parse_rustfmt_config_path(path_str: &str) -> Result { + let path = Path::new(path_str); + + if !path.is_absolute() { + return Err(Error::raw( + ErrorKind::InvalidValue, + "--rustfmt-configuration-file needs to be an absolute path!", + )); + } + + if path.to_str().is_none() { + return Err(Error::raw( + ErrorKind::InvalidUtf8, + "--rustfmt-configuration-file contains non-valid UTF8 characters.", + )); + } + + Ok(path.to_path_buf()) +} + +fn parse_abi_override(abi_override: &str) -> Result<(Abi, String), Error> { + let (regex, abi_str) = abi_override + .rsplit_once('=') + .ok_or_else(|| Error::raw(ErrorKind::InvalidValue, "Missing `=`"))?; + + let abi = abi_str + .parse() + .map_err(|err| Error::raw(ErrorKind::InvalidValue, err))?; + + Ok((abi, regex.to_owned())) +} + +fn parse_custom_derive( + custom_derive: &str, +) -> Result<(Vec, String), Error> { + let (regex, derives) = custom_derive + .rsplit_once('=') + .ok_or_else(|| Error::raw(ErrorKind::InvalidValue, "Missing `=`"))?; + + let derives = derives.split(',').map(|s| s.to_owned()).collect(); + + Ok((derives, regex.to_owned())) +} + #[derive(Parser, Debug)] #[clap( about = "Generates Rust bindings from C/C++ headers.", - override_usage = "bindgen [FLAGS] [OPTIONS] [HEADER] -- [CLANG_ARGS]...", + override_usage = "bindgen
-- ...", trailing_var_arg = true )] struct BindgenCommand { /// C or C++ header file. - header: Option, + header: String, /// Path to write depfile to. #[arg(long)] depfile: Option, - /// The default style of code used to generate enums. - #[arg(long, value_name = "VARIANT")] + /// The default STYLE of code used to generate enums. + #[arg(long, value_name = "STYLE")] default_enum_style: Option, /// Mark any enum whose name matches REGEX as a set of bitfield flags. #[arg(long, value_name = "REGEX")] @@ -73,11 +120,11 @@ struct BindgenCommand { /// Mark any enum whose name matches REGEX as a module of constants. #[arg(long, value_name = "REGEX")] constified_enum_module: Vec, - /// The default signed/unsigned type for C macro constants. - #[arg(long, value_name = "VARIANT")] + /// The default signed/unsigned TYPE for C macro constants. + #[arg(long, value_name = "TYPE")] default_macro_constant_type: Option, - /// The default style of code used to generate typedefs. - #[arg(long, value_name = "VARIANT")] + /// The default STYLE of code used to generate typedefs. + #[arg(long, value_name = "STYLE")] default_alias_style: Option, /// Mark any typedef alias whose name matches REGEX to use normal type aliasing. #[arg(long, value_name = "REGEX")] @@ -88,7 +135,7 @@ struct BindgenCommand { /// Mark any typedef alias whose name matches REGEX to have a new type with Deref and DerefMut to the inner type. #[arg(long, value_name = "REGEX")] new_type_alias_deref: Vec, - /// The default style of code used to generate unions with non-Copy members. Note that ManuallyDrop was first stabilized in Rust 1.20.0. + /// The default STYLE of code used to generate unions with non-Copy members. Note that ManuallyDrop was first stabilized in Rust 1.20.0. #[arg(long, value_name = "STYLE")] default_non_copy_union_style: Option, /// Mark any union whose name matches REGEX and who has a non-Copy member to use a bindgen-generated wrapper for fields. @@ -169,10 +216,10 @@ struct BindgenCommand { /// Output bindings for builtin definitions, e.g. __builtin_va_list. #[arg(long)] builtins: bool, - /// Use the given prefix before raw types instead of ::std::os::raw. + /// Use the given PREFIX before raw types instead of ::std::os::raw. #[arg(long, value_name = "PREFIX")] ctypes_prefix: Option, - /// Use the given prefix for anonymous fields. + /// Use the given PREFIX for anonymous fields. #[arg(long, default_value = DEFAULT_ANON_FIELDS_PREFIX, value_name = "PREFIX")] anon_fields_prefix: String, /// Time the different bindgen phases and print to stderr @@ -184,7 +231,7 @@ struct BindgenCommand { /// Output our internal IR for debugging purposes. #[arg(long)] emit_ir: bool, - /// Dump graphviz dot file. + /// Dump a graphviz dot file to PATH. #[arg(long, value_name = "PATH")] emit_ir_graphviz: Option, /// Enable support for C++ namespaces. @@ -232,8 +279,8 @@ struct BindgenCommand { /// Add a raw line of Rust code at the beginning of output. #[arg(long)] raw_line: Vec, - /// Add a raw line of Rust code to a given module. - #[arg(long, number_of_values = 2, value_names = ["MODULE-NAME", "RAW-LINE"])] + /// Add a RAW_LINE of Rust code to a given module with name MODULE_NAME. + #[arg(long, number_of_values = 2, value_names = ["MODULE_NAME", "RAW_LINE"])] module_raw_line: Vec, #[arg(long, help = rust_target_help())] rust_target: Option, @@ -277,16 +324,16 @@ struct BindgenCommand { /// `--formatter=none` instead. #[arg(long)] no_rustfmt_bindings: bool, - /// Which tool should be used to format the bindings + /// Which FORMATTER should be used for the bindings #[arg( long, value_name = "FORMATTER", conflicts_with = "no_rustfmt_bindings" )] formatter: Option, - /// The absolute path to the rustfmt configuration file. The configuration file will be used for formatting the bindings. This parameter sets `formatter` to `rustfmt`. - #[arg(long, value_name = "PATH", conflicts_with = "no_rustfmt_bindings")] - rustfmt_configuration_file: Option, + /// The absolute PATH to the rustfmt configuration file. The configuration file will be used for formatting the bindings. This parameter sets `formatter` to `rustfmt`. + #[arg(long, value_name = "PATH", conflicts_with = "no_rustfmt_bindings", value_parser=parse_rustfmt_config_path)] + rustfmt_configuration_file: Option, /// Avoid deriving PartialEq for types matching REGEX. #[arg(long, value_name = "REGEX")] no_partialeq: Vec, @@ -311,10 +358,10 @@ struct BindgenCommand { /// Use `*const [T; size]` instead of `*const T` for C arrays #[arg(long)] use_array_pointers_in_arguments: bool, - /// The name to be used in a #[link(wasm_import_module = ...)] statement + /// The NAME to be used in a #[link(wasm_import_module = ...)] statement #[arg(long, value_name = "NAME")] wasm_import_module_name: Option, - /// Use dynamic loading mode with the given library name. + /// Use dynamic loading mode with the given library NAME. #[arg(long, value_name = "NAME")] dynamic_loading: Option, /// Require successful linkage to all functions in the library. @@ -344,36 +391,36 @@ struct BindgenCommand { /// Deduplicates extern blocks. #[arg(long)] merge_extern_blocks: bool, - /// Overrides the ABI of functions matching REGEX. The OVERRIDE value must be of the shape REGEX=ABI where ABI can be one of C, stdcall, efiapi, fastcall, thiscall, aapcs, win64 or C-unwind. - #[arg(long, value_name = "OVERRIDE")] - override_abi: Vec, + /// Overrides the ABI of functions matching REGEX. The OVERRIDE value must be of the shape REGEX=ABI where ABI can be one of C, stdcall, efiapi, fastcall, thiscall, aapcs, win64 or C-unwind<.> + #[arg(long, value_name = "OVERRIDE", value_parser = parse_abi_override)] + override_abi: Vec<(Abi, String)>, /// Wrap unsafe operations in unsafe blocks. #[arg(long)] wrap_unsafe_ops: bool, /// Derive custom traits on any kind of type. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros. - #[arg(long, value_name = "CUSTOM")] - with_derive_custom: Vec, + #[arg(long, value_name = "CUSTOM", value_parser = parse_custom_derive)] + with_derive_custom: Vec<(Vec, String)>, /// Derive custom traits on a `struct`. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros. - #[arg(long, value_name = "CUSTOM")] - with_derive_custom_struct: Vec, + #[arg(long, value_name = "CUSTOM", value_parser = parse_custom_derive)] + with_derive_custom_struct: Vec<(Vec, String)>, /// Derive custom traits on an `enum. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros. - #[arg(long, value_name = "CUSTOM")] - with_derive_custom_enum: Vec, + #[arg(long, value_name = "CUSTOM", value_parser = parse_custom_derive)] + with_derive_custom_enum: Vec<(Vec, String)>, /// Derive custom traits on a `union`. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros. - #[arg(long, value_name = "CUSTOM")] - with_derive_custom_union: Vec, + #[arg(long, value_name = "CUSTOM", value_parser = parse_custom_derive)] + with_derive_custom_union: Vec<(Vec, String)>, /// Generate wrappers for `static` and `static inline` functions. #[arg(long, requires = "experimental")] wrap_static_fns: bool, - /// Sets the path for the source file that must be created due to the presence of `static` and + /// Sets the PATH for the source file that must be created due to the presence of `static` and /// `static inline` functions. #[arg(long, requires = "experimental", value_name = "PATH")] wrap_static_fns_path: Option, - /// Sets the suffix added to the extern wrapper functions generated for `static` and `static + /// Sets the SUFFIX added to the extern wrapper functions generated for `static` and `static /// inline` functions. #[arg(long, requires = "experimental", value_name = "SUFFIX")] wrap_static_fns_suffix: Option, - /// Set the default visibility of fields, including bitfields and accessor methods for + /// Set the default VISIBILITY of fields, including bitfields and accessor methods for /// bitfields. This flag is ignored if the `--respect-cxx-access-specs` flag is used. #[arg(long, value_name = "VISIBILITY")] default_visibility: Option, @@ -542,11 +589,7 @@ where let mut builder = builder(); - if let Some(header) = header { - builder = builder.header(header); - } else { - return Err(Error::new(ErrorKind::Other, "Header not found")); - } + builder = builder.header(header); if let Some(rust_target) = rust_target { builder = builder.rust_target(rust_target); @@ -874,23 +917,7 @@ where builder = builder.formatter(formatter); } - if let Some(path_str) = rustfmt_configuration_file { - let path = PathBuf::from(path_str); - - if !path.is_absolute() { - return Err(Error::new( - ErrorKind::Other, - "--rustfmt-configuration-file needs to be an absolute path!", - )); - } - - if path.to_str().is_none() { - return Err(Error::new( - ErrorKind::Other, - "--rustfmt-configuration-file contains non-valid UTF8 characters.", - )); - } - + if let Some(path) = rustfmt_configuration_file { builder = builder.rustfmt_configuration_file(Some(path)); } @@ -976,13 +1003,7 @@ where builder = builder.merge_extern_blocks(true); } - for abi_override in override_abi { - let (regex, abi_str) = abi_override - .rsplit_once('=') - .expect("Invalid ABI override: Missing `=`"); - let abi = abi_str - .parse() - .unwrap_or_else(|err| panic!("Invalid ABI override: {}", err)); + for (abi, regex) in override_abi { builder = builder.override_abi(abi, regex); } @@ -1052,12 +1073,7 @@ where ), ] { let name = emit_diagnostics.then_some(name); - for custom_derive in custom_derives { - let (regex, derives) = custom_derive - .rsplit_once('=') - .expect("Invalid custom derive argument: Missing `=`"); - let derives = derives.split(',').map(|s| s.to_owned()).collect(); - + for (derives, regex) in custom_derives { let mut regex_set = RegexSet::new(); regex_set.insert(regex); regex_set.build_with_diagnostics(false, name);