Skip to content

Commit 07dcf9f

Browse files
committed
fix unnecessary trait bounds for serde-only fields
1 parent 5e0fbf6 commit 07dcf9f

File tree

3 files changed

+167
-0
lines changed

3 files changed

+167
-0
lines changed

conf_derive/src/proc_macro_options/field_item/parameter_item.rs

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -410,6 +410,14 @@ impl ParameterItem {
410410
self.serde.is_some() && !self.get_serde_skip()
411411
}
412412

413+
/// Returns true if this field has a CLI or env source (not just serde)
414+
pub fn has_cli_or_env_source(&self) -> bool {
415+
self.short_switch.is_some()
416+
|| self.long_switch.is_some()
417+
|| self.env_name.is_some()
418+
|| self.is_positional
419+
}
420+
413421
pub fn gen_push_program_options(
414422
&self,
415423
program_options_ident: &Ident,
@@ -631,6 +639,58 @@ impl ParameterItem {
631639
&self,
632640
conf_context_ident: &Ident,
633641
) -> Result<(TokenStream, bool), syn::Error> {
642+
// If there's no CLI or env source, this parameter wasn't registered with clap,
643+
// so we handle it specially
644+
if !self.has_cli_or_env_source() {
645+
// Check for default value first, before checking if optional
646+
// This ensures that optional fields with defaults get Some(default) not None
647+
if let Some(default_value) = &self.default_value {
648+
// Serde-only field with default: use gen_initializer_helper with callbacks
649+
// that provide the default value instead of reading from conf_context
650+
let default_value_str = default_value.value();
651+
652+
let if_no_conf_context_val = |req: ExprRequest| -> TokenStream {
653+
match req {
654+
ExprRequest::Str => quote! {
655+
(::conf::ConfValueSource::Default, #default_value_str)
656+
},
657+
ExprRequest::OsStr => quote! {
658+
(::conf::ConfValueSource::Default, ::std::ffi::OsStr::new(#default_value_str))
659+
},
660+
}
661+
};
662+
663+
return self.gen_initializer_helper(conf_context_ident, &if_no_conf_context_val, None);
664+
} else if self.is_optional_type.is_some() {
665+
// Serde-only optional field without default: return None
666+
return Ok((
667+
quote! {
668+
{
669+
let _ = #conf_context_ident;
670+
Ok(None)
671+
}
672+
},
673+
false,
674+
));
675+
} else {
676+
// Serde-only required field with no default: this is an error
677+
// This field can only be satisfied from a serde document, and it's not optional,
678+
// so if we're here it means either no document was provided or the field was missing
679+
let id = self.field_name.to_string();
680+
return Ok((
681+
quote! {
682+
{
683+
// Get the program option for this field to use in the error
684+
let opt = #conf_context_ident.get_program_option_by_id(#id)
685+
.expect("internal error: program option should exist for this field");
686+
Err(#conf_context_ident.missing_required_parameter_error(opt))
687+
}
688+
},
689+
false,
690+
));
691+
}
692+
}
693+
634694
// Default behavior when no conf context value is found
635695
let if_no_conf_context_val = |_req: ExprRequest| {
636696
if self.is_optional_type.is_some() {
@@ -656,6 +716,25 @@ impl ParameterItem {
656716
doc_name: &Ident,
657717
doc_val: &Ident,
658718
) -> Result<(TokenStream, bool), Error> {
719+
// If there's no CLI or env source, this parameter wasn't registered with clap,
720+
// so we just return the doc value directly
721+
if !self.has_cli_or_env_source() {
722+
let id = self.field_name.to_string();
723+
return Ok((
724+
quote! {
725+
{
726+
let _ = #conf_context_ident;
727+
#conf_context_ident.log_config_event(
728+
#id,
729+
::conf::ConfValueSource::Document(#doc_name)
730+
);
731+
Ok(#doc_val)
732+
}
733+
},
734+
false,
735+
));
736+
}
737+
659738
let use_value_parser = self
660739
.serde
661740
.as_ref()

src/conf_context.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,13 @@ impl<'a> ConfContext<'a> {
107107
}
108108
}
109109

110+
/// Get a program option by its ID (will prepend the context's id_prefix)
111+
#[doc(hidden)]
112+
pub fn get_program_option_by_id(&self, id: &str) -> Option<&ProgramOption> {
113+
let full_id = self.id_prefix.clone() + id;
114+
self.args.id_to_option().get(full_id.as_str()).copied()
115+
}
116+
110117
fn get_env_os(&self, env_name: &'a str) -> Option<&'a OsStr> {
111118
self.env
112119
.get(env_name)

tests/test_serde_only_vec.rs

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,81 @@
1+
//! Test that serde-only Vec fields don't require FromStr implementation
2+
//!
3+
//! This test documents a bug where serde-only repeat fields (Vec<T>)
4+
//! require FromStr even though they will never be parsed from CLI/env.
5+
6+
#[cfg(feature = "serde")]
7+
#[test]
8+
fn test_serde_only_vec_should_not_require_fromstr() {
9+
use conf::Conf;
10+
use serde_json::json;
11+
12+
#[derive(Conf, Debug)]
13+
#[conf(serde)]
14+
pub struct Config {
15+
#[arg(long)]
16+
pub name: String,
17+
18+
// This is serde-only - no long, short, or env
19+
// It should NOT require FromStr since it will never be parsed from CLI/env
20+
#[arg(serde)]
21+
pub tags: Vec<String>,
22+
}
23+
24+
let result = Config::conf_builder()
25+
.args([".", "--name=test"])
26+
.env::<&str, &str>([])
27+
.doc("config.json", json!({"tags": ["a", "b", "c"]}))
28+
.try_parse()
29+
.unwrap();
30+
31+
assert_eq!(result.name, "test");
32+
assert_eq!(result.tags, vec!["a", "b", "c"]);
33+
}
34+
35+
#[cfg(feature = "serde")]
36+
#[test]
37+
fn test_serde_only_optional_with_default() {
38+
use conf::Conf;
39+
use serde_json::json;
40+
41+
#[derive(Conf, Debug)]
42+
#[conf(serde)]
43+
pub struct Config {
44+
#[arg(long)]
45+
pub name: String,
46+
47+
// Serde-only optional field with default value
48+
// When serde doesn't provide the value, should use the default (Some(5))
49+
#[arg(serde, default_value = "5")]
50+
pub count: Option<u32>,
51+
52+
// Serde-only optional field without default
53+
// When serde doesn't provide the value, should be None
54+
#[arg(serde)]
55+
pub optional_field: Option<String>,
56+
}
57+
58+
// Test with serde providing the value
59+
let result = Config::conf_builder()
60+
.args([".", "--name=test"])
61+
.env::<&str, &str>([])
62+
.doc("config.json", json!({"count": 10}))
63+
.try_parse()
64+
.unwrap();
65+
66+
assert_eq!(result.name, "test");
67+
assert_eq!(result.count, Some(10));
68+
assert_eq!(result.optional_field, None);
69+
70+
// Test without serde providing the value - should use default
71+
let result = Config::conf_builder()
72+
.args([".", "--name=test"])
73+
.env::<&str, &str>([])
74+
.doc("config.json", json!({}))
75+
.try_parse()
76+
.unwrap();
77+
78+
assert_eq!(result.name, "test");
79+
assert_eq!(result.count, Some(5)); // Should be Some(5), not None
80+
assert_eq!(result.optional_field, None);
81+
}

0 commit comments

Comments
 (0)