Add support for nested replacements inside format specifications

[zanieb]

Closes #6442

Python string formatting like "hello {place}".format(place="world") supports format specifications for replaced content such as "hello {place:>10}".format(place="world") which will align the text to the right in a container filled up to ten characters.

Ruff parses formatted strings into FormatParts each of which is either a Field (content in {...}) or a Literal (the normal content). Fields are parsed into name and format specifier sections (we'll ignore conversion specifiers for now).

There are a myriad of specifiers that can be used in a FormatSpec. Unfortunately for linters, the specifier values can be dynamically set. For example, "hello {place:{align}{width}}".format(place="world", align=">", width=10) and "hello {place:{fmt}}".format(place="world", fmt=">10") will yield the same string as before but variables can be used to determine the formatting. In this case, when parsing the format specifier we can't know what kind of specifier is being used as their meaning is determined by both position and value.

Ruff does not support nested replacements and our current data model does not support the concept. Here the data model is updated to support this concept, although linting of specifications with replacements will be inherently limited. We could split format specifications into two types, one without any replacements that we can perform lints with and one with replacements that we cannot inspect. However, it seems excessive to drop all parsing of format specifiers due to the presence of a replacement. Instead, I've opted to parse replacements eagerly and ignore their possible effect on other format specifiers. This will allow us to retain a simple interface for FormatSpec and most syntax checks. We may need to add some handling to relax errors if a replacement was seen previously.

It's worth noting that the nested replacement can also include a format specification although it may fail at runtime if you produce an invalid outer format specification. For example, "hello {place:{fmt:<2}}".format(place="world", fmt=">10") is valid so we need to represent each nested replacement as a full FormatPart.

Test plan

Adding unit tests for FormatSpec parsing and snapshots for PLE1300

[MichaReiser]

Could you extend the PR summary with what parsing logic you changed? It is difficult to me to review this PR because I'm unfamiliar with format specifications and format.rs

[zanieb]

Ya'll are too fast! I'm still working on this — I should have opened it as a draft but was in the process of writing up the body.

I added support for a replacement at the end of the specification (before the format type) but as Charlie has noted I've realized that we actually need to support any of the items in specification being a replacement. I'll need to consider a bit further the best way to accomplish this.

[charliermarsh]

We might need to just detect that the spec has nested parts, and return some other representation that isn't really analyzable by downstream rules (since we can't know how the spec will be resolved).

[zanieb]

Would love suggestions here as this feels awkward.

[zanieb]

An alternative, I guess, is to traverse the entire string twice. Once to collect and remove all of the replacements and a second pass on the cleaned text.

[charliermarsh]

A bit tedious but I don't know that I have much better.

[zanieb]

My decision to just extend FormatSpec may not work since all of these FormatSpec.format_... methods will drop replacements. I'm unsure of the best approach.

[charliermarsh]

What do you mean by "drop replacements"? Can you give an example?

[zanieb]

Sure! The following test will pass

        assert_eq!(
            FormatSpec::parse("{x}d")
                .unwrap()
                .format_int(&BigInt::from_bytes_be(Sign::Plus, b"\x10")),
            Ok("16".to_owned())
        );

but {x} could change the formatting and should not necessarily be ignored. Since all the format_... methods return results we could just return an error if replacements are present. I'm not sure where these methods are really used though.

[zanieb]

With e5d6ec3 I've updated PLE1300 to lint nested replacements as well.

[charliermarsh]

Ahhh I see, you're referring to the methods that actually use this to format things. Can we... remove those? I suspect they came from RustPython which needs to support these at runtime. Otherwise, I'd suggest returning an error in those cases.

[github-actions]

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.01      3.6±0.04ms    11.3 MB/sec    1.00      3.6±0.03ms    11.4 MB/sec
formatter/numpy/ctypeslib.py               1.00    736.1±7.99µs    22.6 MB/sec    1.00    733.6±3.11µs    22.7 MB/sec
formatter/numpy/globals.py                 1.00     75.9±0.42µs    38.9 MB/sec    1.01     77.0±1.59µs    38.3 MB/sec
formatter/pydantic/types.py                1.00  1454.5±13.98µs    17.5 MB/sec    1.00  1459.2±33.25µs    17.5 MB/sec
linter/all-rules/large/dataset.py          1.00     10.5±0.06ms     3.9 MB/sec    1.00     10.5±0.06ms     3.9 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      2.8±0.02ms     6.0 MB/sec    1.00      2.8±0.01ms     6.0 MB/sec
linter/all-rules/numpy/globals.py          1.01    397.9±0.94µs     7.4 MB/sec    1.00    394.5±2.81µs     7.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.4±0.10ms     4.7 MB/sec    1.00      5.4±0.09ms     4.7 MB/sec
linter/default-rules/large/dataset.py      1.00      5.5±0.02ms     7.3 MB/sec    1.01      5.6±0.03ms     7.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01  1227.4±44.65µs    13.6 MB/sec    1.00  1217.0±13.47µs    13.7 MB/sec
linter/default-rules/numpy/globals.py      1.00    141.0±2.59µs    20.9 MB/sec    1.04    145.9±6.01µs    20.2 MB/sec
linter/default-rules/pydantic/types.py     1.00      2.5±0.01ms    10.3 MB/sec    1.00      2.5±0.02ms    10.2 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      4.2±0.10ms     9.7 MB/sec    1.00      4.2±0.08ms     9.7 MB/sec
formatter/numpy/ctypeslib.py               1.00   828.1±20.67µs    20.1 MB/sec    1.00   828.9±17.37µs    20.1 MB/sec
formatter/numpy/globals.py                 1.00     84.7±1.90µs    34.8 MB/sec    1.01     85.6±2.25µs    34.5 MB/sec
formatter/pydantic/types.py                1.00  1669.0±27.39µs    15.3 MB/sec    1.01  1685.2±25.52µs    15.1 MB/sec
linter/all-rules/large/dataset.py          1.00     12.7±0.13ms     3.2 MB/sec    1.05     13.3±0.21ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.5±0.04ms     4.8 MB/sec    1.03      3.6±0.06ms     4.6 MB/sec
linter/all-rules/numpy/globals.py          1.00    440.2±9.70µs     6.7 MB/sec    1.03   454.8±14.80µs     6.5 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.6±0.07ms     3.9 MB/sec    1.05      6.9±0.11ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      7.0±0.09ms     5.8 MB/sec    1.04      7.3±0.12ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1489.7±19.61µs    11.2 MB/sec    1.02  1523.9±27.75µs    10.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    176.5±4.46µs    16.7 MB/sec    1.02    180.4±3.75µs    16.4 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.2±0.06ms     8.1 MB/sec    1.01      3.2±0.04ms     8.0 MB/sec

[zanieb]

Great that works for me #6624

[zanieb]

I'd really like better range information here. Would that be hard for me to add to FormatSpec?

[charliermarsh]

It might be challenging because the thing we get on the left-hand side is the parsed representation which doesn't always match the literal representation. For example, the string on the left could be an implicit concatenation, or it could contain escaped characters, etc...

[charliermarsh]

right is guaranteed to be non-empty because the first character is the closing bracket?

[zanieb]

I guess? Is split_at(idx + 1) clearer? I added a couple test cases and can't make it fail.

[charliermarsh]

Not for this PR, but I feel like these are good candidates for insta::assert_debug_snapshot, so that we don't have to write out and maintain the expectations manually.

[zanieb]

Makes sense, I tried to match the existing pattern but I agree that's more maintainable.

[zanieb]

The former was only used to represent the latter which I found confusing and unnecessarily vague.

[MichaReiser]

Related to your question about better ranges. I have the impression that using Cursor instead of manually constructing strings and iterating over chars could simplify the implementation significantly (and speed it up!). We could even decide to build a small lexer that tokenized the strings, simplifying the "parsing" part.