bpo-28002: Roundtrip f-strings with ast.unparse better #19612

hauntsaninja · 2020-04-20T01:06:02Z

By attempting to avoid backslashes in f-string expressions.
We also now proactively raise errors for some backslashes we can't
avoid while unparsing FormattedValues

https://bugs.python.org/issue28002

Automerge-Triggered-By: GH:isidentical

hauntsaninja · 2020-04-20T01:14:49Z

Some notes:

I'm not particularly familiar with unicode intricacies, so it's possible there is a simpler, more robust or more performant way to do this.
Let me know if there are more tests that should be added.
Maybe we can skip-news, since ast.unparse is new in Python 3.9 anyway?
Part of this patch is based on @simonpercivall's work in astunparse. If there is credit-giving to be given, please let me know how he can be credited for that.

cc @isidentical

hauntsaninja · 2020-04-20T01:52:20Z

The test failure in test_concurrent_futures on Windows x86 seems unrelated to this PR

Lib/ast.py

hauntsaninja · 2020-04-21T22:50:09Z

One issue with the PR as it stands is the perhaps undesirable unparsing of the following, probably reasonably common f-string:

>>> import ast
>>> print(ast.unparse(ast.parse(r'''f"{x}\n" ''')))

f'''{x}
'''

For this to pass a src roundtrip, we'd need to process the buffer a little more intelligently when self.avoid_backslashes is False.

isidentical · 2020-05-20T00:27:11Z

@hauntsaninja would you rebase and (if you liked it) apply the suggestions. So we can move forward, and try to improve it. I have a script that I'll plan to test this PR against top PyPI packages, so we might find more problems / or verify that it works well at the end.

By attempting to avoid backslashes in f-string expressions. We also now proactively raise errors for some backslashes we can't avoid while unparsing FormattedValues

Co-authored-by: Batuhan Taskaya <isidentical@gmail.com>

Lib/test/test_unparse.py

Lib/ast.py

isidentical · 2020-05-22T00:45:13Z

FYI, this patch fails on roundtripping this file https://github.com/numpy/numpy/blob/master/tools/refguide_check.py

isidentical · 2020-05-22T00:48:21Z

What about not including the \n into the exceptions list in should_use_repr?

hauntsaninja · 2020-05-22T22:19:17Z

Thanks, updated to include your two comments!

I think including \n into the exception list is reasonable, and would fix the common f"{x}\n" case. There would be a regression in being able to unparse some of the test cases I added in test_fstrings_complicated, but those are less common, so it would still be the pragmatic thing to do.

Let me see if I can come up with a fix over the weekend, otherwise we can go ahead and make the should_use_repr change.

isidentical · 2020-06-01T12:05:28Z

A gentle ping @hauntsaninja

The previous cosmetic fix means we use triple quotes less often, so this is less likely to occur in practice, but should be fixed anyway.

hauntsaninja · 2020-06-03T02:01:15Z

Thanks for the reminder!

Made changes that should result in better cosmetic unparsing of cases like f"{x}\n" and fixed issues where we'd struggle because we might need to escape the final quote within triple quotes (basically more complicated versions of """\""""). These changes also don't regress against any existing test cases added in this PR.

hauntsaninja · 2020-10-18T20:17:47Z

@isidentical Thanks for taking another look! I made the changes as suggested, except for the ones I replied to

isidentical

@pablogsal would you mind checking out this?

isidentical · 2020-10-18T20:27:32Z

Thanks for the continuous work @hauntsaninja! If you have time, please try the latest version on the most downloaded PyPI packages set, if not please let me know (so I can test it by myself).

isidentical · 2020-10-18T20:29:31Z

Also, you should be able to collapse my reviews (mark them as resolved) so that it won't distract people when reading the history.

hauntsaninja · 2020-10-18T21:47:52Z

The CI failure here looks like a flake. I re-ran AST roundtripping over 4k packages and everything was clear (apart from some pre-existing RecursionErrors)

isidentical · 2020-11-12T19:01:52Z

A gentle ping @ericvsmith. Do you plan to take a look?

hauntsaninja · 2020-11-20T19:59:58Z

I'd love for this to make 3.9.1, if possible! :-)

isidentical · 2020-11-20T20:01:02Z

closing and re-opening for the CI

ericvsmith

Sorry for the delay. This looks reasonable enough to me, but someone who's more familiar with the AST and/or unparsing should take a look. Maybe @ambv?

isidentical · 2020-11-20T20:53:04Z

re-triggering the CI (some irrelevant macos failure)

bedevere-bot · 2020-11-20T21:16:45Z

@isidentical: Please replace # with GH- in the commit message next time. Thanks!

miss-islington · 2020-11-20T21:16:45Z

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

By attempting to avoid backslashes in f-string expressions. We also now proactively raise errors for some backslashes we can't avoid while unparsing FormattedValues Co-authored-by: hauntsaninja <> Co-authored-by: Shantanu <hauntsaninja@users.noreply.github.com> Co-authored-by: Batuhan Taskaya <isidentical@gmail.com> (cherry picked from commit a993e90) Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>

bedevere-bot · 2020-11-20T21:16:57Z

GH-23430 is a backport of this pull request to the 3.9 branch.

…-23430) By attempting to avoid backslashes in f-string expressions. We also now proactively raise errors for some backslashes we can't avoid while unparsing FormattedValues Co-authored-by: hauntsaninja <> Co-authored-by: Shantanu <hauntsaninja@users.noreply.github.com> Co-authored-by: Batuhan Taskaya <isidentical@gmail.com> (cherry picked from commit a993e90) Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com> Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>

isidentical · 2020-11-20T21:48:03Z

Thank you so much for your continuous efforts on this issue @hauntsaninja.

hauntsaninja · 2020-11-20T21:51:57Z

Thank you for making this PR happen! :-)

By attempting to avoid backslashes in f-string expressions. We also now proactively raise errors for some backslashes we can't avoid while unparsing FormattedValues Co-authored-by: hauntsaninja <> Co-authored-by: Shantanu <hauntsaninja@users.noreply.github.com> Co-authored-by: Batuhan Taskaya <isidentical@gmail.com>

the-knights-who-say-ni added the CLA signed label Apr 20, 2020

bedevere-bot added the awaiting review label Apr 20, 2020

hauntsaninja force-pushed the astunparse branch from 811343d to 66fee8a Compare April 20, 2020 01:28

isidentical added the skip news label Apr 20, 2020

isidentical reviewed Apr 20, 2020

View reviewed changes

Lib/ast.py Outdated Show resolved Hide resolved

isidentical requested a review from pablogsal April 20, 2020 10:20

csabella requested a review from ericvsmith April 23, 2020 12:04

hauntsaninja and others added 2 commits May 21, 2020 13:04

bpo-28002: Roundtrip f-strings with ast.unparse better

cc318ee

By attempting to avoid backslashes in f-string expressions. We also now proactively raise errors for some backslashes we can't avoid while unparsing FormattedValues

Update Lib/ast.py

4743d8d

Co-authored-by: Batuhan Taskaya <isidentical@gmail.com>

hauntsaninja force-pushed the astunparse branch from f8b5f19 to 4743d8d Compare May 21, 2020 20:05

isidentical approved these changes May 22, 2020

View reviewed changes

Lib/test/test_unparse.py Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels May 22, 2020

isidentical reviewed May 22, 2020

View reviewed changes

Lib/ast.py Outdated Show resolved Hide resolved

isidentical added the needs backport to 3.9 only security fixes label May 22, 2020

hauntsaninja added 2 commits May 22, 2020 15:07

_Unparser: make avoid_backslashes keyword only

98878ba

test_unparse: add test_fstring to run_always_files

a0a7b26

hauntsaninja added 2 commits June 2, 2020 18:08

unparse fstrings: improve cosmetics by escaping whitespace

0c95e56

unparse: add another test case

ac49be1

hauntsaninja marked this pull request as draft June 3, 2020 01:38

unparse: fix final quote escaping issues

714a269

The previous cosmetic fix means we use triple quotes less often, so this is less likely to occur in practice, but should be fixed anyway.

add to comment when falling back to repr

593b0d2

isidentical approved these changes Oct 18, 2020

View reviewed changes

swap the order of the iterations to save a little effort

fc6166f

isidentical closed this Nov 20, 2020

isidentical reopened this Nov 20, 2020

ericvsmith reviewed Nov 20, 2020

View reviewed changes

isidentical closed this Nov 20, 2020

isidentical reopened this Nov 20, 2020

isidentical added the 🤖 automerge label Nov 20, 2020

isidentical merged commit a993e90 into python:master Nov 20, 2020

bedevere-bot removed the awaiting core review label Nov 20, 2020

bedevere-bot removed the needs backport to 3.9 only security fixes label Nov 20, 2020

hauntsaninja deleted the astunparse branch November 20, 2020 21:21

isidentical mentioned this pull request Apr 30, 2022

Request for Python Triage membership: @hauntsaninja python/core-workflow#443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-28002: Roundtrip f-strings with ast.unparse better #19612

bpo-28002: Roundtrip f-strings with ast.unparse better #19612

hauntsaninja commented Apr 20, 2020 •

edited by miss-islington

Loading

hauntsaninja commented Apr 20, 2020 •

edited

Loading

hauntsaninja commented Apr 20, 2020

hauntsaninja commented Apr 21, 2020 •

edited

Loading

isidentical commented May 20, 2020 •

edited

Loading

isidentical commented May 22, 2020

isidentical commented May 22, 2020

hauntsaninja commented May 22, 2020

isidentical commented Jun 1, 2020

hauntsaninja commented Jun 3, 2020

hauntsaninja commented Oct 18, 2020

isidentical left a comment

isidentical commented Oct 18, 2020

isidentical commented Oct 18, 2020

hauntsaninja commented Oct 18, 2020 •

edited

Loading

isidentical commented Nov 12, 2020

hauntsaninja commented Nov 20, 2020 •

edited

Loading

isidentical commented Nov 20, 2020

ericvsmith left a comment

isidentical commented Nov 20, 2020

bedevere-bot commented Nov 20, 2020

miss-islington commented Nov 20, 2020

bedevere-bot commented Nov 20, 2020

isidentical commented Nov 20, 2020

hauntsaninja commented Nov 20, 2020

bpo-28002: Roundtrip f-strings with ast.unparse better #19612

bpo-28002: Roundtrip f-strings with ast.unparse better #19612

Conversation

hauntsaninja commented Apr 20, 2020 • edited by miss-islington Loading

hauntsaninja commented Apr 20, 2020 • edited Loading

hauntsaninja commented Apr 20, 2020

hauntsaninja commented Apr 21, 2020 • edited Loading

isidentical commented May 20, 2020 • edited Loading

isidentical commented May 22, 2020

isidentical commented May 22, 2020

hauntsaninja commented May 22, 2020

isidentical commented Jun 1, 2020

hauntsaninja commented Jun 3, 2020

hauntsaninja commented Oct 18, 2020

isidentical left a comment

Choose a reason for hiding this comment

isidentical commented Oct 18, 2020

isidentical commented Oct 18, 2020

hauntsaninja commented Oct 18, 2020 • edited Loading

isidentical commented Nov 12, 2020

hauntsaninja commented Nov 20, 2020 • edited Loading

isidentical commented Nov 20, 2020

ericvsmith left a comment

Choose a reason for hiding this comment

isidentical commented Nov 20, 2020

bedevere-bot commented Nov 20, 2020

miss-islington commented Nov 20, 2020

bedevere-bot commented Nov 20, 2020

isidentical commented Nov 20, 2020

hauntsaninja commented Nov 20, 2020

hauntsaninja commented Apr 20, 2020 •

edited by miss-islington

Loading

hauntsaninja commented Apr 20, 2020 •

edited

Loading

hauntsaninja commented Apr 21, 2020 •

edited

Loading

isidentical commented May 20, 2020 •

edited

Loading

hauntsaninja commented Oct 18, 2020 •

edited

Loading

hauntsaninja commented Nov 20, 2020 •

edited

Loading