From 85f67b2ee301ab911734d07fef275e0d49f54d1e Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 18 May 2023 11:17:26 -0400 Subject: [PATCH] Make the AST Checker `pub(crate)` (#4498) --- crates/ruff/src/checkers/ast/mod.rs | 36 +++++++++---------- crates/ruff/src/importer/mod.rs | 16 +++++---- .../rules/flake8_tidy_imports/banned_api.rs | 9 ++--- .../flake8_tidy_imports/relative_imports.rs | 11 ++---- scripts/add_rule.py | 5 +-- 5 files changed, 39 insertions(+), 38 deletions(-) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 7ddf48af78..c6fa2d148b 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -57,7 +57,7 @@ use crate::{autofix, docstrings, noqa, warn_user}; mod deferred; -pub struct Checker<'a> { +pub(crate) struct Checker<'a> { // Settings, static metadata, etc. path: &'a Path, module_path: Option<&'a [String]>, @@ -65,23 +65,23 @@ pub struct Checker<'a> { is_stub: bool, noqa: flags::Noqa, noqa_line_for: &'a NoqaMapping, - pub settings: &'a Settings, - pub locator: &'a Locator<'a>, - pub stylist: &'a Stylist<'a>, - pub indexer: &'a Indexer, - pub importer: Importer<'a>, + pub(crate) settings: &'a Settings, + pub(crate) locator: &'a Locator<'a>, + pub(crate) stylist: &'a Stylist<'a>, + pub(crate) indexer: &'a Indexer, + pub(crate) importer: Importer<'a>, // Stateful fields. - pub ctx: Context<'a>, - pub diagnostics: Vec, - pub deletions: FxHashSet>, + pub(crate) ctx: Context<'a>, + pub(crate) diagnostics: Vec, + pub(crate) deletions: FxHashSet>, deferred: Deferred<'a>, // Check-specific state. - pub flake8_bugbear_seen: Vec<&'a Expr>, + pub(crate) flake8_bugbear_seen: Vec<&'a Expr>, } impl<'a> Checker<'a> { #[allow(clippy::too_many_arguments)] - pub fn new( + pub(crate) fn new( settings: &'a Settings, noqa_line_for: &'a NoqaMapping, noqa: flags::Noqa, @@ -117,12 +117,12 @@ impl<'a> Checker<'a> { impl<'a> Checker<'a> { /// Return `true` if a patch should be generated under the given autofix /// `Mode`. - pub fn patch(&self, code: Rule) -> bool { + pub(crate) fn patch(&self, code: Rule) -> bool { self.settings.rules.should_fix(code) } /// Return `true` if a `Rule` is disabled by a `noqa` directive. - pub fn rule_is_ignored(&self, code: Rule, offset: TextSize) -> bool { + pub(crate) fn rule_is_ignored(&self, code: Rule, offset: TextSize) -> bool { // TODO(charlie): `noqa` directives are mostly enforced in `check_lines.rs`. // However, in rare cases, we need to check them here. For example, when // removing unused imports, we create a single fix that's applied to all @@ -137,7 +137,7 @@ impl<'a> Checker<'a> { } /// Create a [`Generator`] to generate source code based on the current AST state. - pub fn generator(&self) -> Generator { + pub(crate) fn generator(&self) -> Generator { fn quote_style(context: &Context, locator: &Locator, indexer: &Indexer) -> Option { if !context.in_f_string() { return None; @@ -1253,7 +1253,7 @@ where level, module, self.module_path, - &self.settings.flake8_tidy_imports.ban_relative_imports, + self.settings.flake8_tidy_imports.ban_relative_imports, ) { self.diagnostics.push(diagnostic); @@ -4528,7 +4528,7 @@ impl<'a> Checker<'a> { } /// Visit an [`Expr`], and treat it as a type definition. - pub fn visit_type_definition(&mut self, expr: &'a Expr) { + pub(crate) fn visit_type_definition(&mut self, expr: &'a Expr) { let snapshot = self.ctx.flags; self.ctx.flags |= ContextFlags::TYPE_DEFINITION; self.visit_expr(expr); @@ -4536,7 +4536,7 @@ impl<'a> Checker<'a> { } /// Visit an [`Expr`], and treat it as _not_ a type definition. - pub fn visit_non_type_definition(&mut self, expr: &'a Expr) { + pub(crate) fn visit_non_type_definition(&mut self, expr: &'a Expr) { let snapshot = self.ctx.flags; self.ctx.flags -= ContextFlags::TYPE_DEFINITION; self.visit_expr(expr); @@ -4546,7 +4546,7 @@ impl<'a> Checker<'a> { /// Visit an [`Expr`], and treat it as a boolean test. This is useful for detecting whether an /// expressions return value is significant, or whether the calling context only relies on /// its truthiness. - pub fn visit_boolean_test(&mut self, expr: &'a Expr) { + pub(crate) fn visit_boolean_test(&mut self, expr: &'a Expr) { let snapshot = self.ctx.flags; self.ctx.flags |= ContextFlags::BOOLEAN_TEST; self.visit_expr(expr); diff --git a/crates/ruff/src/importer/mod.rs b/crates/ruff/src/importer/mod.rs index 4d0ac0d9fa..993751448a 100644 --- a/crates/ruff/src/importer/mod.rs +++ b/crates/ruff/src/importer/mod.rs @@ -14,7 +14,7 @@ use crate::importer::insertion::Insertion; mod insertion; -pub struct Importer<'a> { +pub(crate) struct Importer<'a> { python_ast: &'a Suite, locator: &'a Locator<'a>, stylist: &'a Stylist<'a>, @@ -22,7 +22,11 @@ pub struct Importer<'a> { } impl<'a> Importer<'a> { - pub fn new(python_ast: &'a Suite, locator: &'a Locator<'a>, stylist: &'a Stylist<'a>) -> Self { + pub(crate) fn new( + python_ast: &'a Suite, + locator: &'a Locator<'a>, + stylist: &'a Stylist<'a>, + ) -> Self { Self { python_ast, locator, @@ -32,7 +36,7 @@ impl<'a> Importer<'a> { } /// Visit a top-level import statement. - pub fn visit_import(&mut self, import: &'a Stmt) { + pub(crate) fn visit_import(&mut self, import: &'a Stmt) { self.ordered_imports.push(import); } @@ -49,7 +53,7 @@ impl<'a> Importer<'a> { /// If there are no existing imports, the new import will be added at the top /// of the file. Otherwise, it will be added after the most recent top-level /// import statement. - pub fn add_import(&self, import: &AnyImport, at: TextSize) -> Edit { + pub(crate) fn add_import(&self, import: &AnyImport, at: TextSize) -> Edit { let required_import = import.to_string(); if let Some(stmt) = self.preceding_import(at) { // Insert after the last top-level import. @@ -64,7 +68,7 @@ impl<'a> Importer<'a> { /// Return the top-level [`Stmt`] that imports the given module using `Stmt::ImportFrom` /// preceding the given position, if any. - pub fn find_import_from(&self, module: &str, at: TextSize) -> Option<&Stmt> { + pub(crate) fn find_import_from(&self, module: &str, at: TextSize) -> Option<&Stmt> { let mut import_from = None; for stmt in &self.ordered_imports { if stmt.start() >= at { @@ -87,7 +91,7 @@ impl<'a> Importer<'a> { } /// Add the given member to an existing `Stmt::ImportFrom` statement. - pub fn add_member(&self, stmt: &Stmt, member: &str) -> Result { + pub(crate) fn add_member(&self, stmt: &Stmt, member: &str) -> Result { let mut tree = match_module(self.locator.slice(stmt.range()))?; let import_from = match_import_from(&mut tree)?; let aliases = match_aliases(import_from)?; diff --git a/crates/ruff/src/rules/flake8_tidy_imports/banned_api.rs b/crates/ruff/src/rules/flake8_tidy_imports/banned_api.rs index ca5b4b0cef..bcaf261967 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/banned_api.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/banned_api.rs @@ -2,11 +2,12 @@ use rustc_hash::FxHashMap; use rustpython_parser::ast::{Expr, Ranged}; use serde::{Deserialize, Serialize}; -use crate::checkers::ast::Checker; use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation, CacheKey}; use ruff_python_ast::call_path::from_qualified_name; +use crate::checkers::ast::Checker; + pub type Settings = FxHashMap; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, CacheKey)] @@ -49,7 +50,7 @@ impl Violation for BannedApi { } /// TID251 -pub fn name_is_banned(checker: &mut Checker, name: String, located: &T) +pub(crate) fn name_is_banned(checker: &mut Checker, name: String, located: &T) where T: Ranged, { @@ -66,7 +67,7 @@ where } /// TID251 -pub fn name_or_parent_is_banned(checker: &mut Checker, name: &str, located: &T) +pub(crate) fn name_or_parent_is_banned(checker: &mut Checker, name: &str, located: &T) where T: Ranged, { @@ -93,7 +94,7 @@ where } /// TID251 -pub fn banned_attribute_access(checker: &mut Checker, expr: &Expr) { +pub(crate) fn banned_attribute_access(checker: &mut Checker, expr: &Expr) { let banned_api = &checker.settings.flake8_tidy_imports.banned_api; if let Some((banned_path, ban)) = checker.ctx.resolve_call_path(expr).and_then(|call_path| { banned_api diff --git a/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs b/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs index c98b9fd485..79ccdd8fdb 100644 --- a/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs +++ b/crates/ruff/src/rules/flake8_tidy_imports/relative_imports.rs @@ -121,25 +121,20 @@ fn fix_banned_relative_import( } /// TID252 -pub fn banned_relative_import( +pub(crate) fn banned_relative_import( checker: &Checker, stmt: &Stmt, level: Option, module: Option<&str>, module_path: Option<&[String]>, - strictness: &Strictness, + strictness: Strictness, ) -> Option { let strictness_level = match strictness { Strictness::All => 0, Strictness::Parents => 1, }; if level? > strictness_level { - let mut diagnostic = Diagnostic::new( - RelativeImports { - strictness: *strictness, - }, - stmt.range(), - ); + let mut diagnostic = Diagnostic::new(RelativeImports { strictness }, stmt.range()); if checker.patch(diagnostic.kind.rule()) { if let Some(fix) = fix_banned_relative_import(stmt, level, module, module_path, checker.generator()) diff --git a/scripts/add_rule.py b/scripts/add_rule.py index f9ffe4451d..f803ec4224 100755 --- a/scripts/add_rule.py +++ b/scripts/add_rule.py @@ -76,7 +76,7 @@ def main(*, name: str, prefix: str, code: str, linter: str) -> None: contents = rules_mod.read_text() parts = contents.split("\n\n") - new_pub_use = f"pub use {rule_name_snake}::{{{rule_name_snake}, {name}}}" + new_pub_use = f"pub(crate) use {rule_name_snake}::{{{rule_name_snake}, {name}}}" new_mod = f"mod {rule_name_snake};" if len(parts) == 2: @@ -105,6 +105,7 @@ use crate::checkers::ast::Checker; #[violation] pub struct {name}; + impl Violation for {name} {{ #[derive_message_formats] fn message(&self) -> String {{ @@ -117,7 +118,7 @@ impl Violation for {name} {{ fp.write( f""" /// {prefix}{code} -pub fn {rule_name_snake}(checker: &mut Checker) {{}} +pub(crate) fn {rule_name_snake}(checker: &mut Checker) {{}} """, )