Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
32 changes: 28 additions & 4 deletions crates/config/src/inline/conf_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"];
Comment on lines +5 to +11
Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this isn't too simplistic?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is for now. But it won't be needed when we have full support for inline config.

/// 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.
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

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

Warnings won't be shown if --quiet is enabled.

Moreover, currently we error out on invalid property keys like fuzz.runssss. This keeps it consistent with that i.e err thrown on invalid keys.

}

if line.contains(&config_key) {
configs.push(line);
}
}

self.try_merge(&configs).map_err(|e| {
let line = natspec.debug_context();
Expand Down
3 changes: 3 additions & 0 deletions crates/config/src/inline/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ pub enum InlineConfigParserError {
/// The property cannot be mapped to the configuration object
#[error("'{0}' is an invalid config property")]
InvalidConfigProperty(String),
/// An invalid config key has been provided
#[error("'{0}' is an invalid config key. Available config keys are: {1}")]
InvalidConfigKey(String, String),
/// An invalid profile has been provided
#[error("'{0}' specifies an invalid profile. Available profiles are: {1}")]
InvalidProfile(String, String),
Expand Down
5 changes: 4 additions & 1 deletion crates/config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,10 @@ mod invariant;
pub use invariant::InvariantConfig;

mod inline;
pub use inline::{validate_profiles, InlineConfig, InlineConfigError, InlineConfigParser, NatSpec};
pub use inline::{
validate_profiles, InlineConfig, InlineConfigError, InlineConfigParser,
InlineConfigParserError, NatSpec,
};

pub mod soldeer;
use soldeer::{SoldeerConfig, SoldeerDependencyConfig};
Expand Down
29 changes: 28 additions & 1 deletion crates/forge/tests/it/inline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use crate::test_helpers::{ForgeTestData, ForgeTestProfile, TEST_DATA_DEFAULT};
use forge::{result::TestKind, TestOptionsBuilder};
use foundry_config::{FuzzConfig, InvariantConfig};
use foundry_test_utils::Filter;
use foundry_test_utils::{forgetest_init, Filter};

#[tokio::test(flavor = "multi_thread")]
async fn inline_config_run_fuzz() {
Expand Down Expand Up @@ -98,3 +98,30 @@ fn build_test_options_just_one_valid_profile() {
// per-test configs for "default" and "ci" profiles
assert!(build_result.is_err());
}

forgetest_init!(tests_invalid_config_key, |prj, cmd| {
prj.wipe_contracts();

prj.insert_ds_test();

prj.add_test(
"InvalidInlineConf.t.sol",
r#"
import {Test} from "forge-std/Test.sol";

contract InvalidInlineConf is Test {
/// forge-config: default.fuzz.runs = 1024
/// forge-config: default.invalid.runs = 1024
function testInvalidInlineConf() public {}
}
"#,
)
.unwrap();

cmd.args(["test", "--mt", "testInvalidInlineConf"]).assert_failure().stderr_eq(
r#"Error: Inline config error detected at forge-config:default.invalid.runs=1024

Context:
- 'invalid' is an invalid config key. Available config keys are: [..]"#,
);
});
Loading