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

Fix annotations for decorated classes #4151

Merged
merged 1 commit into from
May 15, 2021

Conversation

matsjoyce
Copy link
Contributor

This patch fixes a problem with using dataclasses in Cython. Using Cython master, the following code:

# cython: language_level=3

import dataclasses


@dataclasses.dataclass
class Foo:
    bar: int


print(Foo(1))

Fails with:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "x.py", line 11, in init x
    print(Foo(1))
TypeError: __init__() takes 1 positional argument but 2 were given

This is because decorated classes do not have an __annotations__ attribute. The root cause of this problem is that annotations are looked for after wrapping the class with the decorator(s), and since SimpleCallNode does not forward the class' annotations, none are found. This patch fixes the problem by checking for annotations before wrapping the class.

@scoder
Copy link
Contributor

scoder commented May 10, 2021

Hmm, but now we are not calling .analyse_declarations() on the SimpleCallNode call chain that we are building, nor on the decorators. And since that's a recursive operation, we'd now call it twice on the class object if we did that after the fact. I don't think it's all that easy.

@scoder
Copy link
Contributor

scoder commented May 10, 2021

Can't we just call .analyse_annotations() on the original class object instead?

@matsjoyce
Copy link
Contributor Author

@scoder SimpleCallNode.analyse_declarations does not have any effect, since it is the same method as Node.analyse_declarations (it isn't overridden), and the implementation of Node.analyse_declarations is just pass. However, Py3ClassNode also has a noop analyse_declarations, so calling the decorated analyse_declarations should have the same behaviour. Do you want me to change it?

@da-woods
Copy link
Contributor

I think roughly what's being suggested is:

unmodified_class_result = class_result = self.classobj
# code goes here
unmodified_class_result.analyse_annotations(cenv)

Also: I get the feeling that ~90% of people that care about class annotations care because they want to use dataclasses. That suggests to me that maybe we should add at least one test that creates a dataclass in Cython, so that at least we know that this works. It'd obviously need a suitable version-check guard (

(3,7): (operator.lt, lambda x: x in ['run.pycontextvar',
) for the test.

@matsjoyce matsjoyce force-pushed the _fix_decorated_annotated_classes branch from 36ca3f6 to eb4e0fa Compare May 10, 2021 16:36
@matsjoyce
Copy link
Contributor Author

@scoder @da-woods I've adjusted the patch as requested, and added a dataclass test.

@da-woods
Copy link
Contributor

@scoder I think you need to approve github-actions to run for this PR.

@scoder scoder merged commit e93c2c5 into cython:master May 15, 2021
@scoder
Copy link
Contributor

scoder commented May 15, 2021

Beautiful little change with good tests. Thanks!

@scoder scoder added this to the 3.0 milestone May 15, 2021
@matsjoyce matsjoyce deleted the _fix_decorated_annotated_classes branch May 17, 2021 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants