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

Tools/parser/unparse.py does not handle u"bar" correctly #81234

Closed
yan12125 mannequin opened this issue May 26, 2019 · 6 comments
Closed

Tools/parser/unparse.py does not handle u"bar" correctly #81234

yan12125 mannequin opened this issue May 26, 2019 · 6 comments
Labels
3.8 only security fixes easy type-feature A feature request or enhancement

Comments

@yan12125
Copy link
Mannequin

yan12125 mannequin commented May 26, 2019

BPO 37053
Nosy @gvanrossum, @ilevkivskyi, @yan12125, @pablogsal, @miss-islington
PRs
  • bpo-37053: handle strings like u"bar" correctly in Tools/parser/unparse.py #13583
  • 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 = None
    closed_at = <Date 2019-05-26.17:10:39.571>
    created_at = <Date 2019-05-26.12:41:33.312>
    labels = ['easy', 'type-feature', '3.8']
    title = 'Tools/parser/unparse.py does not handle u"bar" correctly'
    updated_at = <Date 2019-05-26.17:10:39.570>
    user = 'https://github.com/yan12125'

    bugs.python.org fields:

    activity = <Date 2019-05-26.17:10:39.570>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-05-26.17:10:39.571>
    closer = 'pablogsal'
    components = ['Demos and Tools']
    creation = <Date 2019-05-26.12:41:33.312>
    creator = 'yan12125'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37053
    keywords = ['patch', 'easy']
    message_count = 6.0
    messages = ['343546', '343554', '343560', '343566', '343567', '343568']
    nosy_count = 5.0
    nosy_names = ['gvanrossum', 'levkivskyi', 'yan12125', 'pablogsal', 'miss-islington']
    pr_nums = ['13583']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue37053'
    versions = ['Python 3.8']

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented May 26, 2019

    Currently (135c6a5) the following test fails:

    ./python -m test.regrtest -v test_tools.test_unparse -u cpu

    (Note that -u cpu is needed to force test_unparse.DirectoryTestCase to check all files under Lib/ and Lib/test/.)

    An example error message:

    ======================================================================
    FAIL: test_files (test.test_tools.test_unparse.DirectoryTestCase) (filename='/home/yen/Projects/cpython/Lib/test/test_typing.py'
    )
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/yen/Projects/cpython/Lib/test/test_tools/test_unparse.py", line 309, in test_files
        self.check_roundtrip(source)
      File "/home/yen/Projects/cpython/Lib/test/test_tools/test_unparse.py", line 132, in check_roundtrip
        self.assertASTEqual(ast1, ast2)
      File "/home/yen/Projects/cpython/Lib/test/test_tools/test_unparse.py", line 124, in assertASTEqual
        self.assertEqual(ast.dump(ast1), ast.dump(ast2))
    AssertionError: 'Modu[88178 chars]kind=\'u\')], ctx=Load())), ctx=Load()))], dec[421987 chars]=[])' != 'Modu[88178 chars]kind=No
    ne)], ctx=Load())), ctx=Load()))], deco[421986 chars]=[])'

    Apparently that's because Tools/parser/unparse.py does not handle strings like u"bar" correctly. I managed to "fix" it with the following patch:

    diff --git a/Tools/parser/unparse.py b/Tools/parser/unparse.py
    index 385902ef4b..a25b5e49df 100644
    --- a/Tools/parser/unparse.py
    +++ b/Tools/parser/unparse.py
    @@ -399,6 +399,8 @@ class Unparser:
             elif value is ...:
                 self.write("...")
             else:
    +            if t.kind == 'u':
    +                self.write("u")
                 self._write_constant(t.value)
         def _List(self, t):

    Not sure if this is the correct approach, though.

    @yan12125 yan12125 mannequin added type-bug An unexpected behavior, bug, or error 3.8 only security fixes labels May 26, 2019
    @serhiy-storchaka serhiy-storchaka added easy type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels May 26, 2019
    @ilevkivskyi
    Copy link
    Member

    On the other hand, if this causes troubles, feel free to just replace u"bar" with "bar" in that test. It is not really important.

    @gvanrossum
    Copy link
    Member

    Please submit a PR.

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented May 26, 2019

    After reading Python/ast.c, I think my patch is correct, so I submitted it to GitHub as #13583 :)

    @miss-islington
    Copy link
    Contributor

    New changeset aaf47ca by Miss Islington (bot) (Chih-Hsuan Yen) in branch 'master':
    bpo-37053: handle strings like u"bar" correctly in Tools/parser/unparse.py (GH-13583)
    aaf47ca

    @pablogsal
    Copy link
    Member

    Confirmed that this fixes the broken buildbots:

    ./python -m test test_tools -vvvv -uall -m test_files
    == CPython 3.8.0a4+ (heads/master:91f4380ced, May 26 2019, 17:30:21) [GCC 8.3.0]
    == Linux-5.0.15-1-MANJARO-x86_64-with-glibc2.29 little-endian
    == cwd: /home/pablogsal/github/cpython/build/test_python_25649
    == CPU count: 12
    == encodings: locale=UTF-8, FS=utf-8
    Run tests sequentially
    0:00:00 load avg: 0.50 [1/1] test_tools
    test_files (test.test_tools.test_unparse.DirectoryTestCase) ... Testing /home/pablogsal/github/cpython/Lib/typing.py
    Testing /home/pablogsal/github/cpython/Lib/test/test_typing.py
    ok

    Ran 1 test in 0.586s

    OK

    == Tests result: SUCCESS ==

    1 test OK.

    Total duration: 671 ms
    Tests result: SUCCESS

    The manual check is needed because the CI in GitHub does not run test_tools over all files so it does not take forever.

    @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.8 only security fixes easy type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants