Fix Stubgen's behavior for Instance Variables in C extensions #12524

shubz1998 · 2022-04-05T09:36:39Z

Description

It is not necessary for instance variables to have the fget attribute (e.g. instance variable in a C class in an extension) but
inspect.isdatadescriptor return True as expected, hence we update the 'is_c_property' check.

Since special attributes (like __dict__ etc) also passes 'is_c_property' check, we ignore all such special attributes in
'generate_c_property_stub' while creating the contents of stub file.

Also, 'is_c_property_readonly' assumed that the property would always have 'fset' attribute which again is not true for instance variables in C extension. Hence make the check defensive by first checking if 'fset' attribute even exists or not.

Fixes #12150.

Test Plan

Ran the test suite as described in Contributing-Running tests. All tests are running fine.
Ran stubgen on a C extension and verified that instance variables are correctly typed now. The C extension is the same as present on Issue#12150.

C extension source code (only relevant part is present here, refer the Issue#12150 for the complete source code.:

typedef struct {
    PyObject_HEAD
    PyObject *var1; 
    PyObject *var2; 
    int var3;
} Custom;

Generated extension.pyi file prior to this change: (Note that the instance variables are incorrectly marked as ClassVar)

from typing import Any, ClassVar

class Custom:
    var1: ClassVar[getset_descriptor] = ...  
    var2: ClassVar[getset_descriptor] = ...
    var3: ClassVar[member_descriptor] = ...
    def display(self, *args, **kwargs) -> Any: ...

Generated extension.pyi file after this change:

from typing import Any

class Custom:
    var1: Any
    var2: Any
    var3: Any
    def display(self, *args, **kwargs) -> Any: ...

JelleZijlstra

Thanks for your contribution! Is it possible to add a unit test for this change?

Also, please give a more precise title than "Fix stubgen".

It is not necessary for instance variables to have the fget attrbute (e.g. instance variable in a C class in an extension) but inspect.isdatadescriptor return True as expected, hence we update the 'is_c_property' check. Since special attributes (like __dict__ etc) also passes 'is_c_property' check, we ignore all such special attributes in 'generate_c_property_stub' while creating the contents of stub file. Also, 'is_c_property_readonly' assumed that the property would always have 'fset' attribute which again is not true for instance variables in C extension. Hence make the check defensive by first checking if 'fset' attribute even exists or not. This fixes python#12150.

shubz1998 · 2022-04-05T14:47:58Z

Is it possible to add a unit test for this change?
I think the existing StubgencSuite suffices here as the change is not adding any new functionality (but rather a fix in existing functionality)

please give a more precise title than "Fix stubgen".
Done for both PR and commit (force pushed).

JelleZijlstra

If the fix changes behavior that can be observed in the test suite, then there should be a new test or a change in the tests.

JelleZijlstra · 2022-04-06T01:37:00Z

And indeed tests are currently failing.

shubz1998

If the fix changes behavior that can be observed in the test suite, then there should be a new test or a change in the tests.
Makes sense. Apologies that I missed.

However, I'll note that test in $REPO_DIR/misc/test-stubgenc.sh is not run when running tests via python3 runtests.py is run (and hence I missed this).

shubz1998 · 2022-04-06T08:17:51Z

test-data/stubgen/pybind11_mypy_demo/basics.pyi

@@ -5,8 +5,6 @@ PI: float

 class Point:
    class AngleUnit:
-        __doc__: ClassVar[str] = ...  # read-only


Since stubs for special dunder attributes is not expected to be generated, we remove these from the test files.

But we had tests explicitly adding them, so I'd like to make sure these aren't important for pybind11 users.

@sizmailov added these tests in #9903; could you comment on whether removing these attributes from the docs is desirable?

I agree, annotation of most of dunder attributes makes a little sense and probably utilized only by few people if any, but I cannot speak for whole pybind11 community.
At the time I've added all the attributes to test just because it was generated by the stubgen and I saw no harm in leaving them.

It looks like dunder attributes hiding is orthogonal to the objectives of #12150.

It looks like dunder attributes hiding is orthogonal to the objectives of #12150.

I agree on this, I had added this change to be in-line with stub generation logic for python files where we ignore special dunder methods (Ref).

We can also choose to not ignore dunder methods in generate_c_property_stub, in that case, we would have to update the below two tests since these will now generate stubs for dunder attributes:

test_generate_c_type_stub_variable_type_annotation
AssertionError: ['class C(KeyError):', ' __weakref__: Any'] != ['class C(KeyError): ...']

test_generate_c_type_inheritance
AssertionError: ['class C:', ' x: ClassVar[int] = ...', ' __dict__: Any', ' __weakref__: Any'] != ['class C:', ' x: ClassVar[int] = ...']

@sizmailov Let me know if this sounds fine to you and I would then go ahead with the change.

I think your change is OK as is. It is definitely an improvement.

My perception is that stubgen is still in the 0.x release state, therefore minor "breaking" changes are totally fine.

Last time I checked, stubgen had little or no configuration options, which results in many hardcoded decisions such as "no dunder attributes". But this problem is definitely out of the scope of this PR.

JelleZijlstra

There are still no tests for the is_c_property and is_c_property_readonly changes, only for the dunder change.

JelleZijlstra · 2022-04-07T00:35:34Z

test-data/stubgen/pybind11_mypy_demo/basics.pyi

@@ -5,8 +5,6 @@ PI: float

 class Point:
    class AngleUnit:
-        __doc__: ClassVar[str] = ...  # read-only


But we had tests explicitly adding them, so I'd like to make sure these aren't important for pybind11 users.

@sizmailov added these tests in #9903; could you comment on whether removing these attributes from the docs is desirable?

shubz1998 · 2022-04-07T15:34:54Z

Thanks @sizmailov for your thoughts.

There are still no tests for the is_c_property and is_c_property_readonly changes, only for the dunder change.

@JelleZijlstra, I see there exist a test test_generate_c_property_with_pybind11 which indirectly test the above two functions.
I've slightly modified it to use is_c_property_readonly instead of hardcoding it and added another test case for writeable properties as well.

JelleZijlstra · 2022-04-11T01:13:42Z

Tests are failing.

shubz1998 · 2022-04-11T06:26:01Z

Oops, somehow missed this earlier, fixed now. All tests should run fine now.
Thanks @JelleZijlstra for the review! Please retrigger the tests.

github-actions · 2022-04-11T15:17:13Z

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

shubz1998 · 2022-04-11T15:40:49Z

One quick question: Will this change be included in the next mypy release ?
Btw What is the release cycle followed by mypy? From the releases page, the release dates doesn't seem to follow a particular cycle.

JelleZijlstra · 2022-04-11T15:42:04Z

It will be in the next release but I don't know when that will be. See #12210.

bluenote10 · 2022-05-03T10:04:40Z

It looks like this change has caused the regression #12717

Since stubs for special dunder attributes is not expected to be generated [...]

This is not such a good idea. In many cases, special dunder attributes are just part of the officially interface. For instance __members__ is an official and fully valid part of the enum interface. And pybind11 properly supports this interface. We have code that makes use of __members__, which is perfectly valid at runtime, but now our code no longer type checks, because this previously correct type annotation has been removed 😕

JelleZijlstra reviewed Apr 5, 2022

View reviewed changes

shubz1998 changed the title ~~Fix stubgen~~ Fix Stubgen's behavior for Instance Variables in C extensions Apr 5, 2022

shubz1998 force-pushed the fix_stubgen_issue#12150 branch from db4be74 to cc739fb Compare April 5, 2022 14:47

shubz1998 requested a review from JelleZijlstra April 5, 2022 14:48

JelleZijlstra requested changes Apr 6, 2022

View reviewed changes

Update stubgen test files

245211c

shubz1998 commented Apr 6, 2022

View reviewed changes

shubz1998 requested a review from JelleZijlstra April 6, 2022 08:18

JelleZijlstra requested changes Apr 7, 2022

View reviewed changes

Add test cases

1a77ea9

shubz1998 requested a review from JelleZijlstra April 7, 2022 15:35

JelleZijlstra approved these changes Apr 11, 2022

View reviewed changes

JelleZijlstra self-assigned this Apr 11, 2022

This comment has been minimized.

Sign in to view

Fix test case

34a2cdf

shubz1998 requested a review from JelleZijlstra April 11, 2022 06:26

JelleZijlstra approved these changes Apr 11, 2022

View reviewed changes

JelleZijlstra merged commit 74df7fb into python:master Apr 11, 2022

shubz1998 deleted the fix_stubgen_issue#12150 branch April 11, 2022 15:56

bluenote10 mentioned this pull request May 3, 2022

Make pybind11 test fixture fully self-contained #12722

Merged

bluenote10 mentioned this pull request May 20, 2022

Bring back type annotation support of dunder methods in stub generator (fix #12717) #12828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Stubgen's behavior for Instance Variables in C extensions #12524

Fix Stubgen's behavior for Instance Variables in C extensions #12524

shubz1998 commented Apr 5, 2022

JelleZijlstra left a comment

shubz1998 commented Apr 5, 2022

JelleZijlstra left a comment

JelleZijlstra commented Apr 6, 2022

shubz1998 left a comment

shubz1998 Apr 6, 2022

JelleZijlstra Apr 7, 2022

sizmailov Apr 7, 2022

shubz1998 Apr 7, 2022

sizmailov Apr 7, 2022

JelleZijlstra left a comment

JelleZijlstra Apr 7, 2022

shubz1998 commented Apr 7, 2022

This comment has been minimized.

JelleZijlstra commented Apr 11, 2022

shubz1998 commented Apr 11, 2022

github-actions bot commented Apr 11, 2022

shubz1998 commented Apr 11, 2022

JelleZijlstra commented Apr 11, 2022

bluenote10 commented May 3, 2022 •

edited

Loading

Fix Stubgen's behavior for Instance Variables in C extensions #12524

Fix Stubgen's behavior for Instance Variables in C extensions #12524

Conversation

shubz1998 commented Apr 5, 2022

Description

Test Plan

JelleZijlstra left a comment

Choose a reason for hiding this comment

shubz1998 commented Apr 5, 2022

JelleZijlstra left a comment

Choose a reason for hiding this comment

JelleZijlstra commented Apr 6, 2022

shubz1998 left a comment

Choose a reason for hiding this comment

shubz1998 Apr 6, 2022

Choose a reason for hiding this comment

JelleZijlstra Apr 7, 2022

Choose a reason for hiding this comment

sizmailov Apr 7, 2022

Choose a reason for hiding this comment

shubz1998 Apr 7, 2022

Choose a reason for hiding this comment

sizmailov Apr 7, 2022

Choose a reason for hiding this comment

JelleZijlstra left a comment

Choose a reason for hiding this comment

JelleZijlstra Apr 7, 2022

Choose a reason for hiding this comment

shubz1998 commented Apr 7, 2022

This comment has been minimized.

JelleZijlstra commented Apr 11, 2022

shubz1998 commented Apr 11, 2022

github-actions bot commented Apr 11, 2022

shubz1998 commented Apr 11, 2022

JelleZijlstra commented Apr 11, 2022

bluenote10 commented May 3, 2022 • edited Loading

bluenote10 commented May 3, 2022 •

edited

Loading