diff --git a/clap_builder/src/parser/parser.rs b/clap_builder/src/parser/parser.rs index 58269974dc1..2fdf0c1b834 100644 --- a/clap_builder/src/parser/parser.rs +++ b/clap_builder/src/parser/parser.rs @@ -1418,6 +1418,10 @@ impl<'cmd> Parser<'cmd> { debug!("Parser::add_env: Checking arg `{arg}`"); if let Some((_, Some(ref val))) = arg.env { debug!("Parser::add_env: Found an opt with value={val:?}"); + if !ok!(self.should_use_env(arg, val)) { + debug!("Parser::add_env: Skipping `{arg}` because env matches default"); + continue; + } let arg_values = vec![val.to_owned()]; let trailing_idx = None; let _ = ok!(self.react( @@ -1434,6 +1438,40 @@ impl<'cmd> Parser<'cmd> { Ok(()) } + #[cfg(feature = "env")] + fn should_use_env(&self, arg: &Arg, val: &OsString) -> ClapResult { + // Only boolean flags are treated as "not explicit" when env matches the default. + if !matches!(arg.get_action(), ArgAction::SetTrue | ArgAction::SetFalse) { + return Ok(true); + } + + let parsed = ok!(arg.get_value_parser().parse_ref( + self.cmd, + Some(arg), + val.as_os_str(), + ValueSource::EnvVariable, + )); + let Some(flag) = parsed.downcast_ref::() else { + return Ok(true); + }; + + let default_value = if let Some(default) = arg.get_default_values().first() { + ok!(arg.get_value_parser().parse_ref( + self.cmd, + Some(arg), + default, + ValueSource::DefaultValue, + )) + .downcast_ref::() + .copied() + } else { + None + } + .unwrap_or(matches!(arg.get_action(), ArgAction::SetFalse)); + + Ok(*flag != default_value) + } + fn add_defaults(&self, matcher: &mut ArgMatcher) -> ClapResult<()> { debug!("Parser::add_defaults"); @@ -1676,3 +1714,59 @@ pub(crate) enum Identifier { Long, Index, } + +#[cfg(all(test, feature = "env"))] +mod tests { + use super::Parser; + use crate::{Arg, ArgAction, Command}; + + #[test] + fn should_use_env_skips_default_flag_value() { + let mut cmd = Command::new("prog") + .arg( + Arg::new("flag_true") + .long("flag-true") + .action(ArgAction::SetTrue) + .value_parser(crate::value_parser!(bool)), + ) + .arg( + Arg::new("flag_false") + .long("flag-false") + .action(ArgAction::SetFalse) + .value_parser(crate::value_parser!(bool)), + ); + cmd._build_self(false); + + let parser = Parser::new(&mut cmd); + let flag_true = parser + .cmd + .get_arguments() + .find(|arg| arg.get_id() == "flag_true") + .expect("flag_true arg exists"); + let flag_false = parser + .cmd + .get_arguments() + .find(|arg| arg.get_id() == "flag_false") + .expect("flag_false arg exists"); + + let use_env_false = parser + .should_use_env(flag_true, &std::ffi::OsString::from("false")) + .expect("env parse succeeds"); + assert!(!use_env_false); + + let use_env_true = parser + .should_use_env(flag_true, &std::ffi::OsString::from("true")) + .expect("env parse succeeds"); + assert!(use_env_true); + + let use_env_true = parser + .should_use_env(flag_false, &std::ffi::OsString::from("true")) + .expect("env parse succeeds"); + assert!(!use_env_true); + + let use_env_false = parser + .should_use_env(flag_false, &std::ffi::OsString::from("false")) + .expect("env parse succeeds"); + assert!(use_env_false); + } +} diff --git a/tests/builder/env.rs b/tests/builder/env.rs index 11ea87f5151..bab95d4034c 100644 --- a/tests/builder/env.rs +++ b/tests/builder/env.rs @@ -3,7 +3,7 @@ use std::env; use std::ffi::OsStr; -use clap::{arg, builder::FalseyValueParser, Arg, ArgAction, Command}; +use clap::{arg, builder::FalseyValueParser, value_parser, Arg, ArgAction, ArgGroup, Command}; #[test] fn env() { @@ -66,6 +66,51 @@ fn env_bool_literal() { assert!(!*m.get_one::("absent").expect("defaulted by clap")); } +#[test] +fn env_false_flag_is_not_explicit() { + env::set_var("CLP_TEST_MEMORY_STORAGE", "false"); + env::set_var( + "CLP_TEST_POSTGRES_CONNECTION_STRING", + "postgres://user:password@localhost:5432/db", + ); + + let r = Command::new("sample") + .group( + ArgGroup::new("storage") + .args(["memory-storage", "postgres"]) + .multiple(false) + .required(true), + ) + .arg( + arg!(--"memory-storage" "Use an in-memory storage backend") + .value_parser(value_parser!(bool)) + .env("CLP_TEST_MEMORY_STORAGE"), + ) + .arg( + arg!(-p --postgres "A postgreSQL connection string") + .env("CLP_TEST_POSTGRES_CONNECTION_STRING"), + ) + .try_get_matches_from(vec!["sample"]); + + assert!(r.is_ok(), "{}", r.unwrap_err()); + let m = r.unwrap(); + assert!(!m.get_flag("memory-storage")); + assert_eq!( + m.value_source("memory-storage"), + Some(clap::parser::ValueSource::DefaultValue) + ); + assert_eq!( + m.get_one::("postgres") + .map(|v| v.as_str()) + .unwrap(), + "postgres://user:password@localhost:5432/db" + ); + assert_eq!( + m.value_source("postgres"), + Some(clap::parser::ValueSource::EnvVariable) + ); +} + #[test] fn env_os() { env::set_var("CLP_TEST_ENV_OS", "env");