Pyupgrade: Format specifiers

[colin99d]

A part of #827. Posting this for visibility. Still has some work to do to be done.

Things that still need done before this is ready:

• Does not work when the item is being assigned to a variable
• Does not work if being used in a function call
• Fix incorrectly removed calls in the function
• Has not been tested with pyupgrade negative test cases

Tests from pyupgrade can be seen here: https://github.com/asottile/pyupgrade/blob/main/tests/features/format_literals_test.py

[colin99d]

I will try to have this done by Sunday night.

[colin99d]

@charliermarsh I have the following function to detect expressions:

pub fn match_expression(expression_text: &str) -> Result<Expression> {
    match libcst_native::parse_expression(expression_text) {
        Ok(expression) => Ok(expression),
        Err(_) => bail!("Failed to extract CST from source"),
    }
}

It works fine for an expression like this:

    'foo{0}' 'bar{1}'.format(1, 2)

But the second you make it multi-line like so:

    'foo{0}'\n    'bar{1}'.format(1, 2)

The whole thing fails. Is this an error with my coding, or an error with libcst_native?

[charliermarsh]

I'm wondering if you need to dedent the code. You might be passing LibCST the indented expression, which would cause an error.

[colin99d]

It looks like the issue still happens with the de-indented code: 'foo{0}'\n'bar{1}'.format(1, 2).

[colin99d]

thread 'main' panicked at 'called Result::unwrap() on an Err value: ParserError(ParseError { location: ParseLoc { start_pos: LineCol { line: 2, column: 4, offset: 13 }, end_pos: LineCol { line: 2, column: 12, offset: 21 } }, expected: ExpectedSet { expected: {"EOF"} } }, "'foo{0}'\n 'bar{1}'.format(1, 2)")', src/pyupgrade/plugins/format_specififiers.rs:57:57

Looks like this is a parse error from the parse_expression function in libcst_native. My thoughts are we either:

1. Make a custom patch in your fork
2. Find a way to modify the string to add a \ at the end of each line so the parser can understand it, and then remove it once we are done
3. Don't use libcst_native ( I dont like this one because I like libcst now)

Just let me know which you would like for me to pursue.

[charliermarsh]

Would you be open to filing an issue on the LibCST repo? In the meantime, we could just skip those cases. (Are they needed for detection, or just autofixing?)

[colin99d]

Here is the issue:
Instagram/LibCST#846
I will just make it a check for now!

[charliermarsh]

Thank you and sorry that this all churned mid-PR for you.

[colin99d]

Im never going to complain about you making this project easier to contribute to.

[colin99d]

Hey @charliermarsh I got all the positive test cases working (except for the one edge case we talked about), and all the negative cases working except one. I wanted to get your feedback before I implemented it.
The following edge case is what causes issues:

'{' '0}'.format(1)

Since the format specifier is split into two different strings, it is NOT supposed to be formatted. However, both the Expr
struct, and the tokens automatically combine this into one string. What would be the best way of detecting that this edge case is preset? Or do we want to ignore it? Since the refactoring would produce changes that could be seen as desirable:

[charliermarsh]

I think it's best just to ignore it (i.e., treat it as if there is no space). It strikes me as exceptionally rare, to the point that it's likely either a mistake or that it's not worth adding more complexity to our own implementation to handle it.

[charliermarsh]

We could probably detect it by tokenizing... but it doesn't seem worth it to me.

[colin99d]

I think it's best just to ignore it (i.e., treat it as if there is no space). It strikes me as exceptionally rare, to the point that it's likely either a mistake or that it's not worth adding more complexity to our own implementation to handle it.

100% agree, especially since the python code will still work.

[colin99d]

I just need to fix one bug with this edge case, and then I should be able to take this PR out of draft.

[colin99d]

@charliermarsh anything else you want to see from this for merge?

[charliermarsh]

@colin99d - Will review this tonight!

[charliermarsh]

When does this happen?

[charliermarsh]

Ah nevermind.

[charliermarsh]

@colin99d - It turns out that we already had a utility for extracting the format positions, which uses the RustPython string parser underneath and so is very robust. Sorry that I didn't flag this earlier -- I didn't realize it could even be reused here, but it should make things more efficient too since we can do one string parse and share that "summary" amongst a bunch of checks.

[charliermarsh]

I feel like one strategy that could work here would be...

• Take the entire text.
• Replace the string part via the regex above.
• Replace the arguments via LibCST.

Kinda tough, hacky, etc... but would solve the missing cases.

[charliermarsh]

Actually, I guess that wouldn't solve the '{' '0}'.format(1) case.

[colin99d]

Thanks for the review!