Skip to content

Commit

Permalink
[flake8-quotes] Fix Autofix Error (Q000, Q002) (#10199)
Browse files Browse the repository at this point in the history
## Summary
In issue #6785 it is reported
that a docstring in the form of `''"assert" ' SAM macro definitions '''`
is autocorrected to `"""assert" ' SAM macro definitions '''` (note the
triple quotes one only one side), which breaks the python program due
`undetermined string lateral`.

* `Q002`: Not only would docstrings in the form of `''"assert" ' SAM
macro definitions '''` (single quotes) be autofixed wrongly, but also
e.g. `""'assert' ' SAM macro definitions '''` (double quotes). The bug
is present for docstrings in all scopes (e.g. module docstrings, class
docstrings, function docstrings)

* `Q000`: The autofix error is not only present for `Q002` (docstrings),
but also for inline strings (`Q000`). Therefore `s = ''"assert" ' SAM
macro definitions '''` will also be wrongly autofixed.

Note that situation in which the first string is non-empty can be fixed,
e.g. `'123'"assert" ' SAM macro definitions '''` -> `"123""assert" ' SAM
macro definitions '''` is valid.

## What
* Change FixAvailability of `Q000` `Q002` to `Sometimes`
* Changed both rules such that docstrings/inline strings that cannot be
fixed are still reported as bad quotes via diagnostics, but no fix is
provided

## Test Plan
* For `Q000`: Add docstrings in different scopes that (partially) would
have been autofixed wrongly
* For `Q002`: Add inline strings that (partially) would have been
autofixed wrongly

Closes #6785
  • Loading branch information
robincaloudis committed Mar 18, 2024
1 parent dc021dd commit 2edd617
Show file tree
Hide file tree
Showing 22 changed files with 589 additions and 13 deletions.
@@ -0,0 +1,9 @@
class SingleLineDocstrings():
""'Start with empty string' ' and lint docstring safely'
""" Not a docstring """

def foo(self, bar="""not a docstring"""):
""'Start with empty string' ' and lint docstring safely'
pass

class Nested(foo()[:]): ""'Start with empty string' ' and lint docstring safely'; pass
@@ -0,0 +1,9 @@
class SingleLineDocstrings():
"Do not"' start with empty string' ' and lint docstring safely'
""" Not a docstring """

def foo(self, bar="""not a docstring"""):
"Do not"' start with empty string' ' and lint docstring safely'
pass

class Nested(foo()[:]): "Do not"' start with empty string' ' and lint docstring safely'; pass
@@ -0,0 +1,5 @@
""'Start with empty string' ' and lint docstring safely'

def foo():
pass
""" this is not a docstring """
@@ -0,0 +1,5 @@
"Do not"' start with empty string' ' and lint docstring safely'

def foo():
pass
""" this is not a docstring """
@@ -0,0 +1,9 @@
class SingleLineDocstrings():
''"Start with empty string" ' and lint docstring safely'
''' Not a docstring '''

def foo(self, bar='''not a docstring'''):
''"Start with empty string" ' and lint docstring safely'
pass

class Nested(foo()[:]): ''"Start with empty string" ' and lint docstring safely'; pass
@@ -0,0 +1,9 @@
class SingleLineDocstrings():
'Do not'" start with empty string" ' and lint docstring safely'
''' Not a docstring '''

def foo(self, bar='''not a docstring'''):
'Do not'" start with empty string" ' and lint docstring safely'
pass

class Nested(foo()[:]): 'Do not'" start with empty string" ' and lint docstring safely'; pass
@@ -0,0 +1,5 @@
''"Start with empty string" ' and lint docstring safely'

def foo():
pass
""" this is not a docstring """
@@ -0,0 +1,5 @@
'Do not'" start with empty string" ' and lint docstring safely'

def foo():
pass
""" this is not a docstring """
@@ -0,0 +1,2 @@
s = ""'Start with empty string' ' and lint docstring safely'
s = "Do not"' start with empty string' ' and lint docstring safely'
@@ -0,0 +1,2 @@
s = ''"Start with empty string" ' and lint docstring safely'
s = 'Do not'" start with empty string" ' and lint docstring safely'
10 changes: 10 additions & 0 deletions crates/ruff_linter/src/rules/flake8_quotes/mod.rs
Expand Up @@ -24,6 +24,7 @@ mod tests {
#[test_case(Path::new("doubles_multiline_string.py"))]
#[test_case(Path::new("doubles_noqa.py"))]
#[test_case(Path::new("doubles_wrapped.py"))]
#[test_case(Path::new("doubles_would_be_triple_quotes.py"))]
fn require_singles(path: &Path) -> Result<()> {
let snapshot = format!("require_singles_over_{}", path.to_string_lossy());
let diagnostics = test_path(
Expand Down Expand Up @@ -93,6 +94,7 @@ mod tests {
#[test_case(Path::new("singles_multiline_string.py"))]
#[test_case(Path::new("singles_noqa.py"))]
#[test_case(Path::new("singles_wrapped.py"))]
#[test_case(Path::new("singles_would_be_triple_quotes.py"))]
fn require_doubles(path: &Path) -> Result<()> {
let snapshot = format!("require_doubles_over_{}", path.to_string_lossy());
let diagnostics = test_path(
Expand Down Expand Up @@ -127,6 +129,10 @@ mod tests {
#[test_case(Path::new("docstring_singles_module_singleline.py"))]
#[test_case(Path::new("docstring_singles_class.py"))]
#[test_case(Path::new("docstring_singles_function.py"))]
#[test_case(Path::new("docstring_singles_mixed_quotes_module_singleline_var_1.py"))]
#[test_case(Path::new("docstring_singles_mixed_quotes_module_singleline_var_2.py"))]
#[test_case(Path::new("docstring_singles_mixed_quotes_class_var_1.py"))]
#[test_case(Path::new("docstring_singles_mixed_quotes_class_var_2.py"))]
fn require_docstring_doubles(path: &Path) -> Result<()> {
let snapshot = format!("require_docstring_doubles_over_{}", path.to_string_lossy());
let diagnostics = test_path(
Expand Down Expand Up @@ -161,6 +167,10 @@ mod tests {
#[test_case(Path::new("docstring_singles_module_singleline.py"))]
#[test_case(Path::new("docstring_singles_class.py"))]
#[test_case(Path::new("docstring_singles_function.py"))]
#[test_case(Path::new("docstring_doubles_mixed_quotes_module_singleline_var_1.py"))]
#[test_case(Path::new("docstring_doubles_mixed_quotes_module_singleline_var_2.py"))]
#[test_case(Path::new("docstring_doubles_mixed_quotes_class_var_1.py"))]
#[test_case(Path::new("docstring_doubles_mixed_quotes_class_var_2.py"))]
fn require_docstring_singles(path: &Path) -> Result<()> {
let snapshot = format!("require_docstring_singles_over_{}", path.to_string_lossy());
let diagnostics = test_path(
Expand Down
Expand Up @@ -2,7 +2,7 @@ use ruff_python_parser::lexer::LexResult;
use ruff_python_parser::Tok;
use ruff_text_size::{TextRange, TextSize};

use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_source_file::Locator;

Expand Down Expand Up @@ -44,7 +44,9 @@ pub struct BadQuotesInlineString {
preferred_quote: Quote,
}

impl AlwaysFixableViolation for BadQuotesInlineString {
impl Violation for BadQuotesInlineString {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let BadQuotesInlineString { preferred_quote } = self;
Expand All @@ -54,11 +56,11 @@ impl AlwaysFixableViolation for BadQuotesInlineString {
}
}

fn fix_title(&self) -> String {
fn fix_title(&self) -> Option<String> {
let BadQuotesInlineString { preferred_quote } = self;
match preferred_quote {
Quote::Double => "Replace single quotes with double quotes".to_string(),
Quote::Single => "Replace double quotes with single quotes".to_string(),
Quote::Double => Some("Replace single quotes with double quotes".to_string()),
Quote::Single => Some("Replace double quotes with single quotes".to_string()),
}
}
}
Expand Down Expand Up @@ -155,7 +157,9 @@ pub struct BadQuotesDocstring {
preferred_quote: Quote,
}

impl AlwaysFixableViolation for BadQuotesDocstring {
impl Violation for BadQuotesDocstring {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
let BadQuotesDocstring { preferred_quote } = self;
Expand All @@ -165,11 +169,11 @@ impl AlwaysFixableViolation for BadQuotesDocstring {
}
}

fn fix_title(&self) -> String {
fn fix_title(&self) -> Option<String> {
let BadQuotesDocstring { preferred_quote } = self;
match preferred_quote {
Quote::Double => "Replace single quotes docstring with double quotes".to_string(),
Quote::Single => "Replace double quotes docstring with single quotes".to_string(),
Quote::Double => Some("Replace single quotes docstring with double quotes".to_string()),
Quote::Single => Some("Replace double quotes docstring with single quotes".to_string()),
}
}
}
Expand All @@ -188,10 +192,10 @@ const fn good_multiline_ending(quote: Quote) -> &'static str {
}
}

const fn good_docstring(quote: Quote) -> &'static str {
const fn good_docstring(quote: Quote) -> char {
match quote {
Quote::Double => "\"",
Quote::Single => "'",
Quote::Double => '"',
Quote::Single => '\'',
}
}

Expand All @@ -203,6 +207,12 @@ struct Trivia<'a> {
is_multiline: bool,
}

impl Trivia<'_> {
fn has_empty_text(&self) -> bool {
self.raw_text == "\"\"" || self.raw_text == "''"
}
}

impl<'a> From<&'a str> for Trivia<'a> {
fn from(value: &'a str) -> Self {
// Remove any prefixes (e.g., remove `u` from `u"foo"`).
Expand Down Expand Up @@ -231,12 +241,38 @@ impl<'a> From<&'a str> for Trivia<'a> {
}
}

/// Returns `true` if the [`TextRange`] is preceded by two consecutive quotes.
fn text_starts_at_consecutive_quote(locator: &Locator, range: TextRange, quote: Quote) -> bool {
let mut previous_two_chars = locator.up_to(range.start()).chars().rev();
previous_two_chars.next() == Some(good_docstring(quote))
&& previous_two_chars.next() == Some(good_docstring(quote))
}

/// Returns `true` if the [`TextRange`] ends at a quote character.
fn text_ends_at_quote(locator: &Locator, range: TextRange, quote: Quote) -> bool {
locator
.after(range.end())
.starts_with(good_docstring(quote))
}

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

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)
{
// Fixing this would result in a one-sided multi-line docstring, which would
// introduce a syntax error.
return Some(Diagnostic::new(
BadQuotesDocstring {
preferred_quote: quotes_settings.docstring_quotes,
},
range,
));
}

if trivia
.raw_text
Expand All @@ -253,7 +289,9 @@ fn docstring(locator: &Locator, range: TextRange, settings: &LinterSettings) ->
);
let quote_count = if trivia.is_multiline { 3 } else { 1 };
let string_contents = &trivia.raw_text[quote_count..trivia.raw_text.len() - quote_count];
let quote = good_docstring(quotes_settings.docstring_quotes).repeat(quote_count);
let quote = good_docstring(quotes_settings.docstring_quotes)
.to_string()
.repeat(quote_count);
let mut fixed_contents =
String::with_capacity(trivia.prefix.len() + string_contents.len() + quote.len() * 2);
fixed_contents.push_str(trivia.prefix);
Expand Down Expand Up @@ -344,6 +382,42 @@ fn strings(
// 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)
{
// 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(
BadQuotesInlineString {
preferred_quote: quotes_settings.inline_quotes,
},
*range,
));
continue;
}

if text_starts_at_consecutive_quote(
locator,
*range,
settings.flake8_quotes.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(
BadQuotesInlineString {
preferred_quote: quotes_settings.inline_quotes,
},
*range,
));
continue;
}

let mut diagnostic = Diagnostic::new(
BadQuotesInlineString {
preferred_quote: quotes_settings.inline_quotes,
Expand Down
@@ -0,0 +1,56 @@
---
source: crates/ruff_linter/src/rules/flake8_quotes/mod.rs
---
docstring_singles_mixed_quotes_class_var_1.py:2:5: Q002 Single quote docstring found but double quotes preferred
|
1 | class SingleLineDocstrings():
2 | ''"Start with empty string" ' and lint docstring safely'
| ^^ Q002
3 | ''' Not a docstring '''
|
= 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
|
1 | class SingleLineDocstrings():
2 | ''"Start with empty string" ' and lint docstring safely'
| ^^^^^^^^^^^^^^^^^^^^^^^^^ Q000
3 | ''' Not a docstring '''
|
= help: Replace double quotes with single quotes

docstring_singles_mixed_quotes_class_var_1.py:6:9: 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'
| ^^ Q002
7 | pass
|
= 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
|
5 | def foo(self, bar='''not a docstring'''):
6 | ''"Start with empty string" ' and lint docstring safely'
| ^^^^^^^^^^^^^^^^^^^^^^^^^ Q000
7 | pass
|
= help: Replace double quotes with single quotes

docstring_singles_mixed_quotes_class_var_1.py:9:29: 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
| ^^ Q002
|
= 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
|
7 | pass
8 |
9 | class Nested(foo()[:]): ''"Start with empty string" ' and lint docstring safely'; pass
| ^^^^^^^^^^^^^^^^^^^^^^^^^ Q000
|
= help: Replace double quotes with single quotes

0 comments on commit 2edd617

Please sign in to comment.