From 6a4437ea816bb153def752acafdd85fab7ca7d51 Mon Sep 17 00:00:00 2001 From: jan Apisu <108279865+aspizu@users.noreply.github.com> Date: Mon, 2 Oct 2023 06:26:33 +0530 Subject: [PATCH] Add documentation for remaining undocumented lint rules (#7750) --- crates/ruff_linter/src/codes.rs | 2 - .../flake8_annotations/rules/definition.rs | 2 +- .../flake8_bandit/rules/shell_injection.rs | 82 +++++++++++++ .../flake8_pyi/rules/any_eq_ne_annotation.rs | 2 +- .../flake8_pyi/rules/docstring_in_stubs.rs | 19 +++ .../rules/future_annotations_in_stub.rs | 11 ++ .../rules/numeric_literal_too_long.rs | 12 +- .../flake8_pyi/rules/pass_in_class_body.rs | 26 +++++ .../rules/quoted_annotation_in_stub.rs | 20 ++++ .../rules/flake8_pyi/rules/simple_defaults.rs | 110 +++++++++++++++++- .../rules/string_or_bytes_too_long.rs | 15 ++- .../rules/stub_body_multiple_statements.rs | 22 ++++ .../flake8_pyi/rules/type_alias_naming.rs | 35 ++++++ .../pygrep_hooks/rules/blanket_type_ignore.rs | 2 +- 14 files changed, 339 insertions(+), 21 deletions(-) diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 9c0da4a5de..e9559fbfb8 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -6,8 +6,6 @@ use std::fmt::Formatter; use strum_macros::{AsRefStr, EnumIter}; -use ruff_diagnostics::Violation; - use crate::registry::{AsRule, Linter}; use crate::rule_selector::is_single_rule_selector; use crate::rules; diff --git a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs index 37831ddc88..faabd02a3b 100644 --- a/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs +++ b/crates/ruff_linter/src/rules/flake8_annotations/rules/definition.rs @@ -412,7 +412,7 @@ impl Violation for MissingReturnTypeClassMethod { /// ## References /// - [PEP 484](https://www.python.org/dev/peps/pep-0484/#the-any-type) /// - [Python documentation: `typing.Any`](https://docs.python.org/3/library/typing.html#typing.Any) -/// - [Mypy: The Any type](https://mypy.readthedocs.io/en/stable/kinds_of_types.html#the-any-type) +/// - [Mypy documentation: The Any type](https://mypy.readthedocs.io/en/stable/kinds_of_types.html#the-any-type) #[violation] pub struct AnyType { name: String, diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs index fa9e01829c..6fde131b5b 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/shell_injection.rs @@ -88,6 +88,32 @@ impl Violation for SubprocessWithoutShellEqualsTrue { } } +/// ## What it does +/// Checks for method calls that set the `shell` parameter to `true` when +/// invoking a subprocess. +/// +/// ## Why is this bad? +/// Setting the `shell` parameter to `true` when invoking a subprocess can +/// introduce security vulnerabilities, as it allows shell metacharacters and +/// whitespace to be passed to child processes, potentially leading to shell +/// injection attacks. It is recommended to avoid using `shell=True` unless +/// absolutely necessary, and when used, to ensure that all inputs are properly +/// sanitized and quoted to prevent such vulnerabilities. +/// +/// ## Known problems +/// Prone to false positives as it is triggered on any function call with a +/// `shell=True` parameter. +/// +/// ## Example +/// ```python +/// import subprocess +/// +/// user_input = input("Enter a command: ") +/// subprocess.run(user_input, shell=True) +/// ``` +/// +/// ## References +/// - [Python documentation: Security Considerations](https://docs.python.org/3/library/subprocess.html#security-considerations) #[violation] pub struct CallWithShellEqualsTrue; @@ -98,6 +124,42 @@ impl Violation for CallWithShellEqualsTrue { } } +/// ## What it does +/// Checks for calls that start a process with a shell, providing guidance on +/// whether the usage is safe or not. +/// +/// ## Why is this bad? +/// Starting a process with a shell can introduce security risks, such as +/// code injection vulnerabilities. It's important to be aware of whether the +/// usage of the shell is safe or not. +/// +/// This rule triggers on functions like `os.system`, `popen`, etc., which +/// start processes with a shell. It evaluates whether the provided command +/// is a literal string or an expression. If the command is a literal string, +/// it's considered safe. If the command is an expression, it's considered +/// (potentially) unsafe. +/// +/// ## Example +/// ```python +/// import os +/// +/// # Safe usage (literal string) +/// command = "ls -l" +/// os.system(command) +/// +/// # Potentially unsafe usage (expression) +/// cmd = get_user_input() +/// os.system(cmd) +/// ``` +/// +/// ## Note +/// The `subprocess` module provides more powerful facilities for spawning new +/// processes and retrieving their results, and using that module is preferable +/// to using `os.system` or similar functions. Consider replacing such usages +/// with `subprocess.call` or related functions. +/// +/// ## References +/// - [Python documentation: `subprocess`](https://docs.python.org/3/library/subprocess.html) #[violation] pub struct StartProcessWithAShell { seems_safe: bool, @@ -114,6 +176,26 @@ impl Violation for StartProcessWithAShell { } } +/// ## What it does +/// Checks for functions that start a process without a shell. +/// +/// ## Why is this bad? +/// The `subprocess` module provides more powerful facilities for spawning new +/// processes and retrieving their results; using that module is preferable to +/// using these functions. +/// +/// ## Example +/// ```python +/// os.spawnlp(os.P_NOWAIT, "/bin/mycmd", "mycmd", "myarg") +/// ``` +/// +/// Use instead: +/// ```python +/// subprocess.Popen(["/bin/mycmd", "myarg"]) +/// ``` +/// +/// ## References +/// - [Python documentation: Replacing the `os.spawn` family](https://docs.python.org/3/library/subprocess.html#replacing-the-os-spawn-family) #[violation] pub struct StartProcessWithNoShell; diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs index 2230fb1bc7..108a190755 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/any_eq_ne_annotation.rs @@ -35,7 +35,7 @@ use crate::registry::AsRule; /// ``` /// ## References /// - [Python documentation: The `Any` type](https://docs.python.org/3/library/typing.html#the-any-type) -/// - [Mypy documentation](https://mypy.readthedocs.io/en/latest/dynamic_typing.html#any-vs-object) +/// - [Mypy documentation: Any vs. object](https://mypy.readthedocs.io/en/latest/dynamic_typing.html#any-vs-object) #[violation] pub struct AnyEqNeAnnotation { method_name: String, diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/docstring_in_stubs.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/docstring_in_stubs.rs index 6f96e8d29e..38d06d0335 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/docstring_in_stubs.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/docstring_in_stubs.rs @@ -6,6 +6,25 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; +/// ## What it does +/// Checks for the presence of docstrings in stub files. +/// +/// ## Why is this bad? +/// Stub files should omit docstrings, as they're intended to provide type +/// hints, rather than documentation. +/// +/// ## Example +/// ```python +/// def func(param: int) -> str: +/// """This is a docstring.""" +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// def func(param: int) -> str: +/// ... +/// ``` #[violation] pub struct DocstringInStub; diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/future_annotations_in_stub.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/future_annotations_in_stub.rs index deaae2b0f7..bdbaf4a274 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/future_annotations_in_stub.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/future_annotations_in_stub.rs @@ -5,6 +5,17 @@ use ruff_macros::{derive_message_formats, violation}; use crate::checkers::ast::Checker; +/// ## What it does +/// Checks for the presence of the `from __future__ import annotations` import +/// statement in stub files. +/// +/// ## Why is this bad? +/// Stub files are already evaluated under `annotations` semantics. As such, +/// the `from __future__ import annotations` import statement has no effect +/// and should be omitted. +/// +/// ## Resources +/// - [Static Typing with Python: Type Stubs](https://typing.readthedocs.io/en/latest/source/stubs.html) #[violation] pub struct FutureAnnotationsInStub; diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/numeric_literal_too_long.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/numeric_literal_too_long.rs index de9293585d..f2211d5c1d 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/numeric_literal_too_long.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/numeric_literal_too_long.rs @@ -7,9 +7,6 @@ use ruff_macros::{derive_message_formats, violation}; use crate::checkers::ast::Checker; use crate::registry::AsRule; -#[violation] -pub struct NumericLiteralTooLong; - /// ## What it does /// Checks for numeric literals with a string representation longer than ten /// characters. @@ -23,13 +20,18 @@ pub struct NumericLiteralTooLong; /// /// ## Example /// ```python -/// def foo(arg: int = 12345678901) -> None: ... +/// def foo(arg: int = 12345678901) -> None: +/// ... /// ``` /// /// Use instead: /// ```python -/// def foo(arg: int = ...) -> None: ... +/// def foo(arg: int = ...) -> None: +/// ... /// ``` +#[violation] +pub struct NumericLiteralTooLong; + impl AlwaysFixableViolation for NumericLiteralTooLong { #[derive_message_formats] fn message(&self) -> String { diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/pass_in_class_body.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/pass_in_class_body.rs index 9b39eec211..2adc7bbfca 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/pass_in_class_body.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/pass_in_class_body.rs @@ -7,6 +7,32 @@ use crate::checkers::ast::Checker; use crate::fix; use crate::registry::AsRule; +/// ## What it does +/// Checks for the presence of the `pass` statement within a class body +/// in a stub (`.pyi`) file. +/// +/// ## Why is this bad? +/// In stub files, class definitions are intended to provide type hints, but +/// are never actually evaluated. As such, it's unnecessary to include a `pass` +/// statement in a class body, since it has no effect. +/// +/// Instead of `pass`, prefer `...` to indicate that the class body is empty +/// and adhere to common stub file conventions. +/// +/// ## Example +/// ```python +/// class MyClass: +/// pass +/// ``` +/// +/// Use instead: +/// ```python +/// class MyClass: +/// ... +/// ``` +/// +/// ## References +/// - [Mypy documentation: Stub files](https://mypy.readthedocs.io/en/stable/stubs.html) #[violation] pub struct PassInClassBody; diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/quoted_annotation_in_stub.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/quoted_annotation_in_stub.rs index 871bb460c5..f1d7e7f9cf 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/quoted_annotation_in_stub.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/quoted_annotation_in_stub.rs @@ -6,6 +6,26 @@ use ruff_macros::{derive_message_formats, violation}; use crate::checkers::ast::Checker; use crate::registry::Rule; +/// ## What it does +/// Checks for quoted type annotations in stub (`.pyi`) files, which should be avoided. +/// +/// ## Why is this bad? +/// Stub files are evaluated using `annotations` semantics, as if +/// `from __future__ import annotations` were included in the file. As such, +/// quotes are never required for type annotations in stub files, and should be +/// omitted. +/// +/// ## Example +/// ```python +/// def function() -> "int": +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// def function() -> int: +/// ... +/// ``` #[violation] pub struct QuotedAnnotationInStub; diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs index 356346def4..daf82807c0 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/simple_defaults.rs @@ -15,6 +15,38 @@ use crate::registry::AsRule; use crate::rules::flake8_pyi::rules::TypingModule; use crate::settings::types::PythonVersion; +/// ## What it does +/// Checks for typed function arguments in stubs with default values that +/// are not "simple" /// (i.e., `int`, `float`, `complex`, `bytes`, `str`, +/// `bool`, `None`, `...`, or simple container literals). +/// +/// ## Why is this bad? +/// Stub (`.pyi`) files exist to define type hints, and are not evaluated at +/// runtime. As such, function arguments in stub files should not have default +/// values, as they are ignored by type checkers. +/// +/// However, the use of default values may be useful for IDEs and other +/// consumers of stub files, and so "simple" values may be worth including and +/// are permitted by this rule. +/// +/// Instead of including and reproducing a complex value, use `...` to indicate +/// that the assignment has a default value, but that the value is non-simple +/// or varies according to the current platform or Python version. +/// +/// ## Example +/// ```python +/// def foo(arg: List[int] = []) -> None: +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// def foo(arg: List[int] = ...) -> None: +/// ... +/// ``` +/// +/// ## References +/// - [`flake8-pyi`](https://github.com/PyCQA/flake8-pyi/blob/main/ERRORCODES.md) #[violation] pub struct TypedArgumentDefaultInStub; @@ -29,6 +61,38 @@ impl AlwaysFixableViolation for TypedArgumentDefaultInStub { } } +/// ## What it does +/// Checks for untyped function arguments in stubs with default values that +/// are not "simple" /// (i.e., `int`, `float`, `complex`, `bytes`, `str`, +/// `bool`, `None`, `...`, or simple container literals). +/// +/// ## Why is this bad? +/// Stub (`.pyi`) files exist to define type hints, and are not evaluated at +/// runtime. As such, function arguments in stub files should not have default +/// values, as they are ignored by type checkers. +/// +/// However, the use of default values may be useful for IDEs and other +/// consumers of stub files, and so "simple" values may be worth including and +/// are permitted by this rule. +/// +/// Instead of including and reproducing a complex value, use `...` to indicate +/// that the assignment has a default value, but that the value is non-simple +/// or varies according to the current platform or Python version. +/// +/// ## Example +/// ```python +/// def foo(arg=[]) -> None: +/// ... +/// ``` +/// +/// Use instead: +/// ```python +/// def foo(arg=...) -> None: +/// ... +/// ``` +/// +/// ## References +/// - [`flake8-pyi`](https://github.com/PyCQA/flake8-pyi/blob/main/ERRORCODES.md) #[violation] pub struct ArgumentDefaultInStub; @@ -43,6 +107,36 @@ impl AlwaysFixableViolation for ArgumentDefaultInStub { } } +/// ## What it does +/// Checks for assignments in stubs with default values that are not "simple" +/// (i.e., `int`, `float`, `complex`, `bytes`, `str`, `bool`, `None`, `...`, or +/// simple container literals). +/// +/// ## Why is this bad? +/// Stub (`.pyi`) files exist to define type hints, and are not evaluated at +/// runtime. As such, assignments in stub files should not include values, +/// as they are ignored by type checkers. +/// +/// However, the use of such values may be useful for IDEs and other consumers +/// of stub files, and so "simple" values may be worth including and are +/// permitted by this rule. +/// +/// Instead of including and reproducing a complex value, use `...` to indicate +/// that the assignment has a default value, but that the value is non-simple +/// or varies according to the current platform or Python version. +/// +/// ## Example +/// ```python +/// foo: str = "..." +/// ``` +/// +/// Use instead: +/// ```python +/// foo: str = ... +/// ``` +/// +/// ## References +/// - [`flake8-pyi`](https://github.com/PyCQA/flake8-pyi/blob/main/ERRORCODES.md) #[violation] pub struct AssignmentDefaultInStub; @@ -57,6 +151,12 @@ impl AlwaysFixableViolation for AssignmentDefaultInStub { } } +/// ## What it does? +/// Checks for unannotated assignments in stub (`.pyi`) files. +/// +/// ## Why is this bad? +/// Stub files exist to provide type hints, and are never executed. As such, +/// all assignments in stub files should be annotated with a type. #[violation] pub struct UnannotatedAssignmentInStub { name: String, @@ -70,11 +170,6 @@ impl Violation for UnannotatedAssignmentInStub { } } -#[violation] -pub struct UnassignedSpecialVariableInStub { - name: String, -} - /// ## What it does /// Checks that `__all__`, `__match_args__`, and `__slots__` variables are /// assigned to values when defined in stub files. @@ -93,6 +188,11 @@ pub struct UnassignedSpecialVariableInStub { /// ```python /// __all__: list[str] = ["foo", "bar"] /// ``` +#[violation] +pub struct UnassignedSpecialVariableInStub { + name: String, +} + impl Violation for UnassignedSpecialVariableInStub { #[derive_message_formats] fn message(&self) -> String { diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs index 485dbe2b4b..14af90cdb4 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/string_or_bytes_too_long.rs @@ -8,11 +8,9 @@ use ruff_text_size::Ranged; use crate::checkers::ast::Checker; use crate::registry::AsRule; -#[violation] -pub struct StringOrBytesTooLong; - /// ## What it does -/// Checks for the use of string and bytes literals longer than 50 characters. +/// Checks for the use of string and bytes literals longer than 50 characters +/// in stub (`.pyi`) files. /// /// ## Why is this bad? /// If a function has a default value where the string or bytes representation @@ -23,13 +21,18 @@ pub struct StringOrBytesTooLong; /// /// ## Example /// ```python -/// def foo(arg: str = "51 character stringgggggggggggggggggggggggggggggggg") -> None: ... +/// def foo(arg: str = "51 character stringgggggggggggggggggggggggggggggggg") -> None: +/// ... /// ``` /// /// Use instead: /// ```python -/// def foo(arg: str = ...) -> None: ... +/// def foo(arg: str = ...) -> None: +/// ... /// ``` +#[violation] +pub struct StringOrBytesTooLong; + impl AlwaysFixableViolation for StringOrBytesTooLong { #[derive_message_formats] fn message(&self) -> String { diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/stub_body_multiple_statements.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/stub_body_multiple_statements.rs index 77c20d5e94..efe4b918f7 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/stub_body_multiple_statements.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/stub_body_multiple_statements.rs @@ -5,6 +5,28 @@ use ruff_python_ast::Stmt; use crate::checkers::ast::Checker; +/// ## What it does +/// Checks for functions in stub (`.pyi`) files that contain multiple +/// statements. +/// +/// ## Why is this bad? +/// Stub files are never executed, and are only intended to define type hints. +/// As such, functions in stub files should not contain functional code, and +/// should instead contain only a single statement (e.g., `...`). +/// +/// ## Example +/// ```python +/// def function(): +/// x = 1 +/// y = 2 +/// return x + y +/// ``` +/// +/// Use instead: +/// ```python +/// def function(): +/// ... +/// ``` #[violation] pub struct StubBodyMultipleStatements; diff --git a/crates/ruff_linter/src/rules/flake8_pyi/rules/type_alias_naming.rs b/crates/ruff_linter/src/rules/flake8_pyi/rules/type_alias_naming.rs index 38b0ea0c5e..e240bd3bd8 100644 --- a/crates/ruff_linter/src/rules/flake8_pyi/rules/type_alias_naming.rs +++ b/crates/ruff_linter/src/rules/flake8_pyi/rules/type_alias_naming.rs @@ -5,6 +5,22 @@ use ruff_macros::{derive_message_formats, violation}; use crate::checkers::ast::Checker; +/// ## What it does +/// Checks for type aliases that do not use the CamelCase naming convention. +/// +/// ## Why is this bad? +/// It's conventional to use the CamelCase naming convention for type aliases, +/// to distinguish them from other variables. +/// +/// ## Example +/// ```python +/// type_alias_name: TypeAlias = int +/// ``` +/// +/// Use instead: +/// ```python +/// TypeAliasName: TypeAlias = int +/// ``` #[violation] pub struct SnakeCaseTypeAlias { name: String, @@ -18,6 +34,25 @@ impl Violation for SnakeCaseTypeAlias { } } +/// ## What it does +/// Checks for private type alias definitions suffixed with 'T'. +/// +/// ## Why is this bad? +/// It's conventional to use the 'T' suffix for type variables; the use of +/// such a suffix implies that the object is a `TypeVar`. +/// +/// Adding the 'T' suffix to a non-`TypeVar`, it can be misleading and should +/// be avoided. +/// +/// ## Example +/// ```python +/// MyTypeT = int +/// ``` +/// +/// Use instead: +/// ```python +/// MyType = int +/// ``` #[violation] pub struct TSuffixedTypeAlias { name: String, diff --git a/crates/ruff_linter/src/rules/pygrep_hooks/rules/blanket_type_ignore.rs b/crates/ruff_linter/src/rules/pygrep_hooks/rules/blanket_type_ignore.rs index e618f3aecd..b5a213feef 100644 --- a/crates/ruff_linter/src/rules/pygrep_hooks/rules/blanket_type_ignore.rs +++ b/crates/ruff_linter/src/rules/pygrep_hooks/rules/blanket_type_ignore.rs @@ -31,7 +31,7 @@ use ruff_text_size::TextSize; /// ``` /// /// ## References -/// - [mypy](https://mypy.readthedocs.io/en/stable/common_issues.html#spurious-errors-and-locally-silencing-the-checker) +/// - [