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

CPython 3.8.4 regression on __setattr__ in multiinheritance with metaclasses #85467

Closed
kam193 mannequin opened this issue Jul 14, 2020 · 19 comments
Closed

CPython 3.8.4 regression on __setattr__ in multiinheritance with metaclasses #85467

kam193 mannequin opened this issue Jul 14, 2020 · 19 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes type-bug An unexpected behavior, bug, or error

Comments

@kam193
Copy link
Mannequin

kam193 mannequin commented Jul 14, 2020

BPO 41295
Nosy @gvanrossum, @doko42, @scoder, @tiran, @ambv, @davidism, @miss-islington, @horazont, @david-caro, @kam193
PRs
  • bpo-41295: Allow overriding __setattr__ on multiple inheritance #21473
  • bpo-41295: Reimplement the Carlo Verre "hackcheck" #21528
  • [3.9] bpo-41295: Reimplement the Carlo Verre "hackcheck" (GH-21528) #21540
  • [3.8] bpo-41295: Reimplement the Carlo Verre "hackcheck" (GH-21528) #21541
  • Files
  • bpo41295.py
  • 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-09-29.07:34:06.895>
    created_at = <Date 2020-07-14.14:57:16.246>
    labels = ['type-bug', '3.8', '3.9', '3.10']
    title = 'CPython 3.8.4 regression on __setattr__ in multiinheritance with metaclasses'
    updated_at = <Date 2020-09-29.07:34:06.894>
    user = 'https://github.com/kam193'

    bugs.python.org fields:

    activity = <Date 2020-09-29.07:34:06.894>
    actor = 'scoder'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-09-29.07:34:06.895>
    closer = 'scoder'
    components = []
    creation = <Date 2020-07-14.14:57:16.246>
    creator = 'kam193'
    dependencies = []
    files = ['49316']
    hgrepos = []
    issue_num = 41295
    keywords = ['patch', '3.8regression', '3.9regression']
    message_count = 19.0
    messages = ['373637', '373640', '373641', '373642', '373644', '373646', '373766', '373803', '373897', '373920', '373921', '373922', '373923', '373924', '373934', '373936', '373965', '373967', '374018']
    nosy_count = 10.0
    nosy_names = ['gvanrossum', 'doko', 'scoder', 'christian.heimes', 'lukasz.langa', 'davidism', 'miss-islington', 'jssfr', 'David Caro', 'kam193']
    pr_nums = ['21473', '21528', '21540', '21541']
    priority = 'critical'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue41295'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @kam193
    Copy link
    Mannequin Author

    kam193 mannequin commented Jul 14, 2020

    CPython 3.8.4 broke previously correct custom __setattr__ implementation, when metaclass inheritances from multiple classes, including not-meta.

    Following code:

    class Meta(type):
        def __setattr__(cls, key, value):
            type.__setattr__(cls, key, value)
    
    class OtherClass:
        pass
    
    
    class DefaultMeta(OtherClass, Meta):
        pass
    
    obj = DefaultMeta('A', (object,), {})
    obj.test = True
    print(obj.test)
    

    Works in Python up to 3.8.3, but in 3.8.4 it raises:

    Traceback (most recent call last):
      File "repr.py", line 13, in <module>
        obj.test = True
      File "repr.py", line 3, in __setattr__
        type.__setattr__(cls, key, value)
    TypeError: can't apply this __setattr__ to DefaultMeta object

    This change affects e.g. pallets-eco/flask-sqlalchemy#852

    @kam193 kam193 mannequin added 3.8 only security fixes type-bug An unexpected behavior, bug, or error labels Jul 14, 2020
    @tiran
    Copy link
    Member

    tiran commented Jul 14, 2020

    The error message is coming from hackcheck function in Objects/typeobject.c,

    cpython/Objects/typeobject.c

    Lines 5811 to 5845 in b4cd77d

    /* Helper to check for object.__setattr__ or __delattr__ applied to a type.
    This is called the Carlo Verre hack after its discoverer. See
    https://mail.python.org/pipermail/python-dev/2003-April/034535.html
    */
    static int
    hackcheck(PyObject *self, setattrofunc func, const char *what)
    {
    PyTypeObject *type = Py_TYPE(self);
    PyObject *mro = type->tp_mro;
    if (!mro) {
    /* Probably ok not to check the call in this case. */
    return 1;
    }
    assert(PyTuple_Check(mro));
    Py_ssize_t i, n;
    n = PyTuple_GET_SIZE(mro);
    for (i = 0; i < n; i++) {
    PyTypeObject *base = (PyTypeObject*) PyTuple_GET_ITEM(mro, i);
    if (base->tp_setattro == func) {
    /* 'func' is the earliest non-Python implementation in the MRO. */
    break;
    } else if (base->tp_setattro != slot_tp_setattro) {
    /* 'base' is not a Python class and overrides 'func'.
    Its tp_setattro should be called instead. */
    PyErr_Format(PyExc_TypeError,
    "can't apply this %s to %s object",
    what,
    type->tp_name);
    return 0;
    }
    }
    /* Either 'func' is not in the mro (which should fail when checking 'self'),
    or it's the right slot function to call. */
    return 1;
    }
    . The issue could be related to bpo-39960. Checking now...

    @tiran
    Copy link
    Member

    tiran commented Jul 14, 2020

    The regression was introduced in commit 8912c18

    $ git checkout v3.8.4
    $ ./configure -C
    $ make
    $ $ ./python bpo41295.py 
    Traceback (most recent call last):
      File "bpo41295.py", line 14, in <module>
        obj.test = True
      File "bpo41295.py", line 3, in __setattr__
        type.__setattr__(cls, key, value)
    TypeError: can't apply this __setattr__ to DefaultMeta object
    $ git revert 8912c182455de83e27d5c120639ec91b18247913
    [detached HEAD 8d3c7847a0] Revert "bpo-39960: Allow heap types in the "Carlo Verre" hack check that override "tp_setattro()" (GH-21092) (GH-21339)"
     3 files changed, 11 insertions(+), 117 deletions(-)
    $ make
    $ ./python bpo41295.py 
    True

    @tiran tiran added 3.9 only security fixes 3.10 only security fixes release-blocker labels Jul 14, 2020
    @tiran
    Copy link
    Member

    tiran commented Jul 14, 2020

    Stefan, please take a look.

    @davidism
    Copy link
    Mannequin

    davidism mannequin commented Jul 14, 2020

    It appears to be solved in Flask-SQLAlchemy's development version already, as the mixins now inherit from type. We caught the issue when we started applying flake8 (possibly through flake8-bugbear).

    @david-caro
    Copy link
    Mannequin

    david-caro mannequin commented Jul 14, 2020

    Just sent a PR that fixes the issue (a first approach), let me know if it's addressing the issue correctly or if there's a better way.

    Thanks!

    @doko42
    Copy link
    Member

    doko42 commented Jul 16, 2020

    David, which issue number is this?

    @david-caro
    Copy link
    Mannequin

    david-caro mannequin commented Jul 17, 2020

    David, which issue number is this?

    It's bpo-41295, pr bpo-21473

    @scoder
    Copy link
    Contributor

    scoder commented Jul 18, 2020

    PR 21528 works for all four test cases that we now have (1x test_capi.py, 3x test_descr.py).

    Any comments? We need to merge a fix before Monday to meet the deadline of the planned hotfix release.

    @kam193, could you please also test that change with your real code?

    @miss-islington
    Copy link
    Contributor

    New changeset c53b310 by scoder in branch 'master':
    bpo-41295: Reimplement the Carlo Verre "hackcheck" (GH-21528)
    c53b310

    @miss-islington
    Copy link
    Contributor

    New changeset 38d930f by Miss Islington (bot) in branch '3.8':
    bpo-41295: Reimplement the Carlo Verre "hackcheck" (GH-21528)
    38d930f

    @miss-islington
    Copy link
    Contributor

    New changeset 01ab963 by Miss Islington (bot) in branch '3.9':
    bpo-41295: Reimplement the Carlo Verre "hackcheck" (GH-21528)
    01ab963

    @gvanrossum
    Copy link
    Member

    We have a buildbot failure: test_asyncio altered the execution environment. What does that mean? Victor?

    @kam193
    Copy link
    Mannequin Author

    kam193 mannequin commented Jul 18, 2020

    @Stefan: Today I run unit tests for Flask-SQLAlchemy (v2.4.3) as well as for aioxmpp (another library reported this problem https://github.com/horazont/aioxmpp/issues/342). In both cases patch is successful: tests fail on Python 3.8.4 but pass on patched Python.

    As for me it looks done. Thanks for everyone involved!

    @horazont
    Copy link
    Mannequin

    horazont mannequin commented Jul 19, 2020

    @kam193 Thanks for running the aioxmpp tests. I built the patched python yesterday, but I didn’t manage to get a virtualenv with it up&running.

    Good to hear that the fix works as expected!

    @scoder
    Copy link
    Contributor

    scoder commented Jul 19, 2020

    test_asyncio altered the execution environment

    That happened several times before in previous builds. I think there's just a part of the asyncio tests that is unreliable. I left a comment in bpo-41273 since it might be related, the sporadic failures started appearing in the build following the merge.

    @kam193
    Copy link
    Mannequin Author

    kam193 mannequin commented Jul 19, 2020

    @jonas: I know, running those tests isn't so easy, I think a few libraries are incompatible with python 3.9/3.10 yet or I didn't find right prerequisites. But fortunately, everything is running on patched 3.8, so after back-porting I can run it

    @gvanrossum
    Copy link
    Member

    @Łukasz: this should be good for the 3.8.5 release.

    @ambv
    Copy link
    Contributor

    ambv commented Jul 20, 2020

    Released.

    @scoder scoder closed this as completed Sep 29, 2020
    @scoder scoder closed this as completed Sep 29, 2020
    @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.8 only security fixes 3.9 only security fixes 3.10 only security fixes type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants