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

tuple subclasses allow arbitrary kwargs #87579

Closed
jaraco opened this issue Mar 6, 2021 · 13 comments
Closed

tuple subclasses allow arbitrary kwargs #87579

jaraco opened this issue Mar 6, 2021 · 13 comments
Assignees
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@jaraco
Copy link
Member

jaraco commented Mar 6, 2021

BPO 43413
Nosy @rhettinger, @jaraco, @serhiy-storchaka, @brandtbucher
PRs
  • bpo-43413: Fix handling keyword arguments in subclasses of some buitin classes #26456
  • bpo-43413: Revert changes in set.__init__ #28403
  • 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 2021-09-12.10:29:49.494>
    created_at = <Date 2021-03-06.01:37:11.195>
    labels = ['interpreter-core', '3.11']
    title = 'tuple subclasses allow arbitrary kwargs'
    updated_at = <Date 2021-12-26.11:27:06.889>
    user = 'https://github.com/jaraco'

    bugs.python.org fields:

    activity = <Date 2021-12-26.11:27:06.889>
    actor = 'serhiy.storchaka'
    assignee = 'serhiy.storchaka'
    closed = True
    closed_date = <Date 2021-09-12.10:29:49.494>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2021-03-06.01:37:11.195>
    creator = 'jaraco'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 43413
    keywords = ['patch', '3.7regression']
    message_count = 13.0
    messages = ['388183', '388184', '388186', '388189', '388191', '388207', '394768', '401660', '402000', '402002', '402055', '402056', '409192']
    nosy_count = 4.0
    nosy_names = ['rhettinger', 'jaraco', 'serhiy.storchaka', 'brandtbucher']
    pr_nums = ['26456', '28403']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue43413'
    versions = ['Python 3.11']

    @jaraco
    Copy link
    Member Author

    jaraco commented Mar 6, 2021

    While troubleshooting a strange problem (jaraco/keyring#498) where a program worked on Python 3.7+ but failed on Python 3.6, I discovered a somewhat unintuitive behavior. On Python 3.7+, keyword arguments to tuple subclasses are allowed and ignored:

    >>> class Foo(tuple): pass
    >>> tuple(name='xyz')
    TypeError: tuple() takes no keyword arguments
    >>> Foo(name='xyz')
    ()

    But on Python 3.6, the keyword parameter causes an error:

    $ python3.6 -c "type('Foo', (tuple,), {})(name='xyz')"
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    TypeError: 'name' is an invalid keyword argument for this function

    I checked out the What's new in Python 3.7 and I see this notice:

    Functions bool(), float(), list() and tuple() no longer take keyword arguments. The first argument of int() can now be passed only as positional argument.

    Hmm, but in my experience, tuple on Python 3.6 doesn't take keyword arguments either:

    importlib_metadata main $ python3.6 -c "tuple(name='xyz')"
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
    TypeError: 'name' is an invalid keyword argument for this function

    So that change may be related, but I'm not sure where or how.

    The main place my expectation is violated is in the subclass. Why should a subclass of tuple allow keyword arguments when the parent class does not? I'd expect that the subclass should reject keyword arguments as well.

    Less importantly, the What's New doc implies that keyword arguments were accepted in Python 3.6; why aren't they?

    @jaraco jaraco added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Mar 6, 2021
    @jaraco
    Copy link
    Member Author

    jaraco commented Mar 6, 2021

    To be abundantly clear, the downstream issue was a coding error, but the coding error was masked on Python 3.7+ when the subclass didn't reject the invalid usage.

    @jaraco
    Copy link
    Member Author

    jaraco commented Mar 6, 2021

    I see that changelog entry traces back to bpo-29695, but I don't believe it's relevant to this issue.

    @jaraco
    Copy link
    Member Author

    jaraco commented Mar 6, 2021

    I suspect bpo-20186 is implicated.

    @jaraco
    Copy link
    Member Author

    jaraco commented Mar 6, 2021

    In particular, this commit.

    @serhiy-storchaka
    Copy link
    Member

    Hmm, but in my experience, tuple on Python 3.6 doesn't take keyword arguments either:

    They do.

    >>> tuple(sequence='abc')
    ('a', 'b', 'c')
    
    >>> list(sequence='abc')
    ['a', 'b', 'c']
    >>> int(x='123')
    123
    >>> bool(x='123')
    True
    >>> float(x='123')
    123.0

    It was changed in bpo-29695.

    But accepting arbitrary keyword arguments is another issue.

    Object creation in Python can be customized be implementing special methods __new__ and __init__. They both are called sequentially with arguments passed to the constructor. If object is mutable, it is enough to implement __init__. If the object contains immutable data which should be initialized at creation time, it needs __new__, and __init__ is not necessary. So the list class has __init__, but the tuple and int classes have __new__. Usually class implements only one of __new__ or __init__, and inherits the other one from parent classes.

    Since positional and keyword arguments are passed to both __new__ and __init__, they should accept same arguments. If __new__ and __init__ are inherited from parent class, they cannot know about arguments supported in the other method. Therefore object's __new__ and __init__ accept and ignore all arguments (if the other method is overridden).

    Implementations of __new__ for most builtin classes which accept only positional arguments accept and ignore also arbitrary keyword arguments in subclasses. It makes easier to implement a subclass with non-trivial __init__. You just add additional keyword arguments which will be ignored in __new__. It is long tradition. Example:

    if (type == &cycle_type && !_PyArg_NoKeywords("cycle()", kwds))
        return NULL;
    

    bpo-20186 just used this idiom for tuple and several other classes. It is a feature which makes subclassing these classes easier. And with converting more classes to Argument Clinic it is now used in more and more classes.

    Now, perhaps it would be more correct to test type->tp_init == cycle_type.tp_init or type->tp_init != PyBaseObject_Type.tp_init instead of type == &cycle_type. It will ignore keyword arguments only if init is overridden. If init is overridden, it is now responsible for validating arguments.

    @jaraco jaraco changed the title tuple subclasses allow kwargs tuple subclasses allow arbitrary kwargs May 27, 2021
    @jaraco jaraco changed the title tuple subclasses allow kwargs tuple subclasses allow arbitrary kwargs May 27, 2021
    @serhiy-storchaka serhiy-storchaka added 3.11 only security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels May 28, 2021
    @serhiy-storchaka serhiy-storchaka self-assigned this May 28, 2021
    @serhiy-storchaka serhiy-storchaka added 3.11 only security fixes and removed 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels May 28, 2021
    @serhiy-storchaka serhiy-storchaka self-assigned this May 28, 2021
    @serhiy-storchaka
    Copy link
    Member

    Surprisingly there were almost no tests for keyword arguments in subclasses.

    PR 26456 makes checks for some classes (like tuple, list, frozenset) more strict: if subclass does not define __init__ or __new__, it will reject arbitrary keyword arguments.

    It also makes the check in set.__init__() more lenient for uniformity with frozenset and list. Subclass of set can now define a __new__() method with additional keyword parameters without overriding also __init__().

    Added tests for some of builtin classes.

    Raymond, please take a look. It touches classes maintained tracked by you: set/frozenset, itertools, random.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 92bf869 by Serhiy Storchaka in branch 'main':
    bpo-43413: Fix handling keyword arguments in subclasses of some buitin classes (GH-26456)
    92bf869

    @rhettinger
    Copy link
    Contributor

    Subclass of set can now define a __new__() method with
    additional keyword parameters without overriding also __init__().

    Is there any use case for this? Offhand, I can't think of any reason.

    The new code in set.__init__ is somewhat opaque and is likely slower, so I prefer the previous code unless there is a legitimate use case being served.

    @serhiy-storchaka
    Copy link
    Member

    I do not have any particular use case. It was a side effect of unification code that uses _PyArg_NoKeywords().

    @jaraco
    Copy link
    Member Author

    jaraco commented Sep 17, 2021

    > Subclass of set can now define
    Is there any use case for this?

    Is your concern about a use-case for the general concept or for set specifically?

    I appreciate that Serhiy has taken the time to evaluate the specific concern I raised and extrapolate it to the implications for most/all built-in types. It seems worthwhile to me that built-in types have consistent behaviors. Moreover, it seems preferable to provide the intuitive behavior (allowing simply overriding __new__) without the baggage of needing to define __init__. I'd not be surprised if there was a real-world use-case in which set.__new__ was overridden in a subclass and the user was forced to add an __init__ just to bypass "set() takes no keyword arguments". Interestingly, searching the web for that exact error doesn't emit any results, so if someone has encountered it, it wasn't posted with the error message.

    After exploring this some, I'm convinced there may not be a strong case for this behavior.

    Raymond, if this new behavior was removed, how would you propose to rewrite the test (specifically

    class subclass_with_new(set):
    def __new__(cls, arg, newarg=None):
    self = super().__new__(cls, arg)
    self.newarg = newarg
    return self
    u = subclass_with_new([1, 2], newarg=3)
    self.assertIs(type(u), subclass_with_new)
    self.assertEqual(set(u), {1, 2})
    self.assertEqual(u.newarg, 3)
    )?

    @jaraco
    Copy link
    Member Author

    jaraco commented Sep 17, 2021

    Oh, and I see now Serhiy has proposed a change that looks reasonable.

    @serhiy-storchaka
    Copy link
    Member

    New changeset ad48578 by Serhiy Storchaka in branch 'main':
    bpo-43413: Revert changes in set.__init__ (GH-28403)
    ad48578

    @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.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants