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

[Stubgen bug] Fix behavior for Instance Variables in C extensions #12150

Closed
shubz1998 opened this issue Feb 8, 2022 · 3 comments · Fixed by #12524
Closed

[Stubgen bug] Fix behavior for Instance Variables in C extensions #12150

shubz1998 opened this issue Feb 8, 2022 · 3 comments · Fixed by #12524

Comments

@shubz1998
Copy link
Contributor

Stubgen generates wrong stubs for instance variables defined in C extensions.

In the attached extension.C, I've created a class named Custom which has 3 instance variables: var1,var2,var3. On running stubgen for this extension, the generated .pyi file contains:

from typing import Any, ClassVar

class Custom:
    // This is wrong. Var{1,2,3} are not Class Variables but are rather instance variables.
    var1: ClassVar[getset_descriptor] = ...
    var2: ClassVar[getset_descriptor] = ...
    var3: ClassVar[member_descriptor] = ...   
    // This is wrong. 

    def display(self, *args, **kwargs) -> Any: ...

Detailed Reproducer:

  1. In some test directory (say $DIR), download the attached file extension.zip to extension folder.
  2. Compile the extension by running: g++ -fPIC -std=c++11 -I. -I/opt/python/python-3.9/include/python3.9 -Iextension -c extension/extension.C -o extension.o in directory $DIR.
  3. Create the .so file by running: g++ -shared -L/opt/python/python-3.9/lib64 -Wl,-R/opt/python/python-3.9/lib64 extension.o -o extension.so in directory $DIR.
  4. Run stubgen using env PYTHONPATH=$DIR stubgen -m extension
  5. Notice that the generated $DIR/out/extension.pyi declares var{1,2,3} as 'Class Variables' which is wrong.

On digging into this, I found this is due to a wrong check in stubgenc.py#is_c_property.

def is_c_property(obj: object) -> bool:
    return inspect.isdatadescriptor(obj) and hasattr(obj, 'fget')

The check should contain "or" (instead of "and") since the instance variables in case of C extension doesn't have the "fget" attribute but inspect.isdatadescription return True.
On making this change locally, stubgen runs fine.

If this looks good to you, I can raise a PR for the same.

extension.zip

@shubz1998
Copy link
Contributor Author

Hi, did anyone get a chance to look at the reproducer?
Happy to add more details if required.

@shubz1998
Copy link
Contributor Author

Gentle ping for the maintainers to have a look at this.

@JelleZijlstra
Copy link
Member

Mypy is a volunteer-run project with limited resources. Any fix for this issue will likely have to come from an external contributor.

shubz1998 pushed a commit to shubz1998/mypy that referenced this issue Apr 5, 2022
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 pushed a commit to shubz1998/mypy that referenced this issue Apr 5, 2022
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 pushed a commit to shubz1998/mypy that referenced this issue Apr 5, 2022
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.
JelleZijlstra pushed a commit that referenced this issue Apr 11, 2022
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.

Fixes #12150.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants