Skip to content

Commit 8241052

Browse files
[pylint] Implement Pylint bad-format-character (E1300) (#6171)
## Summary Relates to #970. Add new `bad-format-character` Pylint rule. I had to make a change in `crates/ruff_python_literal/src/format.rs` to get a more detailed error in case the format character is not correct. I chose to do this since most of the format spec parsing functions are private. It would have required me reimplementing most of the parsing logic just to know if the format char was correct. This PR also doesn't reflect current Pylint functionality in two ways. It supports new format strings correctly, Pylint as of now doesn't. See pylint-dev/pylint#6085. In case there are multiple adjacent string literals delimited by whitespace the index of the wrong format char will relative to the single string. Pylint will instead reported it relative to the concatenated string. Given this: ``` "%s" "%z" % ("hello", "world") ``` Ruff will report this: ```Unsupported format character 'z' (0x7a) at index 1``` Pylint instead: ```Unsupported format character 'z' (0x7a) at index 3``` I believe it's more sensible to report the index relative to the individual string. ## Test Plan Added new snapshot and a small test in `crates/ruff_python_literal/src/format.rs`. --------- Co-authored-by: Charlie Marsh <[email protected]>
1 parent 5b2e973 commit 8241052

File tree

9 files changed

+221
-5
lines changed

9 files changed

+221
-5
lines changed
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
# pylint: disable=missing-docstring,consider-using-f-string, pointless-statement
2+
3+
## Old style formatting
4+
5+
"%s %z" % ("hello", "world") # [bad-format-character]
6+
7+
"%s" "%z" % ("hello", "world") # [bad-format-character]
8+
9+
"""%s %z""" % ("hello", "world") # [bad-format-character]
10+
11+
"""%s""" """%z""" % ("hello", "world") # [bad-format-character]
12+
13+
## New style formatting
14+
15+
"{:s} {:y}".format("hello", "world") # [bad-format-character]
16+
17+
"{:*^30s}".format("centered")
18+
19+
## f-strings
20+
21+
H, W = "hello", "world"
22+
f"{H} {W}"
23+
f"{H:s} {W:z}" # [bad-format-character]
24+
25+
f"{1:z}" # [bad-format-character]
26+
27+
## False negatives
28+
29+
print(("%" "z") % 1)

crates/ruff/src/checkers/ast/analyze/expression.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,14 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
419419
}
420420
}
421421
}
422+
423+
if checker.enabled(Rule::BadStringFormatCharacter) {
424+
pylint::rules::bad_string_format_character::call(
425+
checker,
426+
val.as_str(),
427+
location,
428+
);
429+
}
422430
}
423431
}
424432
}
@@ -1025,6 +1033,9 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
10251033
checker.locator,
10261034
);
10271035
}
1036+
if checker.enabled(Rule::BadStringFormatCharacter) {
1037+
pylint::rules::bad_string_format_character::percent(checker, expr);
1038+
}
10281039
if checker.enabled(Rule::BadStringFormatType) {
10291040
pylint::rules::bad_string_format_type(checker, expr, right);
10301041
}

crates/ruff/src/codes.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
191191
(Pylint, "E1142") => (RuleGroup::Unspecified, rules::pylint::rules::AwaitOutsideAsync),
192192
(Pylint, "E1205") => (RuleGroup::Unspecified, rules::pylint::rules::LoggingTooManyArgs),
193193
(Pylint, "E1206") => (RuleGroup::Unspecified, rules::pylint::rules::LoggingTooFewArgs),
194+
(Pylint, "E1300") => (RuleGroup::Unspecified, rules::pylint::rules::BadStringFormatCharacter),
194195
(Pylint, "E1307") => (RuleGroup::Unspecified, rules::pylint::rules::BadStringFormatType),
195196
(Pylint, "E1310") => (RuleGroup::Unspecified, rules::pylint::rules::BadStrStripCall),
196197
(Pylint, "E1507") => (RuleGroup::Unspecified, rules::pylint::rules::InvalidEnvvarValue),

crates/ruff/src/rules/pylint/mod.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,12 @@ mod tests {
1818
use crate::settings::Settings;
1919
use crate::test::test_path;
2020

21-
#[test_case(Rule::AwaitOutsideAsync, Path::new("await_outside_async.py"))]
2221
#[test_case(Rule::AssertOnStringLiteral, Path::new("assert_on_string_literal.py"))]
22+
#[test_case(Rule::AwaitOutsideAsync, Path::new("await_outside_async.py"))]
23+
#[test_case(
24+
Rule::BadStringFormatCharacter,
25+
Path::new("bad_string_format_character.py")
26+
)]
2327
#[test_case(Rule::BadStrStripCall, Path::new("bad_str_strip_call.py"))]
2428
#[test_case(Rule::BadStringFormatType, Path::new("bad_string_format_type.py"))]
2529
#[test_case(Rule::BidirectionalUnicode, Path::new("bidirectional_unicode.py"))]
Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
use std::str::FromStr;
2+
3+
use ruff_diagnostics::{Diagnostic, Violation};
4+
use ruff_macros::{derive_message_formats, violation};
5+
use ruff_python_ast::str::{leading_quote, trailing_quote};
6+
use ruff_python_ast::{Expr, Ranged};
7+
use ruff_python_literal::{
8+
cformat::{CFormatErrorType, CFormatString},
9+
format::FormatPart,
10+
format::FromTemplate,
11+
format::{FormatSpec, FormatSpecError, FormatString},
12+
};
13+
use ruff_python_parser::{lexer, Mode};
14+
use ruff_text_size::TextRange;
15+
16+
use crate::checkers::ast::Checker;
17+
18+
/// ## What it does
19+
/// Checks for unsupported format types in format strings.
20+
///
21+
/// ## Why is this bad?
22+
/// The format string is not checked at compile time, so it is easy to
23+
/// introduce bugs by mistyping the format string.
24+
///
25+
/// ## Example
26+
/// ```python
27+
/// # `z` is not a valid format type.
28+
/// print("%z" % "1")
29+
///
30+
/// print("{:z}".format("1"))
31+
/// ```
32+
#[violation]
33+
pub struct BadStringFormatCharacter {
34+
format_char: char,
35+
}
36+
37+
impl Violation for BadStringFormatCharacter {
38+
#[derive_message_formats]
39+
fn message(&self) -> String {
40+
format!("Unsupported format character '{}'", self.format_char)
41+
}
42+
}
43+
44+
/// Ex) `"{:z}".format("1")`
45+
pub(crate) fn call(checker: &mut Checker, string: &str, range: TextRange) {
46+
if let Ok(format_string) = FormatString::from_str(string) {
47+
for part in &format_string.format_parts {
48+
let FormatPart::Field { format_spec, .. } = part else {
49+
continue;
50+
};
51+
52+
if let Err(FormatSpecError::InvalidFormatType) = FormatSpec::parse(format_spec) {
53+
checker.diagnostics.push(Diagnostic::new(
54+
BadStringFormatCharacter {
55+
// The format type character is always the last one.
56+
// More info in the official spec:
57+
// https://docs.python.org/3/library/string.html#format-specification-mini-language
58+
format_char: format_spec.chars().last().unwrap(),
59+
},
60+
range,
61+
));
62+
}
63+
}
64+
}
65+
}
66+
67+
/// Ex) `"%z" % "1"`
68+
pub(crate) fn percent(checker: &mut Checker, expr: &Expr) {
69+
// Grab each string segment (in case there's an implicit concatenation).
70+
let mut strings: Vec<TextRange> = vec![];
71+
for (tok, range) in lexer::lex_starts_at(
72+
checker.locator().slice(expr.range()),
73+
Mode::Module,
74+
expr.start(),
75+
)
76+
.flatten()
77+
{
78+
if tok.is_string() {
79+
strings.push(range);
80+
} else if tok.is_percent() {
81+
// Break as soon as we find the modulo symbol.
82+
break;
83+
}
84+
}
85+
86+
// If there are no string segments, abort.
87+
if strings.is_empty() {
88+
return;
89+
}
90+
91+
for range in &strings {
92+
let string = checker.locator().slice(*range);
93+
let (Some(leader), Some(trailer)) = (leading_quote(string), trailing_quote(string)) else {
94+
return;
95+
};
96+
let string = &string[leader.len()..string.len() - trailer.len()];
97+
98+
// Parse the format string (e.g. `"%s"`) into a list of `PercentFormat`.
99+
if let Err(format_error) = CFormatString::from_str(string) {
100+
if let CFormatErrorType::UnsupportedFormatChar(format_char) = format_error.typ {
101+
checker.diagnostics.push(Diagnostic::new(
102+
BadStringFormatCharacter { format_char },
103+
expr.range(),
104+
));
105+
}
106+
};
107+
}
108+
}

crates/ruff/src/rules/pylint/rules/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
pub(crate) use assert_on_string_literal::*;
22
pub(crate) use await_outside_async::*;
33
pub(crate) use bad_str_strip_call::*;
4+
pub(crate) use bad_string_format_character::BadStringFormatCharacter;
45
pub(crate) use bad_string_format_type::*;
56
pub(crate) use bidirectional_unicode::*;
67
pub(crate) use binary_op_exception::*;
@@ -55,6 +56,7 @@ pub(crate) use yield_in_init::*;
5556
mod assert_on_string_literal;
5657
mod await_outside_async;
5758
mod bad_str_strip_call;
59+
pub(crate) mod bad_string_format_character;
5860
mod bad_string_format_type;
5961
mod bidirectional_unicode;
6062
mod binary_op_exception;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
source: crates/ruff/src/rules/pylint/mod.rs
3+
---
4+
bad_string_format_character.py:5:1: PLE1300 Unsupported format character 'z'
5+
|
6+
3 | ## Old style formatting
7+
4 |
8+
5 | "%s %z" % ("hello", "world") # [bad-format-character]
9+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLE1300
10+
6 |
11+
7 | "%s" "%z" % ("hello", "world") # [bad-format-character]
12+
|
13+
14+
bad_string_format_character.py:7:1: PLE1300 Unsupported format character 'z'
15+
|
16+
5 | "%s %z" % ("hello", "world") # [bad-format-character]
17+
6 |
18+
7 | "%s" "%z" % ("hello", "world") # [bad-format-character]
19+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLE1300
20+
8 |
21+
9 | """%s %z""" % ("hello", "world") # [bad-format-character]
22+
|
23+
24+
bad_string_format_character.py:9:1: PLE1300 Unsupported format character 'z'
25+
|
26+
7 | "%s" "%z" % ("hello", "world") # [bad-format-character]
27+
8 |
28+
9 | """%s %z""" % ("hello", "world") # [bad-format-character]
29+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLE1300
30+
10 |
31+
11 | """%s""" """%z""" % ("hello", "world") # [bad-format-character]
32+
|
33+
34+
bad_string_format_character.py:11:1: PLE1300 Unsupported format character 'z'
35+
|
36+
9 | """%s %z""" % ("hello", "world") # [bad-format-character]
37+
10 |
38+
11 | """%s""" """%z""" % ("hello", "world") # [bad-format-character]
39+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLE1300
40+
12 |
41+
13 | ## New style formatting
42+
|
43+
44+

crates/ruff_python_literal/src/format.rs

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,7 @@ impl FormatParse for FormatType {
180180
Some('g') => (Some(Self::GeneralFormat(Case::Lower)), chars.as_str()),
181181
Some('G') => (Some(Self::GeneralFormat(Case::Upper)), chars.as_str()),
182182
Some('%') => (Some(Self::Percentage), chars.as_str()),
183+
Some(_) => (None, chars.as_str()),
183184
_ => (None, text),
184185
}
185186
}
@@ -283,10 +284,20 @@ impl FormatSpec {
283284
let (width, text) = parse_number(text)?;
284285
let (grouping_option, text) = FormatGrouping::parse(text);
285286
let (precision, text) = parse_precision(text)?;
286-
let (format_type, text) = FormatType::parse(text);
287-
if !text.is_empty() {
288-
return Err(FormatSpecError::InvalidFormatSpecifier);
289-
}
287+
let (format_type, _text) = if text.is_empty() {
288+
(None, text)
289+
} else {
290+
// If there's any remaining text, we should yield a valid format type and consume it
291+
// all.
292+
let (format_type, text) = FormatType::parse(text);
293+
if format_type.is_none() {
294+
return Err(FormatSpecError::InvalidFormatType);
295+
}
296+
if !text.is_empty() {
297+
return Err(FormatSpecError::InvalidFormatSpecifier);
298+
}
299+
(format_type, text)
300+
};
290301

291302
if zero && fill.is_none() {
292303
fill.replace('0');
@@ -724,6 +735,7 @@ pub enum FormatSpecError {
724735
DecimalDigitsTooMany,
725736
PrecisionTooBig,
726737
InvalidFormatSpecifier,
738+
InvalidFormatType,
727739
UnspecifiedFormat(char, char),
728740
UnknownFormatCode(char, &'static str),
729741
PrecisionNotAllowed,
@@ -1275,6 +1287,10 @@ mod tests {
12751287
FormatSpec::parse("d "),
12761288
Err(FormatSpecError::InvalidFormatSpecifier)
12771289
);
1290+
assert_eq!(
1291+
FormatSpec::parse("z"),
1292+
Err(FormatSpecError::InvalidFormatType)
1293+
);
12781294
}
12791295

12801296
#[test]

ruff.schema.json

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)