pylint icon indicating copy to clipboard operation
pylint copied to clipboard

False positive not-an-iterable for typing.NewType

Open alien3211 opened this issue 7 years ago • 17 comments

Steps to reproduce

  1. Put this code in test.py:
from typing import List, NewType

a = NewType("a", List[int])

def fun() -> a:
    """Some function."""
    data = [1, 2, 3]

    return a(data)

def fun1():
    """some function."""
    for x in fun():
        pass
  1. Run pylint test.py

Current behavior

test.py:14:13: E1133: Non-iterable value fun() is used in an iterating context (not-an-iterable)

Expected behavior

pylint --version output

pylint 2.0.0 astroid 2.0.0.dev4 Python 3.6.5 (default, May 3 2018, 10:08:28) [GCC 5.4.0 20160609]

alien3211 avatar Jul 16 '18 09:07 alien3211

Hi, thanks for the report! What happens here is that pylint or better to say astroid does not know that NewList acts as a transparent constructor for its arguments, and most likely considers it a type in itself, which is wrong since it should just be a transparent layer so that the type checkers could pick that up.

PCManticore avatar Jul 17 '18 07:07 PCManticore

the typing astroid brain is trying to create a class for this function. I'm not too sure why the commiter wanted that to happen. If I copy the relevant code out of typing and rename the function, the type is fine

brycepg avatar Jul 20 '18 17:07 brycepg

I think if I change the brain to make the psuedo class inherit from the given type (e.g. List[int]) it would fix this issue and provide a platform for future annotation checking

brycepg avatar Jul 20 '18 18:07 brycepg

Same with sympy :

from sympy import Array

az = Array(range(10))

for a in az[::2]:
    pass

Result:

me@me-HP ~ $ pylint --version
pylint 2.3.1
astroid 2.2.2
Python 3.6.7 (default, Oct 22 2018, 11:32:17) 
[GCC 8.2.0]
me@me-HP ~ $ pylint ze.py 
************* Module ze
ze.py:5:9: E1133: Non-iterable value az[::2] is used in an iterating context (not-an-iterable)

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

Linekio avatar May 06 '19 14:05 Linekio

To me this looks like a regression.

I am in progress of moving a Python 2 code base which was checked with pylint 1.9.5 and astroid 1.6.6 to Python3 which is now checked with pylint 2.4.4 and astroid 2.3.3.

In the pylint 2 code base this is detected correctly and works as expected:

# > pylint --version
# Using config file .pylintrc
# pylint 1.9.5,
# astroid 1.6.6
# Python 2.7.17 (default, Nov  7 2019, 10:07:09)
# [GCC 7.4.0]
#!/usr/bin/env python2

from typing import NewType, Text

UserId = NewType("UserId", Text)

# OK
UserId("x").upper()
# [pylint:no-member] Instance of 'UserId' has no 'xupper' member
UserId("x").xupper()
# OK
"".upper()

And the now broken result:

# > pylint --version
# pylint 2.4.4
# astroid 2.3.3
# Python 3.7.3 (default, Apr  3 2019, 19:16:38)
# [GCC 8.0.1 20180414 (experimental) [trunk revision 259383]]
#!/usr/bin/env python3
from typing import NewType, Text

UserId = NewType("UserId", Text)

# [pylint:no-member] Instance of 'UserId' has no 'upper' member
UserId("x").upper()
# [pylint:no-member] Instance of 'UserId' has no 'xupper' member
UserId("x").xupper()
# OK
"".upper()

In the astroid change log of version 2.0 I can find this:

* typing.X[...] and typing.NewType are inferred as classes instead of instances.

Which seems to be related with the file

https://github.com/PyCQA/astroid/commits/b7bdbf1d378bdc5d3f3f0375381242a4a61d0bb0/astroid/brain/brain_typing.py

Is this the relevant part?

This is kind of a blocker at the moment. I even don't have an idea for a good workaround. Any ideas or pointers?

LarsMichelsen avatar Feb 12 '20 15:02 LarsMichelsen

Looks like the problem was introduced with https://github.com/PyCQA/astroid/commit/283befa92865faad7847b2d40a569e03ae00f7cc#diff-82f7976cc16d2319c09d26ba124fc46b

LarsMichelsen avatar Feb 12 '20 16:02 LarsMichelsen

What exactly did https://github.com/PyCQA/astroid/commit/283befa92865faad7847b2d40a569e03ae00f7cc#diff-82f7976cc16d2319c09d26ba124fc46b fix? The previous behavior looked more correct to me, at least it didn't break the (very valuable) NewType. Can't we simply revert that "fix"?

svenpanne avatar Feb 27 '20 07:02 svenpanne

@rogalski @PCManticore Could you answer this so we can move forward on this issue? If this commit can simply be reverted, or the problem otherwise addressed, this will resolve a major blocker that breaks most non-trivial use of NewType with pylint. Thanks!

CAM-Gerlach avatar Jul 25 '21 07:07 CAM-Gerlach

Hey :) We can't revert the commit directly because there are other changes mixed in it. (Lot of use cases in def test_typing_types(self): were added for example). But we need to address the problem that it's still possible to use the attribute of the old type when using NewType so the no-member on upper() is a false positive.

Pierre-Sassoulas avatar Jul 25 '21 12:07 Pierre-Sassoulas

Hey :) We can't revert the commit directly

Thanks, understood—I wasn't necessarily thinking a literal git revert, rather a more abstract reversion of the atomic change (vis a vis the at least nominally greater difficulty of developing some new/modified code to handle this). Sorry that wasn't clear! Of course, before we think about that, I imagine it would be best to hear from the author, committer and maintainer, to understand what should best be done to address this while retaining the other presumed benefits of the change.

CAM-Gerlach avatar Jul 25 '21 19:07 CAM-Gerlach

I agree with you I think a partial minimal revert (changing test case for NewType that aren't accurate for example) is the way to go. I'm under the impression that NewType should be behave as if it was a class inheritance and right now it's simply a class def without inheritance. Maybe the fix is simple. PCManticore won't answer this though, he's retired from pylint :)

Pierre-Sassoulas avatar Jul 25 '21 19:07 Pierre-Sassoulas

Yep, thanks; that makes perfect sense. What are our next steps here? I'd like to offer to help, but I am completely unfamiliar with pylint's internals (I only contributed some minor docs updates in my role as Spyder/Docs maintainer), and don't want to overbook myself.

PCManticore won't answer this though, he's retired from pylint :)

Ah sorry, didn't know that. Thanks for the update.

CAM-Gerlach avatar Jul 26 '21 19:07 CAM-Gerlach

change the brain to make the pseudo class inherit from the given type (e.g. List[int])

I think @brycepg hint is the right one, so the change should be made in brain_typing.py. I'm no astroid expert but If you want to experiment, open à MR, I'll try to review and help :)

Pierre-Sassoulas avatar Jul 26 '21 20:07 Pierre-Sassoulas

Is there any workaround for this 4 years old issue? I kinda need to use a NewType for aliasing Dict[str, Any] and I don't know how to make pylint happy.

ssbarnea avatar Oct 01 '22 18:10 ssbarnea

I give an upvote to this. In my codebase, there are some usages of NewType, and pylint barks at them. Is there any chance that pylint will soon support NewType?

jnussbaum avatar Jun 09 '23 07:06 jnussbaum

Latest development are in https://github.com/pylint-dev/astroid/pull/1301#issuecomment-1162485313

Pierre-Sassoulas avatar Jun 09 '23 12:06 Pierre-Sassoulas