gh-121130: Fix f-string format specifiers with debug expressions #121150

pablogsal · 2024-06-29T09:58:57Z

Issue: Self-documenting f-string in conversion specifier throws ValueError #121130

pablogsal · 2024-06-30T11:12:53Z

@lysnikolaou do you mind taking a look at this approach? I think is not too bad and it solidifies a bit more the concept of being in a format spec

pablogsal · 2024-06-30T11:13:56Z

Parser/action_helpers.c

        conversion_val = (int)'r';
    }

+    expr_ty format_expr = format ? (expr_ty) format->result : NULL;


This here is because when there are debug expressions we are wrapping the values in a node that then is wrapped in turn, but that is not what 3.11 and before does so we need to detect that and unwrap.

lysnikolaou

I like the approach, although it seems like the unpacking does not work correctly.

lysnikolaou · 2024-07-01T12:57:24Z

Parser/lexer/lexer.c

                    return MAKE_TOKEN(_PyTokenizer_syntaxerror(tok, "f-string: expressions nested too deeply"));
                }
                TOK_GET_MODE(tok)->kind = TOK_REGULAR_MODE;
+                current_tok->in_format_spec = 0;


Shouldn't this always be 0 here, if we've correctly reset it when the format spec is ending?

Parser/action_helpers.c

pablogsal · 2024-07-01T15:16:26Z

@lysnikolaou Ugh... this is much more complicated indeed. We need to restructure how we propagate the format specifier and how we capture the f-string expressions on format specifiers :(

pablogsal · 2024-07-01T15:19:51Z

Also the AST is so weird because it removes {. Check this out:

python3.11 -m ast <<< 'f"{datetime.datetime.now():h1{y2=}}"'
Module(
   body=[
      Expr(
         value=JoinedStr(
            values=[
               FormattedValue(
                  value=Call(
                     func=Attribute(
                        value=Attribute(
                           value=Name(id='datetime', ctx=Load()),
                           attr='datetime',
                           ctx=Load()),
                        attr='now',
                        ctx=Load()),
                     args=[],
                     keywords=[]),
                  conversion=-1,
                  format_spec=JoinedStr(
                     values=[
                        Constant(value='h1y2='),
                        FormattedValue(
                           value=Name(id='y2', ctx=Load()),
                           conversion=114)]))]))],
   type_ignores=[])

notice that the node is h1y2 without the { in the middle :S

pablogsal · 2024-07-01T20:46:22Z

I think this should do the trick. Boy this was hard to fix :S

pablogsal · 2024-07-01T23:10:16Z

Hummm I will investigate the failure soon

Lib/test/test_ast.py

pablogsal · 2024-07-02T12:16:05Z

Parser/action_helpers.c

+        res = _PyAST_JoinedStr(spec, lineno, col_offset, end_lineno,
+                                    end_col_offset, p->arena);
+    } else {
+        res = _PyPegen_concatenate_strings(p, spec,


This code is so we merge and concatenate the Constant and JoinedStr nodes. See how the tree originally looks like here: #121150 (comment)

I don't understand why the tree in #121150 is false, but also running that same example with the latest status of your branch gives me the same exact one.

Sorry for the confusion. That tree is the correct one, but without this call it basically has a bunch of Constants all together and the Joined strings are nested (comment the code and check it out to see what I mean).

pablogsal · 2024-07-02T12:16:38Z

Parser/action_helpers.c

+                }
+                n_flattened_elements++;
+                break;
+            case JoinedStr_kind:


This code is to accommodate the case when we call this function with other things than JoinedStr and constants

How would this happen? It probably shouldn't, but I may be missing something.

Because I am flattening collections that contain new nodes in https://github.com/python/cpython/pull/121150/files#r1662410566 (it's a new call over a new array that was not happening before)

pablogsal · 2024-07-02T12:17:40Z

Parser/lexer/lexer.c

    while (end_quote_size != current_tok->f_string_quote_size) {
        int c = tok_nextc(tok);
-        if (tok->done == E_ERROR) {
+        if (tok->done == E_ERROR || tok->done == E_DECODE) {


This was a bug, as if we enter E_DECODE state we were not returning soon enough

Recently came across this in another branch I'm working on. If we check for this here, we can probably remove the if (tok->decoding_erred) about 10 lines below, right?

Good idea, will do

Parser/lexer/lexer.c

lysnikolaou

A few comments/questions.

Parser/action_helpers.c

lysnikolaou · 2024-07-04T13:19:50Z

Parser/action_helpers.c

+        res = _PyAST_JoinedStr(spec, lineno, col_offset, end_lineno,
+                                    end_col_offset, p->arena);
+    } else {
+        res = _PyPegen_concatenate_strings(p, spec,


I don't understand why the tree in #121150 is false, but also running that same example with the latest status of your branch gives me the same exact one.

lysnikolaou · 2024-07-04T13:22:19Z

Parser/action_helpers.c

+                }
+                n_flattened_elements++;
+                break;
+            case JoinedStr_kind:


How would this happen? It probably shouldn't, but I may be missing something.

Parser/lexer/lexer.c

lysnikolaou · 2024-07-04T13:24:19Z

Parser/lexer/lexer.c

    while (end_quote_size != current_tok->f_string_quote_size) {
        int c = tok_nextc(tok);
-        if (tok->done == E_ERROR) {
+        if (tok->done == E_ERROR || tok->done == E_DECODE) {


Recently came across this in another branch I'm working on. If we check for this here, we can probably remove the if (tok->decoding_erred) about 10 lines below, right?

lysnikolaou · 2024-07-04T13:25:29Z

Parser/lexer/lexer.c

            // brackets, we can bypass it here.
-            if (peek == '}' && !in_format_spec) {
+            int cursor = current_tok->curly_bracket_depth;
+            if (peek == '}' && !in_format_spec && cursor == 0) {


Why do we need the cursor check here?

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>

pablogsal · 2024-07-13T13:33:48Z

@lysnikolaou can you take another look so we can get this into rc1?

pablogsal · 2024-07-16T18:57:17Z

I'm landing so this gets into the last 3.13 beta to get some coverage. We can fix anything @lysnikolaou doesn't like later

miss-islington-app · 2024-07-16T18:57:25Z

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington-app · 2024-07-16T18:57:28Z

Sorry, @pablogsal, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c46d64e0ef8e92a6b4ab4805d813d7e4d6663380 3.13

miss-islington-app · 2024-07-16T18:57:41Z

Sorry, @pablogsal, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker c46d64e0ef8e92a6b4ab4805d813d7e4d6663380 3.12

bedevere-app · 2024-07-16T18:59:58Z

GH-121868 is a backport of this pull request to the 3.13 branch.

…ressions (pythonGH-121150) (cherry picked from commit c46d64e) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

…ns (GH-121150) (#121868) (cherry picked from commit c46d64e)

…ressions (pythonGH-121150) (cherry picked from commit c46d64e) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

bedevere-app · 2024-07-20T14:03:24Z

GH-122063 is a backport of this pull request to the 3.12 branch.

…ns (GH-121150) (#122063)

pythongh-121130: Fix f-string format specifiers with debug expressions

c371cc9

pablogsal requested a review from lysnikolaou as a code owner June 29, 2024 09:58

bedevere-app bot mentioned this pull request Jun 29, 2024

Self-documenting f-string in conversion specifier throws ValueError #121130

Closed

bedevere-app bot added the awaiting core review label Jun 29, 2024

Fix unparsing of fstring with format specs

89bd65a

pablogsal commented Jun 30, 2024

View reviewed changes

lysnikolaou reviewed Jul 1, 2024

View reviewed changes

pablogsal marked this pull request as draft July 1, 2024 15:16

bedevere-app bot removed the awaiting core review label Jul 1, 2024

Fixes

f4aed72

pablogsal marked this pull request as ready for review July 1, 2024 20:45

bedevere-app bot added the awaiting core review label Jul 1, 2024

Fixes

93ab4a6

Fixes

b97f4fd

pablogsal commented Jul 2, 2024

View reviewed changes

Lib/test/test_ast.py Show resolved Hide resolved

pablogsal commented Jul 2, 2024

View reviewed changes

pablogsal added 2 commits July 2, 2024 18:57

fix tests

3333a49

fix tests

899cf9a

pablogsal commented Jul 3, 2024

View reviewed changes

Parser/lexer/lexer.c Outdated Show resolved Hide resolved

Update Parser/lexer/lexer.c

4114e0c

lysnikolaou reviewed Jul 4, 2024

View reviewed changes

pablogsal and others added 2 commits July 4, 2024 15:32

Update Parser/lexer/lexer.c

88a0296

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>

Update action_helpers.c

e6e0f26

Co-authored-by: Lysandros Nikolaou <lisandrosnik@gmail.com>

pablogsal added needs backport to 3.13 bugs and security fixes needs backport to 3.12 only security fixes labels Jul 16, 2024

pablogsal merged commit c46d64e into python:main Jul 16, 2024

bedevere-app bot removed the awaiting core review label Jul 16, 2024

miss-islington-app bot assigned pablogsal Jul 16, 2024

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 16, 2024

pablogsal added a commit that referenced this pull request Jul 16, 2024

[3.13] gh-121130: Fix f-string format specifiers with debug expressio…

d24ec8a

…ns (GH-121150) (#121868) (cherry picked from commit c46d64e)

bedevere-app bot removed the needs backport to 3.12 only security fixes label Jul 20, 2024

pablogsal added a commit that referenced this pull request Jul 20, 2024

[3.12] gh-121130: Fix f-string format specifiers with debug expressio…

a9daa4f

…ns (GH-121150) (#122063)

Uh oh!

gh-121130: Fix f-string format specifiers with debug expressions #121150

gh-121130: Fix f-string format specifiers with debug expressions #121150

Uh oh!

Conversation

pablogsal commented Jun 29, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jun 30, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lysnikolaou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pablogsal commented Jul 1, 2024

Uh oh!

pablogsal commented Jul 1, 2024

Uh oh!

pablogsal commented Jul 1, 2024

Uh oh!

pablogsal commented Jul 1, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lysnikolaou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Jul 13, 2024

Uh oh!

pablogsal commented Jul 16, 2024

Uh oh!

miss-islington-app bot commented Jul 16, 2024

Uh oh!

miss-islington-app bot commented Jul 16, 2024

Uh oh!

miss-islington-app bot commented Jul 16, 2024

Uh oh!

bedevere-app bot commented Jul 16, 2024

Uh oh!

bedevere-app bot commented Jul 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

pablogsal commented Jun 29, 2024 •

edited by bedevere-app bot

Loading