Skip to content

Commit

Permalink
Invert quote-style when generating code within f-strings
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed May 18, 2023
1 parent ddd541b commit 867c7bd
Show file tree
Hide file tree
Showing 49 changed files with 281 additions and 163 deletions.
1 change: 1 addition & 0 deletions crates/ruff/resources/test/fixtures/pyupgrade/UP018.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@
bytes(b"foo")
bytes(b"""
foo""")
f"{str()}"
3 changes: 3 additions & 0 deletions crates/ruff/resources/test/fixtures/ruff/RUF005.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,6 @@ def yay(self):

[] + foo + [ # This will be preserved, but doesn't prevent the fix
]

# Uses the non-preferred quote style, which should be retained.
f"{[*a(), 'b']}"
50 changes: 49 additions & 1 deletion crates/ruff/src/checkers/ast/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ use rustpython_parser::ast::{
use ruff_diagnostics::{Diagnostic, Fix};
use ruff_python_ast::all::{extract_all_names, AllNamesFlags};
use ruff_python_ast::helpers::{extract_handled_exceptions, to_module_path};
use ruff_python_ast::source_code::{Indexer, Locator, Stylist};
use ruff_python_ast::source_code::{Generator, Indexer, Locator, Quote, Stylist};
use ruff_python_ast::str::trailing_quote;
use ruff_python_ast::types::{Node, RefEquality};
use ruff_python_ast::typing::{parse_type_annotation, AnnotationKind};
use ruff_python_ast::visitor::{walk_excepthandler, walk_pattern, Visitor};
Expand Down Expand Up @@ -134,6 +135,53 @@ impl<'a> Checker<'a> {
}
noqa::rule_is_ignored(code, offset, self.noqa_line_for, self.locator)
}

/// Create a [`Generator`] to generate source code based on the current AST state.
pub fn generator(&self) -> Generator {
fn quote_style(context: &Context, locator: &Locator, indexer: &Indexer) -> Option<Quote> {
if !context.in_f_string() {
return None;
}

let Some(expr) = context.expr() else {
return None;
};

// Find the f-string containing the current expression.
let start = expr.start();
let string_ranges = indexer.f_string_ranges();
let Ok(string_range_index) = string_ranges.binary_search_by(|range| {
if start < range.start() {
std::cmp::Ordering::Greater
} else if range.contains(start) {
std::cmp::Ordering::Equal
} else {
std::cmp::Ordering::Less
}
}) else {
return None;
};
let string_range = string_ranges[string_range_index];

// Find the quote character used to start the f-string.
let Some(trailing_quote) = trailing_quote(locator.slice(string_range)) else {
return None;
};

// Invert the quote character, if it's a single quote.
match *trailing_quote {
"'" => Some(Quote::Double),
"\"" => Some(Quote::Single),
_ => None,
}
}

Generator::new(
self.stylist.indentation(),
quote_style(&self.ctx, self.locator, self.indexer).unwrap_or(self.stylist.quote()),
self.stylist.line_ending(),
)
}
}

impl<'a, 'b> Visitor<'b> for Checker<'a>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,14 @@ fn unparse_string_format_expression(checker: &mut Checker, expr: &Expr) -> Optio
}) => {
let Some(parent) = checker.ctx.expr_parent() else {
if any_over_expr(expr, &has_string_literal) {
return Some(unparse_expr(expr, checker.stylist));
return Some(unparse_expr(expr, checker.generator()));
}
return None;
};
// Only evaluate the full BinOp, not the nested components.
let Expr::BinOp(_ )= parent else {
if any_over_expr(expr, &has_string_literal) {
return Some(unparse_expr(expr, checker.stylist));
return Some(unparse_expr(expr, checker.generator()));
}
return None;
};
Expand All @@ -81,12 +81,12 @@ fn unparse_string_format_expression(checker: &mut Checker, expr: &Expr) -> Optio
};
// "select * from table where val = {}".format(...)
if attr == "format" && string_literal(value).is_some() {
return Some(unparse_expr(expr, checker.stylist));
return Some(unparse_expr(expr, checker.generator()));
};
None
}
// f"select * from table where val = {val}"
Expr::JoinedStr(_) => Some(unparse_expr(expr, checker.stylist)),
Expr::JoinedStr(_) => Some(unparse_expr(expr, checker.generator())),
_ => None,
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ pub(crate) fn request_without_timeout(
Expr::Constant(ast::ExprConstant {
value: value @ Constant::None,
..
}) => Some(unparse_constant(value, checker.stylist)),
}) => Some(unparse_constant(value, checker.generator())),
_ => None,
} {
checker.diagnostics.push(Diagnostic::new(
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff/src/rules/flake8_bugbear/rules/assert_false.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub(crate) fn assert_false(checker: &mut Checker, stmt: &Stmt, test: &Expr, msg:
if checker.patch(diagnostic.kind.rule()) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_stmt(&assertion_error(msg), checker.stylist),
unparse_stmt(&assertion_error(msg), checker.generator()),
stmt.range(),
)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@ fn duplicate_handler_exceptions<'a>(
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
if unique_elts.len() == 1 {
unparse_expr(unique_elts[0], checker.stylist)
unparse_expr(unique_elts[0], checker.generator())
} else {
unparse_expr(&type_pattern(unique_elts), checker.stylist)
unparse_expr(&type_pattern(unique_elts), checker.generator())
},
expr.range(),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ pub(crate) fn getattr_with_constant(
if checker.patch(diagnostic.kind.rule()) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_expr(&attribute(obj, value), checker.stylist),
unparse_expr(&attribute(obj, value), checker.generator()),
expr.range(),
)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ pub(crate) fn redundant_tuple_in_exception_handler(
};
let mut diagnostic = Diagnostic::new(
RedundantTupleInExceptionHandler {
name: unparse_expr(elt, checker.stylist),
name: unparse_expr(elt, checker.generator()),
},
type_.range(),
);
if checker.patch(diagnostic.kind.rule()) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_expr(elt, checker.stylist),
unparse_expr(elt, checker.generator()),
type_.range(),
)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rustpython_parser::ast::{self, Constant, Expr, ExprContext, Ranged, Stmt};
use ruff_diagnostics::{AlwaysAutofixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::unparse_stmt;
use ruff_python_ast::source_code::Stylist;
use ruff_python_ast::source_code::Generator;
use ruff_python_stdlib::identifiers::{is_identifier, is_mangled_private};

use crate::checkers::ast::Checker;
Expand All @@ -27,7 +27,7 @@ impl AlwaysAutofixableViolation for SetAttrWithConstant {
}
}

fn assignment(obj: &Expr, name: &str, value: &Expr, stylist: &Stylist) -> String {
fn assignment(obj: &Expr, name: &str, value: &Expr, generator: Generator) -> String {
let stmt = Stmt::Assign(ast::StmtAssign {
targets: vec![Expr::Attribute(ast::ExprAttribute {
value: Box::new(obj.clone()),
Expand All @@ -39,7 +39,7 @@ fn assignment(obj: &Expr, name: &str, value: &Expr, stylist: &Stylist) -> String
type_comment: None,
range: TextRange::default(),
});
unparse_stmt(&stmt, stylist)
unparse_stmt(&stmt, generator)
}

/// B010
Expand Down Expand Up @@ -84,7 +84,7 @@ pub(crate) fn setattr_with_constant(
if checker.patch(diagnostic.kind.rule()) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
assignment(obj, name, value, checker.stylist),
assignment(obj, name, value, checker.generator()),
expr.range(),
)));
}
Expand Down
21 changes: 15 additions & 6 deletions crates/ruff/src/rules/flake8_errmsg/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rustpython_parser::ast::{self, Constant, Expr, ExprContext, Ranged, Stmt};
use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::helpers::unparse_stmt;
use ruff_python_ast::source_code::Stylist;
use ruff_python_ast::source_code::{Generator, Stylist};
use ruff_python_ast::whitespace;

use crate::checkers::ast::Checker;
Expand Down Expand Up @@ -183,7 +183,13 @@ impl Violation for DotFormatInException {
/// 1. Insert the exception argument into a variable assignment before the
/// `raise` statement. The variable name is `msg`.
/// 2. Replace the exception argument with the variable name.
fn generate_fix(stylist: &Stylist, stmt: &Stmt, exc_arg: &Expr, indentation: &str) -> Fix {
fn generate_fix(
stmt: &Stmt,
exc_arg: &Expr,
indentation: &str,
stylist: &Stylist,
generator: Generator,
) -> Fix {
let node = Expr::Name(ast::ExprName {
id: "msg".into(),
ctx: ExprContext::Store,
Expand All @@ -195,7 +201,7 @@ fn generate_fix(stylist: &Stylist, stmt: &Stmt, exc_arg: &Expr, indentation: &st
type_comment: None,
range: TextRange::default(),
});
let assignment = unparse_stmt(&node1, stylist);
let assignment = unparse_stmt(&node1, generator);
#[allow(deprecated)]
Fix::unspecified_edits(
Edit::insertion(
Expand Down Expand Up @@ -239,10 +245,11 @@ pub(crate) fn string_in_exception(checker: &mut Checker, stmt: &Stmt, exc: &Expr
if let Some(indentation) = indentation {
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(generate_fix(
checker.stylist,
stmt,
first,
indentation,
checker.stylist,
checker.generator(),
));
}
}
Expand All @@ -266,10 +273,11 @@ pub(crate) fn string_in_exception(checker: &mut Checker, stmt: &Stmt, exc: &Expr
if let Some(indentation) = indentation {
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(generate_fix(
checker.stylist,
stmt,
first,
indentation,
checker.stylist,
checker.generator(),
));
}
}
Expand All @@ -296,10 +304,11 @@ pub(crate) fn string_in_exception(checker: &mut Checker, stmt: &Stmt, exc: &Expr
if let Some(indentation) = indentation {
if checker.patch(diagnostic.kind.rule()) {
diagnostic.set_fix(generate_fix(
checker.stylist,
stmt,
first,
indentation,
checker.stylist,
checker.generator(),
));
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff/src/rules/flake8_pie/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ pub(crate) fn non_unique_enums<'a, 'b>(
if !seen_targets.insert(ComparableExpr::from(value)) {
let diagnostic = Diagnostic::new(
NonUniqueEnums {
value: unparse_expr(value, checker.stylist),
value: unparse_expr(value, checker.generator()),
},
stmt.range(),
);
Expand Down Expand Up @@ -612,7 +612,7 @@ pub(crate) fn multiple_starts_ends_with(checker: &mut Checker, expr: &Expr) {
let bool_op = node;
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_expr(&bool_op, checker.stylist),
unparse_expr(&bool_op, checker.generator()),
expr.range(),
)));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ fn traverse_union<'a>(
if !seen_nodes.insert(expr.into()) {
let mut diagnostic = Diagnostic::new(
DuplicateUnionMember {
duplicate_name: unparse_expr(expr, checker.stylist),
duplicate_name: unparse_expr(expr, checker.generator()),
},
expr.range(),
);
Expand All @@ -82,7 +82,7 @@ fn traverse_union<'a>(
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_expr(
if expr == left.as_ref() { right } else { left },
checker.stylist,
checker.generator(),
),
parent.range(),
)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,7 @@ pub(crate) fn unittest_assertion(
if let Ok(stmt) = unittest_assert.generate_assert(args, keywords) {
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_stmt(&stmt, checker.stylist),
unparse_stmt(&stmt, checker.generator()),
expr.range(),
)));
}
Expand Down
12 changes: 6 additions & 6 deletions crates/ruff/src/rules/flake8_pytest_style/rules/parametrize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ fn elts_to_csv(elts: &[Expr], checker: &Checker) -> Option<String> {
kind: None,
range: TextRange::default(),
});
Some(unparse_expr(&node, checker.stylist))
Some(unparse_expr(&node, checker.generator()))
}

/// Returns the range of the `name` argument of `@pytest.mark.parametrize`.
Expand Down Expand Up @@ -164,7 +164,7 @@ fn check_names(checker: &mut Checker, decorator: &Expr, expr: &Expr) {
});
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
format!("({})", unparse_expr(&node, checker.stylist,)),
format!("({})", unparse_expr(&node, checker.generator())),
name_range,
)));
}
Expand Down Expand Up @@ -195,7 +195,7 @@ fn check_names(checker: &mut Checker, decorator: &Expr, expr: &Expr) {
});
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_expr(&node, checker.stylist),
unparse_expr(&node, checker.generator()),
name_range,
)));
}
Expand Down Expand Up @@ -228,7 +228,7 @@ fn check_names(checker: &mut Checker, decorator: &Expr, expr: &Expr) {
});
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_expr(&node, checker.stylist),
unparse_expr(&node, checker.generator()),
expr.range(),
)));
}
Expand Down Expand Up @@ -278,7 +278,7 @@ fn check_names(checker: &mut Checker, decorator: &Expr, expr: &Expr) {
});
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
format!("({})", unparse_expr(&node, checker.stylist,)),
format!("({})", unparse_expr(&node, checker.generator())),
expr.range(),
)));
}
Expand Down Expand Up @@ -373,7 +373,7 @@ fn handle_single_name(checker: &mut Checker, expr: &Expr, value: &Expr) {
let node = value.clone();
#[allow(deprecated)]
diagnostic.set_fix(Fix::unspecified(Edit::range_replacement(
unparse_expr(&node, checker.stylist),
unparse_expr(&node, checker.generator()),
expr.range(),
)));
}
Expand Down
Loading

0 comments on commit 867c7bd

Please sign in to comment.