Avoid reusing nested, interpolated quotes before Python 3.12#20930
Merged
Avoid reusing nested, interpolated quotes before Python 3.12#20930
Conversation
Contributor
|
Member
|
I like the solution. It's very non-invasive, comes with little code and runtime complexity and it preserves the syntax error as is (which I think is better than trying to fixing syntax errors and than creating an even bigger mess) |
MichaReiser
approved these changes
Oct 17, 2025
ntBre
added a commit
that referenced
this pull request
Oct 17, 2025
Summary -- This PR fixes the issue I added in #20867 and noticed in #20930. Cases like this cause an error on any Python version: ```py f"{1:""}" ``` which gave me a false sense of security before. Cases like this are still invalid only before 3.12 and weren't flagged after the changes in #20867: ```py f'{1: abcd "{'aa'}" }' # ^ reused quote f'{1: abcd "{"\n"}" }' # ^ backslash ``` I didn't recognize these as nested interpolations that also need to be checked for invalid expressions, so filtering out the whole format spec wasn't quite right. There's basically no code change in this PR, I just moved the existing check from `parse_interpolated_string`, which parses the entire string, to `parse_interpolated_element`. This kind of seems more natural anyway and avoids having to try to recursively visit nested elements after the fact in `parse_interpolated_string`. So viewing the diff with something like ``` git diff --color-moved --ignore-space-change --color-moved-ws=allow-indentation-change main...HEAD ``` should make this more clear. Test Plan -- New tests
ntBre
added a commit
that referenced
this pull request
Oct 20, 2025
Summary -- This PR fixes the issue I added in #20867 and noticed in #20930. Cases like this cause an error on any Python version: ```py f"{1:""}" ``` which gave me a false sense of security before. Cases like this are still invalid only before 3.12 and weren't flagged after the changes in #20867: ```py f'{1: abcd "{'aa'}" }' # ^ reused quote f'{1: abcd "{"\n"}" }' # ^ backslash ``` I didn't recognize these as nested interpolations that also need to be checked for invalid expressions, so filtering out the whole format spec wasn't quite right. And `elements.interpolations()` only iterates over the outermost interpolations, not the nested ones. There's basically no code change in this PR, I just moved the existing check from `parse_interpolated_string`, which parses the entire string, to `parse_interpolated_element`. This kind of seems more natural anyway and avoids having to try to recursively visit nested elements after the fact in `parse_interpolated_string`. So viewing the diff with something like ``` git diff --color-moved --ignore-space-change --color-moved-ws=allow-indentation-change main ``` should make this more clear. Test Plan -- New tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #20774 by tracking whether an
InterpolatedStringStateelement is nested inside of another interpolated element. This feels like kind of a naive fix, so I'm welcome to other ideas. But it resolves the problem in the issue and clears up the syntax error in the black compatibility test, without affecting many other cases.The other affected case is actually interesting too because the input is invalid, but the previous quote selection fixed the invalid syntax:
We now preserve the invalid syntax in the input.
Unfortunately, this also seems to be another edge case I didn't consider in #20867 because we don't flag this as a syntax error after 0.14.1:
Shell output
I assumed that was the same
ParseErroras the one caused byf"{1:""}", but this is a nested interpolation inside of the format spec.Test Plan
New test copied from the black compatibility test. I guess this is a duplicate now, I started working on this branch before the new black tests were imported, so I could delete the separate test in our fixtures if that's preferable.