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

codecs.lookup() ignores non-ASCII characters, whereas encodings.normalize_encoding() copies them #83518

Closed
vstinner opened this issue Jan 14, 2020 · 11 comments
Labels
3.10 stdlib

Comments

@vstinner
Copy link
Member

@vstinner vstinner commented Jan 14, 2020

BPO 39337
Nosy @malemburg, @vstinner, @serhiy-storchaka, @shihai1991
PRs
  • #18845
  • #18987
  • #19069
  • #22219
  • #22360
  • #17997
  • 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 2020-10-14.15:46:55.467>
    created_at = <Date 2020-01-14.21:54:42.124>
    labels = ['library', '3.10']
    title = 'codecs.lookup() ignores non-ASCII characters, whereas encodings.normalize_encoding() copies them'
    updated_at = <Date 2020-10-14.15:55:50.301>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2020-10-14.15:55:50.301>
    actor = 'shihai1991'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-10-14.15:46:55.467>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2020-01-14.21:54:42.124>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 39337
    keywords = ['patch']
    message_count = 11.0
    messages = ['360004', '363645', '364180', '364449', '364452', '364456', '377772', '378281', '378626', '378628', '378630']
    nosy_count = 4.0
    nosy_names = ['lemburg', 'vstinner', 'serhiy.storchaka', 'shihai1991']
    pr_nums = ['18845', '18987', '19069', '22219', '22360', '17997']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue39337'
    versions = ['Python 3.10']

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 14, 2020

    bpo-37751 changed codecs.lookup() in a subtle way: non-ASCII characters are now ignored, whereas they were copied unmodified previously.

    I would prefer that codecs.lookup() and encodings.normalize_encoding() behave the same. Either always ignore or always copy.

    Moreover, it seems like there is no test on how the encoding names are normalized in codecs.register(). I recall that using codecs.register() in an unit test causes troubles since there is no API to unregister a search function. Maybe we should just add a private function for test in _testcapi.

    Serhiy Storchaka wrote an example on my PR:
    https://github.com/python/cpython/pull/17997/files

    There are other differences. For example, normalize_encoding("КОИ-8") returns "кои_8", but codecs.lookup normalizes it to "8".

    The comment in the sources is also not correct.

    @vstinner vstinner added 3.9 stdlib labels Jan 14, 2020
    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Mar 8, 2020

    I would prefer that codecs.lookup() and encodings.normalize_encoding() behave the same. Either always ignore or always copy.

    How about calling encodings.normalize_encoding() in codecs.normalizestring() to keep same behavior?(I create PR18845)

    Maybe we should just add a private function for test in _testcapi

    I can try to add some test cases in next weekend ;)

    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Mar 14, 2020

    How about calling encodings.normalize_encoding() in codecs.normalizestring() to keep same behavior?(I create PR18845)

    I have try this idea, but it make the testcase of test_io.py failed because some object will call codecs.Lookup() in __del__().-->extension module will be cleaned before calling __del__().

    I would prefer that codecs.lookup() and encodings.normalize_encoding() behave the same. Either always ignore or always copy.

    I try to add a _Py_normalize_unicode_encoding() in unicodeobject.c to support non-ASCII encoding names' normalization(PR18987), but this PR caused many testcases failed.

    For example:

    In master:
    python3.9 -c "print('a\xac\u1234\u20ac\u8000\U0010ffff'.encode('iso-8859-15', 'namereplace'))"
    result:
    b'a\xac\\N{ETHIOPIC SYLLABLE SEE}\xa4\\N{CJK UNIFIED IDEOGRAPH-8000}\\U0010ffff'

    after PR18987:
    ./python -c "print('a\xac\u1234\u20ac\u8000\U0010ffff'.encode('iso-8859-15', 'namereplace'))"
    result:
    b'a\xac\\N{ETHIOPIC SYLLABLE SEE}\\N{EURO SIGN}\\N{CJK UNIFIED IDEOGRAPH-8000}\\U0010ffff'

    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Mar 17, 2020

    Maybe we should just add a private function for test in _testcapi.

    Oh, there have a problem with this idea:
    struct _is is defined in internal/pycore_pystate.h.
    _testcapi must test the public Python C API, not CPython internal C API

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Mar 17, 2020

    _testcapi must test the public Python C API, not CPython internal C API

    Use _testinternalcapi in this case.

    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Mar 17, 2020

    Use _testinternalcapi in this case.
    oh, forgive me. I don't atttention this extension module before :(

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Oct 1, 2020

    See also bpo-37751 and my PR 17997.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Oct 8, 2020

    New changeset 3f34237 by Hai Shi in branch 'master':
    bpo-39337: Add a test case for normalizing of codec names (GH-19069)
    3f34237

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Oct 14, 2020

    New changeset c5b049b by Hai Shi in branch 'master':
    bpo-39337: encodings.normalize_encoding() now ignores non-ASCII characters (GH-22219)
    c5b049b

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Oct 14, 2020

    Thanks Hai Shi for the change and for new codecs and encodings tests.

    @vstinner vstinner added 3.10 and removed 3.9 labels Oct 14, 2020
    @vstinner vstinner added 3.10 and removed 3.9 labels Oct 14, 2020
    @shihai1991
    Copy link
    Member

    @shihai1991 shihai1991 commented Oct 14, 2020

    Thanks for everyone's continus review :)

    @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.10 stdlib
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants