Skip to content

Commit

Permalink
Use CommentRanges from the parsed output (#11591)
Browse files Browse the repository at this point in the history
## Summary

This PR updates the usages of `CommentRanges` from the `Indexer` to the
parsed output.

First, the `CommentRanges` are now built during the parsing step. In the
future, depending on the performance numbers, it could be updated to be
done after the parsing step. Nevertheless, the struct will be owned by
the parsed output. So, all references should now use the struct from the
parsed output instead of the indexer.

Now, the motivation to build `CommentRanges` while parsing is so that it
can be shared between the linter and the formatter as both requires it.
This also removes the need to have a dedicated method
(`tokens_and_ranges`).

There are two main updates:
1. The functions which only require `CommentRanges` are passed in the
type directly
2. If the functions require other fields from `Program`, then the
program is passed instead
  • Loading branch information
dhruvmanila authored May 29, 2024
1 parent b3d094c commit c0054de
Show file tree
Hide file tree
Showing 55 changed files with 208 additions and 175 deletions.
6 changes: 3 additions & 3 deletions crates/ruff_linter/src/checkers/filesystem.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::path::Path;

use ruff_diagnostics::Diagnostic;
use ruff_python_index::Indexer;
use ruff_python_trivia::CommentRanges;
use ruff_source_file::Locator;

use crate::registry::Rule;
Expand All @@ -13,7 +13,7 @@ pub(crate) fn check_file_path(
path: &Path,
package: Option<&Path>,
locator: &Locator,
indexer: &Indexer,
comment_ranges: &CommentRanges,
settings: &LinterSettings,
) -> Vec<Diagnostic> {
let mut diagnostics: Vec<Diagnostic> = vec![];
Expand All @@ -24,7 +24,7 @@ pub(crate) fn check_file_path(
path,
package,
locator,
indexer,
comment_ranges,
&settings.project_root,
&settings.src,
) {
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/checkers/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ pub(crate) fn check_imports(
settings,
package,
source_type,
program.tokens(),
program,
) {
diagnostics.push(diagnostic);
}
Expand Down
6 changes: 4 additions & 2 deletions crates/ruff_linter/src/checkers/physical_lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
use ruff_diagnostics::Diagnostic;
use ruff_python_codegen::Stylist;
use ruff_python_index::Indexer;
use ruff_python_trivia::CommentRanges;
use ruff_source_file::{Locator, UniversalNewlines};
use ruff_text_size::TextSize;

Expand All @@ -19,6 +20,7 @@ pub(crate) fn check_physical_lines(
locator: &Locator,
stylist: &Stylist,
indexer: &Indexer,
comment_ranges: &CommentRanges,
doc_lines: &[TextSize],
settings: &LinterSettings,
) -> Vec<Diagnostic> {
Expand All @@ -42,7 +44,7 @@ pub(crate) fn check_physical_lines(
.is_some()
{
if enforce_doc_line_too_long {
if let Some(diagnostic) = doc_line_too_long(&line, indexer, settings) {
if let Some(diagnostic) = doc_line_too_long(&line, comment_ranges, settings) {
diagnostics.push(diagnostic);
}
}
Expand All @@ -55,7 +57,7 @@ pub(crate) fn check_physical_lines(
}

if enforce_line_too_long {
if let Some(diagnostic) = line_too_long(&line, indexer, settings) {
if let Some(diagnostic) = line_too_long(&line, comment_ranges, settings) {
diagnostics.push(diagnostic);
}
}
Expand Down
34 changes: 20 additions & 14 deletions crates/ruff_linter/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,16 @@
use std::path::Path;

use ruff_notebook::CellOffsets;
use ruff_python_ast::PySourceType;
use ruff_python_ast::{ModModule, PySourceType};
use ruff_python_codegen::Stylist;

use ruff_diagnostics::Diagnostic;
use ruff_python_index::Indexer;
use ruff_python_parser::Tokens;
use ruff_python_parser::{Program, Tokens};
use ruff_source_file::Locator;
use ruff_text_size::Ranged;

use crate::directives::TodoComment;
use crate::linter::TokenSource;
use crate::registry::{AsRule, Rule};
use crate::rules::pycodestyle::rules::BlankLinesChecker;
use crate::rules::{
Expand All @@ -24,7 +23,7 @@ use crate::settings::LinterSettings;

#[allow(clippy::too_many_arguments)]
pub(crate) fn check_tokens(
tokens: &Tokens,
program: &Program<ModModule>,
path: &Path,
locator: &Locator,
indexer: &Indexer,
Expand All @@ -35,6 +34,9 @@ pub(crate) fn check_tokens(
) -> Vec<Diagnostic> {
let mut diagnostics: Vec<Diagnostic> = vec![];

let tokens = program.tokens();
let comment_ranges = program.comment_ranges();

if settings.rules.any_enabled(&[
Rule::BlankLineBetweenMethods,
Rule::BlankLinesTopLevel,
Expand All @@ -48,18 +50,18 @@ pub(crate) fn check_tokens(
}

if settings.rules.enabled(Rule::BlanketTypeIgnore) {
pygrep_hooks::rules::blanket_type_ignore(&mut diagnostics, indexer, locator);
pygrep_hooks::rules::blanket_type_ignore(&mut diagnostics, comment_ranges, locator);
}

if settings.rules.enabled(Rule::EmptyComment) {
pylint::rules::empty_comments(&mut diagnostics, indexer, locator);
pylint::rules::empty_comments(&mut diagnostics, comment_ranges, locator);
}

if settings
.rules
.enabled(Rule::AmbiguousUnicodeCharacterComment)
{
for range in indexer.comment_ranges() {
for range in comment_ranges {
ruff::rules::ambiguous_unicode_character_comment(
&mut diagnostics,
locator,
Expand All @@ -70,11 +72,16 @@ pub(crate) fn check_tokens(
}

if settings.rules.enabled(Rule::CommentedOutCode) {
eradicate::rules::commented_out_code(&mut diagnostics, locator, indexer, settings);
eradicate::rules::commented_out_code(&mut diagnostics, locator, comment_ranges, settings);
}

if settings.rules.enabled(Rule::UTF8EncodingDeclaration) {
pyupgrade::rules::unnecessary_coding_comment(&mut diagnostics, locator, indexer);
pyupgrade::rules::unnecessary_coding_comment(
&mut diagnostics,
locator,
indexer,
comment_ranges,
);
}

if settings.rules.enabled(Rule::TabIndentation) {
Expand Down Expand Up @@ -139,7 +146,7 @@ pub(crate) fn check_tokens(
}

if source_type.is_stub() && settings.rules.enabled(Rule::TypeCommentInStub) {
flake8_pyi::rules::type_comment_in_stub(&mut diagnostics, locator, indexer);
flake8_pyi::rules::type_comment_in_stub(&mut diagnostics, locator, comment_ranges);
}

if settings.rules.any_enabled(&[
Expand All @@ -149,7 +156,7 @@ pub(crate) fn check_tokens(
Rule::ShebangNotFirstLine,
Rule::ShebangMissingPython,
]) {
flake8_executable::rules::from_tokens(&mut diagnostics, path, locator, indexer);
flake8_executable::rules::from_tokens(&mut diagnostics, path, locator, comment_ranges);
}

if settings.rules.any_enabled(&[
Expand All @@ -165,16 +172,15 @@ pub(crate) fn check_tokens(
Rule::LineContainsTodo,
Rule::LineContainsHack,
]) {
let todo_comments: Vec<TodoComment> = indexer
.comment_ranges()
let todo_comments: Vec<TodoComment> = comment_ranges
.iter()
.enumerate()
.filter_map(|(i, comment_range)| {
let comment = locator.slice(*comment_range);
TodoComment::from_comment(comment, *comment_range, i)
})
.collect();
flake8_todos::rules::todos(&mut diagnostics, &todo_comments, locator, indexer);
flake8_todos::rules::todos(&mut diagnostics, &todo_comments, locator, comment_ranges);
flake8_fixme::rules::todos(&mut diagnostics, &todo_comments);
}

Expand Down
13 changes: 7 additions & 6 deletions crates/ruff_linter/src/directives.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ use std::iter::Peekable;
use std::str::FromStr;

use bitflags::bitflags;
use ruff_python_ast::StringFlags;
use ruff_python_ast::{ModModule, StringFlags};
use ruff_python_parser::lexer::LexResult;
use ruff_python_parser::Tok;
use ruff_python_parser::{Program, Tok};
use ruff_python_trivia::CommentRanges;
use ruff_text_size::{Ranged, TextLen, TextRange, TextSize};

use ruff_python_index::Indexer;
Expand Down Expand Up @@ -68,7 +69,7 @@ pub struct Directives {
}

pub fn extract_directives(
lxr: &[LexResult],
program: &Program<ModModule>,
flags: Flags,
locator: &Locator,
indexer: &Indexer,
Expand All @@ -80,7 +81,7 @@ pub fn extract_directives(
NoqaMapping::default()
},
isort: if flags.intersects(Flags::ISORT) {
extract_isort_directives(locator, indexer)
extract_isort_directives(locator, program.comment_ranges())
} else {
IsortDirectives::default()
},
Expand Down Expand Up @@ -213,12 +214,12 @@ fn extract_noqa_line_for(lxr: &[LexResult], locator: &Locator, indexer: &Indexer
}

/// Extract a set of ranges over which to disable isort.
fn extract_isort_directives(locator: &Locator, indexer: &Indexer) -> IsortDirectives {
fn extract_isort_directives(locator: &Locator, comment_ranges: &CommentRanges) -> IsortDirectives {
let mut exclusions: Vec<TextRange> = Vec::default();
let mut splits: Vec<TextSize> = Vec::default();
let mut off: Option<TextSize> = None;

for range in indexer.comment_ranges() {
for range in comment_ranges {
let comment_text = locator.slice(range);

// `isort` allows for `# isort: skip` and `# isort: skip_file` to include or
Expand Down
57 changes: 39 additions & 18 deletions crates/ruff_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ pub fn check_path(
.any(|rule_code| rule_code.lint_source().is_tokens())
{
diagnostics.extend(check_tokens(
program.tokens(),
&program,
path,
locator,
indexer,
Expand All @@ -120,7 +120,13 @@ pub fn check_path(
.iter_enabled()
.any(|rule_code| rule_code.lint_source().is_filesystem())
{
diagnostics.extend(check_file_path(path, package, locator, indexer, settings));
diagnostics.extend(check_file_path(
path,
package,
locator,
program.comment_ranges(),
settings,
));
}

// Run the logical line-based rules.
Expand Down Expand Up @@ -209,48 +215,63 @@ pub fn check_path(
.any(|rule_code| rule_code.lint_source().is_physical_lines())
{
diagnostics.extend(check_physical_lines(
locator, stylist, indexer, &doc_lines, settings,
locator,
stylist,
indexer,
program.comment_ranges(),
&doc_lines,
settings,
));
}

// Raise violations for internal test rules
#[cfg(any(feature = "test-rules", test))]
{
let comment_ranges = program.comment_ranges();

for test_rule in TEST_RULES {
if !settings.rules.enabled(*test_rule) {
continue;
}
let diagnostic = match test_rule {
Rule::StableTestRule => test_rules::StableTestRule::diagnostic(locator, indexer),
Rule::StableTestRule => {
test_rules::StableTestRule::diagnostic(locator, comment_ranges)
}
Rule::StableTestRuleSafeFix => {
test_rules::StableTestRuleSafeFix::diagnostic(locator, indexer)
test_rules::StableTestRuleSafeFix::diagnostic(locator, comment_ranges)
}
Rule::StableTestRuleUnsafeFix => {
test_rules::StableTestRuleUnsafeFix::diagnostic(locator, indexer)
test_rules::StableTestRuleUnsafeFix::diagnostic(locator, comment_ranges)
}
Rule::StableTestRuleDisplayOnlyFix => {
test_rules::StableTestRuleDisplayOnlyFix::diagnostic(locator, indexer)
test_rules::StableTestRuleDisplayOnlyFix::diagnostic(locator, comment_ranges)
}
Rule::NurseryTestRule => {
test_rules::NurseryTestRule::diagnostic(locator, comment_ranges)
}
Rule::PreviewTestRule => {
test_rules::PreviewTestRule::diagnostic(locator, comment_ranges)
}
Rule::NurseryTestRule => test_rules::NurseryTestRule::diagnostic(locator, indexer),
Rule::PreviewTestRule => test_rules::PreviewTestRule::diagnostic(locator, indexer),
Rule::DeprecatedTestRule => {
test_rules::DeprecatedTestRule::diagnostic(locator, indexer)
test_rules::DeprecatedTestRule::diagnostic(locator, comment_ranges)
}
Rule::AnotherDeprecatedTestRule => {
test_rules::AnotherDeprecatedTestRule::diagnostic(locator, indexer)
test_rules::AnotherDeprecatedTestRule::diagnostic(locator, comment_ranges)
}
Rule::RemovedTestRule => {
test_rules::RemovedTestRule::diagnostic(locator, comment_ranges)
}
Rule::RemovedTestRule => test_rules::RemovedTestRule::diagnostic(locator, indexer),
Rule::AnotherRemovedTestRule => {
test_rules::AnotherRemovedTestRule::diagnostic(locator, indexer)
test_rules::AnotherRemovedTestRule::diagnostic(locator, comment_ranges)
}
Rule::RedirectedToTestRule => {
test_rules::RedirectedToTestRule::diagnostic(locator, indexer)
test_rules::RedirectedToTestRule::diagnostic(locator, comment_ranges)
}
Rule::RedirectedFromTestRule => {
test_rules::RedirectedFromTestRule::diagnostic(locator, indexer)
test_rules::RedirectedFromTestRule::diagnostic(locator, comment_ranges)
}
Rule::RedirectedFromPrefixTestRule => {
test_rules::RedirectedFromPrefixTestRule::diagnostic(locator, indexer)
test_rules::RedirectedFromPrefixTestRule::diagnostic(locator, comment_ranges)
}
_ => unreachable!("All test rules must have an implementation"),
};
Expand Down Expand Up @@ -287,7 +308,7 @@ pub fn check_path(
&mut diagnostics,
path,
locator,
indexer.comment_ranges(),
program.comment_ranges(),
&directives.noqa_line_for,
error.is_none(),
&per_file_ignores,
Expand Down Expand Up @@ -407,7 +428,7 @@ pub fn add_noqa_to_path(
path,
&diagnostics,
&locator,
indexer.comment_ranges(),
program.comment_ranges(),
&settings.external,
&directives.noqa_line_for,
stylist.line_ending(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_index::Indexer;
use ruff_python_trivia::CommentRanges;
use ruff_source_file::Locator;

use crate::settings::LinterSettings;
Expand Down Expand Up @@ -47,14 +47,14 @@ impl Violation for CommentedOutCode {
pub(crate) fn commented_out_code(
diagnostics: &mut Vec<Diagnostic>,
locator: &Locator,
indexer: &Indexer,
comment_ranges: &CommentRanges,
settings: &LinterSettings,
) {
// Skip comments within `/// script` tags.
let mut in_script_tag = false;

// Iterate over all comments in the document.
for range in indexer.comment_ranges() {
for range in comment_ranges {
let line = locator.lines(*range);

// Detect `/// script` tags.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub(crate) fn zip_without_explicit_strict(checker: &mut Checker, call: &ast::Exp
add_argument(
"strict=False",
&call.arguments,
checker.indexer().comment_ranges(),
checker.program().comment_ranges(),
checker.locator().contents(),
),
// If the function call contains `**kwargs`, mark the fix as unsafe.
Expand Down
Loading

0 comments on commit c0054de

Please sign in to comment.