From c84c690f1ee1481274e4e159805efac1e685fa67 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Wed, 27 Nov 2024 13:19:33 +0530 Subject: [PATCH] Avoid invalid syntax for format-spec with quotes for all Python versions (#14625) ## Summary fixes: #14608 The logic that was only applied for 3.12+ target version needs to be applied for other versions as well. ## Test Plan I've moved the existing test cases for 3.12 only to `f_string.py` so that it's tested against the default target version. I think we should probably enabled testing for two target version (pre 3.12 and 3.12) but it won't highlight any issue because the parser doesn't consider this. Maybe we should enable this once we have target version specific syntax errors in place (https://github.com/astral-sh/ruff/issues/6591). --- .../test/fixtures/ruff/expression/fstring.py | 42 +++++ .../expression/fstring_py312.options.json | 5 - .../fixtures/ruff/expression/fstring_py312.py | 45 ----- .../src/string/normalize.rs | 19 +- .../format@expression__fstring.py.snap | 150 +++++++++++++++ .../format@expression__fstring_py312.py.snap | 174 ------------------ 6 files changed, 203 insertions(+), 232 deletions(-) delete mode 100644 crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.options.json delete mode 100644 crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.py delete mode 100644 crates/ruff_python_formatter/tests/snapshots/format@expression__fstring_py312.py.snap diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py index 9da1aa403ee9d2..35001e91ce9708 100644 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py +++ b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring.py @@ -664,3 +664,45 @@ # Regression test for https://github.com/astral-sh/ruff/issues/14487 f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" + +# Quotes reuse +f"{'a'}" + +# 312+, it's okay to change the outer quotes even when there's a debug expression using the same quotes +f'foo {10 + len("bar")=}' +f'''foo {10 + len("""bar""")=}''' + +# 312+, it's okay to change the quotes here without creating an invalid f-string +f'{"""other " """}' +f'{"""other " """ + "more"}' +f'{b"""other " """}' +f'{f"""other " """}' + + +# Regression tests for https://github.com/astral-sh/ruff/issues/13935 +f'{1: hy "user"}' +f'{1:hy "user"}' +f'{1: abcd "{1}" }' +f'{1: abcd "{'aa'}" }' +f'{1=: "abcd {'aa'}}' +f'{x:a{z:hy "user"}} \'\'\'' + +# Changing the outer quotes is fine because the format-spec is in a nested expression. +f'{f'{z=:hy "user"}'} \'\'\'' + + +# We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. +f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error +f'{1=: abcd \'\'}' # Changing the quotes here is fine because the inner quotes aren't the opposite quotes +f'{1=: abcd \"\"}' # Changing the quotes here is fine because the inner quotes are escaped +# Don't change the quotes in the following cases: +f'{x=:hy "user"} \'\'\'' +f'{x=:a{y:hy "user"}} \'\'\'' +f'{x=:a{y:{z:hy "user"}}} \'\'\'' +f'{x:a{y=:{z:hy "user"}}} \'\'\'' + +# This is fine because the debug expression and format spec are in a nested expression + +f"""{1=: "this" is fine}""" +f'''{1=: "this" is fine}''' # Change quotes to double quotes because they're preferred +f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part. diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.options.json b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.options.json deleted file mode 100644 index a622b732441898..00000000000000 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.options.json +++ /dev/null @@ -1,5 +0,0 @@ -[ - { - "target_version": "py312" - } -] diff --git a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.py b/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.py deleted file mode 100644 index 9c0f4ff9c0b91b..00000000000000 --- a/crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.py +++ /dev/null @@ -1,45 +0,0 @@ -# This file contains test cases only for cases where the logic tests for whether -# the target version is 3.12 or later. A user can have 3.12 syntax even if the target -# version isn't set. - -# Quotes reuse -f"{'a'}" - -# 312+, it's okay to change the outer quotes even when there's a debug expression using the same quotes -f'foo {10 + len("bar")=}' -f'''foo {10 + len("""bar""")=}''' - -# 312+, it's okay to change the quotes here without creating an invalid f-string -f'{"""other " """}' -f'{"""other " """ + "more"}' -f'{b"""other " """}' -f'{f"""other " """}' - - -# Regression tests for https://github.com/astral-sh/ruff/issues/13935 -f'{1: hy "user"}' -f'{1:hy "user"}' -f'{1: abcd "{1}" }' -f'{1: abcd "{'aa'}" }' -f'{1=: "abcd {'aa'}}' -f'{x:a{z:hy "user"}} \'\'\'' - -# Changing the outer quotes is fine because the format-spec is in a nested expression. -f'{f'{z=:hy "user"}'} \'\'\'' - - -# We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. -f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error -f'{1=: abcd \'\'}' # Changing the quotes here is fine because the inner quotes aren't the opposite quotes -f'{1=: abcd \"\"}' # Changing the quotes here is fine because the inner quotes are escaped -# Don't change the quotes in the following cases: -f'{x=:hy "user"} \'\'\'' -f'{x=:a{y:hy "user"}} \'\'\'' -f'{x=:a{y:{z:hy "user"}}} \'\'\'' -f'{x:a{y=:{z:hy "user"}}} \'\'\'' - -# This is fine because the debug expression and format spec are in a nested expression - -f"""{1=: "this" is fine}""" -f'''{1=: "this" is fine}''' # Change quotes to double quotes because they're preferred -f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part. diff --git a/crates/ruff_python_formatter/src/string/normalize.rs b/crates/ruff_python_formatter/src/string/normalize.rs index 05223792549b9b..6eb59bb05fb404 100644 --- a/crates/ruff_python_formatter/src/string/normalize.rs +++ b/crates/ruff_python_formatter/src/string/normalize.rs @@ -60,18 +60,19 @@ impl<'a, 'src> StringNormalizer<'a, 'src> { return QuoteStyle::Preserve; } - // There are two cases where it's necessary to preserve the quotes - // if the target version is pre 3.12 and the part is an f-string. - if !self.context.options().target_version().supports_pep_701() { - if let StringLikePart::FString(fstring) = string { + if let StringLikePart::FString(fstring) = string { + // There are two cases where it's necessary to preserve the quotes if the + // target version is pre 3.12 and the part is an f-string. + if !self.context.options().target_version().supports_pep_701() { // An f-string expression contains a debug text with a quote character - // because the formatter will emit the debug expression **exactly** the same as in the source text. + // because the formatter will emit the debug expression **exactly** the + // same as in the source text. if is_fstring_with_quoted_debug_expression(fstring, self.context) { return QuoteStyle::Preserve; } - // An f-string expression that contains a triple quoted string literal expression - // that contains a quote. + // An f-string expression that contains a triple quoted string literal + // expression that contains a quote. if is_fstring_with_triple_quoted_literal_expression_containing_quotes( fstring, self.context, @@ -79,7 +80,9 @@ impl<'a, 'src> StringNormalizer<'a, 'src> { return QuoteStyle::Preserve; } } - } else if let StringLikePart::FString(fstring) = string { + + // An f-string expression element contains a debug text and the corresponding + // format specifier has a literal element with a quote character. if is_fstring_with_quoted_format_spec_and_debug(fstring, self.context) { return QuoteStyle::Preserve; } diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring.py.snap index d4665022334bc0..11293d7dde179e 100644 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring.py.snap +++ b/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring.py.snap @@ -670,6 +670,48 @@ _ = ( # Regression test for https://github.com/astral-sh/ruff/issues/14487 f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" + +# Quotes reuse +f"{'a'}" + +# 312+, it's okay to change the outer quotes even when there's a debug expression using the same quotes +f'foo {10 + len("bar")=}' +f'''foo {10 + len("""bar""")=}''' + +# 312+, it's okay to change the quotes here without creating an invalid f-string +f'{"""other " """}' +f'{"""other " """ + "more"}' +f'{b"""other " """}' +f'{f"""other " """}' + + +# Regression tests for https://github.com/astral-sh/ruff/issues/13935 +f'{1: hy "user"}' +f'{1:hy "user"}' +f'{1: abcd "{1}" }' +f'{1: abcd "{'aa'}" }' +f'{1=: "abcd {'aa'}}' +f'{x:a{z:hy "user"}} \'\'\'' + +# Changing the outer quotes is fine because the format-spec is in a nested expression. +f'{f'{z=:hy "user"}'} \'\'\'' + + +# We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. +f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error +f'{1=: abcd \'\'}' # Changing the quotes here is fine because the inner quotes aren't the opposite quotes +f'{1=: abcd \"\"}' # Changing the quotes here is fine because the inner quotes are escaped +# Don't change the quotes in the following cases: +f'{x=:hy "user"} \'\'\'' +f'{x=:a{y:hy "user"}} \'\'\'' +f'{x=:a{y:{z:hy "user"}}} \'\'\'' +f'{x:a{y=:{z:hy "user"}}} \'\'\'' + +# This is fine because the debug expression and format spec are in a nested expression + +f"""{1=: "this" is fine}""" +f'''{1=: "this" is fine}''' # Change quotes to double quotes because they're preferred +f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part. ``` ## Outputs @@ -1398,6 +1440,48 @@ _ = ( # Regression test for https://github.com/astral-sh/ruff/issues/14487 f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" + +# Quotes reuse +f"{'a'}" + +# 312+, it's okay to change the outer quotes even when there's a debug expression using the same quotes +f'foo {10 + len("bar")=}' +f'''foo {10 + len("""bar""")=}''' + +# 312+, it's okay to change the quotes here without creating an invalid f-string +f'{"""other " """}' +f'{"""other " """ + "more"}' +f'{b"""other " """}' +f'{f"""other " """}' + + +# Regression tests for https://github.com/astral-sh/ruff/issues/13935 +f'{1: hy "user"}' +f'{1:hy "user"}' +f'{1: abcd "{1}" }' +f'{1: abcd "{"aa"}" }' +f'{1=: "abcd {'aa'}}' +f"{x:a{z:hy \"user\"}} '''" + +# Changing the outer quotes is fine because the format-spec is in a nested expression. +f"{f'{z=:hy "user"}'} '''" + + +# We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. +f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error +f"{1=: abcd \'\'}" # Changing the quotes here is fine because the inner quotes aren't the opposite quotes +f"{1=: abcd \"\"}" # Changing the quotes here is fine because the inner quotes are escaped +# Don't change the quotes in the following cases: +f'{x=:hy "user"} \'\'\'' +f'{x=:a{y:hy "user"}} \'\'\'' +f'{x=:a{y:{z:hy "user"}}} \'\'\'' +f'{x:a{y=:{z:hy "user"}}} \'\'\'' + +# This is fine because the debug expression and format spec are in a nested expression + +f"""{1=: "this" is fine}""" +f"""{1=: "this" is fine}""" # Change quotes to double quotes because they're preferred +f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part. ``` @@ -2078,6 +2162,48 @@ _ = ( # Regression test for https://github.com/astral-sh/ruff/issues/14487 f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccccccccccccc" + +# Quotes reuse +f"{'a'}" + +# 312+, it's okay to change the outer quotes even when there's a debug expression using the same quotes +f'foo {10 + len("bar")=}' +f'''foo {10 + len("""bar""")=}''' + +# 312+, it's okay to change the quotes here without creating an invalid f-string +f'{"""other " """}' +f'{"""other " """ + "more"}' +f'{b"""other " """}' +f'{f"""other " """}' + + +# Regression tests for https://github.com/astral-sh/ruff/issues/13935 +f'{1: hy "user"}' +f'{1:hy "user"}' +f'{1: abcd "{1}" }' +f'{1: abcd "{'aa'}" }' +f'{1=: "abcd {'aa'}}' +f'{x:a{z:hy "user"}} \'\'\'' + +# Changing the outer quotes is fine because the format-spec is in a nested expression. +f'{f'{z=:hy "user"}'} \'\'\'' + + +# We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. +f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error +f'{1=: abcd \'\'}' # Changing the quotes here is fine because the inner quotes aren't the opposite quotes +f'{1=: abcd \"\"}' # Changing the quotes here is fine because the inner quotes are escaped +# Don't change the quotes in the following cases: +f'{x=:hy "user"} \'\'\'' +f'{x=:a{y:hy "user"}} \'\'\'' +f'{x=:a{y:{z:hy "user"}}} \'\'\'' +f'{x:a{y=:{z:hy "user"}}} \'\'\'' + +# This is fine because the debug expression and format spec are in a nested expression + +f"""{1=: "this" is fine}""" +f"""{1=: "this" is fine}""" # Change quotes to double quotes because they're preferred +f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part. ``` @@ -2915,4 +3041,28 @@ f"aaaaaaaaaaaaaaaaaaaaaaaaaa {10**27} bbbbbbbbbbbbbbbbbbbbbbbbbb ccccccccccccccc ) # Regression test for https://github.com/astral-sh/ruff/issues/14487 +@@ -678,18 +726,18 @@ + f'{1: hy "user"}' + f'{1:hy "user"}' + f'{1: abcd "{1}" }' +-f'{1: abcd "{'aa'}" }' ++f'{1: abcd "{"aa"}" }' + f'{1=: "abcd {'aa'}}' +-f'{x:a{z:hy "user"}} \'\'\'' ++f"{x:a{z:hy \"user\"}} '''" + + # Changing the outer quotes is fine because the format-spec is in a nested expression. +-f'{f'{z=:hy "user"}'} \'\'\'' ++f"{f'{z=:hy "user"}'} '''" + + + # We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. + f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error +-f'{1=: abcd \'\'}' # Changing the quotes here is fine because the inner quotes aren't the opposite quotes +-f'{1=: abcd \"\"}' # Changing the quotes here is fine because the inner quotes are escaped ++f"{1=: abcd \'\'}" # Changing the quotes here is fine because the inner quotes aren't the opposite quotes ++f"{1=: abcd \"\"}" # Changing the quotes here is fine because the inner quotes are escaped + # Don't change the quotes in the following cases: + f'{x=:hy "user"} \'\'\'' + f'{x=:a{y:hy "user"}} \'\'\'' ``` diff --git a/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring_py312.py.snap b/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring_py312.py.snap deleted file mode 100644 index 77725792d7f21c..00000000000000 --- a/crates/ruff_python_formatter/tests/snapshots/format@expression__fstring_py312.py.snap +++ /dev/null @@ -1,174 +0,0 @@ ---- -source: crates/ruff_python_formatter/tests/fixtures.rs -input_file: crates/ruff_python_formatter/resources/test/fixtures/ruff/expression/fstring_py312.py -snapshot_kind: text ---- -## Input -```python -# This file contains test cases only for cases where the logic tests for whether -# the target version is 3.12 or later. A user can have 3.12 syntax even if the target -# version isn't set. - -# Quotes reuse -f"{'a'}" - -# 312+, it's okay to change the outer quotes even when there's a debug expression using the same quotes -f'foo {10 + len("bar")=}' -f'''foo {10 + len("""bar""")=}''' - -# 312+, it's okay to change the quotes here without creating an invalid f-string -f'{"""other " """}' -f'{"""other " """ + "more"}' -f'{b"""other " """}' -f'{f"""other " """}' - - -# Regression tests for https://github.com/astral-sh/ruff/issues/13935 -f'{1: hy "user"}' -f'{1:hy "user"}' -f'{1: abcd "{1}" }' -f'{1: abcd "{'aa'}" }' -f'{1=: "abcd {'aa'}}' -f'{x:a{z:hy "user"}} \'\'\'' - -# Changing the outer quotes is fine because the format-spec is in a nested expression. -f'{f'{z=:hy "user"}'} \'\'\'' - - -# We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. -f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error -f'{1=: abcd \'\'}' # Changing the quotes here is fine because the inner quotes aren't the opposite quotes -f'{1=: abcd \"\"}' # Changing the quotes here is fine because the inner quotes are escaped -# Don't change the quotes in the following cases: -f'{x=:hy "user"} \'\'\'' -f'{x=:a{y:hy "user"}} \'\'\'' -f'{x=:a{y:{z:hy "user"}}} \'\'\'' -f'{x:a{y=:{z:hy "user"}}} \'\'\'' - -# This is fine because the debug expression and format spec are in a nested expression - -f"""{1=: "this" is fine}""" -f'''{1=: "this" is fine}''' # Change quotes to double quotes because they're preferred -f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part. -``` - -## Outputs -### Output 1 -``` -indent-style = space -line-width = 88 -indent-width = 4 -quote-style = Double -line-ending = LineFeed -magic-trailing-comma = Respect -docstring-code = Disabled -docstring-code-line-width = "dynamic" -preview = Disabled -target_version = Py312 -source_type = Python -``` - -```python -# This file contains test cases only for cases where the logic tests for whether -# the target version is 3.12 or later. A user can have 3.12 syntax even if the target -# version isn't set. - -# Quotes reuse -f"{'a'}" - -# 312+, it's okay to change the outer quotes even when there's a debug expression using the same quotes -f'foo {10 + len("bar")=}' -f'''foo {10 + len("""bar""")=}''' - -# 312+, it's okay to change the quotes here without creating an invalid f-string -f'{"""other " """}' -f'{"""other " """ + "more"}' -f'{b"""other " """}' -f'{f"""other " """}' - - -# Regression tests for https://github.com/astral-sh/ruff/issues/13935 -f'{1: hy "user"}' -f'{1:hy "user"}' -f'{1: abcd "{1}" }' -f'{1: abcd "{'aa'}" }' -f'{1=: "abcd {'aa'}}' -f'{x:a{z:hy "user"}} \'\'\'' - -# Changing the outer quotes is fine because the format-spec is in a nested expression. -f'{f'{z=:hy "user"}'} \'\'\'' - - -# We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. -f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error -f'{1=: abcd \'\'}' # Changing the quotes here is fine because the inner quotes aren't the opposite quotes -f'{1=: abcd \"\"}' # Changing the quotes here is fine because the inner quotes are escaped -# Don't change the quotes in the following cases: -f'{x=:hy "user"} \'\'\'' -f'{x=:a{y:hy "user"}} \'\'\'' -f'{x=:a{y:{z:hy "user"}}} \'\'\'' -f'{x:a{y=:{z:hy "user"}}} \'\'\'' - -# This is fine because the debug expression and format spec are in a nested expression - -f"""{1=: "this" is fine}""" -f"""{1=: "this" is fine}""" # Change quotes to double quotes because they're preferred -f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part. -``` - - -#### Preview changes -```diff ---- Stable -+++ Preview -@@ -6,32 +6,32 @@ - f"{'a'}" - - # 312+, it's okay to change the outer quotes even when there's a debug expression using the same quotes --f'foo {10 + len("bar")=}' --f'''foo {10 + len("""bar""")=}''' -+f"foo {10 + len("bar")=}" -+f"""foo {10 + len("""bar""")=}""" - - # 312+, it's okay to change the quotes here without creating an invalid f-string --f'{"""other " """}' --f'{"""other " """ + "more"}' --f'{b"""other " """}' --f'{f"""other " """}' -+f"{'''other " '''}" -+f"{'''other " ''' + 'more'}" -+f"{b'''other " '''}" -+f"{f'''other " '''}" - - - # Regression tests for https://github.com/astral-sh/ruff/issues/13935 - f'{1: hy "user"}' - f'{1:hy "user"}' - f'{1: abcd "{1}" }' --f'{1: abcd "{'aa'}" }' -+f'{1: abcd "{"aa"}" }' - f'{1=: "abcd {'aa'}}' --f'{x:a{z:hy "user"}} \'\'\'' -+f"{x:a{z:hy \"user\"}} '''" - - # Changing the outer quotes is fine because the format-spec is in a nested expression. --f'{f'{z=:hy "user"}'} \'\'\'' -+f"{f'{z=:hy "user"}'} '''" - - - # We have to be careful about changing the quotes if the f-string has a debug expression because it is inserted verbatim. - f'{1=: "abcd \'\'}' # Don't change the outer quotes, or it results in a syntax error --f'{1=: abcd \'\'}' # Changing the quotes here is fine because the inner quotes aren't the opposite quotes --f'{1=: abcd \"\"}' # Changing the quotes here is fine because the inner quotes are escaped -+f"{1=: abcd \'\'}" # Changing the quotes here is fine because the inner quotes aren't the opposite quotes -+f"{1=: abcd \"\"}" # Changing the quotes here is fine because the inner quotes are escaped - # Don't change the quotes in the following cases: - f'{x=:hy "user"} \'\'\'' - f'{x=:a{y:hy "user"}} \'\'\'' -@@ -42,4 +42,4 @@ - - f"""{1=: "this" is fine}""" - f"""{1=: "this" is fine}""" # Change quotes to double quotes because they're preferred --f'{1=: {'ab"cd"'}}' # It's okay if the quotes are in an expression part. -+f"{1=: {'ab"cd"'}}" # It's okay if the quotes are in an expression part. -```