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

Minor bug: np.number is not subscriptable #4326

Closed
NeilGirdhar opened this issue Apr 9, 2021 · 12 comments · Fixed by pylint-dev/astroid#1148 or pylint-dev/astroid#1183
Closed

Minor bug: np.number is not subscriptable #4326

NeilGirdhar opened this issue Apr 9, 2021 · 12 comments · Fixed by pylint-dev/astroid#1148 or pylint-dev/astroid#1183
Assignees
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs astroid Brain 🧠 Needs a brain tip in astroid (then an astroid upgrade) typing

Comments

@NeilGirdhar
Copy link

NeilGirdhar commented Apr 9, 2021

from __future__ import annotations
from typing import Any  # Python 3.9
import numpy as np  # numpy 1.20
x: np.number[Any] = np.int32(1)
a.py:4:3: E1136: Value 'np.number' is unsubscriptable (unsubscriptable-object)
@Pierre-Sassoulas Pierre-Sassoulas added Bug 🪲 False Positive 🦟 A message is emitted but nothing is wrong with the code typing and removed Bug 🪲 labels Apr 9, 2021
@hippo91
Copy link
Contributor

hippo91 commented Apr 9, 2021

@NeilGirdhar thanks for the report. Looks like numpy brain needs a refresh.

@hippo91 hippo91 added the Needs astroid Brain 🧠 Needs a brain tip in astroid (then an astroid upgrade) label Apr 9, 2021
@NeilGirdhar
Copy link
Author

NeilGirdhar commented Jun 23, 2021

Same thing for np.floating, np.complexfloating, and np.ndarray in numpy 1.21.

@Pierre-Sassoulas
Copy link
Member

If someone is looking to fix this, the files to upgrade are the one starting with brain_numpy in this astroid module.

@hippo91
Copy link
Contributor

hippo91 commented Aug 25, 2021

@NeilGirdhar this bug should be fixed once pylint-dev/astroid#1148 will be merged and by authorizing pylint to dynamically load numpy with --extension-pkg-allow-list=numpy option.
If however you don't want to authorize this loading, then an improvement of astroid's numpy brain is required but may take a very long time to be set up.

@NeilGirdhar
Copy link
Author

@hippo91 Sounds great. Are there plans to update the numpy brain? If so, should we leave this issue open?

@hippo91 hippo91 reopened this Sep 16, 2021
@hippo91
Copy link
Contributor

hippo91 commented Sep 16, 2021

I'm reopening the issue because what was made in pylint-dev/astroid#1148 was not wise.
The issue will be closed through numpy astroid's brain upgrade.

@hippo91
Copy link
Contributor

hippo91 commented Sep 16, 2021

@NeilGirdhar i'm a bit confused with this issue.
The snippet you wrote is quite interesting. I just realized that as soon as from future import annotations is removed, then the code is no more valid and raise a TypeError: 'type' object is not subscriptable. I investigated a bit but i failed to understand the role of annotations in this case.
Have you an idea about this? It could help setting up the brain.

@NeilGirdhar
Copy link
Author

I investigated a bit but i failed to understand the role of annotations in this case.

The future import is parsing the annotations as strings. This way the type checker will use the numpy annotations, but the runtime will not try to index the classes.

The reason that the runtime cannot index the classes is that numpy has not yet defined the appropriate __class_getitem__ methods. These will be added by numpy/numpy#19879. They intend to have these merged by numpy 1.22 (the next minor version).

It could help setting up the brain.

Will the brain just work when numpy/numpy#19879 is merged or do you need to do anything on your end?

@hippo91 hippo91 self-assigned this Sep 16, 2021
@hippo91
Copy link
Contributor

hippo91 commented Sep 16, 2021

I investigated a bit but i failed to understand the role of annotations in this case.

The future import is parsing the annotations as strings. This way the type checker will use the numpy annotations, but the runtime will not try to index the classes.

The reason that the runtime cannot index the classes is that numpy has not yet defined the appropriate __class_getitem__ methods. These will be added by numpy/numpy#19879. They intend to have these merged by numpy 1.22 (the next minor version).

It could help setting up the brain.

Will the brain just work when numpy/numpy#19879 is merged or do you need to do anything on your end?

Thanks for your answer. However i do not yet understand how the annotations influence the runtime so that even whithout __class_getitem__ inside numpy the subscript of a numpy type is valid...

Inside the astroid brain, i try to add the __class_getitem__ to the generic and ndarray classes. It works a little bit too much because pylint will now falsely approve your snippet for all python versions even without the import of the future annotations.

Once the numpy/numpy#19879 pr will be merged, then I will include the __class_getitem__ depending on the numpy version. However handling the future annotations seems quite difficult.

@NeilGirdhar
Copy link
Author

NeilGirdhar commented Sep 17, 2021

Thanks for your answer. However i do not yet understand how the annotations influence the runtime so that even whithout class_getitem inside numpy the subscript of a numpy type is valid...

The future import causes the annotation to be treated as a string. It is as if the programmer had enclosed the annotation in quotation marks. If you're still confused, reading PEP 563 may help.

Inside the astroid brain, i try to add the class_getitem to the generic and ndarray classes. It works a little bit too much because pylint will now falsely approve your snippet for all python versions even without the import of the future annotations.

Once the numpy/numpy#19879 pr will be merged, then I will include the class_getitem depending on the numpy version. However handling the future annotations seems quite difficult.

Thanks for doing this!!

One thing, I don't think you should look at future annotations at all. Pylint is a tool that is essentially doing code verification include type verification. Therefore, it should be reading the annotations regardless of the "future annotations". Also, future annotations are standard starting in Python 3.11.

Second, if you're conditionally enabling the class getitem, I think I would start considering its existence in numpy 1.20 since that's when it was added to the type stubs.

@hippo91
Copy link
Contributor

hippo91 commented Sep 17, 2021

@NeilGirdhar thanks for your explanations. I was not probably well awaked yesterday to not understand PEP563 and your first explanation. Sorry.
I totally agree with you concerning the "future annotations".
However i'm not sure that considering existence of __class_getitem__ in numpy 1.20 is a good idea. Because it will lead pylint to silently accept code as the one you put in the snippet for numpy versions 1.20 and above (until 1.22) whereas the type annotations through class subscript is not supported in the corresponding numpy version.
Have you tested your snippet against mypy?
Personally i think it would make sense to conditionally include the __class_getitem__ method inside the brain for numpy versions starting at 1.22.

@NeilGirdhar
Copy link
Author

. Because it will lead pylint to silently accept code as the one you put in the snippet for numpy versions 1.20 and above (until 1.22) whereas the type annotations through class subscript is not supported in the corresponding numpy version.

You are mistaken: the annotations are supported. What's not supported is running the code like this, but I don't think that's your problem. In short, I think pylint should work like a type checking when it processes annotations, but maybe I'm wrong.

Have you tested your snippet against mypy?

Yes.

Personally i think it would make sense to conditionally include the class_getitem method inside the brain for numpy versions starting at 1.22.

Okay, if that's what you want to do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs astroid Brain 🧠 Needs a brain tip in astroid (then an astroid upgrade) typing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants