From fc465cc2af89572373771d3a89b9753839e43432 Mon Sep 17 00:00:00 2001 From: Nyakku Shigure Date: Mon, 13 Feb 2023 02:02:38 +0800 Subject: [PATCH] [`flake8-pyi`]: add rules for unrecognized platform check (PYI007, PYI008) (#2805) Add two [flake8-pyi](https://github.com/PyCQA/flake8-pyi) rules (Y007, Y008). ref: #848 The specifications are described in [PEP 484 - Version and platform checking](https://peps.python.org/pep-0484/#version-and-platform-checking) The original implementation in flake8-pyi is shown below. - Implemention: https://github.com/PyCQA/flake8-pyi/blob/66f28a4407af2156f95d370941fae4dd98280740/pyi.py#L1429-L1443 - Tests: https://github.com/PyCQA/flake8-pyi/blob/66f28a4407af2156f95d370941fae4dd98280740/tests/sysplatform.pyi --- README.md | 2 + .../test/fixtures/flake8_pyi/PYI007.py | 11 ++ .../test/fixtures/flake8_pyi/PYI007.pyi | 11 ++ .../test/fixtures/flake8_pyi/PYI008.py | 11 ++ .../test/fixtures/flake8_pyi/PYI008.pyi | 11 ++ crates/ruff/src/checkers/ast.rs | 17 ++ crates/ruff/src/registry.rs | 2 + crates/ruff/src/rules/flake8_pyi/mod.rs | 4 + crates/ruff/src/rules/flake8_pyi/rules/mod.rs | 7 + .../{rules.rs => rules/prefix_type_params.rs} | 0 .../flake8_pyi/rules/unrecognized_platform.rs | 154 ++++++++++++++++++ ...__flake8_pyi__tests__PYI007_PYI007.py.snap | 6 + ..._flake8_pyi__tests__PYI007_PYI007.pyi.snap | 35 ++++ ...__flake8_pyi__tests__PYI008_PYI008.py.snap | 6 + ..._flake8_pyi__tests__PYI008_PYI008.pyi.snap | 16 ++ docs/rules/unrecognized-platform-check.md | 33 ++++ docs/rules/unrecognized-platform-name.md | 30 ++++ ruff.schema.json | 2 + 18 files changed, 358 insertions(+) create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI007.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI007.pyi create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI008.py create mode 100644 crates/ruff/resources/test/fixtures/flake8_pyi/PYI008.pyi create mode 100644 crates/ruff/src/rules/flake8_pyi/rules/mod.rs rename crates/ruff/src/rules/flake8_pyi/{rules.rs => rules/prefix_type_params.rs} (100%) create mode 100644 crates/ruff/src/rules/flake8_pyi/rules/unrecognized_platform.rs create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI007_PYI007.py.snap create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI007_PYI007.pyi.snap create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI008_PYI008.py.snap create mode 100644 crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI008_PYI008.pyi.snap create mode 100644 docs/rules/unrecognized-platform-check.md create mode 100644 docs/rules/unrecognized-platform-name.md diff --git a/README.md b/README.md index 065443f01a..972c6c7ea8 100644 --- a/README.md +++ b/README.md @@ -1181,6 +1181,8 @@ For more, see [flake8-pyi](https://pypi.org/project/flake8-pyi/) on PyPI. | Code | Name | Message | Fix | | ---- | ---- | ------- | --- | | PYI001 | [prefix-type-params](https://github.com/charliermarsh/ruff/blob/main/docs/rules/prefix-type-params.md) | Name of private `{kind}` must start with _ | | +| PYI007 | [unrecognized-platform-check](https://github.com/charliermarsh/ruff/blob/main/docs/rules/unrecognized-platform-check.md) | Unrecognized sys.platform check | | +| PYI008 | [unrecognized-platform-name](https://github.com/charliermarsh/ruff/blob/main/docs/rules/unrecognized-platform-name.md) | Unrecognized platform `{platform}` | | ### flake8-pytest-style (PT) diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI007.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI007.py new file mode 100644 index 0000000000..1cb7823a3a --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI007.py @@ -0,0 +1,11 @@ +import sys + +if sys.platform == "platform_name_1": ... # OK + +if sys.platform != "platform_name_2": ... # OK + +if sys.platform in ["linux"]: ... # OK + +if sys.platform > 3: ... # OK + +if sys.platform == 10.12: ... # OK diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI007.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI007.pyi new file mode 100644 index 0000000000..cb6b9b3a9e --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI007.pyi @@ -0,0 +1,11 @@ +import sys + +if sys.platform == "platform_name_1": ... # OK + +if sys.platform != "platform_name_2": ... # OK + +if sys.platform in ["linux"]: ... # Error: PYI007 Unrecognized sys.platform check + +if sys.platform > 3: ... # Error: PYI007 Unrecognized sys.platform check + +if sys.platform == 10.12: ... # Error: PYI007 Unrecognized sys.platform check diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI008.py b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI008.py new file mode 100644 index 0000000000..c0af80fce0 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI008.py @@ -0,0 +1,11 @@ +import sys + +if sys.platform == "linus": ... # OK + +if sys.platform != "linux": ... # OK + +if sys.platform == "win32": ... # OK + +if sys.platform != "darwin": ... # OK + +if sys.platform == "cygwin": ... # OK diff --git a/crates/ruff/resources/test/fixtures/flake8_pyi/PYI008.pyi b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI008.pyi new file mode 100644 index 0000000000..3fe4d68b9b --- /dev/null +++ b/crates/ruff/resources/test/fixtures/flake8_pyi/PYI008.pyi @@ -0,0 +1,11 @@ +import sys + +if sys.platform == "linus": ... # Error: PYI008 Unrecognized platform `linus` + +if sys.platform != "linux": ... # OK + +if sys.platform == "win32": ... # OK + +if sys.platform != "darwin": ... # OK + +if sys.platform == "cygwin": ... # OK diff --git a/crates/ruff/src/checkers/ast.rs b/crates/ruff/src/checkers/ast.rs index 010f960b57..3a35c27221 100644 --- a/crates/ruff/src/checkers/ast.rs +++ b/crates/ruff/src/checkers/ast.rs @@ -3155,6 +3155,23 @@ where if self.settings.rules.enabled(&Rule::YodaConditions) { flake8_simplify::rules::yoda_conditions(self, expr, left, ops, comparators); } + + if self + .settings + .rules + .enabled(&Rule::UnrecognizedPlatformCheck) + || self.settings.rules.enabled(&Rule::UnrecognizedPlatformName) + { + if self.path.extension().map_or(false, |ext| ext == "pyi") { + flake8_pyi::rules::unrecognized_platform( + self, + expr, + left, + ops, + comparators, + ); + } + } } ExprKind::Constant { value: Constant::Str(value), diff --git a/crates/ruff/src/registry.rs b/crates/ruff/src/registry.rs index 8412c42e9b..d915e0f6a0 100644 --- a/crates/ruff/src/registry.rs +++ b/crates/ruff/src/registry.rs @@ -446,6 +446,8 @@ ruff_macros::define_rule_mapping!( EM103 => rules::flake8_errmsg::rules::DotFormatInException, // flake8-pyi PYI001 => rules::flake8_pyi::rules::PrefixTypeParams, + PYI007 => rules::flake8_pyi::rules::UnrecognizedPlatformCheck, + PYI008 => rules::flake8_pyi::rules::UnrecognizedPlatformName, // flake8-pytest-style PT001 => rules::flake8_pytest_style::rules::IncorrectFixtureParenthesesStyle, PT002 => rules::flake8_pytest_style::rules::FixturePositionalArgs, diff --git a/crates/ruff/src/rules/flake8_pyi/mod.rs b/crates/ruff/src/rules/flake8_pyi/mod.rs index 08ca62cd29..3608f8d16a 100644 --- a/crates/ruff/src/rules/flake8_pyi/mod.rs +++ b/crates/ruff/src/rules/flake8_pyi/mod.rs @@ -14,6 +14,10 @@ mod tests { #[test_case(Rule::PrefixTypeParams, Path::new("PYI001.pyi"))] #[test_case(Rule::PrefixTypeParams, Path::new("PYI001.py"))] + #[test_case(Rule::UnrecognizedPlatformCheck, Path::new("PYI007.pyi"))] + #[test_case(Rule::UnrecognizedPlatformCheck, Path::new("PYI007.py"))] + #[test_case(Rule::UnrecognizedPlatformName, Path::new("PYI008.pyi"))] + #[test_case(Rule::UnrecognizedPlatformName, Path::new("PYI008.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/flake8_pyi/rules/mod.rs b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs new file mode 100644 index 0000000000..8939ef7a80 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/rules/mod.rs @@ -0,0 +1,7 @@ +pub use prefix_type_params::{prefix_type_params, PrefixTypeParams}; +pub use unrecognized_platform::{ + unrecognized_platform, UnrecognizedPlatformCheck, UnrecognizedPlatformName, +}; + +mod prefix_type_params; +mod unrecognized_platform; diff --git a/crates/ruff/src/rules/flake8_pyi/rules.rs b/crates/ruff/src/rules/flake8_pyi/rules/prefix_type_params.rs similarity index 100% rename from crates/ruff/src/rules/flake8_pyi/rules.rs rename to crates/ruff/src/rules/flake8_pyi/rules/prefix_type_params.rs diff --git a/crates/ruff/src/rules/flake8_pyi/rules/unrecognized_platform.rs b/crates/ruff/src/rules/flake8_pyi/rules/unrecognized_platform.rs new file mode 100644 index 0000000000..8cc59887b0 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/rules/unrecognized_platform.rs @@ -0,0 +1,154 @@ +use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprKind}; + +use ruff_macros::{define_violation, derive_message_formats}; + +use crate::ast::types::Range; +use crate::checkers::ast::Checker; +use crate::registry::{Diagnostic, Rule}; +use crate::violation::Violation; + +define_violation!( + /// ## What it does + /// Check for unrecognized `sys.platform` checks. Platform checks should be + /// simple string comparisons. + /// + /// **Note**: this rule is only enabled in `.pyi` stub files. + /// + /// ## Why is this bad? + /// Some `sys.platform` checks are too complex for type checkers to + /// understand, and thus result in false positives. `sys.platform` checks + /// should be simple string comparisons, like `sys.platform == "linux"`. + /// + /// ## Example + /// ```python + /// if sys.platform.startswith("linux"): + /// # Linux specific definitions + /// else: + /// # Posix specific definitions + /// ``` + /// + /// Instead, use a simple string comparison, such as `==` or `!=`: + /// ```python + /// if sys.platform == "linux": + /// # Linux specific definitions + /// else: + /// # Posix specific definitions + /// ``` + /// + /// ## References + /// - [PEP 484](https://peps.python.org/pep-0484/#version-and-platform-checking) + pub struct UnrecognizedPlatformCheck; +); +impl Violation for UnrecognizedPlatformCheck { + #[derive_message_formats] + fn message(&self) -> String { + format!("Unrecognized sys.platform check") + } +} + +define_violation!( + /// ## What it does + /// Check for unrecognized platform names in `sys.platform` checks. + /// + /// **Note**: this rule is only enabled in `.pyi` stub files. + /// + /// ## Why is this bad? + /// If a `sys.platform` check compares to a platform name outside of a + /// small set of known platforms (e.g. "linux", "win32", etc.), it's likely + /// a typo or a platform name that is not recognized by type checkers. + /// + /// The list of known platforms is: "linux", "win32", "cygwin", "darwin". + /// + /// ## Example + /// ```python + /// if sys.platform == "linus": + /// ... + /// ``` + /// + /// Use instead: + /// ```python + /// if sys.platform == "linux": + /// ... + /// ``` + /// + /// ## References + /// - [PEP 484](https://peps.python.org/pep-0484/#version-and-platform-checking) + pub struct UnrecognizedPlatformName { + pub platform: String, + } +); +impl Violation for UnrecognizedPlatformName { + #[derive_message_formats] + fn message(&self) -> String { + let UnrecognizedPlatformName { platform } = self; + format!("Unrecognized platform `{platform}`") + } +} + +/// PYI007, PYI008 +pub fn unrecognized_platform( + checker: &mut Checker, + expr: &Expr, + left: &Expr, + ops: &[Cmpop], + comparators: &[Expr], +) { + let ([op], [right]) = (ops, comparators) else { + return; + }; + + let diagnostic_unrecognized_platform_check = + Diagnostic::new(UnrecognizedPlatformCheck, Range::from_located(expr)); + if !checker.resolve_call_path(left).map_or(false, |call_path| { + call_path.as_slice() == ["sys", "platform"] + }) { + return; + } + + // "in" might also make sense but we don't currently have one. + if !matches!(op, Cmpop::Eq | Cmpop::NotEq) + && checker + .settings + .rules + .enabled(&Rule::UnrecognizedPlatformCheck) + { + checker + .diagnostics + .push(diagnostic_unrecognized_platform_check); + return; + } + + match &right.node { + ExprKind::Constant { + value: Constant::Str(value), + .. + } => { + // Other values are possible but we don't need them right now. + // This protects against typos. + if !["linux", "win32", "cygwin", "darwin"].contains(&value.as_str()) + && checker + .settings + .rules + .enabled(&Rule::UnrecognizedPlatformName) + { + checker.diagnostics.push(Diagnostic::new( + UnrecognizedPlatformName { + platform: value.clone(), + }, + Range::from_located(right), + )); + } + } + _ => { + if checker + .settings + .rules + .enabled(&Rule::UnrecognizedPlatformCheck) + { + checker + .diagnostics + .push(diagnostic_unrecognized_platform_check); + } + } + } +} diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI007_PYI007.py.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI007_PYI007.py.snap new file mode 100644 index 0000000000..efcc2d0c99 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI007_PYI007.py.snap @@ -0,0 +1,6 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +expression: diagnostics +--- +[] + diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI007_PYI007.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI007_PYI007.pyi.snap new file mode 100644 index 0000000000..255271d6f9 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI007_PYI007.pyi.snap @@ -0,0 +1,35 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +expression: diagnostics +--- +- kind: + UnrecognizedPlatformCheck: ~ + location: + row: 7 + column: 3 + end_location: + row: 7 + column: 28 + fix: ~ + parent: ~ +- kind: + UnrecognizedPlatformCheck: ~ + location: + row: 9 + column: 3 + end_location: + row: 9 + column: 19 + fix: ~ + parent: ~ +- kind: + UnrecognizedPlatformCheck: ~ + location: + row: 11 + column: 3 + end_location: + row: 11 + column: 24 + fix: ~ + parent: ~ + diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI008_PYI008.py.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI008_PYI008.py.snap new file mode 100644 index 0000000000..efcc2d0c99 --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI008_PYI008.py.snap @@ -0,0 +1,6 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +expression: diagnostics +--- +[] + diff --git a/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI008_PYI008.pyi.snap b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI008_PYI008.pyi.snap new file mode 100644 index 0000000000..8fe25a41cb --- /dev/null +++ b/crates/ruff/src/rules/flake8_pyi/snapshots/ruff__rules__flake8_pyi__tests__PYI008_PYI008.pyi.snap @@ -0,0 +1,16 @@ +--- +source: crates/ruff/src/rules/flake8_pyi/mod.rs +expression: diagnostics +--- +- kind: + UnrecognizedPlatformName: + platform: linus + location: + row: 3 + column: 19 + end_location: + row: 3 + column: 26 + fix: ~ + parent: ~ + diff --git a/docs/rules/unrecognized-platform-check.md b/docs/rules/unrecognized-platform-check.md new file mode 100644 index 0000000000..d77eac1cb5 --- /dev/null +++ b/docs/rules/unrecognized-platform-check.md @@ -0,0 +1,33 @@ +# unrecognized-platform-check (PYI007) + +Derived from the **flake8-pyi** linter. + +## What it does +Check for unrecognized `sys.platform` checks. Platform checks should be +simple string comparisons. + +**Note**: this rule is only enabled in `.pyi` stub files. + +## Why is this bad? +Some `sys.platform` checks are too complex for type checkers to +understand, and thus result in false positives. `sys.platform` checks +should be simple string comparisons, like `sys.platform == "linux"`. + +## Example +```python +if sys.platform.startswith("linux"): + # Linux specific definitions +else: + # Posix specific definitions +``` + +Instead, use a simple string comparison, such as `==` or `!=`: +```python +if sys.platform == "linux": + # Linux specific definitions +else: + # Posix specific definitions +``` + +## References +- [PEP 484](https://peps.python.org/pep-0484/#version-and-platform-checking) \ No newline at end of file diff --git a/docs/rules/unrecognized-platform-name.md b/docs/rules/unrecognized-platform-name.md new file mode 100644 index 0000000000..db21e68195 --- /dev/null +++ b/docs/rules/unrecognized-platform-name.md @@ -0,0 +1,30 @@ +# unrecognized-platform-name (PYI008) + +Derived from the **flake8-pyi** linter. + +## What it does +Check for unrecognized platform names in `sys.platform` checks. + +**Note**: this rule is only enabled in `.pyi` stub files. + +## Why is this bad? +If a `sys.platform` check compares to a platform name outside of a +small set of known platforms (e.g. "linux", "win32", etc.), it's likely +a typo or a platform name that is not recognized by type checkers. + +The list of known platforms is: "linux", "win32", "cygwin", "darwin". + +## Example +```python +if sys.platform == "linus": + ... +``` + +Use instead: +```python +if sys.platform == "linux": + ... +``` + +## References +- [PEP 484](https://peps.python.org/pep-0484/#version-and-platform-checking) \ No newline at end of file diff --git a/ruff.schema.json b/ruff.schema.json index c1db1b62d5..f75c534c74 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -1823,6 +1823,8 @@ "PYI0", "PYI00", "PYI001", + "PYI007", + "PYI008", "Q", "Q0", "Q00",