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

Bug pylint 2721 #664

Merged
merged 49 commits into from
Jun 2, 2019
Merged

Bug pylint 2721 #664

merged 49 commits into from
Jun 2, 2019

Conversation

hippo91
Copy link
Contributor

@hippo91 hippo91 commented Apr 21, 2019

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 is deep rework of the numpy brain.
The original brain has been split into numerous one. Each numpy brain module corresponds to
a numpyone. There are:

  • brain_numpy_core_fromnumeric => numpy.core.fromnumeric
  • brain_numpy_core_function_base => numpy.core.function_base
  • brain_numpy_core_multiarray => numpy.core.multiarray
  • brain_numpy_core_numeric => numpy.core.numeric
  • brain_core_numerictypes => numpy.core.numerictypes
  • brain_core_umath => numpy.core.umath
  • brain_numpy_random_mtrand => numpy.random.mtrand

Last but not least, the numpy.ndarray class has its corresponding astroid's brain (brain_numpy_ndarray).

Each astroid's brain has its own associated unit test.

The major improvement, besides a clearer organisation, is the fact that each method of the ndarray class should now be correctly inferred. Especially those returning ndarray objects.

It has been made possible by ensuring that the return type of those methods is inferred as ndarray and nothing else. To achieve this, the inference_tip function has been used.

When a numpy callable may return two different kinds of objects, for example bool or ndarray, the choice to prefer ndarray has been made in the corresponding astroid brain method.

There is still work to do, especially correcting the return types of the brain_numpy_random_mtrand and brain_numpy_core_numerictypes modules. The last one should also be reworked to correct method signature of the generic class.
But this PR should strongly improve the numpy support for pylint.

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring

Related Issue

Closes pylint-dev/pylint#2747
Closes pylint-dev/pylint#2721
Closes pylint-dev/pylint#2326
Closes pylint-dev/pylint#2021

Related to pylint-dev/pylint#2767
Related to pylint-dev/pylint#2694
Related to pylint-dev/pylint#2784
Related to pylint-dev/pylint#2759

… zeros_like function (pass) and filter tuple for type inferred for a call to zeros_like
…rom types inferred by call to full_like and ones_like
…ay from numpy_core_numerictypes_transform function to infer_numpy_ndarray which is used a inference_tip. Add a _looks_like_numpy_ndarray function. Deletion of the register_transform for linspace function because it is correctly inferred with the inference_tip function for ndarray
…ndarray' and not 'numpy.core.numerictypes.ndarray'
…ure of setitem method. Add of empty_like, ones_like function
…ey are redondant with the one test only ndarray are inferred
@hippo91
Copy link
Contributor Author

hippo91 commented Apr 26, 2019

This PR should also prevent infinite recursion for bug pylint-dev/pylint#2865

@wshanks
Copy link

wshanks commented Apr 26, 2019

For this file:

import numpy as np
a = np.append([0], 0)
a = -a

I still see:

/tmp/test.py:3:4: E1130: bad operand type for unary -: tuple (invalid-unary-operand-type)
/tmp/test.py:3:4: E1130: bad operand type for unary -: list (invalid-unary-operand-type)

Should that have been fixed by this PR?

@hippo91
Copy link
Contributor Author

hippo91 commented Apr 29, 2019

@willsALMANJ sorry for the delay.
The append function is not taken into account through astroid numpy brains.
I don't understand yet why this problem appears. I'll try to have a look ASAP.
Can you, please, open a specific issue for that?
Thanks again for your feedback.

Copy link
Contributor

@PCManticore PCManticore left a comment

Choose a reason for hiding this comment

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

👏 Wow, this is tremendous, thank you so much @hippo91 ! Overall it looks great, but left a couple of comments to be addressed before shipping this, the major one being the use of inference in transform filters. Just looking forward to have a bunch of numpy issues closed at once in pylint's tracker! Thank you again for the great work! 🎉

.gitignore Outdated
@@ -13,3 +13,6 @@ astroid.egg-info/
.cache/
.eggs/
.pytest_cache/
.python-version
Copy link
Contributor

Choose a reason for hiding this comment

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

@hippo91 These should go into your global gitignore, as they are specific to tools you use on your dev environment rather than specific to tools used by the project itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes ! Off-course.

ChangeLog Outdated
- ``numpy.core.umath``
- ``numpy.random.mtrand``

Close PyCQA/pylint2865
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a nice set of closed issues!

return (
isinstance(node, astroid.Attribute)
and node.attrname == member_name
and node.expr.inferred()[0].name.startswith("numpy")
Copy link
Contributor

Choose a reason for hiding this comment

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

@hippo91 Unfortunately using inference in the transform filters could have certain recursion error side effects, given that brain is not fully ready by the time the transform filters are called. Is there any way to prevent having this inference call here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@PCManticore , i changed the way the attribute is meant to be a numpy member.
I also introduced docstrings and type annotations.


def subTest(self, msg=None, **params):
try:
# For python versions above 3.5 this should be ok
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Isn't subTest available from 3.4? If so, we could use it today without the need of subtestMock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well done @PCManticore . You are totally right this mock is useless now.
I deleted it.

from astroid import builder


class SubTestWrapper(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we can't subTest right now (check my previous question on this topic), we should move these definitions into a common test file and use them from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All occurrences of this mock have been deleted.

@hippo91
Copy link
Contributor Author

hippo91 commented May 25, 2019

@PCManticore ,

Thanks for you review and kind words.
I made the changes you suggested.
The Travis CI failed due to the pylint step and i don't really understand why.
I used pylint with an old version of pylint's master branch (approximately 2 months old) and everything went fine. However after updating to last version i got the same problem as in Travis.
Have you any clue about it?

@PCManticore
Copy link
Contributor

Hi @hippo91 This should work now if you try again, there was a bug in pylint's master.

@hippo91
Copy link
Contributor Author

hippo91 commented Jun 1, 2019

Thanks @PCManticore !
It seems that i'v got a problem with python3.8. I will investigate this ASAP.

@PCManticore
Copy link
Contributor

@hippo91 Seems to me that the Python 3.8 tests started to fail for both pylint and astroid. We should fix it separate of this PR though.

@PCManticore PCManticore merged commit 33065ec into pylint-dev:master Jun 2, 2019
@PCManticore
Copy link
Contributor

Thank you again @hippo91 for all the hard work! 👏

@hippo91
Copy link
Contributor Author

hippo91 commented Jun 2, 2019

You are welcome @PCManticore. Thanks for merging it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants