-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
fix(forge
): detect invalid inline config keys
#9363
base: master
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,13 @@ use super::{remove_whitespaces, InlineConfigParserError}; | |
use crate::{inline::INLINE_CONFIG_PREFIX, InlineConfigError, NatSpec}; | ||
use regex::Regex; | ||
|
||
/// Used to detect invalid configuration keys in the inline config. | ||
/// | ||
/// # Example | ||
/// | ||
/// forge-config: default.fuzz.runs = 100 - `fuzz` is a valid key | ||
/// forge-config: default.wrong.runs = 100 - `wrong` is an invalid key | ||
const VALID_CONFIG_KEYS: [&str; 2] = ["fuzz", "invariant"]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this isn't too simplistic? |
||
/// This trait is intended to parse configurations from | ||
/// structured text. Foundry users can annotate Solidity test functions, | ||
/// providing special configs just for the execution of a specific test. | ||
|
@@ -41,10 +48,27 @@ where | |
fn merge(&self, natspec: &NatSpec) -> Result<Option<Self>, InlineConfigError> { | ||
let config_key = Self::config_key(); | ||
|
||
let configs = natspec | ||
.current_profile_configs() | ||
.filter(|l| l.contains(&config_key)) | ||
.collect::<Vec<String>>(); | ||
let mut configs = Vec::new(); | ||
for line in natspec.current_profile_configs() { | ||
if !VALID_CONFIG_KEYS.iter().any(|&key| line.contains(key)) { | ||
let wrong_key = line | ||
.split('.') | ||
.nth(1) // Get the part after the first dot (profile.KEY.value) | ||
.unwrap_or("unknown-config-key") | ||
.to_string(); | ||
return Err(InlineConfigError { | ||
line, | ||
source: InlineConfigParserError::InvalidConfigKey( | ||
wrong_key, | ||
format!("{VALID_CONFIG_KEYS:?}"), | ||
), | ||
}); | ||
Comment on lines
+59
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think a warning would be sufficient here given that the key is simply ignored when continuing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warnings won't be shown if Moreover, currently we error out on invalid property keys like |
||
} | ||
|
||
if line.contains(&config_key) { | ||
configs.push(line); | ||
} | ||
} | ||
|
||
self.try_merge(&configs).map_err(|e| { | ||
let line = natspec.debug_context(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now this is sufficient I think, assuming this functionality will be updated when we have full support for in-line configuration