Skip to content

Commit

Permalink
Add new rule to check for useless quote escapes
Browse files Browse the repository at this point in the history
  • Loading branch information
ThiefMaster committed Nov 12, 2023
1 parent 96b265c commit 7f0bf50
Show file tree
Hide file tree
Showing 11 changed files with 939 additions and 6 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
this_should_raise_Q100 = 'This is a \"string\"'
this_should_raise_Q100 = 'This is \\ a \\\"string\"'
this_is_fine = '"This" is a \"string\"'
this_is_fine = "This is a 'string'"
this_is_fine = "\"This\" is a 'string'"
this_is_fine = r'This is a \"string\"'
this_is_fine = R'This is a \"string\"'
this_should_raise_Q100 = (
'This is a'
'\"string\"'
)

# Same as above, but with f-strings
f'This is a \"string\"' # Q100
f'This is \\ a \\\"string\"' # Q100
f'"This" is a \"string\"'
f"This is a 'string'"
f"\"This\" is a 'string'"
fr'This is a \"string\"'
fR'This is a \"string\"'
this_should_raise_Q100 = (
f'This is a'
f'\"string\"' # Q100
)

# Nested f-strings (Python 3.12+)
#
# The first one is interesting because the fix for it is valid pre 3.12:
#
# f"'foo' {'nested'}"
#
# but as the actual string itself is invalid pre 3.12, we don't catch it.
f'\"foo\" {'nested'}' # Q100
f'\"foo\" {f'nested'}' # Q100
f'\"foo\" {f'\"nested\"'} \"\"' # Q100

f'normal {f'nested'} normal'
f'\"normal\" {f'nested'} normal' # Q100
f'\"normal\" {f'nested'} "double quotes"'
f'\"normal\" {f'\"nested\" {'other'} normal'} "double quotes"' # Q100
f'\"normal\" {f'\"nested\" {'other'} "double quotes"'} normal' # Q100
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
this_should_raise_Q100 = "This is a \'string\'"
this_should_raise_Q100 = "'This' is a \'string\'"
this_is_fine = 'This is a "string"'
this_is_fine = '\'This\' is a "string"'
this_is_fine = r"This is a \'string\'"
this_is_fine = R"This is a \'string\'"
this_should_raise_Q100 = (
"This is a"
"\'string\'"
)

# Same as above, but with f-strings
f"This is a \'string\'" # Q100
f"'This' is a \'string\'" # Q100
f'This is a "string"'
f'\'This\' is a "string"'
fr"This is a \'string\'"
fR"This is a \'string\'"
this_should_raise_Q100 = (
f"This is a"
f"\'string\'" # Q100
)

# Nested f-strings (Python 3.12+)
#
# The first one is interesting because the fix for it is valid pre 3.12:
#
# f'"foo" {"nested"}'
#
# but as the actual string itself is invalid pre 3.12, we don't catch it.
f"\'foo\' {"foo"}" # Q100
f"\'foo\' {f"foo"}" # Q100
f"\'foo\' {f"\'foo\'"} \'\'" # Q100

f"normal {f"nested"} normal"
f"\'normal\' {f"nested"} normal" # Q100
f"\'normal\' {f"nested"} 'single quotes'"
f"\'normal\' {f"\'nested\' {"other"} normal"} 'single quotes'" # Q100
f"\'normal\' {f"\'nested\' {"other"} 'single quotes'"} normal" # Q100
4 changes: 4 additions & 0 deletions crates/ruff_linter/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ pub(crate) fn check_tokens(
flake8_quotes::rules::avoidable_escaped_quote(&mut diagnostics, tokens, locator, settings);
}

if settings.rules.enabled(Rule::UnnecessaryEscapedQuote) {
flake8_quotes::rules::unnecessary_escaped_quote(&mut diagnostics, tokens, locator);
}

if settings.rules.any_enabled(&[
Rule::BadQuotesInlineString,
Rule::BadQuotesMultilineString,
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Flake8Quotes, "001") => (RuleGroup::Stable, rules::flake8_quotes::rules::BadQuotesMultilineString),
(Flake8Quotes, "002") => (RuleGroup::Stable, rules::flake8_quotes::rules::BadQuotesDocstring),
(Flake8Quotes, "003") => (RuleGroup::Stable, rules::flake8_quotes::rules::AvoidableEscapedQuote),
(Flake8Quotes, "100") => (RuleGroup::Stable, rules::flake8_quotes::rules::UnnecessaryEscapedQuote),

// flake8-annotations
(Flake8Annotations, "001") => (RuleGroup::Stable, rules::flake8_annotations::rules::MissingTypeFunctionArgument),
Expand Down
6 changes: 6 additions & 0 deletions crates/ruff_linter/src/rules/flake8_quotes/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ mod tests {

#[test_case(Path::new("doubles.py"))]
#[test_case(Path::new("doubles_escaped.py"))]
#[test_case(Path::new("doubles_escaped_unnecessary.py"))]
#[test_case(Path::new("doubles_implicit.py"))]
#[test_case(Path::new("doubles_multiline_string.py"))]
#[test_case(Path::new("doubles_noqa.py"))]
Expand All @@ -39,6 +40,7 @@ mod tests {
Rule::BadQuotesMultilineString,
Rule::BadQuotesDocstring,
Rule::AvoidableEscapedQuote,
Rule::UnnecessaryEscapedQuote,
])
},
)?;
Expand Down Expand Up @@ -86,6 +88,7 @@ mod tests {

#[test_case(Path::new("singles.py"))]
#[test_case(Path::new("singles_escaped.py"))]
#[test_case(Path::new("singles_escaped_unnecessary.py"))]
#[test_case(Path::new("singles_implicit.py"))]
#[test_case(Path::new("singles_multiline_string.py"))]
#[test_case(Path::new("singles_noqa.py"))]
Expand All @@ -106,6 +109,7 @@ mod tests {
Rule::BadQuotesMultilineString,
Rule::BadQuotesDocstring,
Rule::AvoidableEscapedQuote,
Rule::UnnecessaryEscapedQuote,
])
},
)?;
Expand Down Expand Up @@ -139,6 +143,7 @@ mod tests {
Rule::BadQuotesMultilineString,
Rule::BadQuotesDocstring,
Rule::AvoidableEscapedQuote,
Rule::UnnecessaryEscapedQuote,
])
},
)?;
Expand Down Expand Up @@ -172,6 +177,7 @@ mod tests {
Rule::BadQuotesMultilineString,
Rule::BadQuotesDocstring,
Rule::AvoidableEscapedQuote,
Rule::UnnecessaryEscapedQuote,
])
},
)?;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use super::super::settings::Quote;
use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::str::{is_triple_quote, leading_quote};
Expand Down Expand Up @@ -48,21 +49,65 @@ impl AlwaysFixableViolation for AvoidableEscapedQuote {
}
}

/// ## What it does
/// Checks for strings that include escaped quotes not matching the outer
/// quotes, and suggests removing the unnecessary backslash.
///
/// ## Why is this bad?
/// It's escaping something that does not need escaping.
///
/// ## Example
/// ```python
/// foo = "bar\'s"
/// ```
///
/// Use instead:
/// ```python
/// foo = "bar's"
/// ```
///
/// ## Formatter compatibility
/// We recommend against using this rule alongside the [formatter]. The
/// formatter automatically removes unnecessary escapes, making the rule
/// redundant.
///
/// [formatter]: https://docs.astral.sh/ruff/formatter
#[violation]
pub struct UnnecessaryEscapedQuote;

impl AlwaysFixableViolation for UnnecessaryEscapedQuote {
#[derive_message_formats]
fn message(&self) -> String {
format!("Do not escape quotes that do not need escaping")
}

fn fix_title(&self) -> String {
"Remove backslash from quotes that do not need escaping".to_string()
}
}

struct FStringContext {
/// Whether to check for escaped quotes in the f-string.
check_for_escaped_quote: bool,
/// The range of the f-string start token.
start_range: TextRange,
/// The ranges of the f-string middle tokens containing escaped quotes.
middle_ranges_with_escapes: Vec<TextRange>,
/// The quote style used for the f-string
quote_style: Quote,
}

impl FStringContext {
fn new(check_for_escaped_quote: bool, fstring_start_range: TextRange) -> Self {
fn new(
check_for_escaped_quote: bool,
fstring_start_range: TextRange,
quote_style: Quote,
) -> Self {
Self {
check_for_escaped_quote,
start_range: fstring_start_range,
middle_ranges_with_escapes: vec![],
quote_style,
}
}

Expand Down Expand Up @@ -146,7 +191,10 @@ pub(crate) fn avoidable_escaped_quote(
"{prefix}{quote}{value}{quote}",
prefix = kind.as_str(),
quote = quotes_settings.inline_quotes.opposite().as_char(),
value = unescape_string(string_contents)
value = unescape_string(
string_contents,
quotes_settings.inline_quotes.as_char()
)
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
fixed_contents,
Expand All @@ -162,7 +210,11 @@ pub(crate) fn avoidable_escaped_quote(
let check_for_escaped_quote = text
.contains(quotes_settings.inline_quotes.as_char())
&& !is_triple_quote(text);
fstrings.push(FStringContext::new(check_for_escaped_quote, tok_range));
fstrings.push(FStringContext::new(
check_for_escaped_quote,
tok_range,
quotes_settings.inline_quotes,
));
}
Tok::FStringMiddle {
value: string_contents,
Expand Down Expand Up @@ -207,7 +259,13 @@ pub(crate) fn avoidable_escaped_quote(
.middle_ranges_with_escapes
.iter()
.map(|&range| {
Edit::range_replacement(unescape_string(locator.slice(range)), range)
Edit::range_replacement(
unescape_string(
locator.slice(range),
quotes_settings.inline_quotes.as_char(),
),
range,
)
})
.chain(std::iter::once(
// `FStringEnd` edit
Expand All @@ -231,7 +289,118 @@ pub(crate) fn avoidable_escaped_quote(
}
}

fn unescape_string(value: &str) -> String {
/// Q100
pub(crate) fn unnecessary_escaped_quote(
diagnostics: &mut Vec<Diagnostic>,
lxr: &[LexResult],
locator: &Locator,
) {
let mut fstrings: Vec<FStringContext> = Vec::new();
let mut state_machine = StateMachine::default();

for &(ref tok, tok_range) in lxr.iter().flatten() {
let is_docstring = state_machine.consume(tok);
if is_docstring {
continue;
}

match tok {
Tok::String {
value: string_contents,
kind,
triple_quoted,
} => {
if kind.is_raw() || *triple_quoted {
continue;
}

let leading = match leading_quote(locator.slice(tok_range)) {
Some("\"") => Quote::Double,
Some("'") => Quote::Single,
_ => continue,
};
if !string_contents.contains(leading.opposite().as_escaped()) {
continue;
}

let mut diagnostic = Diagnostic::new(UnnecessaryEscapedQuote, tok_range);
let fixed_contents = format!(
"{prefix}{quote}{value}{quote}",
prefix = kind.as_str(),
quote = leading.as_char(),
value = unescape_string(string_contents, leading.opposite().as_char())
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
fixed_contents,
tok_range,
)));
diagnostics.push(diagnostic);
}
Tok::FStringStart => {
let text = locator.slice(tok_range);
// Check for escaped quote only if we're using the preferred quotation
// style and it isn't a triple-quoted f-string.
let check_for_escaped_quote = !is_triple_quote(text);
let quote_style = if text.contains(Quote::Single.as_char()) {
Quote::Single
} else {
Quote::Double
};
fstrings.push(FStringContext::new(
check_for_escaped_quote,
tok_range,
quote_style,
));
}
Tok::FStringMiddle {
value: string_contents,
is_raw,
} if !is_raw => {
let Some(context) = fstrings.last_mut() else {
continue;
};
if !context.check_for_escaped_quote {
continue;
}
if string_contents.contains(context.quote_style.opposite().as_escaped()) {
context.push_fstring_middle_range(tok_range);
}
}
Tok::FStringEnd => {
let Some(context) = fstrings.pop() else {
continue;
};
if context.middle_ranges_with_escapes.is_empty() {
// There are no `FStringMiddle` tokens containing any escaped
// quotes.
continue;
}
let mut diagnostic = Diagnostic::new(
UnnecessaryEscapedQuote,
TextRange::new(context.start_range.start(), tok_range.end()),
);
let mut fstring_middle_edits =
context.middle_ranges_with_escapes.iter().map(|&range| {
Edit::range_replacement(
unescape_string(
locator.slice(range),
context.quote_style.opposite().as_char(),
),
range,
)
});
diagnostic.set_fix(Fix::safe_edits(
fstring_middle_edits.next().unwrap(),
fstring_middle_edits,
));
diagnostics.push(diagnostic);
}
_ => {}
}
}
}

fn unescape_string(value: &str, remove_quote: char) -> String {
let mut fixed_contents = String::with_capacity(value.len());

let mut chars = value.chars().peekable();
Expand All @@ -246,7 +415,7 @@ fn unescape_string(value: &str) -> String {
continue;
};
// Remove quote escape
if matches!(*next_char, '\'' | '"') {
if *next_char == remove_quote {
continue;
}
fixed_contents.push(char);
Expand Down
Loading

0 comments on commit 7f0bf50

Please sign in to comment.