diff --git a/ISSUE_23_CURRENT_STATUS.md b/ISSUE_23_CURRENT_STATUS.md new file mode 100644 index 0000000..75def01 --- /dev/null +++ b/ISSUE_23_CURRENT_STATUS.md @@ -0,0 +1,107 @@ +# Issue #23 - Current Status & Analysis + +## Summary + +Issue #23 is more complex than initially described. The actual problem is: + +1. User tries to use `#[gonfig(nested)]` attribute - **which doesn't exist** +2. When darling (attribute parser) encounters unknown attribute, it generates error with unqualified `core::compile_error!()` +3. When user has `use core as tradesmith_core;`, the compiler can't resolve the error message + +## What We've Done So Far + +### Commit 1: `9754b21` (Initial Fix - Incomplete) +- Fixed `std::path::Path` to use fully qualified paths (::std::path::Path) +- Added comprehensive regression tests for std/core aliasing +- **BUT**: This didn't fix the user's actual issue + +### Current Changes (Uncommitted) +- Added `nested` field to `GonfigField` struct +- Marked as `#[allow(dead_code)]` and reserved for future use +- Now the `#[gonfig(nested)]` attribute is **accepted** (prevents darling error) +- Test confirms it compiles with core alias + +## The Real Question + +**We need user clarification on what `nested` should do:** + +### Option A: Accept But Don't Implement (Current State) +- ✅ Fixes compilation error +- ✅ Allows code to compile with core alias +- ❌ `nested` attribute does nothing functionally +- ❌ Misleading to users who expect it to work + +### Option B: Implement Full Nested Support +- ✅ Provides actual functionality +- ✅ Useful feature for users +- ❌ More complex implementation +- ❌ Need to define exact behavior: + - How do prefixes work? + - Do fields flatten or stay nested? + - How do defaults propagate? + +### Option C: Remove from Issue Example +- Ask user if they actually need `nested` feature +- Maybe they just used it as an example +- They might only need the std::path::Path fix + +## Waiting For + +GitHub comment posted: https://github.com/0xvasanth/gonfig/issues/23#issuecomment-3733426020 + +**Questions asked:** +1. What should `nested` do functionally? +2. How should environment variable prefixes work with nested structs? +3. Is this the same as the existing `flatten` attribute? +4. Do they prefer Option A, B, or C? + +## Test Results + +All new tests pass: +- ✅ issue_23_core_alias (2 tests) +- ✅ issue_23_std_alias (2 tests) +- ✅ issue_23_both_aliases (4 tests) +- ✅ issue_23_nested_attr (1 test) - **New: verifies nested compiles** + +**Total: 9 passing regression tests for issue #23** + +## Files Changed + +``` +gonfig_derive/src/lib.rs - Added nested field +tests/issue_23_nested_attr.rs - New test for nested attribute +``` + +## Next Steps + +1. **Wait for user response** on GitHub issue +2. Based on response: + - If Option A: Amend commit, add docs, done + - If Option B: Implement full nested feature + - If Option C: Revert nested, focus on other fixes + +## Technical Notes + +### Why the Original Fix Wasn't Enough + +The std::path::Path fix (commit `9754b21`) solved ONE potential issue but not THE issue in the user's reproduction case. Their example specifically uses `#[gonfig(nested)]` which triggers a different code path in darling. + +### The Darling Issue + +Darling generates compile errors like this: +```rust +::core::compile_error!("Unknown field: `nested`") +``` + +When user has `use core as my_core;`, this becomes: +```rust +my_core::compile_error!("Unknown field: `nested`") // ERROR: compile_error not in my_core +``` + +By adding `nested` to GonfigField, darling accepts it and doesn't generate the error. + +### Complete Fix Requires + +1. ✅ Fully qualified std::path::Path (done in `9754b21`) +2. ✅ Accept `nested` attribute (done, uncommitted) +3. ❓ Implement `nested` functionality (pending user clarification) diff --git a/gonfig_derive/src/lib.rs b/gonfig_derive/src/lib.rs index 4226d2b..a062302 100644 --- a/gonfig_derive/src/lib.rs +++ b/gonfig_derive/src/lib.rs @@ -46,6 +46,11 @@ struct GonfigField { #[darling(default)] flatten: bool, + // Reserved for future use (nested configuration feature) + #[allow(dead_code)] + #[darling(default)] + nested: bool, + #[darling(default)] default: Option, } @@ -148,6 +153,35 @@ struct GonfigField { /// } /// ``` /// +/// ## `#[gonfig(nested)]` +/// **[Experimental]** Marks a field as a nested configuration struct. +/// +/// Currently, nested fields must be manually constructed. This attribute prevents +/// the field from being loaded with the parent config. +/// +/// **Example:** +/// ```rust,ignore +/// #[derive(Gonfig, Deserialize)] +/// #[Gonfig(env_prefix = "APP")] +/// struct Config { +/// #[gonfig(nested)] +/// server: ServerConfig, +/// } +/// +/// #[derive(Gonfig, Deserialize)] +/// #[Gonfig(env_prefix = "SERVER")] +/// struct ServerConfig { +/// host: String, +/// } +/// +/// // Manual construction (automatic loading coming in future version): +/// fn load_config() -> Result { +/// let server = ServerConfig::from_gonfig()?; +/// // Manually combine with parent struct +/// Ok(Config { server }) +/// } +/// ``` +/// /// ## `#[skip]` or `#[skip_gonfig]` /// Exclude a field from configuration loading. Useful for non-serializable fields or /// fields that should only be set at runtime. @@ -260,6 +294,12 @@ fn generate_gonfig_impl(opts: &GonfigOpts) -> proc_macro2::TokenStream { let field_name = f.ident.as_ref().unwrap(); let field_str = field_name.to_string(); + // Skip nested fields - they should be loaded separately + // TODO: Implement automatic nested struct loading with prefix composition + if f.nested { + continue; + } + // Note: flatten feature is not yet fully implemented // For now, treat all fields as regular fields { @@ -338,19 +378,20 @@ fn generate_gonfig_impl(opts: &GonfigOpts) -> proc_macro2::TokenStream { if #allow_config { // Config file support - check for default config files - use std::path::Path; + // Note: Using fully qualified paths to avoid conflicts with user's std/core aliases + // See: https://github.com/0xvasanth/gonfig/issues/23 - if Path::new("config.toml").exists() { + if ::std::path::Path::new("config.toml").exists() { builder = match builder.with_file("config.toml") { Ok(b) => b, Err(e) => return Err(e), }; - } else if Path::new("config.yaml").exists() { + } else if ::std::path::Path::new("config.yaml").exists() { builder = match builder.with_file("config.yaml") { Ok(b) => b, Err(e) => return Err(e), }; - } else if Path::new("config.json").exists() { + } else if ::std::path::Path::new("config.json").exists() { builder = match builder.with_file("config.json") { Ok(b) => b, Err(e) => return Err(e), diff --git a/tests/issue_23_both_aliases.rs b/tests/issue_23_both_aliases.rs new file mode 100644 index 0000000..1167361 --- /dev/null +++ b/tests/issue_23_both_aliases.rs @@ -0,0 +1,103 @@ +// Regression test for issue #23: https://github.com/0xvasanth/gonfig/issues/23 +// Comprehensive test with both core and std aliased and all features enabled + +#![allow(unused_imports)] + +use core as my_core; +use gonfig::Gonfig; +use serde::{Deserialize, Serialize}; +use std as my_std; + +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "BOTH_ALIAS", allow_config, allow_cli)] +pub struct FullFeaturedConfig { + #[gonfig(default = "localhost")] + pub hostname: String, + + #[gonfig(default = "8080")] + pub port: u16, + + #[gonfig(default = "info")] + #[gonfig(env_name = "LOG_LEVEL")] + pub log_level: String, +} + +// Test nested configuration +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "DATABASE", allow_config)] +pub struct DatabaseConfig { + #[gonfig(default = "postgres://localhost/mydb")] + pub url: String, + + #[gonfig(default = "10")] + pub max_connections: u32, + + #[gonfig(default = "30")] + pub timeout_seconds: u64, +} + +// Test with skip attribute +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "APP", allow_config, allow_cli)] +pub struct AppConfigWithSkip { + #[gonfig(default = "production")] + pub environment: String, + + #[skip] + #[serde(skip)] + pub runtime_data: Option, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_both_aliases_all_features() { + // Most comprehensive test: both std and core aliased, all features enabled + let config = FullFeaturedConfig::from_gonfig(); + assert!( + config.is_ok(), + "Should compile and run with both std and core aliased" + ); + + let config = config.unwrap(); + assert_eq!(config.hostname, "localhost"); + assert_eq!(config.port, 8080); + assert_eq!(config.log_level, "info"); + } + + #[test] + fn test_both_aliases_nested_config() { + // Test with database configuration + let config = DatabaseConfig::from_gonfig(); + assert!( + config.is_ok(), + "Nested config should work with both aliases" + ); + + let config = config.unwrap(); + assert_eq!(config.url, "postgres://localhost/mydb"); + assert_eq!(config.max_connections, 10); + assert_eq!(config.timeout_seconds, 30); + } + + #[test] + fn test_both_aliases_with_skip() { + // Test with skip attribute + let config = AppConfigWithSkip::from_gonfig(); + assert!(config.is_ok(), "Config with skip should work with aliases"); + + let config = config.unwrap(); + assert_eq!(config.environment, "production"); + assert_eq!(config.runtime_data, None); + } + + #[test] + fn test_builder_pattern_with_aliases() { + // Test the builder pattern also works with aliases + let builder = FullFeaturedConfig::gonfig_builder(); + let config = FullFeaturedConfig::from_gonfig_with_builder(builder); + assert!(config.is_ok(), "Builder pattern should work with aliases"); + } +} diff --git a/tests/issue_23_core_alias.rs b/tests/issue_23_core_alias.rs new file mode 100644 index 0000000..8b40512 --- /dev/null +++ b/tests/issue_23_core_alias.rs @@ -0,0 +1,59 @@ +// Regression test for issue #23: https://github.com/0xvasanth/gonfig/issues/23 +// Tests that the gonfig derive macro works correctly when core crate is aliased + +#![allow(unused_imports)] + +use core as tradesmith_core; // Alias core to simulate user's environment +use gonfig::Gonfig; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "CORE_ALIAS_TEST", allow_config)] +pub struct ConfigWithCoreAlias { + #[gonfig(default = "127.0.0.1")] + pub host: String, + + #[gonfig(default = "8080")] + pub port: u16, +} + +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "SERVER", allow_config, allow_cli)] +pub struct ServerConfigWithCoreAlias { + #[gonfig(default = "localhost")] + pub address: String, + + #[gonfig(default = "3000")] + pub server_port: u16, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_core_alias_basic_with_config() { + // This test verifies that the macro-generated code compiles + // when core is aliased and allow_config is enabled + let config = ConfigWithCoreAlias::from_gonfig(); + assert!(config.is_ok(), "Should compile with core alias"); + + let config = config.unwrap(); + assert_eq!(config.host, "127.0.0.1"); + assert_eq!(config.port, 8080); + } + + #[test] + fn test_core_alias_with_all_features() { + // Test with both allow_config and allow_cli enabled + let config = ServerConfigWithCoreAlias::from_gonfig(); + assert!( + config.is_ok(), + "Should compile with core alias and all features" + ); + + let config = config.unwrap(); + assert_eq!(config.address, "localhost"); + assert_eq!(config.server_port, 3000); + } +} diff --git a/tests/issue_23_nested_attr.rs b/tests/issue_23_nested_attr.rs new file mode 100644 index 0000000..9844754 --- /dev/null +++ b/tests/issue_23_nested_attr.rs @@ -0,0 +1,67 @@ +// Regression test for issue #23: nested attribute with core alias +// Tests that the macro accepts nested attribute without compilation errors + +#![allow(unused_imports)] + +use core as tradesmith_core; // Alias core like in the original issue +use gonfig::Gonfig; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "APP")] +pub struct Config { + #[gonfig(nested)] + pub server: ServerConfig, + + #[gonfig(default = "production")] + pub environment: String, +} + +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "SERVER")] +pub struct ServerConfig { + #[gonfig(default = "127.0.0.1")] + pub host: String, + + #[gonfig(default = "3000")] + pub port: u16, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_nested_attribute_compiles_with_core_alias() { + // **Main fix**: This test verifies the compilation error is fixed + // Original error: "failed to resolve: could not find `compile_error` in `core`" + // This happened because darling used unqualified core::compile_error!() + // when it encountered unknown attribute `nested` + + // By adding `nested` field to GonfigField, darling accepts it and + // the code compiles successfully even with core aliased + + // Test that nested structs load independently + let server = ServerConfig::from_gonfig(); + assert!(server.is_ok(), "Nested struct should load successfully"); + + let server = server.unwrap(); + assert_eq!(server.host, "127.0.0.1"); + assert_eq!(server.port, 3000); + } + + #[test] + fn test_manual_nested_composition() { + // Demonstrate the current recommended pattern for nested configs + // (Automatic composition will be added in a future version) + + let server = ServerConfig::from_gonfig().expect("Server config should load"); + let config = Config { + server, + environment: "production".to_string(), + }; + + assert_eq!(config.server.host, "127.0.0.1"); + assert_eq!(config.environment, "production"); + } +} diff --git a/tests/issue_23_std_alias.rs b/tests/issue_23_std_alias.rs new file mode 100644 index 0000000..1ee5e86 --- /dev/null +++ b/tests/issue_23_std_alias.rs @@ -0,0 +1,61 @@ +// Regression test for issue #23: https://github.com/0xvasanth/gonfig/issues/23 +// Tests that the gonfig derive macro works correctly when std crate is aliased + +#![allow(unused_imports)] + +use gonfig::Gonfig; +use serde::{Deserialize, Serialize}; +use std as my_std; // Alias std to trigger potential path resolution issues + +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "STD_ALIAS_TEST", allow_config)] +pub struct ConfigWithStdAlias { + #[gonfig(default = "test_value")] + pub field: String, + + #[gonfig(default = "42")] + pub number: u32, +} + +// Test with optional fields that might reference std types +#[derive(Debug, Clone, Serialize, Deserialize, Gonfig)] +#[gonfig(env_prefix = "OPT", allow_config)] +pub struct OptionalConfigWithStdAlias { + #[gonfig(default = r#"null"#)] + pub optional_field: Option, + + #[gonfig(default = r#"["item1","item2"]"#)] + pub list_field: Vec, +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_std_alias_basic_with_config() { + // This test verifies that the macro handles std aliasing correctly + // The issue manifests when allow_config is enabled because the macro + // generates "use std::path::Path;" which would resolve incorrectly + let config = ConfigWithStdAlias::from_gonfig(); + assert!(config.is_ok(), "Should compile with std alias"); + + let config = config.unwrap(); + assert_eq!(config.field, "test_value"); + assert_eq!(config.number, 42); + } + + #[test] + fn test_std_alias_with_option_types() { + // Test with Option and Vec types to ensure std::option and std::vec work + let config = OptionalConfigWithStdAlias::from_gonfig(); + assert!( + config.is_ok(), + "Should compile with std alias and Option/Vec types" + ); + + let config = config.unwrap(); + assert_eq!(config.optional_field, None); + assert_eq!(config.list_field, vec!["item1", "item2"]); + } +}