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

Jump to bottom

weakref proxy doesn't support the matrix multiplication operator #80850

Closed
mr-nfamous mannequin opened this issue Apr 19, 2019 · 6 comments
Closed

weakref proxy doesn't support the matrix multiplication operator #80850

mr-nfamous mannequin opened this issue Apr 19, 2019 · 6 comments
Assignees
@mdickinson
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@mr-nfamous
Copy link
Mannequin

mr-nfamous mannequin commented Apr 19, 2019

BPO 36669
Nosy @freddrake, @mdickinson, @njsmith, @mr-nfamous
PRs
  • bpo-36669: Add matmul support for weakref.proxy #12932
    		•

    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/mdickinson'
closed_at = <Date 2019-04-26.08:46:22.259>
created_at = <Date 2019-04-19.14:08:57.116>
labels = ['3.8', 'type-feature', 'library']
title = "weakref proxy doesn't support the matrix multiplication operator"
updated_at = <Date 2019-04-26.08:46:22.258>
user = 'https://github.com/mr-nfamous'

    bugs.python.org fields:

    activity = <Date 2019-04-26.08:46:22.258>
actor = 'mark.dickinson'
assignee = 'mark.dickinson'
closed = True
closed_date = <Date 2019-04-26.08:46:22.259>
closer = 'mark.dickinson'
components = ['Library (Lib)']
creation = <Date 2019-04-19.14:08:57.116>
creator = 'bup'
dependencies = []
files = []
hgrepos = []
issue_num = 36669
keywords = ['patch']
message_count = 6.0
messages = ['340553', '340760', '340763', '340767', '340885', '340894']
nosy_count = 5.0
nosy_names = ['fdrake', 'mark.dickinson', 'njs', 'SilentGhost', 'bup']
pr_nums = ['12932']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue36669'
versions = ['Python 3.8']
    @SilentGhost
    Copy link
    Mannequin

    SilentGhost mannequin commented Apr 19, 2019

    It's not obvious why it should. Do you care to show a use case you had in mind?

    @SilentGhost SilentGhost mannequin added stdlib Python modules in the Lib dir 3.8 only security fixes type-feature A feature request or enhancement labels Apr 19, 2019
    @mdickinson
    Copy link
    Member

    It's not obvious why it should.

    I'd say that it's not obvious why it shouldn't. The "weakproxy" type currently supports every single "PyNumberMethods" slot *except* matrix multiplication: see

    cpython/Objects/weakrefobject.c

    Lines 610 to 645 in bf94cc7

    static PyNumberMethods proxy_as_number = {
    proxy_add, /*nb_add*/
    proxy_sub, /*nb_subtract*/
    proxy_mul, /*nb_multiply*/
    proxy_mod, /*nb_remainder*/
    proxy_divmod, /*nb_divmod*/
    proxy_pow, /*nb_power*/
    proxy_neg, /*nb_negative*/
    proxy_pos, /*nb_positive*/
    proxy_abs, /*nb_absolute*/
    (inquiry)proxy_bool, /*nb_bool*/
    proxy_invert, /*nb_invert*/
    proxy_lshift, /*nb_lshift*/
    proxy_rshift, /*nb_rshift*/
    proxy_and, /*nb_and*/
    proxy_xor, /*nb_xor*/
    proxy_or, /*nb_or*/
    proxy_int, /*nb_int*/
    0, /*nb_reserved*/
    proxy_float, /*nb_float*/
    proxy_iadd, /*nb_inplace_add*/
    proxy_isub, /*nb_inplace_subtract*/
    proxy_imul, /*nb_inplace_multiply*/
    proxy_imod, /*nb_inplace_remainder*/
    proxy_ipow, /*nb_inplace_power*/
    proxy_ilshift, /*nb_inplace_lshift*/
    proxy_irshift, /*nb_inplace_rshift*/
    proxy_iand, /*nb_inplace_and*/
    proxy_ixor, /*nb_inplace_xor*/
    proxy_ior, /*nb_inplace_or*/
    proxy_floor_div, /*nb_floor_divide*/
    proxy_true_div, /*nb_true_divide*/
    proxy_ifloor_div, /*nb_inplace_floor_divide*/
    proxy_itrue_div, /*nb_inplace_true_divide*/
    proxy_index, /*nb_index*/
    };

    It seems likely that this was an unintentional omission when matrix multiplication was added. I can't think of any good reason to support all but one (well, two, including the in-place method) of those PyNumberMethods. Supporting all of them gives a (conceptually) smaller, simpler object.

    I think this is a consistency bug that should be fixed.

    @mdickinson
    Copy link
    Member

    Adding Nathaniel Smith (the matrix multiplication PEP author), just in case he knows some reason why weakref.proxy objects should _not_ support the matrix multiplication operator.

    @mdickinson mdickinson self-assigned this Apr 24, 2019
    @njsmith
    Copy link
    Contributor

    njsmith commented Apr 24, 2019

    Yeah, seems like a simple oversight to me. (Actually this is the first I've heard of weakref.proxy...)

    @mdickinson
    Copy link
    Member

    New changeset 7abb6c0 by Mark Dickinson in branch 'master':
    bpo-36669: add matmul support to weakref.proxy (GH-12932)
    7abb6c0

    @mdickinson
    Copy link
    Member

    Now fixed for 3.8. Although this was an oversight, I'm erring on the side of regarding this as a new feature for 3.8 (and I think the omission is unlikely to bite anyone for Python 3.7 or 3.6), so I wouldn't recommend backporting this.

    Thanks for the report!

    @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

    @mdickinson mdickinson

    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    2 participants
    @njsmith @mdickinson