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

[mypyc] Dict true test #10333

Merged
merged 5 commits into from
Apr 22, 2021
Merged

[mypyc] Dict true test #10333

merged 5 commits into from
Apr 22, 2021

Conversation

97littleleaf11
Copy link
Collaborator

@97littleleaf11 97littleleaf11 commented Apr 16, 2021

Description

Closes mypyc/mypyc#768

  • Modify builtin_len(dict).

Test Plan

An IR test and a functional test.
Similar to 1e96f1d

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm the original implementation may actually be buggy. We should allow subclasses to override __len__, for consistency with other dict operations, and this means that we should have a type guard. This probably makes it too complex to be done directly in IR, and instead we could use a C helper that uses PyDict_GET_SIZE behind a type guard. Sorry about that!

This should work similar to CPython and print 'here':

class D(dict):
    def __len__(self) -> int:
        return 1

if D():
    print('here') 

@97littleleaf11
Copy link
Collaborator Author

Got it. I will start a new branch from beginning later.

@97littleleaf11
Copy link
Collaborator Author

I tried adding this:

Py_ssize_t CPyDict_Size(PyObject *dict) {
    if (PyDict_CheckExact(dict)) {
        return PyDict_Size(dict);
    }
    return PyObject_Size(dict);
}
class MyDict(dict):
    def __init__(self, *args, **kwargs):
        self.update(*args, **kwargs)

    def __len__(self):
        return 1


def test_dict_subclass_to_bool() -> None:
    d = MyDict()
    if d:
        print("true")

However PyObject_Size won't call the overrided __len__.
Does the cast operation cause that?

L0:
    r0 = tmp5.MyDict :: type
    r1 = PyObject_CallFunctionObjArgs(r0, 0)
    if is_error(r1) goto L7 (error at test_dict_subclass_to_bool:45) else goto L1
L1:
    r2 = cast(dict, r1)
    if is_error(r2) goto L7 (error at test_dict_subclass_to_bool:45) else goto L2
L2:
    d = r2
    r3 = CPyDict_Size(d)
    dec_ref d
    r4 = r3 >= 0 :: signed
    if not r4 goto L7 (error at test_dict_subclass_to_bool:45) else goto L3 :: bool

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 22, 2021

However PyObject_Size won't call the overrided len.

Hmm I wonder if this is a bug in PyObject_Size. Anyway, it's already broken for len(d), so not supporting it for boolean checks seems okay. So let's drop the __len__ override test case and use the original implementation for now. Sorry about the back and forth -- there are some gray areas with respect to CPython compatibility where it's often unclear what's the best course of action.

But can you add a new issue to track the issue of overriding __len__? To properly fix it, we should fix it for len() as well.

Does the cast operation cause that?

No, the cast does not modify the operand.

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates! This looks good, and it made a quick benchmark I wrote about 50% faster, which is a nice speedup.

@JukkaL JukkaL merged commit bdcc562 into python:master Apr 22, 2021
@97littleleaf11 97littleleaf11 deleted the dict-true-test branch April 23, 2021 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faster truth value testing
2 participants