bpo-30465: Fix lineno and col_offset in fstring AST nodes #1800

ambv · 2017-05-24T22:57:04Z

f-strings are computed in a separate compiler step. This makes their lineno and col_offset information wrong. This is problematic for flake8 which reports problems inside f-strings on the wrong line (typically the first one in the file).

Attached patch fixes the issue.

https://bugs.python.org/issue30465

markshannon · 2017-05-24T23:12:57Z

Is this intended to handle multi-line and nested f-strings?

ambv · 2017-05-24T23:13:57Z

I'll add more tests to show what works and what doesn't.

ambv · 2017-05-25T02:29:34Z

@markshannon, nested f-strings work correctly, multiline f-strings partially work. Location information about expressions inside FormattedValues is correct. Location information for enclosing nodes (JoinedStr, Str, and FormattedValue itself) is invalid due to bpo-16806. This is not fstring specific. It's a multiline string bug. I'll fix that in a separate patch.

ambv · 2017-05-30T17:01:53Z

@serhiy-storchaka @ericvsmith @markshannon I'd appreciate a review here, I'd like this to go into 3.6.2 and the cut off date is looming.

serhiy-storchaka · 2017-05-30T17:37:28Z

Python/ast.c

+   `n` is the node which locations are going to be fixed relative to parent.
+   `expr_str` is the child node's string representation, incuding braces.
+*/
+static void fstring_fix_node_location(const node *parent, node *n,


For conforming PEP 7 this should be:

static void fstring_fix_node_location(...

serhiy-storchaka · 2017-05-30T17:52:54Z

Python/ast.c

@@ -4327,14 +4335,73 @@ fstring_compile_expr(const char *expr_start, const char *expr_end,
    str[len+2] = 0;

    cf.cf_flags = PyCF_ONLY_AST;
-    mod = PyParser_ASTFromString(str, "<fstring>",
-                                 Py_eval_input, &cf, c->c_arena);
+    mod_n = PyParser_SimpleParseStringFlagsFilename(str, "<fstring>",


Mentioning PyParser_ASTFromString() in the comment above no longer valid.

Good catch, will fix.

serhiy-storchaka · 2017-05-30T18:14:38Z

Python/ast.c

+    while (parent && parent->n_type != STRING)
+        parent = parent->n_child;
+    if (parent && parent->n_str) {
+        substr = strstr(parent->n_str, expr_str);


What if the f-string contains several equivalent subexpressions?

Then we'll report line number and column information for the first occurence in all occurences. This is not currently solvable without tracking already applied occurences, etc. I don't think it's worth fixing, that would complicate this patch.

If we ever redo f-strings to be untokenized properly on first pass, then this will work as expected. The hack to do a separate pass on formatted values is the reason why this entire workaround is necessary in the first place.

Don't expr_start/expr_end give a position of a subexpression in the string?

I tried that. Unfortunately, you'd need to pass the entire string as a new argument through all the fstring_* functions to make this work. Even then, the full string doesn't have the string prefix or the leading quote. That can be fixed, but even then, the full string is different from expr_start if compile-time concatenation is used.

In other words, this edge case requires an inordinate amount of work to get right. Let's treat this as a separate improvement in the future?

I agree that it's not worth complicating the implementation. Especially because the largest use case is to get error reporting working, and I don't see how the error could be on a second instance of the exact same expression.

serhiy-storchaka · 2017-05-30T18:29:44Z

Python/ast.c

+{
+    n->n_col_offset = n->n_col_offset + col_offset;
+    for (int i = 0; i < NCH(n); ++i) {
+        if (n->n_lineno && n->n_lineno < CHILD(n, i)->n_lineno) {


When n->n_lineno is zero?

Some nodes don't contain col_offset and lineno information at all. I don't recall now which ones those are but the tests were failing without this additional check.

Should n->n_col_offset (above) and n->n_lineno (below) be updated for these nodes? They become non-zero, but does the new value make sense?

n->n_lineno is zero for eval_input nodes only. Interestingly, n->n_col_offset is uninitialized then (holds value -875836469 which is 0xCBCBCBCB..., one of the magic byte patterns debug PyMalloc initializes fresh memory, too). So, yes, I think setting new values is better than leaving garbage coordinates.

ericvsmith

I don't have anything to add beyond Serhiy's comments.

serhiy-storchaka · 2017-05-31T19:01:40Z

Python/ast.c

@@ -4265,6 +4266,12 @@ decode_bytes_with_escapes(struct compiling *c, const node *n, const char *s,
    return result;
 }

+/* Forward declarations for recursive functions. */


Yet a note. I don't see a recursion except a self-recursion in fstring_shift_node_locations(). If change the order of function definitions, forward declarations will become not needed.

ambv · 2017-09-06T18:17:59Z

I applied all suggestions by @serhiy-storchaka.

@ericvsmith, this is ready for merging. What this doesn't fix is bpo-31140 since that syntax errors are raised during compilation, not after (when the fixes implemented in this PR are applied).

ericvsmith

Other than the one suggestion to add a new test and the (unneeded?) forward declaration, this looks good to me. Thanks!

ericvsmith · 2017-09-06T22:05:56Z

Lib/test/test_fstring.py

@@ -70,6 +70,253 @@ def __call__(self):
        # Make sure x was called.
        self.assertTrue(x.called)



How about some test that checks the output of eval()? This code doesn't work as-is, because the NameError doesn't include the line number. But I assume there's a way to capture it and inspect it.

I suggest this because this is the behavior that the end user really cares about.

def test_error_line_numbers(self): expr = '''f""" some text other text {x} """''' with self.assertRaisesRegex(NameError, ', line 5,'): eval(expr)

Wait, never mind. I was conflating this with bpo-31140.

ericvsmith · 2017-09-06T22:10:08Z

Python/ast.c

+    while (parent && parent->n_type != STRING)
+        parent = parent->n_child;
+    if (parent && parent->n_str) {
+        substr = strstr(parent->n_str, expr_str);


I agree that it's not worth complicating the implementation. Especially because the largest use case is to get error reporting working, and I don't see how the error could be on a second instance of the exact same expression.

ericvsmith · 2017-09-06T22:12:11Z

Python/ast.c

@@ -4270,6 +4271,58 @@ decode_bytes_with_escapes(struct compiling *c, const node *n, const char *s,
    return result;
 }

+/* Forward declaration for a recursive function. */
+static void fstring_shift_node_locations(node *n, int lineno, int col_offset);


This line isn't needed, is it? You define the function immediately after the forward declaration.

bedevere-bot · 2017-09-06T22:21:15Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ambv · 2017-09-06T22:52:06Z

Alright, unnecessary forward declaration removed.

miss-islington · 2017-09-07T00:28:01Z

🐍🍒⛏🤖 Thanks @ambv for the PR, and @ericvsmith for merging it 🌮🎉.I'm working now to backport this PR to: 3.6.

…onGH-1800) For f-string ast nodes, fix the line and columns so that tools such as flake8 can identify them correctly. (cherry picked from commit e7c566c)

bedevere-bot · 2017-09-07T00:28:09Z

GH-3409 is a backport of this pull request to the 3.6 branch.

…h-3409) For f-string ast nodes, fix the line and columns so that tools such as flake8 can identify them correctly. (cherry picked from commit e7c566c)

the-knights-who-say-ni added the CLA signed label May 24, 2017

ambv force-pushed the fstring_lineno branch 2 times, most recently from 4c76276 to d2dc17c Compare May 25, 2017 02:27

ambv requested a review from ericvsmith May 25, 2017 02:33

ambv added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels May 25, 2017

serhiy-storchaka self-requested a review May 25, 2017 11:03

serhiy-storchaka reviewed May 30, 2017

View reviewed changes

ericvsmith reviewed May 30, 2017

View reviewed changes

serhiy-storchaka reviewed May 31, 2017

View reviewed changes

ambv mentioned this pull request Jun 16, 2017

Wrong reported location for undefined references in f-strings python/mypy#3562

Closed

ambv force-pushed the fstring_lineno branch 3 times, most recently from a50756c to e78be7e Compare September 6, 2017 17:51

ericvsmith requested changes Sep 6, 2017

View reviewed changes

bedevere-bot added the awaiting changes label Sep 6, 2017

bpo-30465: Fix lineno and col_offset in fstring AST nodes

412b119

ambv force-pushed the fstring_lineno branch from e78be7e to 412b119 Compare September 6, 2017 22:51

ericvsmith approved these changes Sep 6, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Sep 6, 2017

ericvsmith merged commit e7c566c into python:master Sep 7, 2017

bedevere-bot removed the needs backport to 3.6 label Sep 7, 2017

gvanrossum mentioned this pull request Sep 15, 2017

Copy f-string line/col fix from CPython python/typed_ast#56

Closed

Mariatta removed the awaiting merge label Oct 8, 2017

asottile mentioned this pull request Oct 21, 2018

bpo-16806: Fix lineno and col_offset for multi-line string tokens. #10021

Merged

ambv deleted the fstring_lineno branch July 12, 2021 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-30465: Fix lineno and col_offset in fstring AST nodes #1800

bpo-30465: Fix lineno and col_offset in fstring AST nodes #1800

ambv commented May 24, 2017 •

edited by bedevere-bot

Loading

markshannon commented May 24, 2017

ambv commented May 24, 2017

ambv commented May 25, 2017

ambv commented May 30, 2017

serhiy-storchaka May 30, 2017

ambv Jun 1, 2017

serhiy-storchaka May 30, 2017

ambv Jun 1, 2017

serhiy-storchaka May 30, 2017

ambv Jun 1, 2017

serhiy-storchaka Jun 6, 2017

ambv Sep 6, 2017

ericvsmith Sep 6, 2017

serhiy-storchaka May 30, 2017

ambv Jun 1, 2017

serhiy-storchaka Jun 5, 2017

ambv Sep 5, 2017

ericvsmith left a comment

serhiy-storchaka May 31, 2017

ambv commented Sep 6, 2017

ericvsmith left a comment

ericvsmith Sep 6, 2017

ericvsmith Sep 6, 2017

ericvsmith Sep 6, 2017

ericvsmith Sep 6, 2017

bedevere-bot commented Sep 6, 2017

ambv commented Sep 6, 2017

miss-islington commented Sep 7, 2017

bedevere-bot commented Sep 7, 2017

		@@ -70,6 +70,253 @@ def __call__(self):
		# Make sure x was called.
		self.assertTrue(x.called)

bpo-30465: Fix lineno and col_offset in fstring AST nodes #1800

bpo-30465: Fix lineno and col_offset in fstring AST nodes #1800

Conversation

ambv commented May 24, 2017 • edited by bedevere-bot Loading

markshannon commented May 24, 2017

ambv commented May 24, 2017

ambv commented May 25, 2017

ambv commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericvsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambv commented Sep 6, 2017

ericvsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 6, 2017

ambv commented Sep 6, 2017

miss-islington commented Sep 7, 2017

bedevere-bot commented Sep 7, 2017

ambv commented May 24, 2017 •

edited by bedevere-bot

Loading