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

Support type stub generation for staticmethod #14934

Merged
merged 13 commits into from
Jan 8, 2024

Conversation

WeilerMarcel
Copy link
Contributor

Fixes #13574

This PR fixes the generation of type hints for static methods of pybind11 classes. The code changes are based on the suggestions in #13574.

The fix introduces an additional check if the property under inspection is of type staticmethod. If it is, the type information is read from the staticmethod's __func__ attribute, instead of the staticmethod instance itself.

I added a test for C++ classes with static methods bound using pybind11. Both, an overloaded and a non-overloaded static method are tested.

mypy/stubgenc.py Outdated Show resolved Hide resolved
@WeilerMarcel
Copy link
Contributor Author

Hi @sobolevn and @AlexWaygood, I updated this PR to work with the latest changes on master. Could you please review the current changes and merge the PR if everything is fine?

@WeilerMarcel
Copy link
Contributor Author

Hey @sobolevn and @AlexWaygood, what is the status of this pull request? I would really like to see it merged into MyPy. Do you need anything from my side to proceed?

@d4n1elchen
Copy link

Looking forward to this getting merged as well +1

@WeilerMarcel
Copy link
Contributor Author

Hi @sobolevn and @AlexWaygood, please give me an update on this PR. Is there a reason why it has not been merged? As far as I can see, I can not merge it myself since I don't have the necessary permissions.

@sobolevn
Copy link
Member

I can't merge this PR, because I don't fully understand the C++ code, but I think that @JukkaL or @msullivan can help us :)

@AlexWaygood
Copy link
Member

I can't merge this PR because I'm not a maintainer

@WeilerMarcel
Copy link
Contributor Author

Thanks for the info :) I was under the impression that you could merge, since you were reviewing my changes.
I will wait for a reply from @JukkaL and @msullivan.

@sobolevn
Copy link
Member

In the meantime you can resolve conflicts :)

@WeilerMarcel
Copy link
Contributor Author

Hi everyone, I integrated my changes into the new master branch and from my perspective it is ready for merging.

I noticed, though, that the mypyc runtime tests with py39-macos seem to be unstable. Since I did not find a way to rerun tests for a commit, I added two empty dummy commits to get the pipeline to succeed. See #16420.

mypy/stubgenc.py Outdated Show resolved Hide resolved
@JukkaL
Copy link
Collaborator

JukkaL commented Nov 8, 2023

You can ignore the mypyc runtime test failures, since it's a known issue happening on master.

mypy/stubgenc.py Outdated
return False
elif self.is_c_module:
raw_lookup = getattr(class_info.cls, "__dict__") # noqa: B009
Copy link
Member

Choose a reason for hiding this comment

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

Why has this been # noqa'd? Wouldn't it be cleaner to do this, as the linter suggests?

Suggested change
raw_lookup = getattr(class_info.cls, "__dict__") # noqa: B009
raw_lookup = class_info.cls.__dict__

Copy link
Contributor Author

@WeilerMarcel WeilerMarcel Nov 9, 2023

Choose a reason for hiding this comment

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

To be honest, I wrote it that way because there are two other places in stubgenc.py where this pattern was used:

  • mypy/mypy/stubgenc.py

    Lines 453 to 455 in 8121e3c

    def get_members(self, obj: object) -> list[tuple[str, Any]]:
    obj_dict: Mapping[str, Any] = getattr(obj, "__dict__") # noqa: B009
    results = []
  • mypy/mypy/stubgenc.py

    Lines 718 to 725 in 8121e3c

    def generate_class_stub(self, class_name: str, cls: type, output: list[str]) -> None:
    """Generate stub for a single class using runtime introspection.
    The result lines will be appended to 'output'. If necessary, any
    required names will be added to 'imports'.
    """
    raw_lookup = getattr(cls, "__dict__") # noqa: B009
    items = self.get_members(cls)

    I will check the file history to see if I can find a specific reason why getattr is used.

Copy link
Contributor Author

@WeilerMarcel WeilerMarcel Nov 10, 2023

Choose a reason for hiding this comment

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

The getattr call originally comes from commit 6bcaf40 and was added to make typeshed happy:

mypy/mypy/stubgenc.py

Lines 140 to 142 in 6bcaf40

# typeshed gives obj.__dict__ the not quite correct type Dict[str, Any]
# (it could be a mappingproxy!), which makes mypyc mad, so obfuscate it.
obj_dict = getattr(obj, '__dict__') # type: Mapping[str, Any]

The # noqa was added in 4287af4:

obj_dict = getattr(obj, '__dict__') # type: Mapping[str, Any] # noqa

@WeilerMarcel
Copy link
Contributor Author

Hi @JukkaL and @msullivan, I resolved conflicts with the current master branch. Can this PR be merged?

@WeilerMarcel
Copy link
Contributor Author

Happy new year everyone! 🥳
Are there plans to merge this PR yet? I would really like to get this off my mind.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 3, 2024

I can merge this PR if somebody who has more context on the relevant code than I have gives a quick thumbs up.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 3, 2024

more context on the relevant code than I have

Or somebody who has any familiarity with the code :-)

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Not that familiar with stubgen, but this mostly looks good to me. I had one question

mypy/stubgenc.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

Thank you! :-)

@hauntsaninja hauntsaninja merged commit 35f402c into python:master Jan 8, 2024
14 checks passed
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.

stubgenc.py does not generate correct lines for staticmethods
6 participants