Update Indexer to use new f-string tokens

[dhruvmanila]

Summary

This PR updates the Indexer to use the new f-string tokens to compute the f_string_ranges for f-strings. It adds a new abstraction which exposes two methods to support extracting the range for the surrounding innermost and outermost f-string. It uses the builder pattern to build the f-string ranges which is similar to how the comment ranges are built.

Test Plan

Add new test cases for f-strings for:

• Tab indentation rule
• Line continuation detection in the indexer
• To get the innermost / outermost f-string range
• All detected f-string ranges

fixes: #7290

[dhruvmanila]

Current dependencies on/for this PR:

• main
  • PR Add support for PEP 701 #7376
    • PR Update logical line rules for the new f-string tokens #7690
    • PR Remove StringKind::FString and StringKind::RawFString #7667
    • PR Ignore quote escapes in expression part of f-string #7597
    • PR Separate Q003 to accomodate f-string context #7588
      • PR Update Q000, Q001 with the new f-string tokens #7589
    • PR Use the new f-string tokens in string formatting #7586
    • PR Update ISC001, ISC002 to check in f-strings #7515
    • PR Update RUF001, RUF003 to check in f-strings #7477
    • PR Update PLE2510, PLE2512-2515 to check in f-strings #7387
    • PR Allow NUL character in f-strings #7378
    • PR Update W605 to check in f-strings #7329
    • PR Update Stylist quote detection with new f-string token #7328
    • PR Update F541 to use new f-string tokens #7327
    • PR Detect noqa directives for multi-line f-strings #7326
    • PR Update Indexer to use new f-string tokens #7325 👈

This comment was auto-generated by Graphite.

[MichaReiser]

Can you add tests for the upstream functions that use the metadata extracted by the Indexer to ensure they are working as intended.

[dhruvmanila]

Nested f-strings:

Pre 3.12, nested f-strings were possible but up to an extent. For example,

f"outside {f'inside'} outside"
# or
f'outside {f"inside"} outside'

The rules which performs checks on strings don’t support nested f-strings. For example,

f"{f"\y"}"

For the above code snippet, the W605 (invalid-escape-sequence) incorrectly auto-fixes by adding the r prefix to the outermost f-string while it should be added to the f-string which contains the escape character.

F-string ranges:

The Indexer includes the range of the outermost f-string while it should probably include the actual range of all the f-strings. This creates a challenge in the way f-string ranges are inserted as the vector needs to be sorted for the binary search to work but that’s an implementation detail.

Currently, the f-string range is only being utilized to get the f-string quote style but this would be incorrect in the case of nested f-strings. For example, in f"foo {f'{bar}'}", for the variable bar, the quote style would give Quote::Double.

The question is: do we want to proceed with how the current implementation is (use the outermost f-string range) or have separate ranges for each f-strings even if it’s nested?

Triple-quoted string ranges:

Currently, if a triple-quoted f-string is encountered, then the range is only recorded as a triple-quoted string but not as a f-string i.e., it’s only being inserted to indexer.triple_quoted_string_ranges and not indexer.f_string_ranges. I’m not sure if that’s intentional or not but with the current changes, I’m inserting it into both.

This is only being utilized in W191 (tab-indentation) which works on physical lines. This creates a problem as we can’t really know if this line is part of a f-string expression or non-expression part (thus, #7379).

Another challenge here is that with f-strings even the triple-quoted strings can be nested:

f"""first level
of triple-quoted {f"""second level
of triple-quoted"""}
f-strings"""

This again brings up the question of how granular we want the triple-quoted string ranges to be?

---

I'm leaning towards keeping the behavior same and we can iterate to fix the issues around nested f-strings. But, if we're sure on how to handle nested f-strings I can make the changes here.

[MichaReiser]

F-string ranges:

Technically, I think we can solve this by using a BTreeMap<TextSize, TextRange> where the key is the start position of the f-string and the value is the string range. Finding the enclosing f-strings for an offset is then map.range(...Include(offset)).rev().filter(|range| range.contains(offset)). This doesn't just give you the enclosing f-string but an iterator over all enclosing fstrings.

Currently, the f-string range is only being utilized to get the f-string quote style but this would be incorrect in the case of nested f-strings.

More specifically, it seems this is only used to guess the appropriate quote style for an fstring. This logic won't be needed for Python 3.12+ because we can pick any quote style (ideally, the default quote style).

It might be okay to leave the implementation as is but we should document the limitation and its intended use case. Not that more call sites are introduced that incorrectly assume it supports nested FStrings correctly.

Currently, if a triple-quoted f-string is encountered, then the range is only recorded as a triple-quoted string but not as a f-string i.e., it’s only being inserted to indexer.triple_quoted_string_ranges and not indexer.f_string_ranges. I’m not sure if that’s intentional or not but with the current changes, I’m inserting it into both.

I don't think this was intentional.

This is only being utilized in W191 (tab-indentation) which works on physical lines. This creates a problem as we can’t really know if this line is part of a f-string expression or non-expression part (thus, #7379).

I wonder if it would be easier to rewrite that rule to a token-based rule rather than trying to extract all the data at a granularity level close to the individual tokens.

Except if we plan to to re-use the multiline-string ranges for add-noqa as well:

ruff/crates/ruff/src/directives.rs

Lines 90 to 114 in fc89976

	let mut string_mappings = Vec::new();
	for (tok, range) in lxr.iter().flatten() {
	match tok {
	Tok::EndOfFile => {
	break;
	}
	// For multi-line strings, we expect `noqa` directives on the last line of the
	// string.
	Tok::String {
	triple_quoted: true,
	..
	} => {
	if locator.contains_line_break(*range) {
	string_mappings.push(TextRange::new(
	locator.line_start(range.start()),
	range.end(),
	));
	}
	}
	_ => {}
	}
	}

I would be interested to get @charliermarsh's opinion on this, who is more familiar with the use cases (I only know the Indexer from when I refactored from column to byte offsets).

[codspeed-hq]

CodSpeed Performance Report

Merging #7325 will not alter performance

_{Comparing dhruv/issue-7290 (0b2e6bb) with dhruv/pep-701 (c496d9c)}

Summary

✅ 25 untouched benchmarks

[MichaReiser]

Could you rebase your PRs so that we get clear benchmark numbers? This all looks worrying but they're probably false positives