Skip to content

Fix unsupported_field! macro to properly handle recursive cases #121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix unsupported_field! macro to properly handle recursive cases #121

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 19 additions & 17 deletions orion-configuration/src/config/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,26 +341,28 @@ pub(crate) mod envoy_conversions {
// it would be nice to allow for x = "y" syntax to overwrite the field name, since some fields are
// named differently in the code vs config file and having the code-local name might confuse an end-user
macro_rules! unsupported_field {
($field:ident) => {
($field:ident) => {{
if $field.is_used() {
#[allow(dropping_copy_types, clippy::drop_non_drop)]
drop($field);
Err(GenericError::UnsupportedField(stringify!($field)))
} else {
#[allow(dropping_copy_types, clippy::drop_non_drop)]
drop($field);
Ok(())
tracing::warn!(
"unsupported field '{}' used in configuration. This field will be ignored.",
stringify!($field)
);
}
};
($field:ident, $($tail:ident),+) => {
if $field.is_used() {
#[allow(dropping_copy_types, clippy::drop_non_drop)]
drop($field);
Err(GenericError::UnsupportedField(stringify!($field)))
} else {
unsupported_field! ($($tail),+)
#[allow(dropping_copy_types, clippy::drop_non_drop)]
drop($field);
Result::<(), GenericError>::Ok(())
}};
($field:ident, $($tail:ident),+) => {{
if $field.is_used() {
tracing::warn!(
"unsupported field '{}' used in configuration. This field will be ignored.",
stringify!($field)
);
}
};
#[allow(dropping_copy_types, clippy::drop_non_drop)]
drop($field);
unsupported_field!($($tail),+)
Copy link

Copilot AI Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recursive call to unsupported_field! is missing a return statement or value propagation. The result of this recursive call should be returned, otherwise the recursion will expand but its result will be discarded. Consider wrapping this in a return or ensuring the value is properly used.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The recursive call unsupported_field!($($tail),+) might not return the Result as intended because the outer block will always return Ok(()). Consider returning the result of the recursive call to propagate any potential errors.

I'm not sure if this is the intended behavior, but it's something to consider.

Suggested change
unsupported_field!($($tail),+)
unsupported_field!($($tail),+)?

}};

}
pub(crate) use unsupported_field;
Expand Down
Loading