Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Left bracket remains in format string result when '\' preceeds it #73290

Closed
DimitrisJim mannequin opened this issue Dec 29, 2016 · 26 comments
Closed

Left bracket remains in format string result when '\' preceeds it #73290

DimitrisJim mannequin opened this issue Dec 29, 2016 · 26 comments
Assignees
Labels
3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@DimitrisJim
Copy link
Mannequin

DimitrisJim mannequin commented Dec 29, 2016

BPO 29104
Nosy @vstinner, @ericvsmith, @serhiy-storchaka, @refi64, @DimitrisJim
PRs
  • bpo-29104: Fixed parsing backslashes in f-strings. #490
  • [3.6] bpo-29104: Fixed parsing backslashes in f-strings. (GH-490) #1812
  • Files
  • fstring_backslash.patch
  • fstring_backslash_2.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ericvsmith'
    closed_at = <Date 2017-05-25.11:19:44.542>
    created_at = <Date 2016-12-29.11:56:25.482>
    labels = ['interpreter-core', 'type-bug', '3.7']
    title = "Left bracket remains in format string result when '\\' preceeds it"
    updated_at = <Date 2017-05-25.11:19:44.541>
    user = 'https://github.com/DimitrisJim'

    bugs.python.org fields:

    activity = <Date 2017-05-25.11:19:44.541>
    actor = 'serhiy.storchaka'
    assignee = 'eric.smith'
    closed = True
    closed_date = <Date 2017-05-25.11:19:44.542>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2016-12-29.11:56:25.482>
    creator = 'Jim Fasarakis-Hilliard'
    dependencies = []
    files = ['46080', '46127']
    hgrepos = []
    issue_num = 29104
    keywords = ['patch']
    message_count = 26.0
    messages = ['284249', '284256', '284257', '284258', '284259', '284260', '284261', '284262', '284266', '284271', '284558', '284578', '284579', '284580', '285949', '288511', '288979', '289645', '289689', '289690', '289694', '290733', '293604', '293608', '294458', '294469']
    nosy_count = 5.0
    nosy_names = ['vstinner', 'eric.smith', 'serhiy.storchaka', 'refi64', 'Jim Fasarakis-Hilliard']
    pr_nums = ['490', '1812']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29104'
    versions = ['Python 3.6', 'Python 3.7']

    @DimitrisJim
    Copy link
    Mannequin Author

    DimitrisJim mannequin commented Dec 29, 2016

    In short:

    >>> f"\{10}"
    

    yields:

    "\\{10"
    

    This is reproducible only when \ precedes the opening bracket, that is:

    >>> f"\ {10}"
    

    results in: "\\ 10"

    @DimitrisJim DimitrisJim mannequin added 3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Dec 29, 2016
    @SilentGhost SilentGhost mannequin added the type-bug An unexpected behavior, bug, or error label Dec 29, 2016
    @ericvsmith
    Copy link
    Member

    I'm not sure this counts as an error. The backslash means to treat the next character literally, which this does. And since \{ is not a valid escape sequence, it keeps both characters, exactly like:

    >>> '\c'
    '\\c'

    Furthermore, since unknown escape sequences are deprecated, this usage is going to be an error in the future. You can see this now with -Werror:

    $ python.exe  -Werror
    Python 3.6.0b1+ (3.6:87de1f12c41c+, Sep 16 2016, 07:05:57) [MSC v.1900 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> f'\{10}'
    DeprecationWarning: invalid escape sequence '\{'
    >>> '\c'
    DeprecationWarning: invalid escape sequence '\c'
    >>>

    Since this works as I expect it to now, and since it will become an error in the future, I don't think any change is warranted.

    @ericvsmith ericvsmith self-assigned this Dec 29, 2016
    @ericvsmith
    Copy link
    Member

    But I admit that dropping the trailing } seems odd. I think this particular usage should be an error: no bare } allowed, similar to:

    >>> f'10}'
      File "<stdin>", line 1
    SyntaxError: f-string: single '}' is not allowed

    @DimitrisJim
    Copy link
    Mannequin Author

    DimitrisJim mannequin commented Dec 29, 2016

    I see, the original "complaint" about this behavior on stack overflow was made due to the discrepancy between the f-string the the format "equivalent": '\{}'.format(10)'.

    @ericvsmith
    Copy link
    Member

    Do you have a link to the SO question?

    @DimitrisJim
    Copy link
    Mannequin Author

    DimitrisJim mannequin commented Dec 29, 2016

    @serhiy-storchaka
    Copy link
    Member

    The problem is not that the trailing } is dropped, but that the starting { starts an f-string expression.

    >>> f'\{2*5}'
    '\\{10'

    I expected either '\\10' as in '\{}'.format(2*5), or at least '\\{2*5}'.

    There is other f-string parsing error:

    >>> f'\\N{2*5}'
    '\\N{2*5}'

    I expected '\\N10'. '\\N' doesn't start a unicode name escape. This is a legitimate expression.

    @ericvsmith
    Copy link
    Member

    Yes:

    >>> f'\{2*5}'
    '\\{10'

    I agree '\\10' would make sense.

    @ericvsmith
    Copy link
    Member

    This problem was no doubt introduced after 3.6a1 when I changed the parsing to disallow backslashes inside {}.

    @serhiy-storchaka
    Copy link
    Member

    Proposed patch fixes parsing backslashes in f-strings.

    P.S. Definitely we should find other name for this term. I hate misleading "f-string".

    @vstinner
    Copy link
    Member

    vstinner commented Jan 3, 2017

    The PEP-498 should be updated to define the expected behaviour of f'\{5}':
    https://www.python.org/dev/peps/pep-0498/#escape-sequences

    + self.assertEqual(f'\{6*7}', '\\42')
    + self.assertEqual(f'\\{6*7}', '\\42')
    + self.assertEqual(fr'\{6*7}', '\\42')
    +
    + AMPERSAND = 123
    + self.assertEqual(f'\N{AMPERSAND}', '&')
    + self.assertEqual(f'\\N{AMPERSAND}', '\\N123')
    + self.assertEqual(fr'\N{AMPERSAND}', '\\N123')
    + self.assertEqual(f'\\\N{AMPERSAND}', '\\&')

    I'm not sure that I like this behaviour.

    f'\\N{AMPERSAND}': reading a local variable looks like a typo or a security vulnerability, rather than a nice feature.

    IMHO if you want an anti-slash (\) in the output string, it *must* be written "\\" since we *do* interpret other escape sequences:

    • f'\n' returns '\n', the newline character U+000A
    • f'\N{Snowman}' returns the nice snowman character U+2603 (☃)
    • etc.

    What is the issue with having to write "\\{10}" to get "\\10" string? It's less error prone.

    @serhiy-storchaka
    Copy link
    Member

    f'\\N{AMPERSAND}': reading a local variable looks like a typo or a security vulnerability, rather than a nice feature.

    This can look as a typo, but how would you write this if this is not a typo? f'\\N{AMPERSAND}' is legitimate syntax, and it would be weird to interpret this expression in any other way than as '\\' + 'N' + format(AMPERSAND).

    What is the issue with having to write "\\{10}" to get "\\10" string? It's less error prone.

    There is no issue with f'\\{10}'. There is an issue with f'\{10}'.

    @vstinner
    Copy link
    Member

    vstinner commented Jan 3, 2017

    There is no issue with f'\\{10}'. There is an issue with f'\{10}'.

    Hum, I'm not sure that my previous comment is explicit: I suggest to raise a syntax error on f'\{10}' (or emit a warning, and raise an error in 3.7 if you insist ;-)).

    @serhiy-storchaka
    Copy link
    Member

    Updated patch adds a warning.

    @serhiy-storchaka
    Copy link
    Member

    Ping.

    @DimitrisJim
    Copy link
    Mannequin Author

    DimitrisJim mannequin commented Feb 24, 2017

    Serhiy, since review activity has dropped on b.p.o now that the move to github was made, would you like to make this into a pull request to get it reviewed and merged faster?

    @serhiy-storchaka
    Copy link
    Member

    I am not experienced with git and am waiting until new workflow be described in the devguide.

    @ericvsmith
    Copy link
    Member

    Serhiy: I plan to find time in the next week or so to look at this. Sorry for the delay. Plus, the new workflow isn't helping me out: I need to get up to speed on it.

    @refi64
    Copy link
    Mannequin

    refi64 mannequin commented Mar 15, 2017

    Serhiy: if you want, you can give me your email and I'll submit a PR for you. If you want to do it yourself, just:

    Download hub: https://hub.github.com/

    git clone python/cpython cpython-git
    cd cpython-git
    git fork
    curl https://raw.githubusercontent.com/mozilla/moz-git-tools/master/hg-patch-to-git-patch > hg-patch-to-git-patch
    curl https://bugs.python.org/file46127/fstring_backslash_2.patch | python2 hg-patch-to-git-patch > fstring.patch
    git checkout -b fstring-fix
    git apply fstring.patch
    git commit -am 'bpo-29104: Commit message here'
    git push -u MY_GITHUB_USERNAME fstring-fix
    git pull-request

    @DimitrisJim
    Copy link
    Mannequin Author

    DimitrisJim mannequin commented Mar 15, 2017

    A PR has been submitted, Ryan. See #490

    @serhiy-storchaka
    Copy link
    Member

    Thank you Ryan. I already created a PR, but I think your recipe can be helpful for me in future.

    @serhiy-storchaka
    Copy link
    Member

    Ping again.

    @serhiy-storchaka
    Copy link
    Member

    Eric?

    @ericvsmith
    Copy link
    Member

    This is on my list of things to review at PyCon, when I should finally have some free time.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 0cd7a3f by Serhiy Storchaka in branch 'master':
    bpo-29104: Fixed parsing backslashes in f-strings. (#490)
    0cd7a3f

    @serhiy-storchaka
    Copy link
    Member

    New changeset 89a3102 by Serhiy Storchaka in branch '3.6':
    [3.6] bpo-29104: Fixed parsing backslashes in f-strings. (GH-490) (bpo-1812)
    89a3102

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants