Update CONTRIBUTING.md guide (#5017)

CONTRIBUTING.md

@@ -21,9 +21,9 @@ Ruff welcomes contributions in the form of Pull Requests.
 For small changes (e.g., bug fixes), feel free to submit a PR.
 
 For larger changes (e.g., new lint rules, new functionality, new configuration options), consider
-creating an [**issue**](https://github.com/astral-sh/ruff/issues) outlining your proposed
+creating an [**issue**](https://github.com/astral-sh/ruff/issues) outlining your proposed change.
-change. You can also join us on [**Discord**](https://discord.gg/c9MhzV8aU5) to discuss your idea with
+You can also join us on [**Discord**](https://discord.gg/c9MhzV8aU5) to discuss your idea with the
-the community.
+community.
 
 If you're looking for a place to start, we recommend implementing a new lint rule (see:
 [_Adding a new lint rule_](#example-adding-a-new-lint-rule), which will allow you to learn from and
@@ -31,8 +31,8 @@ pattern-match against the examples in the existing codebase. Many lint rules are
 existing Python plugins, which can be used as a reference implementation.
 
 As a concrete example: consider taking on one of the rules from the [`flake8-pyi`](https://github.com/astral-sh/ruff/issues/848)
-plugin, and looking to the originating [Python source](https://github.com/PyCQA/flake8-pyi)
+plugin, and looking to the originating [Python source](https://github.com/PyCQA/flake8-pyi) for
-for guidance.
+guidance.
 
 ### Prerequisites
 
@@ -58,8 +58,8 @@ and that it passes both the lint and test validation checks:
 
 ```shell
 cargo fmt  # Auto-formatting...
-cargo clippy --fix --workspace --all-targets --all-features  # Linting...
 cargo test  # Testing...
+cargo clippy --workspace --all-targets --all-features -- -D warnings  # Linting...
 ```
 
 These checks will run on GitHub Actions when you open your Pull Request, but running them locally
@@ -119,52 +119,63 @@ At time of writing, the repository includes the following crates:
 
 At a high level, the steps involved in adding a new lint rule are as follows:
 
-1. Determine a name for the new rule as per our [rule naming convention](#rule-naming-convention).
+1. Determine a name for the new rule as per our [rule naming convention](#rule-naming-convention)
+   (e.g., `AssertFalse`, as in, "allow `assert False`").
 
-1. Create a file for your rule (e.g., `crates/ruff/src/rules/flake8_bugbear/rules/abstract_base_class.rs`).
+1. Create a file for your rule (e.g., `crates/ruff/src/rules/flake8_bugbear/rules/assert_false.rs`).
 
-1. In that file, define a violation struct. You can grep for `#[violation]` to see examples.
+1. In that file, define a violation struct (e.g., `pub struct AssertFalse`). You can grep for
+   `#[violation]` to see examples.
 
-1. Map the violation struct to a rule code in `crates/ruff/src/codes.rs` (e.g., `E402`).
+1. In that file, define a function that adds the violation to the diagnostic list as appropriate
+   (e.g., `pub(crate) fn assert_false`) based on whatever inputs are required for the rule (e.g.,
+   an `ast::StmtAssert` node).
 
 1. Define the logic for triggering the violation in `crates/ruff/src/checkers/ast/mod.rs` (for
    AST-based checks), `crates/ruff/src/checkers/tokens.rs` (for token-based checks),
    `crates/ruff/src/checkers/lines.rs` (for text-based checks), or
    `crates/ruff/src/checkers/filesystem.rs` (for filesystem-based checks).
 
+1. Map the violation struct to a rule code in `crates/ruff/src/codes.rs` (e.g., `B011`).
+
 1. Add proper [testing](#rule-testing-fixtures-and-snapshots) for your rule.
 
 1. Update the generated files (documentation and generated code).
 
-To define the violation, start by creating a dedicated file for your rule under the appropriate
+To trigger the violation, you'll likely want to augment the logic in `crates/ruff/src/checkers/ast.rs`
-rule linter (e.g., `crates/ruff/src/rules/flake8_bugbear/rules/abstract_base_class.rs`). That file should
+to call your new function at the appropriate time and with the appropriate inputs. The `Checker`
-contain a struct defined via `#[violation]`, along with a function that creates the violation
+defined therein is a Python AST visitor, which iterates over the AST, building up a semantic model,
-based on any required inputs.
+and calling out to lint rule analyzer functions as it goes.
-
-To trigger the violation, you'll likely want to augment the logic in `crates/ruff/src/checkers/ast.rs`,
-which defines the Python AST visitor, responsible for iterating over the abstract syntax tree and
-collecting diagnostics as it goes.
 
 If you need to inspect the AST, you can run `cargo dev print-ast` with a Python file. Grep
-for the `Check::new` invocations to understand how other, similar rules are implemented.
+for the `Diagnostic::new` invocations to understand how other, similar rules are implemented.
 
 Once you're satisfied with your code, add tests for your rule. See [rule testing](#rule-testing-fixtures-and-snapshots)
 for more details.
 
-Finally, regenerate the documentation and generated code with `cargo dev generate-all`.
+Finally, regenerate the documentation and other generated assets (like our JSON Schema) with:
+`cargo dev generate-all`.
 
 #### Rule naming convention
 
-The rule name should make sense when read as "allow _rule-name_" or "allow _rule-name_ items".
+Like Clippy, Ruff's rule names should make grammatical and logical sense when read as "allow
+${rule}" or "allow ${rule} items", as in the context of suppression comments.
 
-This implies that rule names:
+For example, `AssertFalse` fits this convention: it flags `assert False` statements, and so a
+suppression comment would be framed as "allow `assert False`".
 
-- should state the bad thing being checked for
+As such, rule names should...
 
-- should not contain instructions on what you should use instead
+- Highlight the pattern that is being linted against, rather than the preferred alternative.
-  (these belong in the rule documentation and the `autofix_title` for rules that have autofix)
+  For example, `AssertFalse` guards against `assert False` statements.
 
-When re-implementing rules from other linters, this convention is given more importance than
+- _Not_ contain instructions on how to fix the violation, which instead belong in the rule
+  documentation and the `autofix_title`.
+
+- _Not_ contain a redundant prefix, like `Disallow` or `Banned`, which are already implied by the
+  convention.
+
+When re-implementing rules from other linters, we prioritize adhering to this convention over
 preserving the original rule name.
 
 #### Rule testing: fixtures and snapshots

crates/ruff/src/checkers/ast/mod.rs

@@ -1356,9 +1356,9 @@ where
                         pyflakes::rules::raise_not_implemented(self, expr);
                     }
                 }
-                if self.enabled(Rule::CannotRaiseLiteral) {
+                if self.enabled(Rule::RaiseLiteral) {
                     if let Some(exc) = exc {
-                        flake8_bugbear::rules::cannot_raise_literal(self, exc);
+                        flake8_bugbear::rules::raise_literal(self, exc);
                     }
                 }
                 if self.any_enabled(&[

crates/ruff/src/codes.rs

@@ -232,7 +232,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Flake8Bugbear, "013") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::RedundantTupleInExceptionHandler),
         (Flake8Bugbear, "014") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::DuplicateHandlerException),
         (Flake8Bugbear, "015") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::UselessComparison),
-        (Flake8Bugbear, "016") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::CannotRaiseLiteral),
+        (Flake8Bugbear, "016") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::RaiseLiteral),
         (Flake8Bugbear, "017") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::AssertRaisesException),
         (Flake8Bugbear, "018") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::UselessExpression),
         (Flake8Bugbear, "019") => (RuleGroup::Unspecified, rules::flake8_bugbear::rules::CachedInstanceMethod),

crates/ruff/src/rules/flake8_bugbear/mod.rs

@@ -19,7 +19,7 @@ mod tests {
     #[test_case(Rule::AssertRaisesException, Path::new("B017.py"))]
     #[test_case(Rule::AssignmentToOsEnviron, Path::new("B003.py"))]
     #[test_case(Rule::CachedInstanceMethod, Path::new("B019.py"))]
-    #[test_case(Rule::CannotRaiseLiteral, Path::new("B016.py"))]
+    #[test_case(Rule::RaiseLiteral, Path::new("B016.py"))]
     #[test_case(Rule::DuplicateHandlerException, Path::new("B014.py"))]
     #[test_case(Rule::DuplicateTryBlockException, Path::new("B025.py"))]
     #[test_case(Rule::DuplicateValue, Path::new("B033.py"))]

crates/ruff/src/rules/flake8_bugbear/rules/mod.rs

@@ -6,7 +6,6 @@ pub(crate) use assert_false::{assert_false, AssertFalse};
 pub(crate) use assert_raises_exception::{assert_raises_exception, AssertRaisesException};
 pub(crate) use assignment_to_os_environ::{assignment_to_os_environ, AssignmentToOsEnviron};
 pub(crate) use cached_instance_method::{cached_instance_method, CachedInstanceMethod};
-pub(crate) use cannot_raise_literal::{cannot_raise_literal, CannotRaiseLiteral};
 pub(crate) use duplicate_exceptions::{
     duplicate_exceptions, DuplicateHandlerException, DuplicateTryBlockException,
 };
@@ -29,6 +28,7 @@ pub(crate) use loop_variable_overrides_iterator::{
 };
 pub(crate) use mutable_argument_default::{mutable_argument_default, MutableArgumentDefault};
 pub(crate) use no_explicit_stacklevel::{no_explicit_stacklevel, NoExplicitStacklevel};
+pub(crate) use raise_literal::{raise_literal, RaiseLiteral};
 pub(crate) use raise_without_from_inside_except::{
     raise_without_from_inside_except, RaiseWithoutFromInsideExcept,
 };
@@ -65,7 +65,6 @@ mod assert_false;
 mod assert_raises_exception;
 mod assignment_to_os_environ;
 mod cached_instance_method;
-mod cannot_raise_literal;
 mod duplicate_exceptions;
 mod duplicate_value;
 mod except_with_empty_tuple;
@@ -78,6 +77,7 @@ mod jump_statement_in_finally;
 mod loop_variable_overrides_iterator;
 mod mutable_argument_default;
 mod no_explicit_stacklevel;
+mod raise_literal;
 mod raise_without_from_inside_except;
 mod redundant_tuple_in_exception_handler;
 mod reuse_of_groupby_generator;

crates/ruff/src/rules/flake8_bugbear/rules/raise_literal.rs

@@ -6,9 +6,9 @@ use ruff_macros::{derive_message_formats, violation};
 use crate::checkers::ast::Checker;
 
 #[violation]
-pub struct CannotRaiseLiteral;
+pub struct RaiseLiteral;
 
-impl Violation for CannotRaiseLiteral {
+impl Violation for RaiseLiteral {
     #[derive_message_formats]
     fn message(&self) -> String {
         format!("Cannot raise a literal. Did you intend to return it or raise an Exception?")
@@ -16,11 +16,10 @@ impl Violation for CannotRaiseLiteral {
 }
 
 /// B016
-pub(crate) fn cannot_raise_literal(checker: &mut Checker, expr: &Expr) {
+pub(crate) fn raise_literal(checker: &mut Checker, expr: &Expr) {
-    let Expr::Constant ( _) = expr else {
+    if expr.is_constant_expr() {
-        return;
-    };
         checker
             .diagnostics
-        .push(Diagnostic::new(CannotRaiseLiteral, expr.range()));
+            .push(Diagnostic::new(RaiseLiteral, expr.range()));
+    }
 }