From d33424ec9d914d17845d90f1106fdd838aaef55f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 18 Jan 2023 20:44:24 -0500 Subject: [PATCH] Enable suppression of magic values by type (#1987) Closes #1949. --- README.md | 19 ++++ .../fixtures/pylint/magic_value_comparison.py | 38 ++++---- ruff.schema.json | 41 +++++++++ src/flake8_to_ruff/converter.rs | 7 ++ src/rules/pylint/mod.rs | 17 ++++ .../pylint/rules/magic_value_comparison.rs | 25 +++--- src/rules/pylint/settings.rs | 87 +++++++++++++++++++ ...ts__PLR2004_magic_value_comparison.py.snap | 38 ++++---- ...ylint__tests__allow_magic_value_types.snap | 38 ++++++++ src/settings/configuration.rs | 5 +- src/settings/defaults.rs | 3 +- src/settings/hashable.rs | 6 +- src/settings/mod.rs | 4 +- src/settings/options.rs | 5 +- src/settings/pyproject.rs | 6 ++ src/violations.rs | 10 ++- 16 files changed, 287 insertions(+), 62 deletions(-) create mode 100644 src/rules/pylint/settings.rs create mode 100644 src/rules/pylint/snapshots/ruff__rules__pylint__tests__allow_magic_value_types.snap diff --git a/README.md b/README.md index b0a70fc8b9..c002969d05 100644 --- a/README.md +++ b/README.md @@ -3407,6 +3407,25 @@ convention = "google" --- +### `pylint` + +#### [`allow-magic-value-types`](#allow-magic-value-types) + +Constant types to ignore when used as "magic values". + +**Default value**: `["str"]` + +**Type**: `Vec` + +**Example usage**: + +```toml +[tool.ruff.pylint] +allow-magic-value-types = ["int"] +``` + +--- + ### `pyupgrade` #### [`keep-runtime-typing`](#keep-runtime-typing) diff --git a/resources/test/fixtures/pylint/magic_value_comparison.py b/resources/test/fixtures/pylint/magic_value_comparison.py index c6a57de11d..85e0b2a0ea 100644 --- a/resources/test/fixtures/pylint/magic_value_comparison.py +++ b/resources/test/fixtures/pylint/magic_value_comparison.py @@ -1,71 +1,69 @@ -"""Check that magic values are not used in comparisons""" -import cmath +"""Check that magic values are not used in comparisons.""" user_input = 10 -if 10 > user_input: # [magic-value-comparison] +if 10 > user_input: # [magic-value-comparison] pass -if 10 == 100: # [comparison-of-constants] R0133 +if 10 == 100: # [comparison-of-constants] R0133 pass -if 1 == 3: # [comparison-of-constants] R0133 +if 1 == 3: # [comparison-of-constants] R0133 pass x = 0 -if 4 == 3 == x: # [comparison-of-constants] R0133 +if 4 == 3 == x: # [comparison-of-constants] R0133 pass time_delta = 7224 ONE_HOUR = 3600 -if time_delta > ONE_HOUR: # correct +if time_delta > ONE_HOUR: # correct pass argc = 1 -if argc != -1: # correct +if argc != -1: # correct pass -if argc != 0: # correct +if argc != 0: # correct pass -if argc != 1: # correct +if argc != 1: # correct pass -if argc != 2: # [magic-value-comparison] +if argc != 2: # [magic-value-comparison] pass -if __name__ == "__main__": # correct +if __name__ == "__main__": # correct pass ADMIN_PASSWORD = "SUPERSECRET" input_password = "password" -if input_password == "": # correct +if input_password == "": # correct pass -if input_password == ADMIN_PASSWORD: # correct +if input_password == ADMIN_PASSWORD: # correct pass -if input_password == "Hunter2": # [magic-value-comparison] +if input_password == "Hunter2": # [magic-value-comparison] pass PI = 3.141592653589793238 pi_estimation = 3.14 -if pi_estimation == 3.141592653589793238: # [magic-value-comparison] +if pi_estimation == 3.141592653589793238: # [magic-value-comparison] pass -if pi_estimation == PI: # correct +if pi_estimation == PI: # correct pass HELLO_WORLD = b"Hello, World!" user_input = b"Hello, There!" -if user_input == b"something": # [magic-value-comparison] +if user_input == b"something": # [magic-value-comparison] pass -if user_input == HELLO_WORLD: # correct +if user_input == HELLO_WORLD: # correct pass - diff --git a/ruff.schema.json b/ruff.schema.json index 2152c6ae12..2e7e2cc3f2 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -341,6 +341,17 @@ } ] }, + "pylint": { + "description": "Options for the `pylint` plugin.", + "anyOf": [ + { + "$ref": "#/definitions/PylintOptions" + }, + { + "type": "null" + } + ] + }, "pyupgrade": { "description": "Options for the `pyupgrade` plugin.", "anyOf": [ @@ -461,6 +472,20 @@ }, "additionalProperties": false }, + "ConstantType": { + "type": "string", + "enum": [ + "bool", + "bytes", + "complex", + "ellipsis", + "float", + "int", + "none", + "str", + "tuple" + ] + }, "Convention": { "oneOf": [ { @@ -1046,6 +1071,22 @@ }, "additionalProperties": false }, + "PylintOptions": { + "type": "object", + "properties": { + "allow-magic-value-types": { + "description": "Constant types to ignore when used as \"magic values\".", + "type": [ + "array", + "null" + ], + "items": { + "$ref": "#/definitions/ConstantType" + } + } + }, + "additionalProperties": false + }, "PythonVersion": { "type": "string", "enum": [ diff --git a/src/flake8_to_ruff/converter.rs b/src/flake8_to_ruff/converter.rs index 5269369058..b7b72bed53 100644 --- a/src/flake8_to_ruff/converter.rs +++ b/src/flake8_to_ruff/converter.rs @@ -454,6 +454,7 @@ mod tests { pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }); assert_eq!(actual, expected); @@ -520,6 +521,7 @@ mod tests { pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }); assert_eq!(actual, expected); @@ -586,6 +588,7 @@ mod tests { pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }); assert_eq!(actual, expected); @@ -652,6 +655,7 @@ mod tests { pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }); assert_eq!(actual, expected); @@ -723,6 +727,7 @@ mod tests { pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }); assert_eq!(actual, expected); @@ -795,6 +800,7 @@ mod tests { pydocstyle: Some(pydocstyle::settings::Options { convention: Some(Convention::Numpy), }), + pylint: None, pyupgrade: None, }); assert_eq!(actual, expected); @@ -867,6 +873,7 @@ mod tests { pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }); assert_eq!(actual, expected); diff --git a/src/rules/pylint/mod.rs b/src/rules/pylint/mod.rs index de02b819a7..36c2de76a7 100644 --- a/src/rules/pylint/mod.rs +++ b/src/rules/pylint/mod.rs @@ -1,5 +1,6 @@ //! Rules from [Pylint](https://pypi.org/project/pylint/2.15.7/). pub(crate) mod rules; +pub mod settings; #[cfg(test)] mod tests { @@ -10,6 +11,7 @@ mod tests { use crate::linter::test_path; use crate::registry::RuleCode; + use crate::rules::pylint; use crate::settings::Settings; #[test_case(RuleCode::PLC0414, Path::new("import_aliasing.py"); "PLC0414")] @@ -42,4 +44,19 @@ mod tests { insta::assert_yaml_snapshot!(snapshot, diagnostics); Ok(()) } + + #[test] + fn allow_magic_value_types() -> Result<()> { + let diagnostics = test_path( + Path::new("./resources/test/fixtures/pylint/magic_value_comparison.py"), + &Settings { + pylint: pylint::settings::Settings { + allow_magic_value_types: vec![pylint::settings::ConstantType::Int], + }, + ..Settings::for_rules(vec![RuleCode::PLR2004]) + }, + )?; + insta::assert_yaml_snapshot!(diagnostics); + Ok(()) + } } diff --git a/src/rules/pylint/rules/magic_value_comparison.rs b/src/rules/pylint/rules/magic_value_comparison.rs index 839a51df06..6007c22ec0 100644 --- a/src/rules/pylint/rules/magic_value_comparison.rs +++ b/src/rules/pylint/rules/magic_value_comparison.rs @@ -4,20 +4,25 @@ use rustpython_ast::{Constant, Expr, ExprKind}; use crate::ast::types::Range; use crate::checkers::ast::Checker; use crate::registry::Diagnostic; +use crate::rules::pylint::settings::ConstantType; use crate::violations; -fn is_magic_value(constant: &Constant) -> bool { +fn is_magic_value(constant: &Constant, allowed_types: &[ConstantType]) -> bool { + if allowed_types.contains(&constant.into()) { + return false; + } match constant { + // Ignore `None`, `Bool`, and `Ellipsis` constants. Constant::None => false, - // E712 `if True == do_something():` Constant::Bool(_) => false, + Constant::Ellipsis => false, + // Otherwise, special-case some common string and integer types. Constant::Str(value) => !matches!(value.as_str(), "" | "__main__"), - Constant::Bytes(_) => true, Constant::Int(value) => !matches!(value.try_into(), Ok(-1 | 0 | 1)), + Constant::Bytes(_) => true, Constant::Tuple(_) => true, Constant::Float(_) => true, Constant::Complex { .. } => true, - Constant::Ellipsis => true, } } @@ -38,13 +43,13 @@ pub fn magic_value_comparison(checker: &mut Checker, left: &Expr, comparators: & for comparison_expr in std::iter::once(left).chain(comparators.iter()) { if let ExprKind::Constant { value, .. } = &comparison_expr.node { - if is_magic_value(value) { - let diagnostic = Diagnostic::new( - violations::MagicValueComparison(value.to_string()), + if is_magic_value(value, &checker.settings.pylint.allow_magic_value_types) { + checker.diagnostics.push(Diagnostic::new( + violations::MagicValueComparison { + value: value.to_string(), + }, Range::from_located(comparison_expr), - ); - - checker.diagnostics.push(diagnostic); + )); } } } diff --git a/src/rules/pylint/settings.rs b/src/rules/pylint/settings.rs new file mode 100644 index 0000000000..e50a05c762 --- /dev/null +++ b/src/rules/pylint/settings.rs @@ -0,0 +1,87 @@ +//! Settings for the `pylint` plugin. + +use ruff_macros::ConfigurationOptions; +use rustpython_ast::Constant; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize, Hash, JsonSchema)] +#[serde(deny_unknown_fields, rename_all = "kebab-case")] +pub enum ConstantType { + Bool, + Bytes, + Complex, + Ellipsis, + Float, + Int, + None, + Str, + Tuple, +} + +impl From<&Constant> for ConstantType { + fn from(value: &Constant) -> Self { + match value { + Constant::Bool(..) => Self::Bool, + Constant::Bytes(..) => Self::Bytes, + Constant::Complex { .. } => Self::Complex, + Constant::Ellipsis => Self::Ellipsis, + Constant::Float(..) => Self::Float, + Constant::Int(..) => Self::Int, + Constant::None => Self::None, + Constant::Str(..) => Self::Str, + Constant::Tuple(..) => Self::Tuple, + } + } +} + +#[derive( + Debug, PartialEq, Eq, Serialize, Deserialize, Default, ConfigurationOptions, JsonSchema, +)] +#[serde( + deny_unknown_fields, + rename_all = "kebab-case", + rename = "PylintOptions" +)] +pub struct Options { + #[option( + default = r#"["str"]"#, + value_type = "Vec", + example = r#" + allow-magic-value-types = ["int"] + "# + )] + /// Constant types to ignore when used as "magic values". + pub allow_magic_value_types: Option>, +} + +#[derive(Debug, Hash)] +pub struct Settings { + pub allow_magic_value_types: Vec, +} + +impl Default for Settings { + fn default() -> Self { + Self { + allow_magic_value_types: vec![ConstantType::Str], + } + } +} + +impl From for Settings { + fn from(options: Options) -> Self { + Self { + allow_magic_value_types: options + .allow_magic_value_types + .unwrap_or_else(|| vec![ConstantType::Str]), + } + } +} + +impl From for Options { + fn from(settings: Settings) -> Self { + Self { + allow_magic_value_types: Some(settings.allow_magic_value_types), + } + } +} diff --git a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR2004_magic_value_comparison.py.snap b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR2004_magic_value_comparison.py.snap index e0e92c2775..76d3539aa6 100644 --- a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR2004_magic_value_comparison.py.snap +++ b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLR2004_magic_value_comparison.py.snap @@ -3,52 +3,46 @@ source: src/rules/pylint/mod.rs expression: diagnostics --- - kind: - MagicValueComparison: "10" + MagicValueComparison: + value: "10" location: - row: 6 + row: 5 column: 3 end_location: - row: 6 + row: 5 column: 5 fix: ~ parent: ~ - kind: - MagicValueComparison: "2" + MagicValueComparison: + value: "2" location: - row: 36 + row: 35 column: 11 end_location: - row: 36 + row: 35 column: 12 fix: ~ parent: ~ - kind: - MagicValueComparison: "'Hunter2'" + MagicValueComparison: + value: "3.141592653589793" location: - row: 51 - column: 21 - end_location: - row: 51 - column: 30 - fix: ~ - parent: ~ -- kind: - MagicValueComparison: "3.141592653589793" - location: - row: 57 + row: 56 column: 20 end_location: - row: 57 + row: 56 column: 40 fix: ~ parent: ~ - kind: - MagicValueComparison: "b'something'" + MagicValueComparison: + value: "b'something'" location: - row: 66 + row: 65 column: 17 end_location: - row: 66 + row: 65 column: 29 fix: ~ parent: ~ diff --git a/src/rules/pylint/snapshots/ruff__rules__pylint__tests__allow_magic_value_types.snap b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__allow_magic_value_types.snap new file mode 100644 index 0000000000..7c7bfe0b40 --- /dev/null +++ b/src/rules/pylint/snapshots/ruff__rules__pylint__tests__allow_magic_value_types.snap @@ -0,0 +1,38 @@ +--- +source: src/rules/pylint/mod.rs +expression: diagnostics +--- +- kind: + MagicValueComparison: + value: "'Hunter2'" + location: + row: 50 + column: 21 + end_location: + row: 50 + column: 30 + fix: ~ + parent: ~ +- kind: + MagicValueComparison: + value: "3.141592653589793" + location: + row: 56 + column: 20 + end_location: + row: 56 + column: 40 + fix: ~ + parent: ~ +- kind: + MagicValueComparison: + value: "b'something'" + location: + row: 65 + column: 17 + end_location: + row: 65 + column: 29 + fix: ~ + parent: ~ + diff --git a/src/settings/configuration.rs b/src/settings/configuration.rs index 6092e32c3c..712c3e3118 100644 --- a/src/settings/configuration.rs +++ b/src/settings/configuration.rs @@ -17,7 +17,7 @@ use crate::registry::RuleCodePrefix; use crate::rules::{ flake8_annotations, flake8_bandit, flake8_bugbear, flake8_errmsg, flake8_import_conventions, flake8_pytest_style, flake8_quotes, flake8_tidy_imports, flake8_unused_arguments, isort, - mccabe, pep8_naming, pycodestyle, pydocstyle, pyupgrade, + mccabe, pep8_naming, pycodestyle, pydocstyle, pylint, pyupgrade, }; use crate::settings::options::Options; use crate::settings::pyproject::load_options; @@ -72,6 +72,7 @@ pub struct Configuration { pub pep8_naming: Option, pub pycodestyle: Option, pub pydocstyle: Option, + pub pylint: Option, pub pyupgrade: Option, } @@ -178,6 +179,7 @@ impl Configuration { pep8_naming: options.pep8_naming, pycodestyle: options.pycodestyle, pydocstyle: options.pydocstyle, + pylint: options.pylint, pyupgrade: options.pyupgrade, }) } @@ -248,6 +250,7 @@ impl Configuration { pep8_naming: self.pep8_naming.or(config.pep8_naming), pycodestyle: self.pycodestyle.or(config.pycodestyle), pydocstyle: self.pydocstyle.or(config.pydocstyle), + pylint: self.pylint.or(config.pylint), pyupgrade: self.pyupgrade.or(config.pyupgrade), } } diff --git a/src/settings/defaults.rs b/src/settings/defaults.rs index ec21323fd4..fd1208a246 100644 --- a/src/settings/defaults.rs +++ b/src/settings/defaults.rs @@ -10,7 +10,7 @@ use crate::registry::RuleCodePrefix; use crate::rules::{ flake8_annotations, flake8_bandit, flake8_bugbear, flake8_errmsg, flake8_import_conventions, flake8_pytest_style, flake8_quotes, flake8_tidy_imports, flake8_unused_arguments, isort, - mccabe, pep8_naming, pycodestyle, pydocstyle, pyupgrade, + mccabe, pep8_naming, pycodestyle, pydocstyle, pylint, pyupgrade, }; pub const PREFIXES: &[RuleCodePrefix] = &[RuleCodePrefix::E, RuleCodePrefix::F]; @@ -84,6 +84,7 @@ impl Default for Settings { pep8_naming: pep8_naming::settings::Settings::default(), pycodestyle: pycodestyle::settings::Settings::default(), pydocstyle: pydocstyle::settings::Settings::default(), + pylint: pylint::settings::Settings::default(), pyupgrade: pyupgrade::settings::Settings::default(), } } diff --git a/src/settings/hashable.rs b/src/settings/hashable.rs index 57b68d1b6f..b7c8b98e28 100644 --- a/src/settings/hashable.rs +++ b/src/settings/hashable.rs @@ -12,7 +12,7 @@ use super::types::FilePattern; pub struct HashableRegex(Regex); impl Hash for HashableRegex { - fn hash(&self, state: &mut H) { + fn hash(&self, state: &mut H) { self.0.as_str().hash(state); } } @@ -49,7 +49,7 @@ impl Deref for HashableGlobMatcher { } impl Hash for HashableGlobMatcher { - fn hash(&self, state: &mut H) { + fn hash(&self, state: &mut H) { self.0.glob().hash(state); } } @@ -87,7 +87,7 @@ impl Deref for HashableGlobSet { } impl Hash for HashableGlobSet { - fn hash(&self, state: &mut H) { + fn hash(&self, state: &mut H) { for pattern in self.patterns.iter().sorted() { pattern.hash(state); } diff --git a/src/settings/mod.rs b/src/settings/mod.rs index f639b95d57..6dc0e177b9 100644 --- a/src/settings/mod.rs +++ b/src/settings/mod.rs @@ -18,7 +18,7 @@ use crate::registry::{RuleCode, RuleCodePrefix, SuffixLength, CATEGORIES, INCOMP use crate::rules::{ flake8_annotations, flake8_bandit, flake8_bugbear, flake8_errmsg, flake8_import_conventions, flake8_pytest_style, flake8_quotes, flake8_tidy_imports, flake8_unused_arguments, isort, - mccabe, pep8_naming, pycodestyle, pydocstyle, pyupgrade, + mccabe, pep8_naming, pycodestyle, pydocstyle, pylint, pyupgrade, }; use crate::settings::configuration::Configuration; use crate::settings::types::{PerFileIgnore, PythonVersion, SerializationFormat, Version}; @@ -117,6 +117,7 @@ pub struct Settings { pub pep8_naming: pep8_naming::settings::Settings, pub pycodestyle: pycodestyle::settings::Settings, pub pydocstyle: pydocstyle::settings::Settings, + pub pylint: pylint::settings::Settings, pub pyupgrade: pyupgrade::settings::Settings, } @@ -200,6 +201,7 @@ impl Settings { pep8_naming: config.pep8_naming.map(Into::into).unwrap_or_default(), pycodestyle: config.pycodestyle.map(Into::into).unwrap_or_default(), pydocstyle: config.pydocstyle.map(Into::into).unwrap_or_default(), + pylint: config.pylint.map(Into::into).unwrap_or_default(), pyupgrade: config.pyupgrade.map(Into::into).unwrap_or_default(), }) } diff --git a/src/settings/options.rs b/src/settings/options.rs index fcba305532..cae0e58934 100644 --- a/src/settings/options.rs +++ b/src/settings/options.rs @@ -9,7 +9,7 @@ use crate::registry::RuleCodePrefix; use crate::rules::{ flake8_annotations, flake8_bandit, flake8_bugbear, flake8_errmsg, flake8_import_conventions, flake8_pytest_style, flake8_quotes, flake8_tidy_imports, flake8_unused_arguments, isort, - mccabe, pep8_naming, pycodestyle, pydocstyle, pyupgrade, + mccabe, pep8_naming, pycodestyle, pydocstyle, pylint, pyupgrade, }; use crate::settings::types::{PythonVersion, SerializationFormat, Version}; @@ -462,6 +462,9 @@ pub struct Options { /// Options for the `pydocstyle` plugin. pub pydocstyle: Option, #[option_group] + /// Options for the `pylint` plugin. + pub pylint: Option, + #[option_group] /// Options for the `pyupgrade` plugin. pub pyupgrade: Option, // Tables are required to go last. diff --git a/src/settings/pyproject.rs b/src/settings/pyproject.rs index ae189c376e..497fb3b765 100644 --- a/src/settings/pyproject.rs +++ b/src/settings/pyproject.rs @@ -208,6 +208,7 @@ mod tests { pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }) }) @@ -268,6 +269,7 @@ line-length = 79 pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }) }) @@ -328,6 +330,7 @@ exclude = ["foo.py"] pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }) }) @@ -388,6 +391,7 @@ select = ["E501"] pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }) }) @@ -449,6 +453,7 @@ ignore = ["E501"] pep8_naming: None, pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, }) }) @@ -630,6 +635,7 @@ other-attribute = 1 }), pycodestyle: None, pydocstyle: None, + pylint: None, pyupgrade: None, } ); diff --git a/src/violations.rs b/src/violations.rs index ad38a6385d..292e6af241 100644 --- a/src/violations.rs +++ b/src/violations.rs @@ -1251,18 +1251,22 @@ impl AlwaysAutofixableViolation for UseSysExit { } define_violation!( - pub struct MagicValueComparison(pub String); + pub struct MagicValueComparison { + pub value: String, + } ); impl Violation for MagicValueComparison { fn message(&self) -> String { - let MagicValueComparison(value) = self; + let MagicValueComparison { value } = self; format!( "Magic value used in comparison, consider replacing {value} with a constant variable" ) } fn placeholder() -> Self { - MagicValueComparison("magic".to_string()) + MagicValueComparison { + value: "magic".to_string(), + } } }