Add `pathlib` brain to handle `.parents` inference #1442

deepyaman · 2022-03-03T23:55:39Z

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

The PurePath.parents sequence supports slices since Python 3.10. This brain allows pylint to understand that passing an index (as opposed to a slice) to PurePath.parents will still yield a path, not a tuple.

Given test_index.py:

from pathlib import Path


def _main() -> None:
    current_path = Path().resolve()
    parent_path = current_path.parents[2].resolve()
    print(f"{current_path=}")
    print(f"{parent_path=}")


if __name__ == "__main__":
    _main()

Running pylint yields:

(astroid-3.10) WASH-178551-C02X31K9JHD4:customerone deepyaman_datta$ pylint test_index.py --disable missing-module-docstring

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

Given test_slice.py:

from pathlib import Path


def _main() -> None:
    current_path = Path().resolve()
    parent_paths = [p.resolve() for p in current_path.parents[:2]]
    print(f"{current_path=}")
    print(f"{parent_paths=}")


if __name__ == "__main__":
    _main()

Running pylint yields:

(astroid-3.10) WASH-178551-C02X31K9JHD4:customerone deepyaman_datta$ pylint test_slice.py --disable missing-module-docstring

--------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00)

Type of Changes

	Type
✓	🐛 Bug fix
	✨ New feature
	🔨 Refactoring
	📜 Docs

Related Issue

Closes with a test case pylint-dev/pylint#5783
Partially addresses pylint-dev/pylint#5832

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

DanielNoord

Thanks @deepyaman, left some initial comments!

I also updated your OP not to directly close the pylint issue. We tend to fix issues in astroid and then add a regression test to pylint to fully close the issue. If you want you can make a PR with a regression test for pylint as well!

astroid/brain/brain_pathlib.py

tests/unittest_brain_pathlib.py

DanielNoord · 2022-03-04T08:50:26Z

tests/unittest_brain_pathlib.py

@@ -0,0 +1,42 @@
+import pytest


Just for "totality" could you also add a test for the inference of parents itself?

I feel like I'm almost there, but it's not currently working for:

from pathlib import Path def _main() -> None: current_path = Path().resolve() path_parents = current_path.parents parent_path = path_parents[2].resolve() print(f"{current_path=}") print(f"{parent_path=}") if __name__ == "__main__": _main()

It fails with the same test.py:7:18: E1101: Instance of 'tuple' has no 'resolve' member (no-member).

Seems to be why my attempt at adding a nicer-looking test failed:

def test_inference_parents_subscript_index() -> None: """Test inference of ``pathlib.Path.parents``, accessed by index.""" parents, path = astroid.extract_node( """ from pathlib import Path current_path = Path().resolve() path_parents = current_path.parents path_parents #@ path_parents[2] #@ """ ) inferred = parents.inferred() assert len(inferred) == 1 assert isinstance(inferred[0], bases.Instance) assert inferred[0].qname() == "pathlib._PathParents" inferred = path.inferred() assert len(inferred) == 1 if PY310_PLUS: assert isinstance(inferred[0], bases.Instance) assert inferred[0].qname() == "pathlib.Path" else: assert inferred[0] is Uninferable

astroid/brain/brain_pathlib.py

deepyaman · 2022-03-04T15:12:07Z

@DanielNoord Thank you for the detailed feedback! I will try and implement the requested changes/feedback over the next few days and get back to you. :)

astroid/brain/brain_pathlib.py

tests/unittest_brain_pathlib.py

astroid/brain/brain_pathlib.py

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

This reverts commit e5b2e18.

DanielNoord · 2022-04-28T10:02:32Z

@deepyaman Sorry for letting this rest so long. I have been quite busy and haven't had the time to look into this enough. I hope to find some time for it this weekend. Again, sorry for the long delay!

deepyaman · 2022-04-28T10:41:42Z

@deepyaman Sorry for letting this rest so long. I have been quite busy and haven't had the time to look into this enough. I hope to find some time for it this weekend. Again, sorry for the long delay!

No problem, and thanks again for taking the time to guide me through this!

DanielNoord

@deepyaman Sorry this took so long to review. When I finally got to it I was in so deep that it seemed easier to just push the fixes I found.

Sorry for taking over your PR with prior notice like that.

In this new iteration we now also correctly infer an index lookup. I do this by creating an inference of the normal ClassDef for Path and then instantiating an instance from it.
I am not completely sure Uninferable for a Slice is correct on <3.9. I'll let another maintainer comment on that: do we infer Uninferable for code that would raise warnings?

I'll re-ask @cdce8p for a review since he had some valuable input in the first round.

deepyaman

@DanielNoord No problem! Thanks for resolving it!

deepyaman · 2022-06-03T16:24:47Z

tests/unittest_brain_pathlib.py

@@ -72,7 +77,7 @@ class A:
        parents = 42

    c = A()
-    error = c.parents[2]
+    error = c.parents[:2]


Did this have to be changed?

Originally I hadn't fixed the normal index look up, so this was not really testing anything:
The brain returned Uninferable and the non-brain would return Uninferable. So we weren't actually testing if this doesn't actually enter the brain. By using the slice we can see that this doesn't trigger the brain as it would give a different result for PY3.10+.

Now that I fixed the brain to work with index as well the change is perhaps not necessary anymore.

astroid/brain/brain_pathlib.py

Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

ChangeLog

cdce8p · 2022-06-05T16:26:14Z

astroid/brain/brain_pathlib.py

+PATH = extract_node(
+    """from pathlib import Path
+Path
+"""
+)
+PATH_CLASSDEF: nodes.ClassDef = next(PATH.infer())


Inference calls are non-trivial. I'm don't think it's a good idea to do them at import. Better to only define the code template as global const and call extract_node (or even _extract_single_node) in infer_parents_subscript.

But it isn't it better to use the actual ClassDef? You're more experienced here, but this seems to pass tests and doesn't this give us more information than a constructed mimic of Path?

It's probably fine to use extract_node and infer to get the ClassDef, just don't do it at Module level / import time.

As example, this is how the typing brain does something similar
https://github.com/PyCQA/astroid/blob/e57ac42bca80c105bbdad4bae61637b39288a47b/astroid/brain/brain_typing.py#L99-L103
https://github.com/PyCQA/astroid/blob/e57ac42bca80c105bbdad4bae61637b39288a47b/astroid/brain/brain_typing.py#L167-L168

I'll refactor!

astroid/brain/brain_pathlib.py

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

ChangeLog

cdce8p · 2022-06-05T21:05:57Z

astroid/brain/brain_pathlib.py

+PATH = extract_node(
+    """from pathlib import Path
+Path
+"""
+)
+PATH_CLASSDEF: nodes.ClassDef = next(PATH.infer())


It's probably fine to use extract_node and infer to get the ClassDef, just don't do it at Module level / import time.

As example, this is how the typing brain does something similar
https://github.com/PyCQA/astroid/blob/e57ac42bca80c105bbdad4bae61637b39288a47b/astroid/brain/brain_typing.py#L99-L103
https://github.com/PyCQA/astroid/blob/e57ac42bca80c105bbdad4bae61637b39288a47b/astroid/brain/brain_typing.py#L167-L168

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

…atch-1

cdce8p · 2022-06-05T21:40:09Z

@DanielNoord I moved extract_node inside infer_parents_subscript as well. We should try to do as little computation as possible in the global scope. With iter, I switched the one-item tuple with a list. Although correct, it's quite easy to accidentally remove the trailing comma or overlook it. A list is a bit safer there.

cdce8p

Good to merge once the CI checks pass.

DanielNoord · 2022-06-05T21:41:20Z

@DanielNoord I moved extract_node inside infer_parents_subscript as well. We should try to do as little computation as possible in the global scope. With iter, I switched the one-item tuple with a list. Although correct, it's quite easy to accidentally remove the trailing comma or overlook it. A list is a bit safer there.

Thanks! I'm still finding my way through the astroid brains. I'll keep the global computation in mind 😄

cdce8p · 2022-06-05T21:44:28Z

I'll keep the global computation in mind 😄

Off-topic: There is an open proposal for lazy imports in Python: PEP 690. Thankfully opt-in, but nevertheless will be quite challenging as it is to make pylint and astroid compatible.

DanielNoord · 2022-06-05T21:51:02Z

Thanks @deepyaman for the initial PR and thanks to @cdce8p for the reviews!

Happy to finally see this merged!

deepyaman added 2 commits March 3, 2022 18:52

Create brain_pathlib.py

7aa14ed

Update ChangeLog

cdd3e4b

deepyaman changed the title ~~Create brain_pathlib.py~~ Add pathlib brain to handle parents in Python 3.10 Mar 4, 2022

pre-commit-ci bot and others added 4 commits March 4, 2022 00:04

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

488c6e2

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

Create unittest_brain_pathlib.py

4d07baf

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

f58c7f4

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

Update unittest_brain_pathlib.py

019e4dd

deepyaman marked this pull request as ready for review March 4, 2022 02:43

deepyaman added 2 commits March 3, 2022 23:30

Add type hints

9087ffb

Fix type hints

d37953c

Pierre-Sassoulas added Bug 🪳 Needs review 🔍 Needs to be reviewed by one or multiple more persons python 3.10 labels Mar 4, 2022

Pierre-Sassoulas added this to the 2.11.0 milestone Mar 4, 2022

DanielNoord self-requested a review March 4, 2022 07:51

DanielNoord reviewed Mar 4, 2022

View reviewed changes

DanielNoord removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Mar 4, 2022

cdce8p self-requested a review March 4, 2022 21:04

cdce8p requested changes Mar 6, 2022

View reviewed changes

cdce8p modified the milestones: 2.11.0, 2.12.0 Mar 9, 2022

deepyaman and others added 8 commits March 10, 2022 17:23

Use from astroid import nodes as per code review

b98706a

Remove redundant isinstance(nodes.Subscript) check

0aee344

Ensure that inference tip is only applied to paths

b7d2969

Merge branch 'main' into patch-1

9a663d4

Test that non-path unit test fails without changes

e5b2e18

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

e003e39

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

Fix old variable name, and standardize the headers

2e38eba

Revert "Test that non-path unit test fails without changes"

8e0b826

This reverts commit e5b2e18.

DanielNoord self-requested a review April 28, 2022 10:01

deepyaman and others added 2 commits May 16, 2022 22:02

Merge branch 'main' into patch-1

c4ba539

Fix typing and inference of index look up

55778fb

DanielNoord approved these changes Jun 2, 2022

View reviewed changes

DanielNoord requested review from cdce8p and removed request for cdce8p June 2, 2022 08:36

DanielNoord added the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jun 2, 2022

deepyaman commented Jun 3, 2022

View reviewed changes

Update astroid/brain/brain_pathlib.py

be6afe8

Co-authored-by: Deepyaman Datta <deepyaman.datta@utexas.edu>

cdce8p requested changes Jun 5, 2022

View reviewed changes

DanielNoord and others added 2 commits June 5, 2022 22:55

Move changelog

f878b7f

Update astroid/brain/brain_pathlib.py

3b13695

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

cdce8p reviewed Jun 5, 2022

View reviewed changes

DanielNoord and others added 4 commits June 5, 2022 23:14

Don't infer at module level

da526bb

Update ChangeLog

f37febf

Co-authored-by: Marc Mueller <30130371+cdce8p@users.noreply.github.com>

Merge branch 'patch-1' of https://github.com/deepyaman/astroid into p…

b355331

…atch-1

Move extract_node too

fa122f4

cdce8p approved these changes Jun 5, 2022

View reviewed changes

DanielNoord merged commit 9b114ad into pylint-dev:main Jun 5, 2022

deepyaman deleted the patch-1 branch June 7, 2022 00:45

This was referenced Jun 25, 2022

25% performance regression on main #1667

Closed

Avoid inferring all subscripted names in pathlib brain #1671

Merged

jacobtylerwalls removed the Needs review 🔍 Needs to be reviewed by one or multiple more persons label Jun 28, 2022

jacobtylerwalls mentioned this pull request Jun 30, 2022

Add regression test for #5832 pylint-dev/pylint#7105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `pathlib` brain to handle `.parents` inference #1442

Add `pathlib` brain to handle `.parents` inference #1442

deepyaman commented Mar 3, 2022 •

edited by DanielNoord

Loading

DanielNoord left a comment

DanielNoord Mar 4, 2022

deepyaman Mar 16, 2022

deepyaman commented Mar 4, 2022

DanielNoord commented Apr 28, 2022

deepyaman commented Apr 28, 2022

DanielNoord left a comment

deepyaman left a comment

deepyaman Jun 3, 2022

DanielNoord Jun 4, 2022

cdce8p Jun 5, 2022

DanielNoord Jun 5, 2022

cdce8p Jun 5, 2022

DanielNoord Jun 5, 2022

cdce8p Jun 5, 2022

cdce8p commented Jun 5, 2022

cdce8p left a comment

DanielNoord commented Jun 5, 2022

cdce8p commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

Add pathlib brain to handle .parents inference #1442

Add pathlib brain to handle .parents inference #1442

Conversation

deepyaman commented Mar 3, 2022 • edited by DanielNoord Loading

Steps

Description

Type of Changes

Related Issue

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

deepyaman commented Mar 4, 2022

DanielNoord commented Apr 28, 2022

deepyaman commented Apr 28, 2022

DanielNoord left a comment

Choose a reason for hiding this comment

deepyaman 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

cdce8p commented Jun 5, 2022

cdce8p left a comment

Choose a reason for hiding this comment

DanielNoord commented Jun 5, 2022

cdce8p commented Jun 5, 2022

DanielNoord commented Jun 5, 2022

Add `pathlib` brain to handle `.parents` inference #1442

Add `pathlib` brain to handle `.parents` inference #1442

deepyaman commented Mar 3, 2022 •

edited by DanielNoord

Loading