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

Explicitly ban overriding extend as part of a --config flag #10135

Merged
merged 2 commits into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
96 changes: 56 additions & 40 deletions crates/ruff/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -745,38 +745,34 @@ fn resolve_bool_arg(yes: bool, no: bool) -> Option<bool> {
}
}

/// Enumeration of various errors that could occur
/// when trying to parse a --config CLI flag value
#[derive(Debug)]
enum TomlParseFailureKind {
SyntaxError,
UnknownOption,
enum InvalidConfigFlagError {
InvalidToml(toml::de::Error),
/// It was valid TOML, but not a valid ruff config file.
/// E.g. the user tried to select a rule that doesn't exist,
/// or tried to enable a setting that doesn't exist
ValidTomlButInvalidRuffSchema(toml::de::Error),
/// It was a valid ruff config file, but the user tried to pass a
/// value for `extend` as part of the config override.
// `extend` is special, because it affects which config files we look at
/// in the first place. We currently only parse --config overrides *after*
/// we've combined them with all the arguments from the various config files
/// that we found, so trying to override `extend` as part of a --config
/// override is forbidden.
ExtendPassedViaConfigFlag,
}

impl std::fmt::Display for TomlParseFailureKind {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
let display = match self {
Self::SyntaxError => "The supplied argument is not valid TOML",
Self::UnknownOption => {
impl InvalidConfigFlagError {
AlexWaygood marked this conversation as resolved.
Show resolved Hide resolved
const fn description(&self) -> &'static str {
match self {
Self::InvalidToml(_) => "The supplied argument is not valid TOML",
Self::ValidTomlButInvalidRuffSchema(_) => {
"Could not parse the supplied argument as a `ruff.toml` configuration option"
}
};
write!(f, "{display}")
}
}

#[derive(Debug)]
struct TomlParseFailure {
kind: TomlParseFailureKind,
underlying_error: toml::de::Error,
}

impl std::fmt::Display for TomlParseFailure {
fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
let TomlParseFailure {
kind,
underlying_error,
} = self;
let display = format!("{kind}:\n\n{underlying_error}");
write!(f, "{}", display.trim_end())
Self::ExtendPassedViaConfigFlag => "Cannot include `extend` in a --config flag value",
}
}
}

Expand Down Expand Up @@ -827,18 +823,19 @@ impl TypedValueParser for ConfigArgumentParser {
.to_str()
.ok_or_else(|| clap::Error::new(clap::error::ErrorKind::InvalidUtf8))?;

let toml_parse_error = match toml::Table::from_str(value) {
Ok(table) => match table.try_into() {
Ok(option) => return Ok(SingleConfigArgument::SettingsOverride(Arc::new(option))),
Err(underlying_error) => TomlParseFailure {
kind: TomlParseFailureKind::UnknownOption,
underlying_error,
},
},
Err(underlying_error) => TomlParseFailure {
kind: TomlParseFailureKind::SyntaxError,
underlying_error,
let config_parse_error = match toml::Table::from_str(value) {
Ok(table) => match table.try_into::<Options>() {
Ok(option) => {
if option.extend.is_none() {
return Ok(SingleConfigArgument::SettingsOverride(Arc::new(option)));
}
InvalidConfigFlagError::ExtendPassedViaConfigFlag
}
Err(underlying_error) => {
InvalidConfigFlagError::ValidTomlButInvalidRuffSchema(underlying_error)
}
},
Err(underlying_error) => InvalidConfigFlagError::InvalidToml(underlying_error),
};

let mut new_error = clap::Error::new(clap::error::ErrorKind::ValueValidation).with_cmd(cmd);
Expand All @@ -853,6 +850,21 @@ impl TypedValueParser for ConfigArgumentParser {
clap::error::ContextValue::String(value.to_string()),
);

let underlying_error = match &config_parse_error {
InvalidConfigFlagError::ExtendPassedViaConfigFlag => {
let tip = config_parse_error.description().into();
new_error.insert(
clap::error::ContextKind::Suggested,
clap::error::ContextValue::StyledStrs(vec![tip]),
);
return Err(new_error);
}
InvalidConfigFlagError::InvalidToml(underlying_error)
| InvalidConfigFlagError::ValidTomlButInvalidRuffSchema(underlying_error) => {
underlying_error
}
};

// small hack so that multiline tips
// have the same indent on the left-hand side:
let tip_indent = " ".repeat(" tip: ".len());
Expand Down Expand Up @@ -881,12 +893,16 @@ The path `{value}` does not exist"
));
}
} else if value.contains('=') {
tip.push_str(&format!("\n\n{toml_parse_error}"));
tip.push_str(&format!(
"\n\n{}:\n\n{underlying_error}",
config_parse_error.description()
));
}
let tip = tip.trim_end().to_owned().into();

new_error.insert(
clap::error::ContextKind::Suggested,
clap::error::ContextValue::StyledStrs(vec![tip.into()]),
clap::error::ContextValue::StyledStrs(vec![tip]),
);

Err(new_error)
Expand Down
18 changes: 18 additions & 0 deletions crates/ruff/tests/lint.rs
Copy link
Member Author

Choose a reason for hiding this comment

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

This should also ban passing extend as part of a --config flag when doing ruff format, but I didn't bother adding an equivalent test in crates/ruff/tests/format.rs, as it just felt duplicative. Lmk if you disagree!

Original file line number Diff line number Diff line change
Expand Up @@ -595,6 +595,24 @@ fn too_many_config_files() -> Result<()> {
Ok(())
}

#[test]
fn extend_passed_via_config_argument() {
assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME))
.args(STDIN_BASE_OPTIONS)
.args(["--config", "extend = 'foo.toml'", "."]), @r###"
success: false
exit_code: 2
----- stdout -----

----- stderr -----
error: invalid value 'extend = 'foo.toml'' for '--config <CONFIG_OPTION>'

tip: Cannot include `extend` in a --config flag value

For more information, try '--help'.
"###);
}

#[test]
fn config_file_and_isolated() -> Result<()> {
let tempdir = TempDir::new()?;
Expand Down