Skip to content

Commit

Permalink
Move Q001-3 to AST based checker (astral-sh#10312)
Browse files Browse the repository at this point in the history
## Summary

Continuing with astral-sh#7595, this PR
moves the `Q001`, `Q002`, `Q003` rules to the AST based checker.

## Test Plan

Make sure all of the existing test cases pass and verify there are no
ecosystem changes.
  • Loading branch information
dhruvmanila authored Mar 23, 2024
1 parent 0c194f5 commit 895d9df
Show file tree
Hide file tree
Showing 8 changed files with 108 additions and 177 deletions.
9 changes: 8 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/string_like.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use ruff_python_ast::StringLike;

use crate::checkers::ast::Checker;
use crate::codes::Rule;
use crate::rules::{flake8_bandit, flake8_pyi, ruff};
use crate::rules::{flake8_bandit, flake8_pyi, flake8_quotes, ruff};

/// Run lint rules over a [`StringLike`] syntax nodes.
pub(crate) fn string_like(string_like: StringLike, checker: &mut Checker) {
Expand All @@ -23,4 +23,11 @@ pub(crate) fn string_like(string_like: StringLike, checker: &mut Checker) {
flake8_pyi::rules::string_or_bytes_too_long(checker, string_like);
}
}
if checker.any_enabled(&[
Rule::BadQuotesInlineString,
Rule::BadQuotesMultilineString,
Rule::BadQuotesDocstring,
]) {
flake8_quotes::rules::check_string_quotes(checker, string_like);
}
}
8 changes: 0 additions & 8 deletions crates/ruff_linter/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,14 +130,6 @@ pub(crate) fn check_tokens(
flake8_quotes::rules::unnecessary_escaped_quote(&mut diagnostics, tokens, locator);
}

if settings.rules.any_enabled(&[
Rule::BadQuotesInlineString,
Rule::BadQuotesMultilineString,
Rule::BadQuotesDocstring,
]) {
flake8_quotes::rules::check_string_quotes(&mut diagnostics, tokens, locator, settings);
}

if settings.rules.any_enabled(&[
Rule::SingleLineImplicitStringConcatenation,
Rule::MultiLineImplicitStringConcatenation,
Expand Down
3 changes: 0 additions & 3 deletions crates/ruff_linter/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -257,9 +257,6 @@ impl Rule {
| Rule::TrailingWhitespace => LintSource::PhysicalLines,
Rule::AmbiguousUnicodeCharacterComment
| Rule::AvoidableEscapedQuote
| Rule::BadQuotesDocstring
| Rule::BadQuotesInlineString
| Rule::BadQuotesMultilineString
| Rule::BlanketNOQA
| Rule::BlanketTypeIgnore
| Rule::BlankLineAfterDecorator
Expand Down
177 changes: 40 additions & 137 deletions crates/ruff_linter/src/rules/flake8_quotes/rules/check_string_quotes.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,10 @@
use ruff_python_parser::lexer::LexResult;
use ruff_python_parser::Tok;
use ruff_text_size::{TextRange, TextSize};

use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::StringLike;
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};

use crate::lex::docstring_detection::StateMachine;

use crate::settings::LinterSettings;
use crate::checkers::ast::Checker;

use super::super::settings::Quote;

Expand Down Expand Up @@ -256,29 +252,31 @@ fn text_ends_at_quote(locator: &Locator, range: TextRange, quote: Quote) -> bool
}

/// Q002
fn docstring(locator: &Locator, range: TextRange, settings: &LinterSettings) -> Option<Diagnostic> {
let quotes_settings = &settings.flake8_quotes;
fn docstring(checker: &mut Checker, range: TextRange) {
let quotes_settings = &checker.settings.flake8_quotes;
let locator = checker.locator();

let text = locator.slice(range);
let trivia: Trivia = text.into();
if trivia.has_empty_text()
&& text_ends_at_quote(locator, range, settings.flake8_quotes.docstring_quotes)
&& text_ends_at_quote(locator, range, quotes_settings.docstring_quotes)
{
// Fixing this would result in a one-sided multi-line docstring, which would
// introduce a syntax error.
return Some(Diagnostic::new(
checker.diagnostics.push(Diagnostic::new(
BadQuotesDocstring {
preferred_quote: quotes_settings.docstring_quotes,
},
range,
));
return;
}

if trivia
.raw_text
.contains(good_docstring(quotes_settings.docstring_quotes))
{
return None;
return;
}

let mut diagnostic = Diagnostic::new(
Expand All @@ -302,18 +300,13 @@ fn docstring(locator: &Locator, range: TextRange, settings: &LinterSettings) ->
fixed_contents,
range,
)));
Some(diagnostic)
checker.diagnostics.push(diagnostic);
}

/// Q000, Q001
fn strings(
locator: &Locator,
sequence: &[TextRange],
settings: &LinterSettings,
) -> Vec<Diagnostic> {
let mut diagnostics = vec![];

let quotes_settings = &settings.flake8_quotes;
fn strings(checker: &mut Checker, sequence: &[TextRange]) {
let quotes_settings = &checker.settings.flake8_quotes;
let locator = checker.locator();

let trivia = sequence
.iter()
Expand Down Expand Up @@ -377,20 +370,20 @@ fn strings(
fixed_contents,
*range,
)));
diagnostics.push(diagnostic);
checker.diagnostics.push(diagnostic);
} else if trivia.last_quote_char != quotes_settings.inline_quotes.as_char()
// If we're not using the preferred type, only allow use to avoid escapes.
&& !relax_quote
{
if trivia.has_empty_text()
&& text_ends_at_quote(locator, *range, settings.flake8_quotes.inline_quotes)
&& text_ends_at_quote(locator, *range, quotes_settings.inline_quotes)
{
// Fixing this would introduce a syntax error. For example, changing the initial
// single quotes to double quotes would result in a syntax error:
// ```python
// ''"assert" ' SAM macro definitions '''
// ```
diagnostics.push(Diagnostic::new(
checker.diagnostics.push(Diagnostic::new(
BadQuotesInlineString {
preferred_quote: quotes_settings.inline_quotes,
},
Expand All @@ -399,17 +392,13 @@ fn strings(
continue;
}

if text_starts_at_consecutive_quote(
locator,
*range,
settings.flake8_quotes.inline_quotes,
) {
if text_starts_at_consecutive_quote(locator, *range, quotes_settings.inline_quotes) {
// Fixing this would introduce a syntax error. For example, changing the double
// doubles to single quotes would result in a syntax error:
// ```python
// ''"assert" ' SAM macro definitions '''
// ```
diagnostics.push(Diagnostic::new(
checker.diagnostics.push(Diagnostic::new(
BadQuotesInlineString {
preferred_quote: quotes_settings.inline_quotes,
},
Expand All @@ -436,120 +425,34 @@ fn strings(
fixed_contents,
*range,
)));
diagnostics.push(diagnostic);
checker.diagnostics.push(diagnostic);
}
}

diagnostics
}

/// A builder for the f-string range.
///
/// For now, this is limited to the outermost f-string and doesn't support
/// nested f-strings.
#[derive(Debug, Default)]
struct FStringRangeBuilder {
start_location: TextSize,
end_location: TextSize,
nesting: u32,
}

impl FStringRangeBuilder {
fn visit_token(&mut self, token: &Tok, range: TextRange) {
match token {
Tok::FStringStart(_) => {
if self.nesting == 0 {
self.start_location = range.start();
}
self.nesting += 1;
}
Tok::FStringEnd => {
self.nesting = self.nesting.saturating_sub(1);
if self.nesting == 0 {
self.end_location = range.end();
}
}
_ => {}
}
}

/// Returns `true` if the lexer is currently inside of a f-string.
///
/// It'll return `false` once the `FStringEnd` token for the outermost
/// f-string is visited.
const fn in_fstring(&self) -> bool {
self.nesting > 0
}

/// Returns the complete range of the previously visited f-string.
///
/// This method should only be called once the lexer is outside of any
/// f-string otherwise it might return an invalid range.
///
/// It doesn't consume the builder because there can be multiple f-strings
/// throughout the source code.
fn finish(&self) -> TextRange {
debug_assert!(!self.in_fstring());
TextRange::new(self.start_location, self.end_location)
}
}

/// Generate `flake8-quote` diagnostics from a token stream.
pub(crate) fn check_string_quotes(
diagnostics: &mut Vec<Diagnostic>,
lxr: &[LexResult],
locator: &Locator,
settings: &LinterSettings,
) {
// Keep track of sequences of strings, which represent implicit string
// concatenation, and should thus be handled as a single unit.
let mut sequence = vec![];
let mut state_machine = StateMachine::default();
let mut fstring_range_builder = FStringRangeBuilder::default();
for &(ref tok, range) in lxr.iter().flatten() {
fstring_range_builder.visit_token(tok, range);
if fstring_range_builder.in_fstring() {
continue;
}
pub(crate) fn check_string_quotes(checker: &mut Checker, string_like: StringLike) {
// If the string is part of a f-string, ignore it.
if checker
.indexer()
.fstring_ranges()
.outermost(string_like.start())
.is_some_and(|outer| outer.start() < string_like.start() && string_like.end() < outer.end())
{
return;
}

let is_docstring = state_machine.consume(tok);
let ranges: Vec<TextRange> = match string_like {
StringLike::String(node) => node.value.iter().map(Ranged::range).collect(),
StringLike::Bytes(node) => node.value.iter().map(Ranged::range).collect(),
StringLike::FString(node) => node.value.iter().map(Ranged::range).collect(),
};

// If this is a docstring, consume the existing sequence, then consume the
// docstring, then move on.
if is_docstring {
if !sequence.is_empty() {
diagnostics.extend(strings(locator, &sequence, settings));
sequence.clear();
}
if let Some(diagnostic) = docstring(locator, range, settings) {
diagnostics.push(diagnostic);
}
} else {
match tok {
Tok::String { .. } => {
// If this is a string, add it to the sequence.
sequence.push(range);
}
Tok::FStringEnd => {
// If this is the end of an f-string, add the entire f-string
// range to the sequence.
sequence.push(fstring_range_builder.finish());
}
Tok::Comment(..) | Tok::NonLogicalNewline => continue,
_ => {
// Otherwise, consume the sequence.
if !sequence.is_empty() {
diagnostics.extend(strings(locator, &sequence, settings));
sequence.clear();
}
}
}
if checker.semantic().in_docstring() {
for range in ranges {
docstring(checker, range);
}
}

// If we have an unterminated sequence, consume it.
if !sequence.is_empty() {
diagnostics.extend(strings(locator, &sequence, settings));
sequence.clear();
} else {
strings(checker, &ranges);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,22 @@ docstring_singles_mixed_quotes_class_var_1.py:2:5: Q002 Single quote docstring f
|
= help: Replace single quotes docstring with double quotes

docstring_singles_mixed_quotes_class_var_1.py:2:7: Q000 Double quotes found but single quotes preferred
docstring_singles_mixed_quotes_class_var_1.py:2:33: Q002 [*] Single quote docstring found but double quotes preferred
|
1 | class SingleLineDocstrings():
2 | ''"Start with empty string" ' and lint docstring safely'
| ^^^^^^^^^^^^^^^^^^^^^^^^^ Q000
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Q002
3 | ''' Not a docstring '''
|
= help: Replace double quotes with single quotes
= help: Replace single quotes docstring with double quotes

Safe fix
1 1 | class SingleLineDocstrings():
2 |- ''"Start with empty string" ' and lint docstring safely'
2 |+ ''"Start with empty string" " and lint docstring safely"
3 3 | ''' Not a docstring '''
4 4 |
5 5 | def foo(self, bar='''not a docstring'''):

docstring_singles_mixed_quotes_class_var_1.py:6:9: Q002 Single quote docstring found but double quotes preferred
|
Expand All @@ -28,14 +36,24 @@ docstring_singles_mixed_quotes_class_var_1.py:6:9: Q002 Single quote docstring f
|
= help: Replace single quotes docstring with double quotes

docstring_singles_mixed_quotes_class_var_1.py:6:11: Q000 Double quotes found but single quotes preferred
docstring_singles_mixed_quotes_class_var_1.py:6:37: Q002 [*] Single quote docstring found but double quotes preferred
|
5 | def foo(self, bar='''not a docstring'''):
6 | ''"Start with empty string" ' and lint docstring safely'
| ^^^^^^^^^^^^^^^^^^^^^^^^^ Q000
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Q002
7 | pass
|
= help: Replace double quotes with single quotes
= help: Replace single quotes docstring with double quotes

Safe fix
3 3 | ''' Not a docstring '''
4 4 |
5 5 | def foo(self, bar='''not a docstring'''):
6 |- ''"Start with empty string" ' and lint docstring safely'
6 |+ ''"Start with empty string" " and lint docstring safely"
7 7 | pass
8 8 |
9 9 | class Nested(foo()[:]): ''"Start with empty string" ' and lint docstring safely'; pass

docstring_singles_mixed_quotes_class_var_1.py:9:29: Q002 Single quote docstring found but double quotes preferred
|
Expand All @@ -46,11 +64,18 @@ docstring_singles_mixed_quotes_class_var_1.py:9:29: Q002 Single quote docstring
|
= help: Replace single quotes docstring with double quotes

docstring_singles_mixed_quotes_class_var_1.py:9:31: Q000 Double quotes found but single quotes preferred
docstring_singles_mixed_quotes_class_var_1.py:9:57: Q002 [*] Single quote docstring found but double quotes preferred
|
7 | pass
8 |
9 | class Nested(foo()[:]): ''"Start with empty string" ' and lint docstring safely'; pass
| ^^^^^^^^^^^^^^^^^^^^^^^^^ Q000
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Q002
|
= help: Replace double quotes with single quotes
= help: Replace single quotes docstring with double quotes

Safe fix
6 6 | ''"Start with empty string" ' and lint docstring safely'
7 7 | pass
8 8 |
9 |- class Nested(foo()[:]): ''"Start with empty string" ' and lint docstring safely'; pass
9 |+ class Nested(foo()[:]): ''"Start with empty string" " and lint docstring safely"; pass
Loading

0 comments on commit 895d9df

Please sign in to comment.