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

Have -b warn when directly comparing ints and bytes #67869

Closed
brettcannon opened this issue Mar 16, 2015 · 10 comments
Closed

Have -b warn when directly comparing ints and bytes #67869

brettcannon opened this issue Mar 16, 2015 · 10 comments
Assignees
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-feature A feature request or enhancement

Comments

@brettcannon
Copy link
Member

BPO 23681
Nosy @brettcannon, @pfmoore, @vstinner, @larryhastings, @PCManticore, @berkerpeksag, @vadmium, @serhiy-storchaka
Files
  • bytes_to_int_compare.patch
  • bytes_to_int_compare_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/serhiy-storchaka'
    closed_at = <Date 2015-03-20.14:59:45.984>
    created_at = <Date 2015-03-16.17:05:30.088>
    labels = ['interpreter-core', 'type-feature', 'release-blocker']
    title = 'Have -b warn when directly comparing ints and bytes'
    updated_at = <Date 2015-03-20.14:59:45.983>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2015-03-20.14:59:45.983>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2015-03-20.14:59:45.984>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2015-03-16.17:05:30.088>
    creator = 'brett.cannon'
    dependencies = []
    files = ['38511', '38591']
    hgrepos = []
    issue_num = 23681
    keywords = ['patch']
    message_count = 10.0
    messages = ['238228', '238236', '238237', '238261', '238501', '238626', '238636', '238637', '238665', '238687']
    nosy_count = 9.0
    nosy_names = ['brett.cannon', 'paul.moore', 'vstinner', 'larry', 'Claudiu.Popa', 'python-dev', 'berker.peksag', 'martin.panter', 'serhiy.storchaka']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue23681'
    versions = ['Python 3.4', 'Python 3.5']

    @brettcannon
    Copy link
    Member Author

    To help writing Python 2/3 code the -b flag should switch on a warning when comparing an int to a bytes object in Python 2. This will help when someone writes something like b'abcd'[2] == b'c' and it always returns False thanks to the indexing returning 99 in Python 3.

    @brettcannon brettcannon self-assigned this Mar 16, 2015
    @brettcannon brettcannon added interpreter-core (Objects, Python, Grammar, and Parser dirs) type-feature A feature request or enhancement deferred-blocker release-blocker and removed deferred-blocker labels Mar 16, 2015
    @serhiy-storchaka
    Copy link
    Member

    6 tests failed:
    test_buffer test_poplib test_quopri test_smtpd test_sunau
    test_tokenize

    And all of them look as bugs.

    @serhiy-storchaka
    Copy link
    Member

    Here is a patch that adds required feature and fixes existing bugs in the stdlib and tests.

    No one of failed tests was false positive.

    @berkerpeksag
    Copy link
    Member

    [...] fixes existing bugs in the stdlib and tests.

    These changes should probably be backported to 3.4.

    @pfmoore
    Copy link
    Member

    pfmoore commented Mar 19, 2015

    LGTM, just one minor comment in review.

    @vstinner
    Copy link
    Member

    I didn't understand the issue.

    $ python2
    >>> b'A'[0] == 65
    False
    
    $ python3
    Python 3.4.1 (default, Nov  3 2014, 14:38:10) 
    >>> b'A'[0] == 65
    True

    Oh ok, now I get it: Python 2 and Python 3 behaves differently when comparing a string of a single byte and an integer.

    Ok to raise a warning when -b command line option is used.

    @serhiy-storchaka
    Copy link
    Member

    @brett: Yes, the patch handles 42 == b'*' as well.

    @victor: The problem in such code:

    x = b'Array'
    x[0] == b'A'

    Added explicit tests for bytes on the right side of the comparison and replaced b'.'[0] to ord(b'.').

    @vstinner
    Copy link
    Member

    This will help when someone writes something like b'abcd'[2] == b'c'

    What if someone writes line[-1] == 0 and line is a Unicode string? Should we emit a warning?

    I patched locally PyUnicode_RichCompare() to emit a warning. Hum, there are *many* warnings in argparse, http.client, and many other modules. I don't think that it's a good idea. It looks common to use a string for a sentinel (ex: state = "UNKNOWN") and then store an int (ex: state = 0). So comparison need to check the type (ex: isinstance(state, str) and state == "UNKNOWN") which is verbose and annoying.

    So no, we should not emit a warning :-)

    @brettcannon
    Copy link
    Member Author

    I did one final pass on the patch and only had wording comments, so tweak those and it LGTM. Assigned to Serhiy since it's obviously his patch. =)

    Only other thing is to either open up a separate bug or at least apply the fixes to the stdlib in Python 3.4 as well as 3.5.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 20, 2015

    New changeset 104c55bc2276 by Serhiy Storchaka in branch '3.4':
    Issue bpo-23681: Fixed Python 2 to 3 poring bugs.
    https://hg.python.org/cpython/rev/104c55bc2276

    New changeset 817f1f47824c by Serhiy Storchaka in branch 'default':
    Issue bpo-23681: Fixed Python 2 to 3 poring bugs.
    https://hg.python.org/cpython/rev/817f1f47824c

    New changeset 68e9cf0ae93d by Serhiy Storchaka in branch 'default':
    Issue bpo-23681: The -b option now affects comparisons of bytes with int.
    https://hg.python.org/cpython/rev/68e9cf0ae93d

    @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
    interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants