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.

Sign up for GitHub

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

Jump to bottom

Coercing strings and non-integer numbers to interpreter ID and channel ID #82186

Closed
serhiy-storchaka opened this issue Sep 2, 2019 · 12 comments
Closed

Coercing strings and non-integer numbers to interpreter ID and channel ID #82186

serhiy-storchaka opened this issue Sep 2, 2019 · 12 comments
Labels
3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@serhiy-storchaka
Copy link
Member

BPO 38005
Nosy @terryjreedy, @mdickinson, @ericsnowcurrently, @serhiy-storchaka, @miss-islington
PRs
  • bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. #15652
  • [3.8] bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. (GH-15652) #16129
  • [3.8] bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. (GH-15652) #16145
  • bpo-38005: Remove support of string argument in InterpreterID(). #16227
  • [3.8] bpo-38005: Remove support of string argument in InterpreterID(). (GH-16227) #16394
    		•

    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-09-14.16:40:54.346>
created_at = <Date 2019-09-02.09:11:14.881>
labels = ['interpreter-core', '3.8', '3.9', 'type-crash']
title = 'Coercing strings and non-integer numbers to interpreter ID and channel ID'
updated_at = <Date 2019-09-25.15:56:08.382>
user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2019-09-25.15:56:08.382>
actor = 'miss-islington'
assignee = 'none'
closed = True
closed_date = <Date 2019-09-14.16:40:54.346>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2019-09-02.09:11:14.881>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 38005
keywords = ['patch']
message_count = 12.0
messages = ['350972', '350984', '351025', '351274', '351322', '351326', '352390', '352442', '352443', '352575', '353215', '353218']
nosy_count = 5.0
nosy_names = ['terry.reedy', 'mark.dickinson', 'eric.snow', 'serhiy.storchaka', 'miss-islington']
pr_nums = ['15652', '16129', '16145', '16227', '16394']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue38005'
versions = ['Python 3.8', 'Python 3.9']
    @serhiy-storchaka
    Copy link
    Member Author

    Is it correct that strings, bytes objects, bytearrays, floats, fractions, decimals can be coerced to an interpreter ID?

    >>> import _xxsubinterpreters as interpreters
>>> id = interpreters.InterpreterID('10', force=True)
>>> id == 10
True
>>> id == 10.1
True
>>> id == '1_0'
True
>>> id == bytearray(b'10')
True

    Should not the constructor accept only ints and int-like objects and the comparison return True only for integer numbers?

    There are other bugs here: raising OverflowError on comparison with large numbers and silencing all errors in some circumstances that can lead to returning a wrong result when errors like KeyboardInterrupt, RecursionError, MemoryError are occurred.

    @mdickinson
    Copy link
    Member

    Urk! Having 10 == id == 10.1 should imply 10 == 10.1, by transitivity.

    Are interpreter.InterpreterID objects hashable? If so, we've got a violation of the rule that x == y implies hash(x) == hash(y).

    @serhiy-storchaka serhiy-storchaka changed the title Coercing strings and non-integer numbers to interpreter ID Coercing strings and non-integer numbers to interpreter ID and channel ID Sep 2, 2019
    @serhiy-storchaka
    Copy link
    Member Author

    PR 15652 fixes some bugs. But I am not sure that floats and strings (and bytearrays, and fractions) should be accepted in constructors and several other functions. Although there was a test for strings. What is a use case? Should not an ID be an integer?

    @serhiy-storchaka serhiy-storchaka added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.8 only security fixes 3.9 only security fixes type-bug An unexpected behavior, bug, or error labels Sep 2, 2019
    @terryjreedy
    Copy link
    Member

    Regardless of the intention of the _xx... author, intransitive equality is objectionable as it violates the assumption of sets and dicts. We went through this before with decimal.Decimal when the original implementation had 1.0 == 1 == Decimal(1) but 1.0 != Decimal(1). The latter was fixed. Here, I think the fix should be to not make the id be equal to everything it can be derived from.

    @serhiy-storchaka
    Copy link
    Member Author

    I have doubts about accepting floats and bytes objects in all function that accept InterpreterID or ChannelID. Is it intentional? There are tests for strings.

    @serhiy-storchaka
    Copy link
    Member Author

    There is more serious issue: InterpreterID and ChannelID are declared as int subclasses, but actually they have incompatible structure, so using them as int causes a crash. For example, the following code is crashed:

    float(int(id)) == id

    @serhiy-storchaka serhiy-storchaka added type-crash A hard crash of the interpreter, possibly with a core dump and removed type-bug An unexpected behavior, bug, or error labels Sep 8, 2019
    @serhiy-storchaka
    Copy link
    Member Author

    New changeset bf16991 by Serhiy Storchaka in branch 'master':
    bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. (GH-15652)
    bf16991

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset f37a983 by Serhiy Storchaka in branch '3.8':
    [3.8] bpo-38005: Fixed comparing and creating of InterpreterID and ChannelID. (GH-15652) (GH-16145)
    f37a983

    @serhiy-storchaka
    Copy link
    Member Author

    I would remove also converting a string to InterpreterID. The code would be simpler without it. I left it because there was explicit tests for it, but what is a use case for InterpreterID(string)?

    @ericsnowcurrently
    Copy link
    Member

    Yeah, dropping str support is fine. It wouldn't be hard to add it back in if later we find it's useful. :)

    @serhiy-storchaka
    Copy link
    Member Author

    New changeset 543a395 by Serhiy Storchaka in branch 'master':
    bpo-38005: Remove support of string argument in InterpreterID(). (GH-16227)
    543a395

    @miss-islington
    Copy link
    Contributor

    New changeset ca14f04 by Miss Islington (bot) in branch '3.8':
    bpo-38005: Remove support of string argument in InterpreterID(). (GH-16227)
    ca14f04

    @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
    Assignees
    No one assigned
    Labels
    3.8 only security fixes 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @mdickinson @ericsnowcurrently @serhiy-storchaka @terryjreedy @miss-islington