Get argument infos from method descriptors #1368

tristanlatr · 2022-01-24T00:31:58Z

Steps

For new features or bug fixes, add a ChangeLog entry describing what your PR does.
Write a good description on what the PR does.

Description

This PR adds argument infos from method descriptors, meaning from functions defined in python C-extensions.

Type of Changes

	Type
✓	✨ New feature
✓	🔨 Refactoring

astroid/raw_building.py

for more information, see https://pre-commit.ci

tristanlatr · 2022-01-24T05:12:34Z

tests/unittest_builder.py

+    def _find_dot_so_files(path: Path) -> Iterator[Path]:
+        for entry in path.iterdir():
+            if entry.suffix in importlib.machinery.EXTENSION_SUFFIXES:
+                yield entry
+
+    def _import_module(path: Path, module_full_name: str) -> types.ModuleType:
+        spec = importlib.util.spec_from_file_location(module_full_name, path)
+        if spec is None:
+            raise RuntimeError(
+                f"Cannot find spec for module {module_full_name} at {path}"
+            )
+        py_mod = importlib.util.module_from_spec(spec)
+        loader = spec.loader
+        assert isinstance(loader, importlib.abc.Loader), loader
+        loader.exec_module(py_mod)
+        return py_mod


There is probably a utility in astroid to do this...

I assume it's OK to include these private helpers, it's only going to be used in the tests. And I didn't see anything the does such imports in astroid, but maybe I did not look at the right place.

tristanlatr · 2022-01-24T18:05:40Z

tests/unittest_builder.py

@@ -790,5 +797,53 @@ def test_parse_module_with_invalid_type_comments_does_not_crash():
    assert isinstance(node, nodes.Module)


+def test_c_module_text_signature():


I think this test cannot pass on pypy, since they do not support CPython extensions natively.

We should be able to tell pytest to skip it right?

Although I must confess I don't know if this is actually true. Did you find any documentation about this?

We should be able to tell pytest to skip it right?

Yes we can

Although I must confess I don't know if this is actually true. Did you find any documentation about this?

Yes I've read that from pypy's documentation, but I can't find the link now. But here's what the pypy wiki says: "There is a compatibility layer for CPython C API extensions called CPyExt, but it is incomplete and experimental.". So I'm positive that, by default, pypy has no support for c-modules built for cpython.

Let's skip then!

tristanlatr · 2022-01-24T20:48:45Z

astroid/raw_building.py

-    # set node's arguments to None to notice that we have no information, not
-    # and empty argument list


To still be inline with this, I should set None to args if we get a ValueError calling signature.signature().

Yes! Let's do that!

for more information, see https://pre-commit.ci

tristanlatr · 2022-02-16T18:19:07Z

Also it seems that the python 3.10 tests are consistently failling the following three tests:

tests/unittest_brain.py::SixBrainTest::test_attribute_access 
tests/unittest_brain.py::test_http_client_brain
tests/unittest_regrtest.py::NonRegressionTests::test_ssl_protocol

I'll looks into that, but I'de like to hear opinions in favour of such features before I spend too much time on it and at the end it does not fit in your agenda..

Thanks.

Still Todo:

Change the Note in the docstring of Arguments.args: https://pylint.pycqa.org/projects/astroid/en/latest/api/astroid.nodes.html#astroid.nodes.Arguments.args saying that we now get infos from function defined in c-extensions.
Skip c-module test on pypy
args should be None if we get a ValueError calling signature.signature()

for more information, see https://pre-commit.ci

tristanlatr · 2022-03-25T19:37:03Z

Up!

DanielNoord · 2022-03-28T08:33:19Z

@tristanlatr Could you resolve the merge conflicts?

tristanlatr · 2022-03-28T14:31:43Z

@DanielNoord,

Conflicts are resolved, but I've not yet lint the code. There are still a couple points to discuss first.

Thanks

tristanlatr · 2022-03-28T14:35:19Z

It looks like linux python 3.10 tests are sill failing, any clue @DanielNoord ?

Thanks

Talk to you later

DanielNoord

I find it difficult to review this as I have absolutely zero knowledge of C-extensions or C in general. I'm not sure we actually have anybody in the maintainers team who does..

At least I answered one question 😅

DanielNoord · 2022-03-29T20:14:54Z

astroid/raw_building.py

-    """create astroid for a living function object"""
-    signature = inspect.signature(member)
+def _get_args_info_from_callable(
+    member: Callable,


Do we know more about this Callable?

No, it can be any python callable.

Okay, so I finally got some time to review this after having spent some time in the module myself (see #1589).

I don't think this typing is correct as normally all members are either type or narrowed down types from the types packages.

Can you more explicit and maybe suggest another typing for the function ?

astroid/raw_building.py

DanielNoord · 2022-03-29T20:16:32Z

astroid/raw_building.py

-    # set node's arguments to None to notice that we have no information, not
-    # and empty argument list


Yes! Let's do that!

DanielNoord · 2022-03-29T20:16:49Z

tests/testdata/python3/data/c_module_invalid_text_signature/setup.py

+    name="mymod",
+    ext_modules=[cmodule],
+    packages=['mymod'],
+)


Suggested change

)

)

DanielNoord · 2022-03-29T20:17:26Z

tests/unittest_builder.py

@@ -790,5 +797,53 @@ def test_parse_module_with_invalid_type_comments_does_not_crash():
    assert isinstance(node, nodes.Module)


+def test_c_module_text_signature():


We should be able to tell pytest to skip it right?

Although I must confess I don't know if this is actually true. Did you find any documentation about this?

tristanlatr · 2022-03-29T20:38:09Z

There is still the issue of the failing tests...
I'll work on this when I have some time.

This is definitely a required feature for pydoctor to adopt astroid, because we offer support for c-modules.
I hope in can be merged once I've fixed the issues.

Talk to you later,

Tristan

DanielNoord · 2022-06-04T10:56:43Z

astroid/raw_building.py

-    """create astroid for a living function object"""
-    signature = inspect.signature(member)
+def _get_args_info_from_callable(
+    member: Callable,


Okay, so I finally got some time to review this after having spent some time in the module myself (see #1589).

I don't think this typing is correct as normally all members are either type or narrowed down types from the types packages.

DanielNoord · 2022-06-04T10:59:36Z

astroid/raw_building.py

+        # __text_signature__ attribute
+        if getattr(member, "__text_signature__", None) is not None:
+            f"Cannot parse __text_signature__ signature of {member}"
+        signature = inspect.Signature()


This is a change in behaviour, at least for ClassDefs. Instead of setting the arguments to None we now set it to empty lists, implying that there are no arguments. That should be reverted to mimic the current situation.

Yes you are right, this was part of the stuff that I needed to change.

DanielNoord · 2022-06-04T10:59:36Z

astroid/raw_building.py

+        # __text_signature__ attribute
+        if getattr(member, "__text_signature__", None) is not None:
+            f"Cannot parse __text_signature__ signature of {member}"
+        signature = inspect.Signature()


This is a change in behaviour, at least for ClassDefs. Instead of setting the arguments to None we now set it to empty lists, implying that there are no arguments. That should be reverted to mimic the current situation.

DanielNoord · 2022-06-04T11:04:00Z

@tristanlatr I have done some exploring in the module and am now more confident in merging changes here. Please let me know if you have any further questions. We should be cautious about changing stuff here, but I'm willing to help get this merged.

One thing I believe would help is if you separated the creation of the new method from also using it in object_build_methoddescriptor. That makes this the diff smaller and more reviewable.

tristanlatr · 2022-06-04T15:59:18Z

Hi @DanielNoord,

I don’t think we’re going to use astroid with pydoctor in the end, turns out to be much more complicated than I thought because of divergences compared to the standard library.

So I probably won’t update this PR anytime soon. If one likes this feature please fell free to build off my changes and open another PR.

Cheers,

DanielNoord · 2022-06-05T07:50:11Z

Hi @DanielNoord,

I don’t think we’re going to use astroid with pydoctor in the end, turns out to be much more complicated than I thought because of divergences compared to the standard library.

So I probably won’t update this PR anytime soon. If one likes this feature please fell free to build off my changes and open another PR.

Cheers,

Thanks for the headups! I'll likely cherry pick some changes from this PR and see what I can merge without too much trouble.

DanielNoord · 2022-06-05T09:54:53Z

@tristanlatr I have opened #1593 based on your work and some previous cherry picking. I'm going to close this PR as the discussion is quite long and most issues have been resolved in that PR.

Would you be willing to do a review of that PR? I think most issue have been addressed, but considering I have no experience with C I'd like some other reviews before undrafting it.

tristanlatr added 2 commits January 23, 2022 19:27

get argument infos from method descriptors

7d0f650

add a test for the signature handling with method descriptors

d8f82ad

tristanlatr commented Jan 24, 2022

View reviewed changes

astroid/raw_building.py Show resolved Hide resolved

[pre-commit.ci] auto fixes from pre-commit.com hooks

58f7b71

for more information, see https://pre-commit.ci

tristanlatr commented Jan 24, 2022

View reviewed changes

Pierre-Sassoulas added this to the 2.10.0 milestone Jan 24, 2022

Pierre-Sassoulas added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jan 24, 2022

tristanlatr commented Jan 24, 2022

View reviewed changes

DanielNoord self-requested a review January 24, 2022 18:10

tristanlatr commented Jan 24, 2022

View reviewed changes

tristanlatr and others added 2 commits February 16, 2022 13:10

Merge branch 'dev'

6adc1df

[pre-commit.ci] auto fixes from pre-commit.com hooks

f356627

for more information, see https://pre-commit.ci

cdce8p modified the milestones: 2.10.0, 2.11.0 Feb 26, 2022

tristanlatr and others added 2 commits February 28, 2022 17:14

Merge branch 'main' into main

8a6cead

[pre-commit.ci] auto fixes from pre-commit.com hooks

7a29b45

for more information, see https://pre-commit.ci

Pierre-Sassoulas self-requested a review March 25, 2022 19:42

Merge branch 'main' into main

846c13c

DanielNoord reviewed Mar 29, 2022

View reviewed changes

DanielNoord reviewed Jun 4, 2022

View reviewed changes

DanielNoord mentioned this pull request Jun 5, 2022

Create _get_args_info_from_callable #1592

Merged

1 task

DanielNoord removed this from the 2.12.0 milestone Jun 5, 2022

DanielNoord mentioned this pull request Jun 5, 2022

Get argument info from method descriptors #1593

Closed

2 tasks

DanielNoord closed this Jun 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get argument infos from method descriptors #1368

Get argument infos from method descriptors #1368

tristanlatr commented Jan 24, 2022 •

edited

Loading

tristanlatr Jan 24, 2022

tristanlatr Apr 1, 2022

tristanlatr Jan 24, 2022

DanielNoord Mar 29, 2022

tristanlatr Mar 29, 2022

DanielNoord Mar 29, 2022

tristanlatr Jan 24, 2022

DanielNoord Mar 29, 2022

tristanlatr commented Feb 16, 2022

tristanlatr commented Mar 25, 2022

DanielNoord commented Mar 28, 2022

tristanlatr commented Mar 28, 2022

tristanlatr commented Mar 28, 2022

DanielNoord left a comment

DanielNoord Mar 29, 2022

tristanlatr Mar 29, 2022

DanielNoord Jun 4, 2022

tristanlatr Jun 4, 2022

DanielNoord Mar 29, 2022

DanielNoord Mar 29, 2022

DanielNoord Mar 29, 2022

tristanlatr commented Mar 29, 2022

DanielNoord Jun 4, 2022

DanielNoord Jun 4, 2022

tristanlatr Jun 4, 2022

DanielNoord Jun 4, 2022

DanielNoord commented Jun 4, 2022

tristanlatr commented Jun 4, 2022

DanielNoord commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

		@@ -790,5 +797,53 @@ def test_parse_module_with_invalid_type_comments_does_not_crash():
		assert isinstance(node, nodes.Module)


		def test_c_module_text_signature():

		# set node's arguments to None to notice that we have no information, not
		# and empty argument list

Get argument infos from method descriptors #1368

Get argument infos from method descriptors #1368

Conversation

tristanlatr commented Jan 24, 2022 • edited Loading

Steps

Description

Type of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristanlatr commented Feb 16, 2022

tristanlatr commented Mar 25, 2022

DanielNoord commented Mar 28, 2022

tristanlatr commented Mar 28, 2022

tristanlatr commented Mar 28, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tristanlatr commented Mar 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielNoord commented Jun 4, 2022

tristanlatr commented Jun 4, 2022

DanielNoord commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

tristanlatr commented Jan 24, 2022 •

edited

Loading