Skip to content

Commit

Permalink
Consider unterminated f-strings in FStringRanges (#8154)
Browse files Browse the repository at this point in the history
## Summary

This PR removes the `debug_assertion` in the `Indexer` to allow
unterminated f-strings. This is mainly a fix in the development build
which now matches the release build.

The fix is simple: remove the `debug_assertion` which means that the
there could be `FStringStart` and possibly `FStringMiddle` tokens
without a corresponding f-string range in the `Indexer`. This means that
the code requesting for the f-string index need to account for the
`None` case, making the code safer.

This also updates the code which queries the `FStringRanges` to account
for the `None` case. This will happen when the `FStringStart` /
`FStringMiddle` tokens are present but the `FStringEnd` token isn't
which means that the `Indexer` won't contain the range for that
f-string.

## Test Plan

`cargo test`

Taking the following code as an example:

```python
f"{123}
```

This only emits a `FStringStart` token, but no `FStringMiddle` or
`FStringEnd` tokens.

And,

```python
f"\.png${
```

This emits a `FStringStart` and `FStringMiddle` token, but no
`FStringEnd` token.

fixes: #8065
  • Loading branch information
dhruvmanila committed Oct 27, 2023
1 parent cd8e1ba commit 097e703
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 29 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -110,18 +110,27 @@ pub(crate) fn implicit(
{
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(),
),
(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(),
),
(Tok::String { .. }, Tok::FStringStart) => {
match indexer.fstring_ranges().innermost(b_range.start()) {
Some(b_range) => (*a_range, b_range),
None => continue,
}
}
(Tok::FStringEnd, Tok::String { .. }) => {
match indexer.fstring_ranges().innermost(a_range.start()) {
Some(a_range) => (a_range, *b_range),
None => continue,
}
}
(Tok::FStringEnd, Tok::FStringStart) => {
match (
indexer.fstring_ranges().innermost(a_range.start()),
indexer.fstring_ranges().innermost(b_range.start()),
) {
(Some(a_range), Some(b_range)) => (a_range, b_range),
_ => continue,
}
}
_ => continue,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,21 @@ pub(crate) fn invalid_escape_sequence(
token: &Tok,
token_range: TextRange,
) {
let token_source_code = match token {
let (token_source_code, string_start_location) = match token {
Tok::FStringMiddle { value, is_raw } => {
if *is_raw {
return;
}
value.as_str()
let Some(range) = indexer.fstring_ranges().innermost(token_range.start()) else {
return;
};
(value.as_str(), range.start())
}
Tok::String { kind, .. } => {
if kind.is_raw() {
return;
}
locator.slice(token_range)
(locator.slice(token_range), token_range.start())
}
_ => return,
};
Expand Down Expand Up @@ -206,17 +209,6 @@ pub(crate) fn invalid_escape_sequence(
invalid_escape_sequence.push(diagnostic);
}
} else {
let tok_start = if token.is_f_string_middle() {
// SAFETY: If this is a `FStringMiddle` token, then the indexer
// must have the f-string range.
indexer
.fstring_ranges()
.innermost(token_range.start())
.unwrap()
.start()
} else {
token_range.start()
};
// Turn into raw string.
for invalid_escape_char in &invalid_escape_chars {
let diagnostic = Diagnostic::new(
Expand All @@ -231,8 +223,8 @@ pub(crate) fn invalid_escape_sequence(
// `assert`, etc.) and the string. For example, `return"foo"` is valid, but
// `returnr"foo"` is not.
Fix::safe_edit(Edit::insertion(
pad_start("r".to_string(), tok_start, locator),
tok_start,
pad_start("r".to_string(), string_start_location, locator),
string_start_location,
)),
);
invalid_escape_sequence.push(diagnostic);
Expand Down
4 changes: 3 additions & 1 deletion crates/ruff_python_index/src/fstring_ranges.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ use ruff_text_size::{TextRange, TextSize};

/// Stores the ranges of all f-strings in a file sorted by [`TextRange::start`].
/// There can be multiple overlapping ranges for nested f-strings.
///
/// Note that the ranges for all unterminated f-strings are not stored.
#[derive(Debug)]
pub struct FStringRanges {
// Mapping from the f-string start location to its range.
raw: BTreeMap<TextSize, TextRange>,
}

Expand Down Expand Up @@ -89,7 +92,6 @@ impl FStringRangesBuilder {
}

pub(crate) fn finish(self) -> FStringRanges {
debug_assert!(self.start_locations.is_empty());
FStringRanges { raw: self.raw }
}
}

0 comments on commit 097e703

Please sign in to comment.