Stop detecting modules compiled by `cffi` as namespace packages #1777

jacobtylerwalls · 2022-09-10T22:30:12Z

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 is_namespace() changes in 2.12 broke introspection of modules compiled by cffi, which as quoted in the issue, patch sys.modules. The only reason we are looking at sys.modules is to support the edge case of old pkg_resources-style namespace packages. The principle of false negatives are better than letting things crash suggests that we should just loosen the check to top level modnames ("pylint" rather than "pylint.lint"), because that allows the cffi modules to not crash without failing the pkg_resources tests. (Another way of saying: I'm not aware of any false negatives we're causing with pkg_resources-style packages, since the tests pass, but I can't rule it out.)

No tests--did not want to introduce a test dependency on cffi.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Closes #1776
Closes pylint-dev/pylint#7399

@dalcinl, does this resolve your issue?

coveralls · 2022-09-10T22:33:01Z

Pull Request Test Coverage Report for Build 3073456457

1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.009%) to 92.42%

Files with Coverage Reduction	New Missed Lines	%
astroid/interpreter/_import/util.py	1	93.18%

Totals
Change from base Build 3073454630:	-0.009%
Covered Lines:	9790
Relevant Lines:	10593

💛 - Coveralls

dalcinl · 2022-09-11T07:19:44Z

@dalcinl, does this resolve your issue?

I'm having trouble to reproduce using my usual workflow.
Something fishy is going on with installation:

$ git clone -b cffi https://github.com/jacobtylerwalls/astroid
Cloning into 'astroid'...
remote: Enumerating objects: 28602, done.
remote: Counting objects: 100% (339/339), done.
remote: Compressing objects: 100% (169/169), done.
remote: Total 28602 (delta 229), reused 229 (delta 166), pack-reused 28263
Receiving objects: 100% (28602/28602), 16.02 MiB | 11.88 MiB/s, done.
Resolving deltas: 100% (17660/17660), done.

$ cd astroid/

$ pip install -U .
Defaulting to user installation because normal site-packages is not writeable
Processing /tmp/astroid
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: UNKNOWN
  Building wheel for UNKNOWN (pyproject.toml) ... done
  Created wheel for UNKNOWN: filename=UNKNOWN-0.0.0-py3-none-any.whl size=14192 sha256=fbeb6557180aaaf819a561e9dff4abd88a0f0b5f705145c133fceee03aadc793
  Stored in directory: /tmp/pip-ephem-wheel-cache-z9qsa0dx/wheels/f0/82/7c/de80ba3afb992294acfa1eb14309533e9df9441d37974f9de1
Successfully built UNKNOWN
Installing collected packages: UNKNOWN
  Attempting uninstall: UNKNOWN
    Found existing installation: UNKNOWN 0.0.0
    Uninstalling UNKNOWN-0.0.0:
      Successfully uninstalled UNKNOWN-0.0.0
Successfully installed UNKNOWN-0.0.0

Note that this happens when trying to install to the user site-packages. Virtualenv installations seems to be OK.
The following invocation should work as a quick reproducer to avoid messing with the default user site.

PYTHONUSERBASE=/tmp/user python -m pip install .

Pierre-Sassoulas · 2022-09-11T14:16:33Z

Could you try installing in a virtualenv with pip install pylint -U then pip install -U git+https://github.com/jacobtylerwalls/astroid@cffi, please ?

dalcinl · 2022-09-11T15:42:00Z

@Pierre-Sassoulas I think I've not been clear enough. Within a virtualenv (python -m venv), everything works OK. Even the last release of astroid is OK, no issue at all. If I install astroid from your cffi branch, it also work, not problem at all.

All the trouble happens when trying to install in the user site-packages directory:

pylint raises exception if using astroid last release (the issue this PR is addressing)
I cannot pip install -U git+https://github.com/jacobtylerwalls/astroid@cffi to test your branch.

Pierre-Sassoulas · 2022-09-11T19:33:02Z

All the trouble happens when trying to install in the user site-packages directory:

I don't know if I understood correctly what user site package is but wouldn't pip install -U git+https://github.com/jacobtylerwalls/astroid@cffi --user works then ?

dalcinl · 2022-09-12T05:44:28Z

I don't know if I understood correctly what user site package is but wouldn't pip install -U git+https://github.com/jacobtylerwalls/astroid@cffi --user works then ?

No! That's exactly what is broken (in recent pip releases, the --user flag is implicit if the user does not have permissions to install in the system site-packages):

$ pip install -U git+https://github.com/jacobtylerwalls/astroid@cffi --user
Collecting git+https://github.com/jacobtylerwalls/astroid@cffi
  Cloning https://github.com/jacobtylerwalls/astroid (to revision cffi) to /tmp/pip-req-build-sq86tbq0
  Running command git clone --filter=blob:none --quiet https://github.com/jacobtylerwalls/astroid /tmp/pip-req-build-sq86tbq0
  Running command git checkout -b cffi --track origin/cffi
  Switched to a new branch 'cffi'
  branch 'cffi' set up to track 'origin/cffi'.
  Resolved https://github.com/jacobtylerwalls/astroid to commit 108f675190b57bda9c67ae6c9dbfadf6b3fa2236
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: UNKNOWN
  Building wheel for UNKNOWN (pyproject.toml) ... done
  Created wheel for UNKNOWN: filename=UNKNOWN-0.0.0-py3-none-any.whl size=14192 sha256=2ab4f866e79db6988aa41d1ac39c7ea51f257b903cdd24180031697f5bbf2472
  Stored in directory: /tmp/pip-ephem-wheel-cache-_xn105fv/wheels/ab/08/1c/bc59387b93efa124d6c0ac7fdebb60a524ad44a805035d026d
Successfully built UNKNOWN
Installing collected packages: UNKNOWN
Successfully installed UNKNOWN-0.0.0

Note the UNKNOWN-0.0.0 package name and version!

I install stuff in the user site-packages directory (--user flag) all the time. This is the first time ever I see such failure. Maybe it is a pip/setuptools issue, but as I said, this is the first time ever I see this happening.

dalcinl · 2022-09-12T05:56:37Z

OK, this is what I did to manually try this PR

Clone your git repo at the cffi branch.
python -m build --wheel
pip install dist/astroid-2.13.0.dev0-py3-none-any.whl

After that, I ran pylint on my cffi-based project, and the issue is resolved.
Therefore, from my side, this PR is good to merge. Thank you for the quick fix!

However, the issue of installing with pip from a git repo is still there. My git clone is in "/tmp/astroid":

$ cd /tmp/astroid
$ pip install . --user
Processing /tmp/astroid
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Preparing metadata (pyproject.toml) ... done
Building wheels for collected packages: UNKNOWN
  Building wheel for UNKNOWN (pyproject.toml) ... done
  Created wheel for UNKNOWN: filename=UNKNOWN-0.0.0-py3-none-any.whl size=14192 sha256=960182c3b0f0ce5a4f50e2c58a20166c474365f4765533a49b9987c856fd0a6f
  Stored in directory: /tmp/pip-ephem-wheel-cache-1gupxqry/wheels/f0/82/7c/de80ba3afb992294acfa1eb14309533e9df9441d37974f9de1
Successfully built UNKNOWN
Installing collected packages: UNKNOWN
Successfully installed UNKNOWN-0.0.0

BTW, this issue occurs with the main branch also, so this is definitely not a regression from your fixes for cffi.

Pierre-Sassoulas · 2022-09-12T06:54:15Z

OK, thank you for walking me through it. We switched to pyproject.toml in #1670 this is probably the reason why it fail. What is your version of setuptools ?

rjarry · 2022-09-12T06:59:40Z

This commit fixes pylint-dev/pylint#7399

Thanks!

dalcinl · 2022-09-12T07:04:58Z

@rjarry I'm using latest pip and setuptools (both installed in the user site-packages directory) with Fedora 36 system Python 3.10.6.

$ python -m pip list --user | egrep "pip|setuptools"
pip                           22.2.2
setuptools                    65.3.0

rjarry · 2022-09-12T07:12:00Z

@dalcinl I do not install with --user. I am using a virtual env. Here are the package versions with which it works:

astroid @ git+https://github.com/jacobtylerwalls/astroid@108f675190b57bda9c67ae6c9dbfadf6b3fa2236
dill==0.3.5.1
isort==5.10.1
lazy-object-proxy==1.7.1
mccabe==0.7.0
platformdirs==2.5.2
pylint==2.15.2
tomli==2.0.1
tomlkit==0.11.4
wrapt==1.14.1

dalcinl · 2022-09-12T07:18:24Z

@rjarry Yes, I know, virtual envs are working fine. It is with the --user flag that things go wrong. Did you try? You can reproduce the issue the following way (to not alter your default user site-packages) by setting the PYTHONUSERSITE environment variable.

PYTHONUSERSITE=/tmp/user python -m pip install git+https://github.com/jacobtylerwalls/astroid@108f675190b57bda9c67ae6c9dbfadf6b3fa2236

DanielNoord · 2022-09-12T07:47:55Z

@jacobtylerwalls Do you know if cffi is also affected by #1752?

jacobtylerwalls · 2022-09-16T12:03:05Z

Do you know if cffi is also affected by #1752?

I don't, sorry.

#1752 won't be backported, so what do you think about going ahead and getting this bugfix out?

DanielNoord · 2022-09-16T12:04:23Z

Do you know if cffi is also affected by #1752?

I don't, sorry.

#1752 won't be backported, so what do you think about going ahead and getting this bugfix out?

I'll have a look if this is fixed by it. If so, this will probably become uncovered on 2.13 so we might want to add a TODO about it.
Changes itself look fine though so I'll merge after my investigation!

Pierre-Sassoulas

Thank you @jacobtylerwalls ! Let's release astroid 2.12.10 and pylint 2.15.3 once this is merged :)

DanielNoord

This is not affected by custom import hooks as fixed in #1752.

jacobtylerwalls · 2022-09-17T14:12:11Z

Thanks for investigating, I appreciate it!

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

See https://stackoverflow.com/a/42962529. Let's take the following contents as an example: ```python import celery.result ``` From pylint-dev#1777, astroid started to use `processed_components` for namespace check. In the above case, the `modname` is `celery.result`, it first checks for `celery` and then `celery.result`. Before that PR, it'd always check for `celery.result`. But if you have imported it as `celery.result`, `sys.modules["celery"].__spec__` is going to be `None`, and hence the function will return True, and but below where we try to load get `submodule_path`/`__path__` for `celery.result` will fail as it is not a package. See https://github.com/PyCQA/astroid/blob/056d8e5fab7a167f73115d524ab92170b3ed5f9f/astroid/interpreter/_import/spec.py#L205-L207 --- The `celery.result` gets imported for me when pylint-pytest plugin tries to load fixtures, but this could happen anytime if any plugin imports packages. In that case, `find_spec("celery")` will raise ValueError since it's already in `sys.modules` and does not have a spec. I still think there's a bug for the `ExplicitNamespacePackageFinder.find_module` for namespace packages, since `modname` might be `namespace.package.module` but since `is_namespace` package goes through components, the successive `sys.modules[modname].__path__` may raise error, since modname could be not a package in a namespaced package. But I am not quite sure about that."

See https://stackoverflow.com/a/42962529. Let's take the following contents as an example: ```python import celery.result ``` From pylint-dev#1777, astroid started to use `processed_components` for namespace check. In the above case, the `modname` is `celery.result`, it first checks for `celery` and then `celery.result`. Before that PR, it'd always check for `celery.result`. But if you have imported it as `celery.result`, `sys.modules["celery"].__spec__` is going to be `None`, and hence the function will return True, and but below where we try to load get `submodule_path`/`__path__` for `celery.result` will fail as it is not a package. See https://github.com/PyCQA/astroid/blob/056d8e5fab7a167f73115d524ab92170b3ed5f9f/astroid/interpreter/_import/spec.py#L205-L207 --- The `celery.result` gets imported for me when pylint-pytest plugin tries to load fixtures, but this could happen anytime if any plugin imports packages. In that case, `find_spec("celery")` will raise ValueError since it's already in `sys.modules` and does not have a spec. I still think there's a bug for the `ExplicitNamespacePackageFinder.find_module` for namespace packages, since `modname` might be `namespace.package.module` but since `is_namespace` package goes through components, the successive `sys.modules[modname].__path__` may raise error, since modname could be not a package in a namespaced package. But I am not quite sure about that. Fixes #7488.

See https://stackoverflow.com/a/42962529. Let's take the following contents as an example: ```python import celery.result ``` From pylint-dev#1777, astroid started to use `processed_components` for namespace check. In the above case, the `modname` is `celery.result`, it first checks for `celery` and then `celery.result`. Before that PR, it'd always check for `celery.result`. But if you have imported it as `celery.result`, `sys.modules["celery"].__spec__` is going to be `None`, and hence the function will return True, and but below where we try to load get `submodule_path`/`__path__` for `celery.result` will fail as it is not a package. See https://github.com/PyCQA/astroid/blob/056d8e5fab7a167f73115d524ab92170b3ed5f9f/astroid/interpreter/_import/spec.py#L205-L207 --- The `celery.result` gets imported for me when pylint-pytest plugin tries to load fixtures, but this could happen anytime if any plugin imports packages. In that case, `find_spec("celery")` will raise ValueError since it's already in `sys.modules` and does not have a spec. I still think there's a bug for the `ExplicitNamespacePackageFinder.find_module` for namespace packages, since `modname` might be `namespace.package.module` but since `is_namespace` package goes through components, the successive `sys.modules[modname].__path__` may raise error, since modname could be not a package in a namespaced package. But I am not quite sure about that. Fixes pylint-dev/pylint#7488.

See https://stackoverflow.com/a/42962529. Let's take the following contents as an example: ```python import celery.result ``` From pylint-dev#1777, astroid started to use `processed_components` for namespace check. In the above case, the `modname` is `celery.result`, it first checks for `celery` and then `celery.result`. Before that PR, it'd always check for `celery.result`. But if you have imported it as `celery.result`, `sys.modules["celery"].__spec__` is going to be `None`, and hence the function will return True, and but below where we try to load get `submodule_path`/`__path__` for `celery.result` will fail as it is not a package. See https://github.com/PyCQA/astroid/blob/056d8e5fab7a167f73115d524ab92170b3ed5f9f/astroid/interpreter/_import/spec.py#L205-L207 --- The `celery.result` gets imported for me when pylint-pytest plugin tries to load fixtures, but this could happen anytime if any plugin imports packages. In that case, `find_spec("celery")` will raise ValueError since it's already in `sys.modules` and does not have a spec. Fixes pylint-dev/pylint#7488.

See https://stackoverflow.com/a/42962529. Let's take the following contents as an example: ```python import celery.result ``` From pylint-dev#1777, astroid started to use `processed_components` for namespace check. In the above case, the `modname` is `celery.result`, it first checks for `celery` and then `celery.result`. Before that PR, it'd always check for `celery.result`. `celery` is recreating module to make it lazily load. See https://github.com/celery/celery/blob/34533ab44d2a6492004bc3df44dc04ad5c6611e7/celery/__init__.py#L150. This module does not have `__spec__` set. Reading through Python's docs, it seems that `__spec__` can be set to None, so it seems like it's not a thing that we can depend upon for namespace checks. See https://docs.python.org/3/reference/import.html#spec__. --- The `celery.result` gets imported for me when pylint-pytest plugin tries to load fixtures, but this could happen anytime if any plugin imports packages. In that case, `importlib.util._find_spec_from_path("celery")` will raise ValueError since it's already in `sys.modules` and does not have a spec. Fixes pylint-dev/pylint#7488.

See https://stackoverflow.com/a/42962529. Let's take the following contents as an example: ```python import celery.result ``` From #1777, astroid started to use `processed_components` for namespace check. In the above case, the `modname` is `celery.result`, it first checks for `celery` and then `celery.result`. Before that PR, it'd always check for `celery.result`. `celery` is recreating module to make it lazily load. See https://github.com/celery/celery/blob/34533ab44d2a6492004bc3df44dc04ad5c6611e7/celery/__init__.py#L150. This module does not have `__spec__` set. Reading through Python's docs, it seems that `__spec__` can be set to None, so it seems like it's not a thing that we can depend upon for namespace checks. See https://docs.python.org/3/reference/import.html#spec__. --- The `celery.result` gets imported for me when pylint-pytest plugin tries to load fixtures, but this could happen anytime if any plugin imports packages. In that case, `importlib.util._find_spec_from_path("celery")` will raise ValueError since it's already in `sys.modules` and does not have a spec. Fixes pylint-dev/pylint#7488.

Stop detecting modules compiled by cffi as namespace packages

108f675

jacobtylerwalls added Needs backport Crash 💥 labels Sep 10, 2022

jacobtylerwalls added this to the 2.12.10 milestone Sep 10, 2022

jacobtylerwalls added the pylint-tested label Sep 10, 2022

jacobtylerwalls mentioned this pull request Sep 11, 2022

Introspection of CFFI compiled modules is broken in 2.15 pylint-dev/pylint#7399

Closed

jacobtylerwalls requested a review from DanielNoord September 16, 2022 12:03

Pierre-Sassoulas approved these changes Sep 16, 2022

View reviewed changes

Merge branch 'main' into cffi

ed86795

DanielNoord approved these changes Sep 17, 2022

View reviewed changes

DanielNoord merged commit 18a303f into pylint-dev:main Sep 17, 2022

jacobtylerwalls deleted the cffi branch September 17, 2022 14:11

Pierre-Sassoulas added a commit that referenced this pull request Sep 17, 2022

Stop detecting modules compiled by cffi as namespace packages (#1777)

3fe9286

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

Pierre-Sassoulas added the Backported label Sep 17, 2022

Pierre-Sassoulas removed the Needs backport label Sep 17, 2022

Pierre-Sassoulas added a commit that referenced this pull request Sep 17, 2022

Stop detecting modules compiled by cffi as namespace packages (#1777)

b3f0ea7

Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>

skshetry mentioned this pull request Sep 21, 2022

Improve is_namespace() check for modules where __spec__ is None #1802

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop detecting modules compiled by `cffi` as namespace packages #1777

Stop detecting modules compiled by `cffi` as namespace packages #1777

jacobtylerwalls commented Sep 10, 2022 •

edited

Loading

coveralls commented Sep 10, 2022 •

edited

Loading

dalcinl commented Sep 11, 2022

Pierre-Sassoulas commented Sep 11, 2022

dalcinl commented Sep 11, 2022

Pierre-Sassoulas commented Sep 11, 2022

dalcinl commented Sep 12, 2022

dalcinl commented Sep 12, 2022

Pierre-Sassoulas commented Sep 12, 2022

rjarry commented Sep 12, 2022

dalcinl commented Sep 12, 2022

rjarry commented Sep 12, 2022

dalcinl commented Sep 12, 2022

DanielNoord commented Sep 12, 2022

jacobtylerwalls commented Sep 16, 2022

DanielNoord commented Sep 16, 2022

Pierre-Sassoulas left a comment

DanielNoord left a comment

jacobtylerwalls commented Sep 17, 2022

Stop detecting modules compiled by cffi as namespace packages #1777

Stop detecting modules compiled by cffi as namespace packages #1777

Conversation

jacobtylerwalls commented Sep 10, 2022 • edited Loading

Steps

Description

Type of Changes

Related Issue

coveralls commented Sep 10, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3073456457

💛 - Coveralls

dalcinl commented Sep 11, 2022

Pierre-Sassoulas commented Sep 11, 2022

dalcinl commented Sep 11, 2022

Pierre-Sassoulas commented Sep 11, 2022

dalcinl commented Sep 12, 2022

dalcinl commented Sep 12, 2022

Pierre-Sassoulas commented Sep 12, 2022

rjarry commented Sep 12, 2022

dalcinl commented Sep 12, 2022

rjarry commented Sep 12, 2022

dalcinl commented Sep 12, 2022

DanielNoord commented Sep 12, 2022

jacobtylerwalls commented Sep 16, 2022

DanielNoord commented Sep 16, 2022

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord left a comment

Choose a reason for hiding this comment

jacobtylerwalls commented Sep 17, 2022

Stop detecting modules compiled by `cffi` as namespace packages #1777

Stop detecting modules compiled by `cffi` as namespace packages #1777

jacobtylerwalls commented Sep 10, 2022 •

edited

Loading

coveralls commented Sep 10, 2022 •

edited

Loading