bpo-16806: Fix lineno and col_offset for multi-line string tokens. #10021
Conversation
asottile force-pushed the bpo-16806 branch 3 times, most recently from 0957b8b to 9f8316c Compare 
asottile force-pushed the bpo-16806 branch 2 times, most recently from 0284c2a to 33f03d9 Compare 
Lib/test/test_fstring.py Outdated
There was a problem hiding this comment.
I suspect this was incorrect before (being adjusted by -1 due to the multiline string).
For instance, here's the ast dump of (using astpretty)
x = 1 + 1$ python ~/workspace/astpretty/astpretty.py t.py Module( body=[ Assign( lineno=1, col_offset=0, targets=[Name(lineno=1, col_offset=0, id='x', ctx=Store())], value=BinOp( lineno=1, col_offset=4, left=Num(lineno=1, col_offset=4, n=1), op=Add(), right=Num(lineno=1, col_offset=8, n=1), ), ), ], ) You'll notice that the BinOp and its left (Num) have the same col_offset
Parser/parsetok.c Outdated
There was a problem hiding this comment.
Just a question: after fixing this, when a < line_start possible?
There was a problem hiding this comment.
I would think it's not possible now, though I don't know of all the cases for every other token. I think tqs are the only multiline token (since iirc escaped newlines are not a token)
Python/ast.c Outdated
There was a problem hiding this comment.
I don't understand this function well.
int lines = LINENO(parent) - 1; int cols = parent->n_col_offset; These value are get from parent, before this loop:
/* Find the full fstring to fix location information in `n`. */ while (parent && parent->n_type != STRING) parent = parent->n_child; Is this OK?
There was a problem hiding this comment.
yeah I think so, what's happening here (and the variable names aren't super great):
- find the original ast offset of the parent node
- find the position in the string that the fstring expression appears at
- adjust the line/col based on that
From reading #1800 (where this function was introduced) it was discussed briefly but I didn't pick up the rationale for that loop there. When I removed / adjusted it during testing it broke all of the f-string column / offset tests so I think it's necessary.
| (I've rebased to regen importlib to resolve the conflict) |
| (I've rebased to regen importlib to resolve the conflict) |
| Diff and tests looks good to me. @ericvsmith would you review this? You seems reviewed #1800. |
| I'll take a look, but it might take me a while to get to it. Unfortunately I have a project that's due at the end of the year that's taking up all of my time. So while I'll try, you might not want to wait for me. I'm working on a fix to bpo-34364 that will completely back out the changes implemented in #1800. I'm not sure if that will interact with this change, though. |
| (I've rebased to regen importlib to resolve the conflict) Commands for me for next time since I expect this to not be the last: git fetch --all git checkout [bpo-16806](https://bugs.python.org/issue16806) git reset --hard asottile/[bpo-16806](https://bugs.python.org/issue16806) git pull --rebase origin master git checkout origin/master -- Python/importlib* # when rebase fails make regen-importlib git add Python git rebase --continue git push asottile HEAD -f |
| urgh, the bot edited my comment and broke my code block :( |
| (I've rebased to regen importlib to resolve the conflict) Please take a look, this is tedious -- I'd like to be done with this branch! |
| (I've rebased to regen importlib to resolve the conflict) Please take a look, this is tedious -- I'd like to be done with this branch! |
| happy new year! friendly ping :) |
| changes to the ast generally shouldn't be backported because they break linters / code formatters which make assumptions about how minor versions of python work in this particular case, this was discussed on bpo if you want more information |
| @asottile Sorry, should've read that better! Thanks! |
6 participants
I revived the patch listed in the linked bpo issue.
https://bugs.python.org/issue16806