laskoviymishka commented on code in PR #2970: URL: https://github.com/apache/iceberg-rust/pull/2970#discussion_r3739131051
########## crates/property-macro/src/properties.rs: ########## @@ -0,0 +1,632 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + option_inner_type: Option<Type>, + map_value_type: Option<Type>, + public_getter: bool, + doc_attributes: Vec<Attribute>, +} + +struct PublicGetter; + +enum PropertyOption { + Key(Expr), + AdditionalKeys(Vec<Expr>), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + ParsePropertiesWith(Path), + Getter(PublicGetter), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + public_getter: bool, +} + +impl Parse for PublicGetter { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + input.parse::<Token![pub]>()?; Review Comment: `pub(getter)` borrows Rust's `pub(crate)`/`pub(super)` visibility syntax for something that isn't a visibility — it means "generate a getter". My worry is we've spoken for that syntax, so if we ever want real `pub(crate)` getters we've boxed ourselves in. Would a plain `getter` (or `accessor`) keyword read better? Non-blocking, but cheaper to settle before it lands in `public-api.txt`. wdyt? ########## crates/property-macro/README.md: ########## @@ -0,0 +1,240 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. +--> + +# Iceberg property derive macro + +`Properties` parses a typed struct from a flat `HashMap<String, String>` and +can generate opt-in read-only getters. It deliberately does not generate +property-map serialization or implement `Default`, `Serialize`, `Deserialize`, +or any other trait. + +## Generated API + +For every annotated struct, `#[derive(Properties)]` generates this inherent +constructor: + +```text +impl MyProperties { + pub fn from_properties( + properties: &HashMap<String, String>, + ) -> Result<Self, String>; +} +``` + +`from_properties` borrows the source map, parses every modeled property, and +uses its annotated default when a property is absent. Unknown keys are ignored. +An invalid value returns an error containing its primary property key. + +Adding `pub(getter)` to a field generates an immutable accessor with the field +name. Structurally known `Copy` types return `T`; other types return `&T`. +Documentation attributes on the field are copied to the generated getter. The +macro generates no setters, backing fields, or conversion back to a property +map. + +## Complete example + +This example covers exact keys and defaults, optional values, case-insensitive +booleans, prefixed maps, nested groups, custom single-value parsing, custom +multi-key parsing, lists of additional keys, read-only getters, ignored unknown +keys, and contextual errors. + +```rust +use std::collections::HashMap; + +use iceberg_property_macro::Properties; + +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FANOUT: &str = "write.fanout.enabled"; Review Comment: I don't think `write.fanout.enabled` is an actual Iceberg property — Java only has the (deprecated) `write.spark.fanout.enabled`, and in iceberg-rust the key is `write.datafusion.fanout.enabled` (`PROPERTY_DATAFUSION_WRITE_FANOUT_ENABLED` in `crates/iceberg/src/spec/table_properties.rs`). Same with `write.data.path` on line 66 — that's not a standard key either; we use `write.metadata.path`. Since this README is teaching material in Iceberg's own repo, I'd use the real keys so nobody bakes a nonexistent one into a config. Worth a note that `default = true` for fanout matches our datafusion extension but is the opposite of the closest Java constant (`SPARK_WRITE_PARTITIONED_FANOUT_ENABLED_DEFAULT = false`), so flagging it as engine-specific would help. ########## crates/property-macro/src/properties.rs: ########## @@ -0,0 +1,632 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + option_inner_type: Option<Type>, + map_value_type: Option<Type>, + public_getter: bool, + doc_attributes: Vec<Attribute>, +} + +struct PublicGetter; + +enum PropertyOption { + Key(Expr), + AdditionalKeys(Vec<Expr>), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + ParsePropertiesWith(Path), + Getter(PublicGetter), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + public_getter: bool, +} + +impl Parse for PublicGetter { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + input.parse::<Token![pub]>()?; + let content; + parenthesized!(content in input); + let accessor = content.parse::<Ident>()?; + if !content.is_empty() { + return Err(content.error("expected getter")); + } + + if accessor == "getter" { + Ok(Self) + } else { + Err(Error::new_spanned(accessor, "expected getter")) + } + } +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + if input.peek(Token![pub]) { + return input.parse().map(Self::Getter); + } + + let name = input.parse::<Ident>()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + + input.parse::<Token![=]>()?; + let expression = input.parse::<Expr>()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_keys" => { + expression_list(expression, "additional_keys").map(Self::AdditionalKeys) + } + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result<TokenStream2> { + let struct_name = input.ident; + let generics = input.generics; + let fields = match input.data { + Data::Struct(data) => match data.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs with named fields", + )); + } + }, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs", + )); + } + }; + + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let parses = fields.iter().map(parse_field); + let accessors = fields.iter().map(field_getter); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::std::result::Result<Self, ::std::string::String> { + Ok(Self { + #(#parses,)* + }) + } + } + }) +} + +fn parse_property_field( + field: &Field, + property_options: PropertyOptions, +) -> syn::Result<PropertyField> { + let ident = field + .ident + .clone() + .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; + let PropertyOptions { + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + public_getter, + } = property_options; + + if usize::from(key.is_some()) + usize::from(prefix.is_some()) + usize::from(nested) != 1 { + return Err(Error::new_spanned( + field, + "Properties fields must declare exactly one of key, prefix, or nested in #[property(...)]", + )); + } + + if nested && default.is_some() { + return Err(Error::new_spanned( + field, + "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", + )); + } + if !nested && default.is_none() { + return Err(Error::new_spanned( + field, + "Properties leaf fields must declare default in #[property(...)]", + )); + } + + let map_value_type = hash_map_value_type(&field.ty); + if prefix.is_some() && map_value_type.is_none() { + return Err(Error::new_spanned( + &field.ty, + "property prefix fields must have type HashMap<String, T>", + )); + } + + if additional_keys.is_some() && parse_properties_with.is_none() { + return Err(Error::new_spanned( + field, + "additional_keys requires parse_properties_with in #[property(...)]", + )); + } + if (prefix.is_some() || nested) + && (additional_keys.is_some() || parse_with.is_some() || parse_properties_with.is_some()) + { + return Err(Error::new_spanned( + field, + "prefix and nested fields do not support custom parse functions", + )); + } + if parse_with.is_some() && parse_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both parse_with and parse_properties_with", + )); + } + Ok(PropertyField { + ident, + ty: field.ty.clone(), + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + option_inner_type: option_inner_type(&field.ty), + map_value_type, + public_getter, + doc_attributes: field + .attrs + .iter() + .filter(|attribute| attribute.path().is_ident("doc")) + .cloned() + .collect(), + }) +} + +fn property_options(field: &Field) -> syn::Result<PropertyOptions> { + let Some(attribute) = find_attribute(&field.attrs, "property")? else { + return Err(Error::new_spanned( + field, + "Properties fields must declare #[property(...)]", + )); + }; + + let parsed = + attribute.parse_args_with(Punctuated::<PropertyOption, Token![,]>::parse_terminated)?; + if parsed.is_empty() { + return Err(Error::new_spanned( + attribute, + "property must declare at least one option", + )); + } + + let mut options = PropertyOptions::default(); + for option in parsed { + match option { + PropertyOption::Key(value) => { + set_property_option(&mut options.key, value, attribute, "key")? + } + PropertyOption::AdditionalKeys(value) => set_property_option( + &mut options.additional_keys, + value, + attribute, + "additional_keys", + )?, + PropertyOption::Prefix(value) => { + set_property_option(&mut options.prefix, value, attribute, "prefix")? + } + PropertyOption::Nested => { + if options.nested { + return Err(Error::new_spanned( + attribute, + "duplicate nested property option", + )); + } + options.nested = true; + } + PropertyOption::Default(value) => { + set_property_option(&mut options.default, value, attribute, "default")? + } + PropertyOption::ParseWith(value) => { + set_property_option(&mut options.parse_with, value, attribute, "parse_with")? + } + PropertyOption::ParsePropertiesWith(value) => set_property_option( + &mut options.parse_properties_with, + value, + attribute, + "parse_properties_with", + )?, + PropertyOption::Getter(_) => { + if options.public_getter { + return Err(Error::new_spanned(attribute, "duplicate property accessor")); + } + options.public_getter = true; + } + } + } + + Ok(options) +} + +fn set_property_option<T>( + target: &mut Option<T>, + value: T, + attribute: &Attribute, + name: &str, +) -> syn::Result<()> { + if target.is_some() { + return Err(Error::new_spanned( + attribute, + format!("duplicate {name} property option"), + )); + } + *target = Some(value); + Ok(()) +} + +fn field_getter(field: &PropertyField) -> TokenStream2 { + if !field.public_getter { + return TokenStream2::new(); + } + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + if is_copy_type(ty) { + quote! { + #(#docs)* + pub fn #ident(&self) -> #ty { + self.#ident + } + } + } else { + quote! { + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident + } + } + } +} + +fn expression_path(expression: Expr, name: &str) -> syn::Result<Path> { + match expression { + Expr::Path(ExprPath { path, .. }) => Ok(path), + _ => Err(Error::new_spanned( + expression, + format!("{name} must be a path"), + )), + } +} + +fn expression_list(expression: Expr, name: &str) -> syn::Result<Vec<Expr>> { + let Expr::Array(array) = expression else { + return Err(Error::new_spanned( + expression, + format!("{name} must be an array of keys"), + )); + }; + if array.elems.is_empty() { + return Err(Error::new_spanned( + array, + format!("{name} must contain at least one key"), + )); + } + Ok(array.elems.into_iter().collect()) +} + +fn find_attribute<'a>( + attributes: &'a [Attribute], + name: &str, +) -> syn::Result<Option<&'a Attribute>> { + let mut matching = attributes + .iter() + .filter(|attribute| attribute.path().is_ident(name)); + let first = matching.next(); + if let Some(duplicate) = matching.next() { + return Err(Error::new_spanned( + duplicate, + format!("duplicate #[{name}] attribute"), + )); + } + Ok(first) +} + +fn parse_field(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + let ty = &field.ty; + return quote!(#ident: <#ty>::from_properties(properties)?); + } + + let ty = &field.ty; + let default = typed_default(field); + + if let Some(parse_properties_with) = &field.parse_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match &field.additional_keys { + Some(additional_keys) => { + quote!(#parse_properties_with(properties, #key, &[#(#additional_keys),*], #default)) + } + None => quote!(#parse_properties_with(properties, #key, #default)), + }; + return quote! { + #ident: #parse.map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }; + } + + if let Some(prefix) = &field.prefix { + let value_type = field + .map_value_type + .as_ref() + .expect("prefix fields are validated as maps"); + let parse = if is_bool(value_type) { + quote!(value.to_ascii_lowercase().parse::<#value_type>()) + } else { + quote!(value.parse::<#value_type>()) + }; + return quote! { + #ident: { + let parsed = properties + .iter() + .filter_map(|(key, value)| { + key.strip_prefix(#prefix).map(|suffix| { + #parse + .map(|parsed| (suffix.to_string(), parsed)) + .map_err(|error| format!("Invalid value for {key}: {error}")) + }) + }) + .collect::<::std::result::Result< + ::std::collections::HashMap<_, _>, + ::std::string::String, + >>()?; + if parsed.is_empty() { Review Comment: This `if parsed.is_empty() { #default }` means a non-empty default only applies when zero keys match — the moment one key matches, the default entries are dropped rather than merged. That's an asymmetric substitution that'll surprise anyone using a non-empty default map. I'd drop the branch: a prefix scan should just produce its (possibly empty) map, and let an empty default fall out naturally from no matches. wdyt? ########## crates/property-macro/tests/properties.rs: ########## @@ -0,0 +1,225 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::collections::HashMap; + +use iceberg_property_macro::Properties; +use serde::{Deserialize, Serialize}; + +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FORMAT: &str = "write.format.default"; +const FANOUT_ENABLED: &str = "write.fanout.enabled"; +const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; +const WIDTH: &str = "dimensions.width"; +const HEIGHT: &str = "dimensions.height"; +const DEPTH: &str = "dimensions.depth"; + +fn parse_dimensions( + properties: &HashMap<String, String>, + width_key: &str, + additional_keys: &[&str], + default: (u64, u64, u64), +) -> Result<(u64, u64, u64), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } + let parse = |property_key: &str, default| { + properties + .get(property_key) + .map(|value| value.parse::<u64>().map_err(|error| error.to_string())) + .transpose() + .map(|value| value.unwrap_or(default)) + }; + + Ok(( + parse(width_key, default.0)?, + parse(additional_keys[0], default.1)?, + parse(additional_keys[1], default.2)?, + )) +} + +#[derive(Debug, Properties)] +struct TestProperties { + #[property(key = RETRIES, default = 4, pub(getter))] + retries: u64, + + #[property(key = OWNER, default = None, pub(getter))] + owner: Option<String>, + + #[property(key = FORMAT, default = "parquet", pub(getter))] + format: String, + + #[property(key = FANOUT_ENABLED, default = true, pub(getter))] + fanout_enabled: bool, + + #[property( + prefix = COLUMN_FPP_PREFIX, + default = HashMap::new(), + pub(getter) + )] + column_fpp: HashMap<String, f64>, + + #[property( + key = WIDTH, + additional_keys = [HEIGHT, DEPTH], + default = (640, 480, 320), + parse_properties_with = parse_dimensions, + pub(getter) + )] + dimensions: (u64, u64, u64), +} + +#[test] +fn reads_defaults_through_generated_getters() { + let properties = TestProperties::from_properties(&HashMap::new()).unwrap(); + + assert_eq!(properties.retries(), 4); + assert_eq!(properties.owner(), &None); + assert_eq!(properties.format(), "parquet"); + assert!(properties.fanout_enabled()); + assert!(properties.column_fpp().is_empty()); + assert_eq!(properties.dimensions(), (640, 480, 320)); +} + +#[test] +fn reads_overrides_and_ignores_unknown_properties() { + let raw = HashMap::from([ + (RETRIES.to_string(), "8".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FORMAT.to_string(), "orc".to_string()), + (FANOUT_ENABLED.to_string(), "FALSE".to_string()), + (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), + ("unknown".to_string(), "ignored".to_string()), + ]); + let properties = TestProperties::from_properties(&raw).unwrap(); + + assert_eq!(properties.retries(), 8); + assert_eq!(properties.owner().as_deref(), Some("iceberg")); + assert_eq!(properties.format(), "orc"); + assert!(!properties.fanout_enabled()); + assert_eq!(properties.column_fpp()["id"], 0.01); + assert_eq!(properties.dimensions(), (1920, 1080, 720)); +} + +#[test] +fn reports_the_property_with_an_invalid_value() { + let numeric_error = TestProperties::from_properties(&HashMap::from([( Review Comment: These runtime value-error cases are good, but the macro's compile-time error messages are just as much its public API and nothing covers them. I'd add a small `tests/compile-fail/` with trybuild for the misuse paths — missing `#[property]`, multiple of key/prefix/nested, `nested` + `default`, `prefix` on a non-`HashMap`, and the `parse_properties_with`/`additional_keys` mismatch from above — so the wording can't regress silently. ########## crates/property-macro/src/properties.rs: ########## @@ -0,0 +1,632 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + option_inner_type: Option<Type>, + map_value_type: Option<Type>, + public_getter: bool, + doc_attributes: Vec<Attribute>, +} + +struct PublicGetter; + +enum PropertyOption { + Key(Expr), + AdditionalKeys(Vec<Expr>), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + ParsePropertiesWith(Path), + Getter(PublicGetter), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + public_getter: bool, +} + +impl Parse for PublicGetter { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + input.parse::<Token![pub]>()?; + let content; + parenthesized!(content in input); + let accessor = content.parse::<Ident>()?; + if !content.is_empty() { + return Err(content.error("expected getter")); + } + + if accessor == "getter" { + Ok(Self) + } else { + Err(Error::new_spanned(accessor, "expected getter")) + } + } +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + if input.peek(Token![pub]) { + return input.parse().map(Self::Getter); + } + + let name = input.parse::<Ident>()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + + input.parse::<Token![=]>()?; + let expression = input.parse::<Expr>()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_keys" => { + expression_list(expression, "additional_keys").map(Self::AdditionalKeys) + } + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result<TokenStream2> { + let struct_name = input.ident; + let generics = input.generics; + let fields = match input.data { + Data::Struct(data) => match data.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs with named fields", + )); + } + }, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs", + )); + } + }; + + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let parses = fields.iter().map(parse_field); + let accessors = fields.iter().map(field_getter); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::std::result::Result<Self, ::std::string::String> { + Ok(Self { + #(#parses,)* + }) + } + } + }) +} + +fn parse_property_field( + field: &Field, + property_options: PropertyOptions, +) -> syn::Result<PropertyField> { + let ident = field + .ident + .clone() + .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; + let PropertyOptions { + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + public_getter, + } = property_options; + + if usize::from(key.is_some()) + usize::from(prefix.is_some()) + usize::from(nested) != 1 { + return Err(Error::new_spanned( + field, + "Properties fields must declare exactly one of key, prefix, or nested in #[property(...)]", + )); + } + + if nested && default.is_some() { + return Err(Error::new_spanned( + field, + "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", + )); + } + if !nested && default.is_none() { + return Err(Error::new_spanned( + field, + "Properties leaf fields must declare default in #[property(...)]", + )); + } + + let map_value_type = hash_map_value_type(&field.ty); + if prefix.is_some() && map_value_type.is_none() { + return Err(Error::new_spanned( + &field.ty, + "property prefix fields must have type HashMap<String, T>", + )); + } + + if additional_keys.is_some() && parse_properties_with.is_none() { + return Err(Error::new_spanned( + field, + "additional_keys requires parse_properties_with in #[property(...)]", + )); + } + if (prefix.is_some() || nested) + && (additional_keys.is_some() || parse_with.is_some() || parse_properties_with.is_some()) + { + return Err(Error::new_spanned( + field, + "prefix and nested fields do not support custom parse functions", + )); + } + if parse_with.is_some() && parse_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both parse_with and parse_properties_with", + )); + } + Ok(PropertyField { + ident, + ty: field.ty.clone(), + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + option_inner_type: option_inner_type(&field.ty), + map_value_type, + public_getter, + doc_attributes: field + .attrs + .iter() + .filter(|attribute| attribute.path().is_ident("doc")) + .cloned() + .collect(), + }) +} + +fn property_options(field: &Field) -> syn::Result<PropertyOptions> { + let Some(attribute) = find_attribute(&field.attrs, "property")? else { + return Err(Error::new_spanned( + field, + "Properties fields must declare #[property(...)]", + )); + }; + + let parsed = + attribute.parse_args_with(Punctuated::<PropertyOption, Token![,]>::parse_terminated)?; + if parsed.is_empty() { + return Err(Error::new_spanned( + attribute, + "property must declare at least one option", + )); + } + + let mut options = PropertyOptions::default(); + for option in parsed { + match option { + PropertyOption::Key(value) => { + set_property_option(&mut options.key, value, attribute, "key")? + } + PropertyOption::AdditionalKeys(value) => set_property_option( + &mut options.additional_keys, + value, + attribute, + "additional_keys", + )?, + PropertyOption::Prefix(value) => { + set_property_option(&mut options.prefix, value, attribute, "prefix")? + } + PropertyOption::Nested => { + if options.nested { + return Err(Error::new_spanned( + attribute, + "duplicate nested property option", + )); + } + options.nested = true; + } + PropertyOption::Default(value) => { + set_property_option(&mut options.default, value, attribute, "default")? + } + PropertyOption::ParseWith(value) => { + set_property_option(&mut options.parse_with, value, attribute, "parse_with")? + } + PropertyOption::ParsePropertiesWith(value) => set_property_option( + &mut options.parse_properties_with, + value, + attribute, + "parse_properties_with", + )?, + PropertyOption::Getter(_) => { + if options.public_getter { + return Err(Error::new_spanned(attribute, "duplicate property accessor")); + } + options.public_getter = true; + } + } + } + + Ok(options) +} + +fn set_property_option<T>( + target: &mut Option<T>, + value: T, + attribute: &Attribute, + name: &str, +) -> syn::Result<()> { + if target.is_some() { + return Err(Error::new_spanned( + attribute, + format!("duplicate {name} property option"), + )); + } + *target = Some(value); + Ok(()) +} + +fn field_getter(field: &PropertyField) -> TokenStream2 { + if !field.public_getter { + return TokenStream2::new(); + } + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + if is_copy_type(ty) { + quote! { + #(#docs)* + pub fn #ident(&self) -> #ty { + self.#ident + } + } + } else { + quote! { + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident + } + } + } +} + +fn expression_path(expression: Expr, name: &str) -> syn::Result<Path> { + match expression { + Expr::Path(ExprPath { path, .. }) => Ok(path), + _ => Err(Error::new_spanned( + expression, + format!("{name} must be a path"), + )), + } +} + +fn expression_list(expression: Expr, name: &str) -> syn::Result<Vec<Expr>> { + let Expr::Array(array) = expression else { + return Err(Error::new_spanned( + expression, + format!("{name} must be an array of keys"), + )); + }; + if array.elems.is_empty() { + return Err(Error::new_spanned( + array, + format!("{name} must contain at least one key"), + )); + } + Ok(array.elems.into_iter().collect()) +} + +fn find_attribute<'a>( + attributes: &'a [Attribute], + name: &str, +) -> syn::Result<Option<&'a Attribute>> { + let mut matching = attributes + .iter() + .filter(|attribute| attribute.path().is_ident(name)); + let first = matching.next(); + if let Some(duplicate) = matching.next() { + return Err(Error::new_spanned( + duplicate, + format!("duplicate #[{name}] attribute"), + )); + } + Ok(first) +} + +fn parse_field(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + let ty = &field.ty; + return quote!(#ident: <#ty>::from_properties(properties)?); + } + + let ty = &field.ty; + let default = typed_default(field); + + if let Some(parse_properties_with) = &field.parse_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match &field.additional_keys { + Some(additional_keys) => { + quote!(#parse_properties_with(properties, #key, &[#(#additional_keys),*], #default)) + } + None => quote!(#parse_properties_with(properties, #key, #default)), + }; + return quote! { + #ident: #parse.map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }; + } + + if let Some(prefix) = &field.prefix { + let value_type = field + .map_value_type + .as_ref() + .expect("prefix fields are validated as maps"); + let parse = if is_bool(value_type) { + quote!(value.to_ascii_lowercase().parse::<#value_type>()) + } else { + quote!(value.parse::<#value_type>()) + }; + return quote! { + #ident: { + let parsed = properties + .iter() + .filter_map(|(key, value)| { + key.strip_prefix(#prefix).map(|suffix| { + #parse + .map(|parsed| (suffix.to_string(), parsed)) + .map_err(|error| format!("Invalid value for {key}: {error}")) + }) + }) + .collect::<::std::result::Result< + ::std::collections::HashMap<_, _>, + ::std::string::String, + >>()?; + if parsed.is_empty() { + #default + } else { + parsed + } + } + }; + } + + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match (&field.parse_with, &field.option_inner_type) { + (Some(parse_with), _) => quote! { + #parse_with(value).map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + (None, Some(inner_type)) if is_bool(inner_type) => quote! { + Some(value.to_ascii_lowercase().parse::<#inner_type>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })?) + }, + (None, Some(inner_type)) => quote! { + Some(value.parse::<#inner_type>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })?) + }, + (None, None) if is_bool(ty) => quote! { + value.to_ascii_lowercase().parse::<#ty>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + (None, None) => quote! { + value.parse::<#ty>().map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }, + }; + + quote! { + #ident: match properties.get(#key) { + Some(value) => #parse, + None => #default, + } + } +} + +fn typed_default(field: &PropertyField) -> TokenStream2 { + let ty = &field.ty; + let default = default_value( + field.default.as_ref().expect("leaf fields have defaults"), + ty, + ); + quote!({ + let value: #ty = #default; + value + }) +} + +fn default_value(default: &Expr, ty: &Type) -> TokenStream2 { + if matches!( + default, + Expr::Lit(ExprLit { + lit: Lit::Str(_), + .. + }) | Expr::Path(_) + ) { + quote!(::std::convert::Into::<#ty>::into(#default)) + } else { + quote!(#default) + } +} + +fn option_inner_type(ty: &Type) -> Option<Type> { + let Type::Path(type_path) = ty else { + return None; + }; + + let segment = type_path.path.segments.last()?; + if segment.ident != "Option" { + return None; + } + + let PathArguments::AngleBracketed(arguments) = &segment.arguments else { + return None; + }; + let Some(GenericArgument::Type(inner_type)) = arguments.args.first() else { + return None; + }; + + Some(inner_type.clone()) +} + +fn hash_map_value_type(ty: &Type) -> Option<Type> { + let Type::Path(type_path) = ty else { + return None; + }; + + let segment = type_path.path.segments.last()?; + if segment.ident != "HashMap" { + return None; + } + + let PathArguments::AngleBracketed(arguments) = &segment.arguments else { + return None; + }; + let mut arguments = arguments.args.iter(); + let Some(GenericArgument::Type(key_type)) = arguments.next() else { + return None; + }; + let Some(GenericArgument::Type(value_type)) = arguments.next() else { + return None; + }; + if !is_named_type(key_type, "String") { + return None; + } + + Some(value_type.clone()) +} + +fn is_bool(ty: &Type) -> bool { + is_named_type(ty, "bool") +} + +fn is_copy_type(ty: &Type) -> bool { Review Comment: You added the by-value `Copy` getters I asked for on #2955 (returning `T` rather than `&T`) — thanks, that drops the `*properties.gc_enabled()` awkwardness at call sites. One gap remains: `is_copy_type` detects `Copy` structurally from the AST, so it only recognizes the hardcoded primitive names — a user `Copy` newtype like `Meters(u64)` still falls back to the `&T` branch, which contradicts the README's promise that `Copy` types return `T`. A proc-macro genuinely can't resolve `Copy` impls, so I'd at least document the limitation (by-value only for the listed primitives) and add a test or two so it can't silently drift. wdyt? ########## crates/property-macro/src/properties.rs: ########## @@ -0,0 +1,632 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + option_inner_type: Option<Type>, + map_value_type: Option<Type>, + public_getter: bool, + doc_attributes: Vec<Attribute>, +} + +struct PublicGetter; + +enum PropertyOption { + Key(Expr), + AdditionalKeys(Vec<Expr>), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + ParsePropertiesWith(Path), + Getter(PublicGetter), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + public_getter: bool, +} + +impl Parse for PublicGetter { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + input.parse::<Token![pub]>()?; + let content; + parenthesized!(content in input); + let accessor = content.parse::<Ident>()?; + if !content.is_empty() { + return Err(content.error("expected getter")); + } + + if accessor == "getter" { + Ok(Self) + } else { + Err(Error::new_spanned(accessor, "expected getter")) + } + } +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + if input.peek(Token![pub]) { + return input.parse().map(Self::Getter); + } + + let name = input.parse::<Ident>()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + + input.parse::<Token![=]>()?; + let expression = input.parse::<Expr>()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_keys" => { + expression_list(expression, "additional_keys").map(Self::AdditionalKeys) + } + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result<TokenStream2> { + let struct_name = input.ident; + let generics = input.generics; + let fields = match input.data { + Data::Struct(data) => match data.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs with named fields", + )); + } + }, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs", + )); + } + }; + + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let parses = fields.iter().map(parse_field); + let accessors = fields.iter().map(field_getter); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::std::result::Result<Self, ::std::string::String> { + Ok(Self { + #(#parses,)* + }) + } + } + }) +} + +fn parse_property_field( + field: &Field, + property_options: PropertyOptions, +) -> syn::Result<PropertyField> { + let ident = field + .ident + .clone() + .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; + let PropertyOptions { + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + public_getter, + } = property_options; + + if usize::from(key.is_some()) + usize::from(prefix.is_some()) + usize::from(nested) != 1 { + return Err(Error::new_spanned( + field, + "Properties fields must declare exactly one of key, prefix, or nested in #[property(...)]", + )); + } + + if nested && default.is_some() { + return Err(Error::new_spanned( + field, + "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", + )); + } + if !nested && default.is_none() { + return Err(Error::new_spanned( + field, + "Properties leaf fields must declare default in #[property(...)]", + )); + } + + let map_value_type = hash_map_value_type(&field.ty); + if prefix.is_some() && map_value_type.is_none() { + return Err(Error::new_spanned( + &field.ty, + "property prefix fields must have type HashMap<String, T>", + )); + } + + if additional_keys.is_some() && parse_properties_with.is_none() { + return Err(Error::new_spanned( + field, + "additional_keys requires parse_properties_with in #[property(...)]", + )); + } + if (prefix.is_some() || nested) + && (additional_keys.is_some() || parse_with.is_some() || parse_properties_with.is_some()) + { + return Err(Error::new_spanned( + field, + "prefix and nested fields do not support custom parse functions", + )); + } + if parse_with.is_some() && parse_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both parse_with and parse_properties_with", + )); + } + Ok(PropertyField { + ident, + ty: field.ty.clone(), + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + option_inner_type: option_inner_type(&field.ty), + map_value_type, + public_getter, + doc_attributes: field + .attrs + .iter() + .filter(|attribute| attribute.path().is_ident("doc")) + .cloned() + .collect(), + }) +} + +fn property_options(field: &Field) -> syn::Result<PropertyOptions> { + let Some(attribute) = find_attribute(&field.attrs, "property")? else { + return Err(Error::new_spanned( + field, + "Properties fields must declare #[property(...)]", + )); + }; + + let parsed = + attribute.parse_args_with(Punctuated::<PropertyOption, Token![,]>::parse_terminated)?; + if parsed.is_empty() { + return Err(Error::new_spanned( + attribute, + "property must declare at least one option", + )); + } + + let mut options = PropertyOptions::default(); + for option in parsed { + match option { + PropertyOption::Key(value) => { + set_property_option(&mut options.key, value, attribute, "key")? + } + PropertyOption::AdditionalKeys(value) => set_property_option( + &mut options.additional_keys, + value, + attribute, + "additional_keys", + )?, + PropertyOption::Prefix(value) => { + set_property_option(&mut options.prefix, value, attribute, "prefix")? + } + PropertyOption::Nested => { + if options.nested { + return Err(Error::new_spanned( + attribute, + "duplicate nested property option", + )); + } + options.nested = true; + } + PropertyOption::Default(value) => { + set_property_option(&mut options.default, value, attribute, "default")? + } + PropertyOption::ParseWith(value) => { + set_property_option(&mut options.parse_with, value, attribute, "parse_with")? + } + PropertyOption::ParsePropertiesWith(value) => set_property_option( + &mut options.parse_properties_with, + value, + attribute, + "parse_properties_with", + )?, + PropertyOption::Getter(_) => { + if options.public_getter { + return Err(Error::new_spanned(attribute, "duplicate property accessor")); + } + options.public_getter = true; + } + } + } + + Ok(options) +} + +fn set_property_option<T>( + target: &mut Option<T>, + value: T, + attribute: &Attribute, + name: &str, +) -> syn::Result<()> { + if target.is_some() { + return Err(Error::new_spanned( + attribute, + format!("duplicate {name} property option"), + )); + } + *target = Some(value); + Ok(()) +} + +fn field_getter(field: &PropertyField) -> TokenStream2 { + if !field.public_getter { + return TokenStream2::new(); + } + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + if is_copy_type(ty) { + quote! { + #(#docs)* + pub fn #ident(&self) -> #ty { + self.#ident + } + } + } else { + quote! { + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident + } + } + } +} + +fn expression_path(expression: Expr, name: &str) -> syn::Result<Path> { + match expression { + Expr::Path(ExprPath { path, .. }) => Ok(path), + _ => Err(Error::new_spanned( + expression, + format!("{name} must be a path"), + )), + } +} + +fn expression_list(expression: Expr, name: &str) -> syn::Result<Vec<Expr>> { + let Expr::Array(array) = expression else { + return Err(Error::new_spanned( + expression, + format!("{name} must be an array of keys"), + )); + }; + if array.elems.is_empty() { + return Err(Error::new_spanned( + array, + format!("{name} must contain at least one key"), + )); + } + Ok(array.elems.into_iter().collect()) +} + +fn find_attribute<'a>( + attributes: &'a [Attribute], + name: &str, +) -> syn::Result<Option<&'a Attribute>> { + let mut matching = attributes + .iter() + .filter(|attribute| attribute.path().is_ident(name)); + let first = matching.next(); + if let Some(duplicate) = matching.next() { + return Err(Error::new_spanned( + duplicate, + format!("duplicate #[{name}] attribute"), + )); + } + Ok(first) +} + +fn parse_field(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + let ty = &field.ty; + return quote!(#ident: <#ty>::from_properties(properties)?); + } + + let ty = &field.ty; + let default = typed_default(field); + + if let Some(parse_properties_with) = &field.parse_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); Review Comment: These `.expect()`s run at macro-expansion time, so if an invariant ever slips they surface as `error: proc-macro panicked` with a backtrace rather than a clean `compile_error!`. The invariants are enforced by `parse_property_field` today, but that coupling is implicit and fragile. I'd propagate a `syn::Error` via `ok_or_else(|| Error::new_spanned(...))` instead — there are a few of these (also around lines 429, 459, and 499). ########## crates/property-macro/Cargo.toml: ########## @@ -0,0 +1,47 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +[package] +edition = { workspace = true } +homepage = { workspace = true } +name = "iceberg-property-macro" +publish = true Review Comment: This is the split I asked for on #2955 — thanks for pulling the macro out. It went a bit further than the 1:1 port though: the `TableProperties` port that would prove the DSL fits our real property structs isn't here yet, and the crate is `publish = true` at 0.10.0 with nothing consuming it. Validating the macro against the existing properties was the whole point of porting first. I'd want the port alongside this — or `publish = false` plus a tracking issue until it lands, before the API locks into `public-api.txt`. #2955 linked #2877; this PR's body is the empty template, so it'd help to link an issue here too. wdyt? ########## crates/property-macro/tests/properties.rs: ########## @@ -0,0 +1,225 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::collections::HashMap; + +use iceberg_property_macro::Properties; +use serde::{Deserialize, Serialize}; + +const RETRIES: &str = "commit.retry.num-retries"; +const OWNER: &str = "owner"; +const FORMAT: &str = "write.format.default"; +const FANOUT_ENABLED: &str = "write.fanout.enabled"; +const COLUMN_FPP_PREFIX: &str = "write.parquet.bloom-filter-fpp.column."; +const WIDTH: &str = "dimensions.width"; +const HEIGHT: &str = "dimensions.height"; +const DEPTH: &str = "dimensions.depth"; + +fn parse_dimensions( + properties: &HashMap<String, String>, + width_key: &str, + additional_keys: &[&str], + default: (u64, u64, u64), +) -> Result<(u64, u64, u64), String> { + if additional_keys.len() != 2 { + return Err("dimensions require height and depth keys".to_string()); + } + let parse = |property_key: &str, default| { + properties + .get(property_key) + .map(|value| value.parse::<u64>().map_err(|error| error.to_string())) + .transpose() + .map(|value| value.unwrap_or(default)) + }; + + Ok(( + parse(width_key, default.0)?, + parse(additional_keys[0], default.1)?, + parse(additional_keys[1], default.2)?, + )) +} + +#[derive(Debug, Properties)] +struct TestProperties { + #[property(key = RETRIES, default = 4, pub(getter))] + retries: u64, + + #[property(key = OWNER, default = None, pub(getter))] + owner: Option<String>, + + #[property(key = FORMAT, default = "parquet", pub(getter))] + format: String, + + #[property(key = FANOUT_ENABLED, default = true, pub(getter))] + fanout_enabled: bool, + + #[property( + prefix = COLUMN_FPP_PREFIX, + default = HashMap::new(), + pub(getter) + )] + column_fpp: HashMap<String, f64>, + + #[property( + key = WIDTH, + additional_keys = [HEIGHT, DEPTH], + default = (640, 480, 320), + parse_properties_with = parse_dimensions, + pub(getter) + )] + dimensions: (u64, u64, u64), +} + +#[test] +fn reads_defaults_through_generated_getters() { + let properties = TestProperties::from_properties(&HashMap::new()).unwrap(); + + assert_eq!(properties.retries(), 4); + assert_eq!(properties.owner(), &None); + assert_eq!(properties.format(), "parquet"); + assert!(properties.fanout_enabled()); + assert!(properties.column_fpp().is_empty()); + assert_eq!(properties.dimensions(), (640, 480, 320)); +} + +#[test] +fn reads_overrides_and_ignores_unknown_properties() { + let raw = HashMap::from([ + (RETRIES.to_string(), "8".to_string()), + (OWNER.to_string(), "iceberg".to_string()), + (FORMAT.to_string(), "orc".to_string()), + (FANOUT_ENABLED.to_string(), "FALSE".to_string()), + (format!("{COLUMN_FPP_PREFIX}id"), "0.01".to_string()), + (WIDTH.to_string(), "1920".to_string()), + (HEIGHT.to_string(), "1080".to_string()), + (DEPTH.to_string(), "720".to_string()), + ("unknown".to_string(), "ignored".to_string()), + ]); + let properties = TestProperties::from_properties(&raw).unwrap(); + + assert_eq!(properties.retries(), 8); + assert_eq!(properties.owner().as_deref(), Some("iceberg")); + assert_eq!(properties.format(), "orc"); + assert!(!properties.fanout_enabled()); + assert_eq!(properties.column_fpp()["id"], 0.01); + assert_eq!(properties.dimensions(), (1920, 1080, 720)); +} + +#[test] +fn reports_the_property_with_an_invalid_value() { + let numeric_error = TestProperties::from_properties(&HashMap::from([( + RETRIES.to_string(), + "many".to_string(), + )])) + .unwrap_err(); + assert!(numeric_error.contains(RETRIES)); + + let boolean_error = TestProperties::from_properties(&HashMap::from([( + FANOUT_ENABLED.to_string(), + "sometimes".to_string(), + )])) + .unwrap_err(); + assert!(boolean_error.contains(FANOUT_ENABLED)); + + let prefixed_key = format!("{COLUMN_FPP_PREFIX}id"); + let prefix_error = TestProperties::from_properties(&HashMap::from([( + prefixed_key.clone(), + "low".to_string(), + )])) + .unwrap_err(); + assert!(prefix_error.contains(&prefixed_key)); +} + +#[derive(Debug, Properties)] +struct CommitProperties { + /// Maximum number of times to retry a commit. + #[property(key = RETRIES, default = 4, pub(getter))] + retries: u64, +} + +#[derive(Debug, Properties)] +struct NestedProperties { + #[property(nested, pub(getter))] + commit: CommitProperties, +} + +#[test] +fn nested_properties_read_the_same_flat_map() { + let raw = HashMap::from([(RETRIES.to_string(), "9".to_string())]); + let properties = NestedProperties::from_properties(&raw).unwrap(); + + assert_eq!(properties.commit().retries(), 9); +} + +fn parse_non_empty(value: &str) -> Result<String, &'static str> { + let value = value.trim(); + if value.is_empty() { + Err("value must not be empty") + } else { + Ok(value.to_string()) + } +} + +#[derive(Debug, Properties)] +struct ValidatedProperties { + #[property( + key = "location", + default = "default", + parse_with = parse_non_empty, + pub(getter) + )] + location: String, +} + +#[test] +fn custom_single_value_parser_can_validate_and_normalize() { + let parsed = ValidatedProperties::from_properties(&HashMap::from([( Review Comment: This only exercises the present-key path. When the key is absent, `from_properties` uses `#default` directly and skips `parse_with` entirely — so a default the parser would reject still slips through. I'd add a case that omits `location` and asserts the default, to lock in that the default bypasses the parser (or decide it shouldn't). ########## crates/property-macro/src/properties.rs: ########## @@ -0,0 +1,632 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + option_inner_type: Option<Type>, + map_value_type: Option<Type>, + public_getter: bool, + doc_attributes: Vec<Attribute>, +} + +struct PublicGetter; + +enum PropertyOption { + Key(Expr), + AdditionalKeys(Vec<Expr>), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + ParsePropertiesWith(Path), + Getter(PublicGetter), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + public_getter: bool, +} + +impl Parse for PublicGetter { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + input.parse::<Token![pub]>()?; + let content; + parenthesized!(content in input); + let accessor = content.parse::<Ident>()?; + if !content.is_empty() { + return Err(content.error("expected getter")); + } + + if accessor == "getter" { + Ok(Self) + } else { + Err(Error::new_spanned(accessor, "expected getter")) + } + } +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + if input.peek(Token![pub]) { + return input.parse().map(Self::Getter); + } + + let name = input.parse::<Ident>()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + + input.parse::<Token![=]>()?; + let expression = input.parse::<Expr>()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_keys" => { + expression_list(expression, "additional_keys").map(Self::AdditionalKeys) + } + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result<TokenStream2> { + let struct_name = input.ident; + let generics = input.generics; + let fields = match input.data { + Data::Struct(data) => match data.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs with named fields", + )); + } + }, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs", + )); + } + }; + + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let parses = fields.iter().map(parse_field); + let accessors = fields.iter().map(field_getter); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::std::result::Result<Self, ::std::string::String> { + Ok(Self { + #(#parses,)* + }) + } + } + }) +} + +fn parse_property_field( + field: &Field, + property_options: PropertyOptions, +) -> syn::Result<PropertyField> { + let ident = field + .ident + .clone() + .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; + let PropertyOptions { + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + public_getter, + } = property_options; + + if usize::from(key.is_some()) + usize::from(prefix.is_some()) + usize::from(nested) != 1 { + return Err(Error::new_spanned( + field, + "Properties fields must declare exactly one of key, prefix, or nested in #[property(...)]", + )); + } + + if nested && default.is_some() { + return Err(Error::new_spanned( + field, + "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", + )); + } + if !nested && default.is_none() { + return Err(Error::new_spanned( + field, + "Properties leaf fields must declare default in #[property(...)]", + )); + } + + let map_value_type = hash_map_value_type(&field.ty); + if prefix.is_some() && map_value_type.is_none() { + return Err(Error::new_spanned( + &field.ty, + "property prefix fields must have type HashMap<String, T>", + )); + } + + if additional_keys.is_some() && parse_properties_with.is_none() { + return Err(Error::new_spanned( + field, + "additional_keys requires parse_properties_with in #[property(...)]", + )); + } + if (prefix.is_some() || nested) + && (additional_keys.is_some() || parse_with.is_some() || parse_properties_with.is_some()) + { + return Err(Error::new_spanned( + field, + "prefix and nested fields do not support custom parse functions", + )); + } + if parse_with.is_some() && parse_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both parse_with and parse_properties_with", + )); + } + Ok(PropertyField { + ident, + ty: field.ty.clone(), + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + option_inner_type: option_inner_type(&field.ty), + map_value_type, + public_getter, + doc_attributes: field + .attrs + .iter() + .filter(|attribute| attribute.path().is_ident("doc")) + .cloned() + .collect(), + }) +} + +fn property_options(field: &Field) -> syn::Result<PropertyOptions> { + let Some(attribute) = find_attribute(&field.attrs, "property")? else { + return Err(Error::new_spanned( + field, + "Properties fields must declare #[property(...)]", + )); + }; + + let parsed = + attribute.parse_args_with(Punctuated::<PropertyOption, Token![,]>::parse_terminated)?; + if parsed.is_empty() { + return Err(Error::new_spanned( + attribute, + "property must declare at least one option", + )); + } + + let mut options = PropertyOptions::default(); + for option in parsed { + match option { + PropertyOption::Key(value) => { + set_property_option(&mut options.key, value, attribute, "key")? + } + PropertyOption::AdditionalKeys(value) => set_property_option( + &mut options.additional_keys, + value, + attribute, + "additional_keys", + )?, + PropertyOption::Prefix(value) => { + set_property_option(&mut options.prefix, value, attribute, "prefix")? + } + PropertyOption::Nested => { + if options.nested { + return Err(Error::new_spanned( + attribute, + "duplicate nested property option", + )); + } + options.nested = true; + } + PropertyOption::Default(value) => { + set_property_option(&mut options.default, value, attribute, "default")? + } + PropertyOption::ParseWith(value) => { + set_property_option(&mut options.parse_with, value, attribute, "parse_with")? + } + PropertyOption::ParsePropertiesWith(value) => set_property_option( + &mut options.parse_properties_with, + value, + attribute, + "parse_properties_with", + )?, + PropertyOption::Getter(_) => { + if options.public_getter { + return Err(Error::new_spanned(attribute, "duplicate property accessor")); + } + options.public_getter = true; + } + } + } + + Ok(options) +} + +fn set_property_option<T>( + target: &mut Option<T>, + value: T, + attribute: &Attribute, + name: &str, +) -> syn::Result<()> { + if target.is_some() { + return Err(Error::new_spanned( + attribute, + format!("duplicate {name} property option"), + )); + } + *target = Some(value); + Ok(()) +} + +fn field_getter(field: &PropertyField) -> TokenStream2 { + if !field.public_getter { + return TokenStream2::new(); + } + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + if is_copy_type(ty) { + quote! { + #(#docs)* + pub fn #ident(&self) -> #ty { + self.#ident + } + } + } else { + quote! { + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident + } + } + } +} + +fn expression_path(expression: Expr, name: &str) -> syn::Result<Path> { + match expression { + Expr::Path(ExprPath { path, .. }) => Ok(path), + _ => Err(Error::new_spanned( + expression, + format!("{name} must be a path"), + )), + } +} + +fn expression_list(expression: Expr, name: &str) -> syn::Result<Vec<Expr>> { + let Expr::Array(array) = expression else { + return Err(Error::new_spanned( + expression, + format!("{name} must be an array of keys"), + )); + }; + if array.elems.is_empty() { + return Err(Error::new_spanned( + array, + format!("{name} must contain at least one key"), + )); + } + Ok(array.elems.into_iter().collect()) +} + +fn find_attribute<'a>( + attributes: &'a [Attribute], + name: &str, +) -> syn::Result<Option<&'a Attribute>> { + let mut matching = attributes + .iter() + .filter(|attribute| attribute.path().is_ident(name)); + let first = matching.next(); + if let Some(duplicate) = matching.next() { + return Err(Error::new_spanned( + duplicate, + format!("duplicate #[{name}] attribute"), + )); + } + Ok(first) +} + +fn parse_field(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + let ty = &field.ty; + return quote!(#ident: <#ty>::from_properties(properties)?); + } + + let ty = &field.ty; + let default = typed_default(field); + + if let Some(parse_properties_with) = &field.parse_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match &field.additional_keys { + Some(additional_keys) => { + quote!(#parse_properties_with(properties, #key, &[#(#additional_keys),*], #default)) + } + None => quote!(#parse_properties_with(properties, #key, #default)), + }; + return quote! { + #ident: #parse.map_err(|error| { + format!("Invalid value for {}: {error}", #key) + })? + }; + } + + if let Some(prefix) = &field.prefix { + let value_type = field + .map_value_type + .as_ref() + .expect("prefix fields are validated as maps"); + let parse = if is_bool(value_type) { + quote!(value.to_ascii_lowercase().parse::<#value_type>()) + } else { + quote!(value.parse::<#value_type>()) + }; + return quote! { + #ident: { + let parsed = properties + .iter() + .filter_map(|(key, value)| { + key.strip_prefix(#prefix).map(|suffix| { + #parse + .map(|parsed| (suffix.to_string(), parsed)) + .map_err(|error| format!("Invalid value for {key}: {error}")) + }) + }) + .collect::<::std::result::Result< + ::std::collections::HashMap<_, _>, + ::std::string::String, + >>()?; + if parsed.is_empty() { + #default + } else { + parsed + } + } + }; + } + + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match (&field.parse_with, &field.option_inner_type) { + (Some(parse_with), _) => quote! { Review Comment: This is the same `parse_with`-on-`Option<T>` corner I flagged on #2955 — it's a hard type error now rather than a latent one. The `(Some(parse_with), _)` arm fires unconditionally, ignoring whether the field is `Option<T>`, so for an `Option<String>` field with a `parse_with` returning `Result<String, _>` the generated code assigns a `String` into an `Option<String>` — and the error points into macro output, with nothing rejecting it up front. I'd either reject `parse_with` on `Option<T>`, or generate `Some(#parse_with(value)?)` for option fields. ########## crates/property-macro/src/properties.rs: ########## @@ -0,0 +1,632 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + option_inner_type: Option<Type>, + map_value_type: Option<Type>, + public_getter: bool, + doc_attributes: Vec<Attribute>, +} + +struct PublicGetter; + +enum PropertyOption { + Key(Expr), + AdditionalKeys(Vec<Expr>), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + ParsePropertiesWith(Path), + Getter(PublicGetter), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + public_getter: bool, +} + +impl Parse for PublicGetter { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + input.parse::<Token![pub]>()?; + let content; + parenthesized!(content in input); + let accessor = content.parse::<Ident>()?; + if !content.is_empty() { + return Err(content.error("expected getter")); + } + + if accessor == "getter" { + Ok(Self) + } else { + Err(Error::new_spanned(accessor, "expected getter")) + } + } +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + if input.peek(Token![pub]) { + return input.parse().map(Self::Getter); + } + + let name = input.parse::<Ident>()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + + input.parse::<Token![=]>()?; + let expression = input.parse::<Expr>()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_keys" => { + expression_list(expression, "additional_keys").map(Self::AdditionalKeys) + } + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result<TokenStream2> { + let struct_name = input.ident; + let generics = input.generics; + let fields = match input.data { + Data::Struct(data) => match data.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs with named fields", + )); + } + }, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs", + )); + } + }; + + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let parses = fields.iter().map(parse_field); + let accessors = fields.iter().map(field_getter); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::std::result::Result<Self, ::std::string::String> { + Ok(Self { + #(#parses,)* + }) + } + } + }) +} + +fn parse_property_field( + field: &Field, + property_options: PropertyOptions, +) -> syn::Result<PropertyField> { + let ident = field + .ident + .clone() + .ok_or_else(|| Error::new_spanned(field, "Properties fields must be named"))?; + let PropertyOptions { + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + public_getter, + } = property_options; + + if usize::from(key.is_some()) + usize::from(prefix.is_some()) + usize::from(nested) != 1 { + return Err(Error::new_spanned( + field, + "Properties fields must declare exactly one of key, prefix, or nested in #[property(...)]", + )); + } + + if nested && default.is_some() { + return Err(Error::new_spanned( + field, + "nested fields obtain defaults from their own property annotations and cannot declare default in #[property(...)]", + )); + } + if !nested && default.is_none() { + return Err(Error::new_spanned( + field, + "Properties leaf fields must declare default in #[property(...)]", + )); + } + + let map_value_type = hash_map_value_type(&field.ty); + if prefix.is_some() && map_value_type.is_none() { + return Err(Error::new_spanned( + &field.ty, + "property prefix fields must have type HashMap<String, T>", + )); + } + + if additional_keys.is_some() && parse_properties_with.is_none() { + return Err(Error::new_spanned( + field, + "additional_keys requires parse_properties_with in #[property(...)]", + )); + } + if (prefix.is_some() || nested) + && (additional_keys.is_some() || parse_with.is_some() || parse_properties_with.is_some()) + { + return Err(Error::new_spanned( + field, + "prefix and nested fields do not support custom parse functions", + )); + } + if parse_with.is_some() && parse_properties_with.is_some() { + return Err(Error::new_spanned( + field, + "fields cannot declare both parse_with and parse_properties_with", + )); + } + Ok(PropertyField { + ident, + ty: field.ty.clone(), + key, + additional_keys, + prefix, + nested, + default, + parse_with, + parse_properties_with, + option_inner_type: option_inner_type(&field.ty), + map_value_type, + public_getter, + doc_attributes: field + .attrs + .iter() + .filter(|attribute| attribute.path().is_ident("doc")) + .cloned() + .collect(), + }) +} + +fn property_options(field: &Field) -> syn::Result<PropertyOptions> { + let Some(attribute) = find_attribute(&field.attrs, "property")? else { + return Err(Error::new_spanned( + field, + "Properties fields must declare #[property(...)]", + )); + }; + + let parsed = + attribute.parse_args_with(Punctuated::<PropertyOption, Token![,]>::parse_terminated)?; + if parsed.is_empty() { + return Err(Error::new_spanned( + attribute, + "property must declare at least one option", + )); + } + + let mut options = PropertyOptions::default(); + for option in parsed { + match option { + PropertyOption::Key(value) => { + set_property_option(&mut options.key, value, attribute, "key")? + } + PropertyOption::AdditionalKeys(value) => set_property_option( + &mut options.additional_keys, + value, + attribute, + "additional_keys", + )?, + PropertyOption::Prefix(value) => { + set_property_option(&mut options.prefix, value, attribute, "prefix")? + } + PropertyOption::Nested => { + if options.nested { + return Err(Error::new_spanned( + attribute, + "duplicate nested property option", + )); + } + options.nested = true; + } + PropertyOption::Default(value) => { + set_property_option(&mut options.default, value, attribute, "default")? + } + PropertyOption::ParseWith(value) => { + set_property_option(&mut options.parse_with, value, attribute, "parse_with")? + } + PropertyOption::ParsePropertiesWith(value) => set_property_option( + &mut options.parse_properties_with, + value, + attribute, + "parse_properties_with", + )?, + PropertyOption::Getter(_) => { + if options.public_getter { + return Err(Error::new_spanned(attribute, "duplicate property accessor")); + } + options.public_getter = true; + } + } + } + + Ok(options) +} + +fn set_property_option<T>( + target: &mut Option<T>, + value: T, + attribute: &Attribute, + name: &str, +) -> syn::Result<()> { + if target.is_some() { + return Err(Error::new_spanned( + attribute, + format!("duplicate {name} property option"), + )); + } + *target = Some(value); + Ok(()) +} + +fn field_getter(field: &PropertyField) -> TokenStream2 { + if !field.public_getter { + return TokenStream2::new(); + } + let ident = &field.ident; + let ty = &field.ty; + let docs = &field.doc_attributes; + if is_copy_type(ty) { + quote! { + #(#docs)* + pub fn #ident(&self) -> #ty { + self.#ident + } + } + } else { + quote! { + #(#docs)* + pub fn #ident(&self) -> &#ty { + &self.#ident + } + } + } +} + +fn expression_path(expression: Expr, name: &str) -> syn::Result<Path> { + match expression { + Expr::Path(ExprPath { path, .. }) => Ok(path), + _ => Err(Error::new_spanned( + expression, + format!("{name} must be a path"), + )), + } +} + +fn expression_list(expression: Expr, name: &str) -> syn::Result<Vec<Expr>> { + let Expr::Array(array) = expression else { + return Err(Error::new_spanned( + expression, + format!("{name} must be an array of keys"), + )); + }; + if array.elems.is_empty() { + return Err(Error::new_spanned( + array, + format!("{name} must contain at least one key"), + )); + } + Ok(array.elems.into_iter().collect()) +} + +fn find_attribute<'a>( + attributes: &'a [Attribute], + name: &str, +) -> syn::Result<Option<&'a Attribute>> { + let mut matching = attributes + .iter() + .filter(|attribute| attribute.path().is_ident(name)); + let first = matching.next(); + if let Some(duplicate) = matching.next() { + return Err(Error::new_spanned( + duplicate, + format!("duplicate #[{name}] attribute"), + )); + } + Ok(first) +} + +fn parse_field(field: &PropertyField) -> TokenStream2 { + let ident = &field.ident; + if field.nested { + let ty = &field.ty; + return quote!(#ident: <#ty>::from_properties(properties)?); + } + + let ty = &field.ty; + let default = typed_default(field); + + if let Some(parse_properties_with) = &field.parse_properties_with { + let key = field.key.as_ref().expect("exact-key fields have a key"); + let parse = match &field.additional_keys { + Some(additional_keys) => { + quote!(#parse_properties_with(properties, #key, &[#(#additional_keys),*], #default)) + } + None => quote!(#parse_properties_with(properties, #key, #default)), Review Comment: This is the same shape as the "reject unpaired annotations at compile time" thread from #2955 — back then it was `#[write_properties_with]` without `#[parse_properties_with]`. The analogue here: `parse_properties_with` without `additional_keys` emits a 3-arg call, with it a 4-arg call, and the README only documents the 4-arg form. The reverse check exists (`additional_keys` requires `parse_properties_with`) but not this direction, so the mismatch is a confusing arity error deep in generated code. I'd reject the mismatch at compile time, or always pass the slice (`&[]` when absent) for one stable signature. ########## crates/property-macro/src/properties.rs: ########## @@ -0,0 +1,632 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use proc_macro2::TokenStream as TokenStream2; +use quote::quote; +use syn::parse::{Parse, ParseStream}; +use syn::punctuated::Punctuated; +use syn::{ + Attribute, Data, DeriveInput, Error, Expr, ExprLit, ExprPath, Field, Fields, GenericArgument, + Ident, Lit, Path, PathArguments, Token, Type, parenthesized, +}; + +struct PropertyField { + ident: Ident, + ty: Type, + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + option_inner_type: Option<Type>, + map_value_type: Option<Type>, + public_getter: bool, + doc_attributes: Vec<Attribute>, +} + +struct PublicGetter; + +enum PropertyOption { + Key(Expr), + AdditionalKeys(Vec<Expr>), + Prefix(Expr), + Nested, + Default(Expr), + ParseWith(Path), + ParsePropertiesWith(Path), + Getter(PublicGetter), +} + +#[derive(Default)] +struct PropertyOptions { + key: Option<Expr>, + additional_keys: Option<Vec<Expr>>, + prefix: Option<Expr>, + nested: bool, + default: Option<Expr>, + parse_with: Option<Path>, + parse_properties_with: Option<Path>, + public_getter: bool, +} + +impl Parse for PublicGetter { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + input.parse::<Token![pub]>()?; + let content; + parenthesized!(content in input); + let accessor = content.parse::<Ident>()?; + if !content.is_empty() { + return Err(content.error("expected getter")); + } + + if accessor == "getter" { + Ok(Self) + } else { + Err(Error::new_spanned(accessor, "expected getter")) + } + } +} + +impl Parse for PropertyOption { + fn parse(input: ParseStream<'_>) -> syn::Result<Self> { + if input.peek(Token![pub]) { + return input.parse().map(Self::Getter); + } + + let name = input.parse::<Ident>()?; + let option_name = name.to_string(); + if option_name == "nested" { + return Ok(Self::Nested); + } + + input.parse::<Token![=]>()?; + let expression = input.parse::<Expr>()?; + match option_name.as_str() { + "key" => Ok(Self::Key(expression)), + "additional_keys" => { + expression_list(expression, "additional_keys").map(Self::AdditionalKeys) + } + "prefix" => Ok(Self::Prefix(expression)), + "default" => Ok(Self::Default(expression)), + "parse_with" => expression_path(expression, "parse_with").map(Self::ParseWith), + "parse_properties_with" => { + expression_path(expression, "parse_properties_with").map(Self::ParsePropertiesWith) + } + _ => Err(Error::new_spanned(name, "unknown property option")), + } + } +} + +pub(crate) fn expand_properties(input: DeriveInput) -> syn::Result<TokenStream2> { + let struct_name = input.ident; + let generics = input.generics; + let fields = match input.data { + Data::Struct(data) => match data.fields { + Fields::Named(fields) => fields.named, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs with named fields", + )); + } + }, + _ => { + return Err(Error::new_spanned( + struct_name, + "Properties can only be derived for structs", + )); + } + }; + + let fields = fields + .iter() + .map(|field| parse_property_field(field, property_options(field)?)) + .collect::<syn::Result<Vec<_>>>()?; + let parses = fields.iter().map(parse_field); + let accessors = fields.iter().map(field_getter); + let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); + + Ok(quote! { + impl #impl_generics #struct_name #type_generics #where_clause { + #(#accessors)* + + pub fn from_properties( + properties: &::std::collections::HashMap< + ::std::string::String, + ::std::string::String, + >, + ) -> ::std::result::Result<Self, ::std::string::String> { Review Comment: `from_properties` returns `Result<Self, String>`, but the rest of the codebase speaks `iceberg::Error`/`ErrorKind::DataInvalid` — `TableProperties::try_from` and `parse_property` both return `Result<_, iceberg::Error>`. Every callsite in the eventual port would have to `.map_err` to bridge the two, which cuts against the "no observable behavior changes" bar. I'd either generate an `iceberg::Error` (the macro itself needs no iceberg dep since the code expands in the consumer crate), make the error type configurable, or justify the `String` choice in the PR description. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
