Fix regression with import resolver #1211

DanielNoord · 2021-10-12T08:14:22Z

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 fixes the regression reported in pylint-dev/pylint#5131

Basically the previous iteration of the import resolver wrongly assumed that the current path was also the directory of the module which was checked for existence of __init__.py. We now check whether the nodes.Module has a path and then check the directory of that path for a __init__.py.
The check for existence of self.path is because test_relative_to_absolute_name in tests/unittest_scoped_nodes.py failed. I wonder if it is actually relevant for any real examples as I think a nodes.Module should always have a path, but I think that is something for another time.

While I was working on this I also added basic typing to the function in the new format.

Type of Changes

	Type
✓	🐛 Bug fix

Related Issue

Ref. #1186, #1200, pylint-dev/pylint#5059

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

Pierre-Sassoulas

Amazing 🎉 This is going to remove so much false positives.

cdce8p · 2021-10-12T11:09:35Z

@DanielNoord I think you accidentally committed the first commit to main this morning. I reverted it, as black was failing.
https://github.com/PyCQA/astroid/commits/main

The changes here seem to work, I'll take a closer look next.

Pierre-Sassoulas · 2021-10-12T12:09:17Z

accidentally committed the first commit to main

We might want to protect this branch if this happen more often, but this is very convenient to be able to push directly when releasing.

cdce8p · 2021-10-12T12:21:13Z

accidentally committed the first commit to main

We might want to protect this branch if this happen more often, but this is very convenient to be able to push directly when releasing.

Maybe something like this: https://github.com/pre-commit/pre-commit-hooks#no-commit-to-branch ?

  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.0.1
    hooks:
      - id: no-commit-to-branch
        args:
          - --branch=main

That would only be a pre-commit check, thus it could be skipped by using git commit -n.

--
Edit: Actually main is a default for no-commit-to-branch, so no args needed.

DanielNoord · 2021-10-12T12:23:28Z

I think it was because I pressed 'sync' in VS Code for the 'pylint' branch which included the tests. Because I had opened a file from 'astroid' in that workplace previously it opened the local 'astroid' repo, which was based on main. Even after closing said file, the repo remains in the source control tab.
Syncing can thus also sync repo's that are no longer in the workspace... Going to disable that button. 😅

cdce8p

Did some last tests: The change works as expected. Thanks @DanielNoord 🚀

Can you add a small changelog entry? The line from the pylint PR should work (together with closes ...).

Interestingly, I wasn't able to make the test you added fail for astroid, only for pylint. Maybe you have an idea, if not don't know if it makes sense to add something that doesn't test anything.

DanielNoord · 2021-10-12T13:25:01Z

Did some last tests: The change works as expected. Thanks @DanielNoord

I think we might want to do some tests for some of the other issues reported with namespace packages, as this commit might help bring those closer to a solution.

Can you add a small changelog entry? The line from the pylint PR should work (together with closes ...).

Will do this this afternoon and merge afterwards!

Interestingly, I wasn't able to make the test you added fail for astroid, only for pylint. Maybe you have an idea, if not don't know if it makes sense to add something that doesn't test anything.

I have no idea. @Pierre-Sassoulas and I struggled to come up with good tests in #1186 and he finally merged it with the tests I included. I also don't know how we should test this in astroid. Without this fix it is still able to read and analyse the files, but does so incorrectly. However, incorrect analyzation of files is mostly tested in pylint..

cdce8p · 2021-10-12T13:27:37Z

Interestingly, I wasn't able to make the test you added fail for astroid, only for pylint. Maybe you have an idea, if not don't know if it makes sense to add something that doesn't test anything.

I have no idea. @Pierre-Sassoulas and I struggled to come up with good tests in #1186 and he finally merged it with the tests I included. I also don't know how we should test this in astroid. Without this fix it is still able to read and analyse the files, but does so incorrectly. However, incorrect analyzation of files is mostly tested in pylint..

In that case, maybe it's not worth keeping this one in astroid if it isn't testing anything.

DanielNoord · 2021-10-12T14:41:20Z

Do we want to add other tests or are we then going to merge this without any tests?

And if yes, how are we going to come up with those test.. 😄

And, do we want to remove the (probably) non-sensical tests introduced with #1186 and #1200?

…import

cdce8p · 2021-10-12T15:12:02Z

Do we want to add other tests or are we then going to merge this without any tests?
And if yes, how are we going to come up with those test.. 😄

Usually I would prefer tests, but tbh I couldn't come up with one that worked. What might work would be a strict unit test just for relative_to_absolute_name, although that will probably break way too easily and also won't be of much use.

Don't know if @Pierre-Sassoulas has any ideas.

And, do we want to remove the (probably) non-sensical tests introduced with #1186 and #1200?

I would say we should remove the test added with this PR. I haven't tested it, but the other ones might actually work. There we needed to test that build_file actually worked (I think 😅). So maybe just leave them be for now.

--
In the end, it's definitely useful that we still have pylint-dev/pylint#5059

Pierre-Sassoulas · 2021-10-13T19:45:51Z

Maybe it's the time to put "real world" test in place where we run pylint on open-source python code available online. numpy, pandas, requests, colorama, botocore, setuptools, ... there's a lot of choices and probably some variety and some namespace packages too. We could run those only for "risky" changes. Real world examples are hard to do well. black is doing something similar I don't remember how they call it. It would reduce the risk we take during release greatly. Plus there are already some slow tests that we're not running in the CI for pylint (acceptance).

cdce8p · 2021-10-13T22:23:54Z

Maybe it's the time to put "real world" test in place where we run pylint on open-source python code available online. numpy, pandas, requests, colorama, botocore, setuptools, ... there's a lot of choices and probably some variety and some namespace packages too. We could run those only for "risky" changes. Real world examples are hard to do well. black is doing something similar I don't remember how they call it. It would reduce the risk we take during release greatly. Plus there are already some slow tests that we're not running in the CI for pylint (acceptance).

pylint-dev/pylint#4413 😄
Unfortunately, it's easier said than done. Mypy has mypy_primer, black has black_primer. The difference is that both are focused on maintaining stability whereas we constantly add new things as well. I see it with Home Assistant where I do something similar just manually. It's a lot of effort just keeping the repos in sync, comparing new error messages, checking if they are intended or false-positives, updating the existing code and pushing the changes back to Home Assistant.

What I'm trying to say: I would love it if we could automate it, just not sure it's actually possible / useful for our development cycle.

DanielNoord · 2021-10-14T07:52:23Z

There is:
https://github.com/PyCQA/astroid/blob/d4674781d974bd73ca3bfe82fd0816684f2a97d4/tests/unittest_scoped_nodes.py#L201-L236

However, this particular issue depends on file being present in the directory of the checked module. Which I think can't be emulated in such a simple function as these two.

Pierre-Sassoulas · 2021-10-14T19:00:22Z

The difference is that both are focused on maintaining stability whereas we constantly add new things as well.

Ha, very true. But we can check that pylint is "not crashing". And I think we could check for the presence of error. There's a very slim possibility that there is a real error in a release of those very big libraries but I would bet on a pylint false positive or regression every time. And in particular if suddenly there's a ton of import-error or no-module-error because namespace packages are broken we would detect it.

DanielNoord requested a review from cdce8p October 12, 2021 08:14

DanielNoord added the Bug 🪳 label Oct 12, 2021

DanielNoord added this to the 2.8.3 milestone Oct 12, 2021

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

3256c06

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

This was referenced Oct 12, 2021

Add test for relative-beyond-top-level false positive pylint-dev/pylint#5059

Merged

Relative imports from utils modules give no-name-in-module pylint-dev/pylint#5131

Closed

Pierre-Sassoulas approved these changes Oct 12, 2021

View reviewed changes

DanielNoord and others added 2 commits October 12, 2021 13:12

Fix regression with import resolver

d9bdcce

Fix black

677d15e

cdce8p force-pushed the import branch from b2f0d9e to 677d15e Compare October 12, 2021 11:14

cdce8p added the pylint-tested PRs that don't cause major regressions with pylint label Oct 12, 2021

cdce8p reviewed Oct 12, 2021

View reviewed changes

DanielNoord added 2 commits October 12, 2021 16:42

Merge branch 'import' of https://github.com/DanielNoord/astroid into …

786b508

…import

Add Changelog

0fad7c9

Pierre-Sassoulas mentioned this pull request Oct 15, 2021

Primer acceptance tests like in black or mypy pylint-dev/pylint#4413

Closed

Merge branch 'main' into import

895598e

Pierre-Sassoulas merged commit abfad38 into pylint-dev:main Oct 17, 2021

DanielNoord deleted the import branch October 17, 2021 19:05

DanielNoord mentioned this pull request Jun 20, 2022

Correct typing of relative_to_absolute_name #1637

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression with import resolver #1211

Fix regression with import resolver #1211

DanielNoord commented Oct 12, 2021

Pierre-Sassoulas left a comment

cdce8p commented Oct 12, 2021

Pierre-Sassoulas commented Oct 12, 2021

cdce8p commented Oct 12, 2021 •

edited

Loading

DanielNoord commented Oct 12, 2021

cdce8p left a comment

DanielNoord commented Oct 12, 2021

cdce8p commented Oct 12, 2021

DanielNoord commented Oct 12, 2021 •

edited

Loading

cdce8p commented Oct 12, 2021

Pierre-Sassoulas commented Oct 13, 2021

cdce8p commented Oct 13, 2021

DanielNoord commented Oct 14, 2021

Pierre-Sassoulas commented Oct 14, 2021

Fix regression with import resolver #1211

Fix regression with import resolver #1211

Conversation

DanielNoord commented Oct 12, 2021

Steps

Description

Type of Changes

Related Issue

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

cdce8p commented Oct 12, 2021

Pierre-Sassoulas commented Oct 12, 2021

cdce8p commented Oct 12, 2021 • edited Loading

DanielNoord commented Oct 12, 2021

cdce8p left a comment

Choose a reason for hiding this comment

DanielNoord commented Oct 12, 2021

cdce8p commented Oct 12, 2021

DanielNoord commented Oct 12, 2021 • edited Loading

cdce8p commented Oct 12, 2021

Pierre-Sassoulas commented Oct 13, 2021

cdce8p commented Oct 13, 2021

DanielNoord commented Oct 14, 2021

Pierre-Sassoulas commented Oct 14, 2021

cdce8p commented Oct 12, 2021 •

edited

Loading

DanielNoord commented Oct 12, 2021 •

edited

Loading