Skip to content
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

Update ISC001, ISC002 to check in f-strings #7515

Merged
merged 1 commit into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,23 @@
_ = foo + bar + "abc"
_ = "abc" + foo + bar
_ = foo + "abc" + bar

# Multiple strings nested inside a f-string
_ = f"a {'b' 'c' 'd'} e"
_ = f"""abc {"def" "ghi"} jkl"""
_ = f"""abc {
"def"
"ghi"
} jkl"""
Comment on lines +64 to +69
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't detect this in earlier version but with the granularity of the tokens, we can detect this as well.


# Nested f-strings
_ = "a" f"b {f"c" f"d"} e" "f"
_ = f"b {f"c" f"d {f"e" f"f"} g"} h"
_ = f"b {f"abc" \
f"def"} g"

# Explicitly concatenated nested f-strings
_ = f"a {f"first"
+ f"second"} d"
_ = f"a {f"first {f"middle"}"
+ f"second"} d"
1 change: 1 addition & 0 deletions crates/ruff/src/checkers/tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ pub(crate) fn check_tokens(
tokens,
&settings.flake8_implicit_str_concat,
locator,
indexer,
);
}

Expand Down
54 changes: 36 additions & 18 deletions crates/ruff/src/rules/flake8_implicit_str_concat/rules/implicit.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use itertools::Itertools;
use ruff_python_parser::lexer::LexResult;
use ruff_python_parser::Tok;
use ruff_text_size::{Ranged, TextRange};

use ruff_diagnostics::{AutofixKind, Diagnostic, Edit, Fix, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::str::{leading_quote, trailing_quote};
use ruff_python_index::Indexer;
use ruff_source_file::Locator;

use crate::rules::flake8_implicit_str_concat::settings::Settings;
Expand Down Expand Up @@ -94,6 +96,7 @@ pub(crate) fn implicit(
tokens: &[LexResult],
settings: &Settings,
locator: &Locator,
indexer: &Indexer,
) {
for ((a_tok, a_range), (b_tok, b_range)) in tokens
.iter()
Expand All @@ -103,24 +106,39 @@ pub(crate) fn implicit(
})
.tuple_windows()
{
if a_tok.is_string() && b_tok.is_string() {
if locator.contains_line_break(TextRange::new(a_range.end(), b_range.start())) {
diagnostics.push(Diagnostic::new(
MultiLineImplicitStringConcatenation,
TextRange::new(a_range.start(), b_range.end()),
));
} else {
let mut diagnostic = Diagnostic::new(
SingleLineImplicitStringConcatenation,
TextRange::new(a_range.start(), b_range.end()),
);

if let Some(fix) = concatenate_strings(*a_range, *b_range, locator) {
diagnostic.set_fix(fix);
}

diagnostics.push(diagnostic);
};
let (a_range, b_range) = match (a_tok, b_tok) {
(Tok::String { .. }, Tok::String { .. }) => (*a_range, *b_range),
(Tok::String { .. }, Tok::FStringStart) => (
*a_range,
indexer.fstring_ranges().innermost(b_range.start()).unwrap(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the use of fstring_ranges here a bit unfortunate because:

  • Looking up the range is at least: O(log(n))
  • So many unwraps

But I guess it's fine, considering that implicit concatenated strings are somewhat rare

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I agree. This could be solved with the introduction of a new StringList node though as the AST would contain the range, so no lookups and no unwraps 😉

),
(Tok::FStringEnd, Tok::String { .. }) => (
indexer.fstring_ranges().innermost(a_range.start()).unwrap(),
*b_range,
),
(Tok::FStringEnd, Tok::FStringStart) => (
indexer.fstring_ranges().innermost(a_range.start()).unwrap(),
indexer.fstring_ranges().innermost(b_range.start()).unwrap(),
),
_ => continue,
};

if locator.contains_line_break(TextRange::new(a_range.end(), b_range.start())) {
diagnostics.push(Diagnostic::new(
MultiLineImplicitStringConcatenation,
TextRange::new(a_range.start(), b_range.end()),
));
} else {
let mut diagnostic = Diagnostic::new(
SingleLineImplicitStringConcatenation,
TextRange::new(a_range.start(), b_range.end()),
);

if let Some(fix) = concatenate_strings(a_range, b_range, locator) {
diagnostic.set_fix(fix);
}

diagnostics.push(diagnostic);
};
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,147 @@ ISC.py:52:5: ISC001 [*] Implicitly concatenated string literals on one line
54 54 | # Single-line explicit concatenation should be ignored.
55 55 | _ = "abc" + "def" + "ghi"

ISC.py:64:10: ISC001 [*] Implicitly concatenated string literals on one line
|
63 | # Multiple strings nested inside a f-string
64 | _ = f"a {'b' 'c' 'd'} e"
| ^^^^^^^ ISC001
65 | _ = f"""abc {"def" "ghi"} jkl"""
66 | _ = f"""abc {
|
= help: Combine string literals

ℹ Fix
61 61 | _ = foo + "abc" + bar
62 62 |
63 63 | # Multiple strings nested inside a f-string
64 |-_ = f"a {'b' 'c' 'd'} e"
64 |+_ = f"a {'bc' 'd'} e"
65 65 | _ = f"""abc {"def" "ghi"} jkl"""
66 66 | _ = f"""abc {
67 67 | "def"

ISC.py:64:14: ISC001 [*] Implicitly concatenated string literals on one line
|
63 | # Multiple strings nested inside a f-string
64 | _ = f"a {'b' 'c' 'd'} e"
| ^^^^^^^ ISC001
65 | _ = f"""abc {"def" "ghi"} jkl"""
66 | _ = f"""abc {
|
= help: Combine string literals

ℹ Fix
61 61 | _ = foo + "abc" + bar
62 62 |
63 63 | # Multiple strings nested inside a f-string
64 |-_ = f"a {'b' 'c' 'd'} e"
64 |+_ = f"a {'b' 'cd'} e"
65 65 | _ = f"""abc {"def" "ghi"} jkl"""
66 66 | _ = f"""abc {
67 67 | "def"

ISC.py:65:14: ISC001 [*] Implicitly concatenated string literals on one line
|
63 | # Multiple strings nested inside a f-string
64 | _ = f"a {'b' 'c' 'd'} e"
65 | _ = f"""abc {"def" "ghi"} jkl"""
| ^^^^^^^^^^^ ISC001
66 | _ = f"""abc {
67 | "def"
|
= help: Combine string literals

ℹ Fix
62 62 |
63 63 | # Multiple strings nested inside a f-string
64 64 | _ = f"a {'b' 'c' 'd'} e"
65 |-_ = f"""abc {"def" "ghi"} jkl"""
65 |+_ = f"""abc {"defghi"} jkl"""
66 66 | _ = f"""abc {
67 67 | "def"
68 68 | "ghi"

ISC.py:72:5: ISC001 Implicitly concatenated string literals on one line
|
71 | # Nested f-strings
72 | _ = "a" f"b {f"c" f"d"} e" "f"
| ^^^^^^^^^^^^^^^^^^^^^^ ISC001
73 | _ = f"b {f"c" f"d {f"e" f"f"} g"} h"
74 | _ = f"b {f"abc" \
|
= help: Combine string literals

ISC.py:72:9: ISC001 Implicitly concatenated string literals on one line
|
71 | # Nested f-strings
72 | _ = "a" f"b {f"c" f"d"} e" "f"
| ^^^^^^^^^^^^^^^^^^^^^^ ISC001
73 | _ = f"b {f"c" f"d {f"e" f"f"} g"} h"
74 | _ = f"b {f"abc" \
|
= help: Combine string literals

ISC.py:72:14: ISC001 [*] Implicitly concatenated string literals on one line
|
71 | # Nested f-strings
72 | _ = "a" f"b {f"c" f"d"} e" "f"
| ^^^^^^^^^ ISC001
73 | _ = f"b {f"c" f"d {f"e" f"f"} g"} h"
74 | _ = f"b {f"abc" \
|
= help: Combine string literals

ℹ Fix
69 69 | } jkl"""
70 70 |
71 71 | # Nested f-strings
72 |-_ = "a" f"b {f"c" f"d"} e" "f"
72 |+_ = "a" f"b {f"cd"} e" "f"
73 73 | _ = f"b {f"c" f"d {f"e" f"f"} g"} h"
74 74 | _ = f"b {f"abc" \
75 75 | f"def"} g"

ISC.py:73:10: ISC001 [*] Implicitly concatenated string literals on one line
|
71 | # Nested f-strings
72 | _ = "a" f"b {f"c" f"d"} e" "f"
73 | _ = f"b {f"c" f"d {f"e" f"f"} g"} h"
| ^^^^^^^^^^^^^^^^^^^^^^^ ISC001
74 | _ = f"b {f"abc" \
75 | f"def"} g"
|
= help: Combine string literals

ℹ Fix
70 70 |
71 71 | # Nested f-strings
72 72 | _ = "a" f"b {f"c" f"d"} e" "f"
73 |-_ = f"b {f"c" f"d {f"e" f"f"} g"} h"
73 |+_ = f"b {f"cd {f"e" f"f"} g"} h"
74 74 | _ = f"b {f"abc" \
75 75 | f"def"} g"
76 76 |

ISC.py:73:20: ISC001 [*] Implicitly concatenated string literals on one line
|
71 | # Nested f-strings
72 | _ = "a" f"b {f"c" f"d"} e" "f"
73 | _ = f"b {f"c" f"d {f"e" f"f"} g"} h"
| ^^^^^^^^^ ISC001
74 | _ = f"b {f"abc" \
75 | f"def"} g"
|
= help: Combine string literals

ℹ Fix
70 70 |
71 71 | # Nested f-strings
72 72 | _ = "a" f"b {f"c" f"d"} e" "f"
73 |-_ = f"b {f"c" f"d {f"e" f"f"} g"} h"
73 |+_ = f"b {f"c" f"d {f"ef"} g"} h"
74 74 | _ = f"b {f"abc" \
75 75 | f"def"} g"
76 76 |


Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,16 @@ ISC.py:5:5: ISC002 Implicitly concatenated string literals over multiple lines
8 | _ = (
|

ISC.py:74:10: ISC002 Implicitly concatenated string literals over multiple lines
|
72 | _ = "a" f"b {f"c" f"d"} e" "f"
73 | _ = f"b {f"c" f"d {f"e" f"f"} g"} h"
74 | _ = f"b {f"abc" \
| __________^
75 | | f"def"} g"
| |__________^ ISC002
76 |
77 | # Explicitly concatenated nested f-strings
|


Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,25 @@ ISC.py:19:3: ISC003 Explicitly concatenated string should be implicitly concaten
21 | )
|

ISC.py:78:10: ISC003 Explicitly concatenated string should be implicitly concatenated
|
77 | # Explicitly concatenated nested f-strings
78 | _ = f"a {f"first"
| __________^
79 | | + f"second"} d"
| |_______________^ ISC003
80 | _ = f"a {f"first {f"middle"}"
81 | + f"second"} d"
|

ISC.py:80:10: ISC003 Explicitly concatenated string should be implicitly concatenated
|
78 | _ = f"a {f"first"
79 | + f"second"} d"
80 | _ = f"a {f"first {f"middle"}"
| __________^
81 | | + f"second"} d"
| |_______________^ ISC003
|