-
Notifications
You must be signed in to change notification settings - Fork 880
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pyupgrade: Format specifiers #1594
Changes from all commits
672c984
c127480
5e686c4
e599734
c68a9da
b8c06e7
6f3c4e5
37d25dd
287f90f
cb836f9
7645bc5
dfee10f
aa714a8
ec15215
257c828
782fce3
db92e9b
5adbe05
1fd88cd
fc6af20
7920747
aa0c2cb
29241ef
ef493b2
0d395a3
76fbc56
5121008
c237e4e
75ca3ba
4e84dd6
14a01ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
# Invalid calls; errors expected. | ||
|
||
"{0}" "{1}" "{2}".format(1, 2, 3) | ||
|
||
"a {3} complicated {1} string with {0} {2}".format( | ||
"first", "second", "third", "fourth" | ||
) | ||
|
||
'{0}'.format(1) | ||
|
||
'{0:x}'.format(30) | ||
|
||
x = '{0}'.format(1) | ||
|
||
'''{0}\n{1}\n'''.format(1, 2) | ||
|
||
x = "foo {0}" \ | ||
"bar {1}".format(1, 2) | ||
|
||
("{0}").format(1) | ||
|
||
"\N{snowman} {0}".format(1) | ||
|
||
'{' '0}'.format(1) | ||
|
||
# These will not change because we are waiting for libcst to fix this issue: | ||
# https://github.com/Instagram/LibCST/issues/846 | ||
print( | ||
'foo{0}' | ||
'bar{1}'.format(1, 2) | ||
) | ||
|
||
print( | ||
'foo{0}' # ohai\n" | ||
'bar{1}'.format(1, 2) | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# Valid calls; no errors expected. | ||
|
||
'{}'.format(1) | ||
|
||
|
||
x = ('{0} {1}',) | ||
|
||
'{0} {0}'.format(1) | ||
|
||
'{0:<{1}}'.format(1, 4) | ||
|
||
f"{0}".format(a) | ||
|
||
f"{0}".format(1) | ||
|
||
print(f"{0}".format(1)) | ||
|
||
# I did not include the following tests because ruff does not seem to work with | ||
# invalid python syntax (which is a good thing) | ||
|
||
# "{0}"format(1) | ||
# '{'.format(1)", "'}'.format(1) | ||
# ("{0}" # {1}\n"{2}").format(1, 2, 3) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1606,6 +1606,8 @@ | |
"UP027", | ||
"UP028", | ||
"UP029", | ||
"UP03", | ||
"UP030", | ||
"W", | ||
"W2", | ||
"W29", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,118 @@ | ||
use anyhow::{anyhow, bail, Result}; | ||
use libcst_native::{Arg, Codegen, CodegenState, Expression}; | ||
use once_cell::sync::Lazy; | ||
use regex::Regex; | ||
use rustpython_ast::Expr; | ||
|
||
use crate::ast::types::Range; | ||
use crate::autofix::Fix; | ||
use crate::checkers::ast::Checker; | ||
use crate::cst::matchers::{match_call, match_expression}; | ||
use crate::pyflakes::format::FormatSummary; | ||
use crate::registry::Diagnostic; | ||
use crate::violations; | ||
|
||
// An opening curly brace, followed by any integer, followed by any text, | ||
// followed by a closing brace. | ||
static FORMAT_SPECIFIER: Lazy<Regex> = | ||
Lazy::new(|| Regex::new(r"\{(?P<int>\d+)(?P<fmt>.*?)}").unwrap()); | ||
|
||
/// Returns a string without the format specifiers. | ||
/// Ex. "Hello {0} {1}" -> "Hello {} {}" | ||
fn remove_specifiers(raw_specifiers: &str) -> String { | ||
FORMAT_SPECIFIER | ||
.replace_all(raw_specifiers, "{$fmt}") | ||
.to_string() | ||
} | ||
|
||
/// Return the corrected argument vector. | ||
fn generate_arguments<'a>( | ||
old_args: &[Arg<'a>], | ||
correct_order: &'a [usize], | ||
) -> Result<Vec<Arg<'a>>> { | ||
let mut new_args: Vec<Arg> = Vec::with_capacity(old_args.len()); | ||
for (idx, given) in correct_order.iter().enumerate() { | ||
// We need to keep the formatting in the same order but move the values. | ||
let values = old_args | ||
.get(*given) | ||
.ok_or_else(|| anyhow!("Failed to extract argument at: {given}"))?; | ||
let formatting = old_args | ||
.get(idx) | ||
.ok_or_else(|| anyhow!("Failed to extract argument at: {idx}"))?; | ||
let new_arg = Arg { | ||
value: values.value.clone(), | ||
comma: formatting.comma.clone(), | ||
equal: None, | ||
keyword: None, | ||
star: values.star, | ||
whitespace_after_star: formatting.whitespace_after_star.clone(), | ||
whitespace_after_arg: formatting.whitespace_after_arg.clone(), | ||
}; | ||
new_args.push(new_arg); | ||
} | ||
Ok(new_args) | ||
} | ||
|
||
/// Returns the corrected function call. | ||
fn generate_call(module_text: &str, correct_order: &[usize]) -> Result<String> { | ||
let mut expression = match_expression(module_text)?; | ||
let mut call = match_call(&mut expression)?; | ||
|
||
// Fix the call arguments. | ||
call.args = generate_arguments(&call.args, correct_order)?; | ||
|
||
// Fix the string itself. | ||
let Expression::Attribute(item) = &*call.func else { | ||
panic!("Expected: Expression::Attribute") | ||
}; | ||
|
||
let mut state = CodegenState::default(); | ||
item.codegen(&mut state); | ||
let cleaned = remove_specifiers(&state.to_string()); | ||
|
||
call.func = Box::new(match_expression(&cleaned)?); | ||
|
||
let mut state = CodegenState::default(); | ||
expression.codegen(&mut state); | ||
if module_text == state.to_string() { | ||
// Ex) `'{' '0}'.format(1)` | ||
bail!("Failed to generate call expression for: {module_text}") | ||
} | ||
Ok(state.to_string()) | ||
} | ||
|
||
/// UP030 | ||
pub(crate) fn format_literals(checker: &mut Checker, summary: &FormatSummary, expr: &Expr) { | ||
// The format we expect is, e.g.: `"{0} {1}".format(...)` | ||
if summary.has_nested_parts { | ||
return; | ||
} | ||
if !summary.keywords.is_empty() { | ||
return; | ||
} | ||
if !summary.autos.is_empty() { | ||
return; | ||
} | ||
if !(0..summary.indexes.len()).all(|index| summary.indexes.contains(&index)) { | ||
return; | ||
} | ||
|
||
let mut diagnostic = Diagnostic::new(violations::FormatLiterals, Range::from_located(expr)); | ||
if checker.patch(diagnostic.kind.code()) { | ||
// Currently, the only issue we know of is in LibCST: | ||
// https://github.com/Instagram/LibCST/issues/846 | ||
if let Ok(contents) = generate_call( | ||
&checker | ||
.locator | ||
.slice_source_code_range(&Range::from_located(expr)), | ||
&summary.indexes, | ||
) { | ||
diagnostic.amend(Fix::replacement( | ||
contents, | ||
expr.location, | ||
expr.end_location.unwrap(), | ||
)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like one strategy that could work here would be...
Kinda tough, hacky, etc... but would solve the missing cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I guess that wouldn't solve the |
||
}; | ||
} | ||
checker.diagnostics.push(diagnostic); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colin99d - It turns out that we already had a utility for extracting the format positions, which uses the RustPython string parser underneath and so is very robust. Sorry that I didn't flag this earlier -- I didn't realize it could even be reused here, but it should make things more efficient too since we can do one string parse and share that "summary" amongst a bunch of checks.