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

pylint lacks type-narrowing control-flow knowledge from isinstance #1162

Open
azmeuk opened this issue Nov 10, 2016 · 12 comments
Open

pylint lacks type-narrowing control-flow knowledge from isinstance #1162

azmeuk opened this issue Nov 10, 2016 · 12 comments
Labels
Control flow Requires control flow understanding

Comments

@azmeuk
Copy link

azmeuk commented Nov 10, 2016

Steps to reproduce

  1. Use pylint -E on a file containing this piece of code:
class FoobarException(Exception):
    foobar = None

try:
    pass
except Exception as ex:
    if isinstance(ex, FoobarException):
        ex.foobar

Current behavior

This error message is displayed:
Instance of 'Exception' has no 'foobar' member (no-member)

Expected behavior

No error message at all.

I am aware that this code do not use python good practices. A except FoobarException as ex would have been the right thing to do in that particular case.

Still, beeing inside the if condition implies that ex type is FoobarException and not just Exception.

That would not be the case with the following piece of code for instance:

class FoobarException(Exception):
    foobar = None

try:
    pass
except Exception as ex:
    if isinstance(ex, FoobarException) or someRandomResultFunction():
        ex.foobar

pylint --version output

pylint 1.6.4, 
astroid 1.4.8
Python 2.7.12 (default, Sep 29 2016, 13:30:34) 
[GCC 6.2.1 20160916 (Red Hat 6.2.1-2)]
@azmeuk azmeuk changed the title isinstance misunderstanding pylint might consider understand isinstance Nov 10, 2016
@PCManticore PCManticore added the Control flow Requires control flow understanding label Nov 10, 2016
@PCManticore
Copy link
Contributor

Thank you for reporting this issue.

The problem here is that pylint cannot understand even simple if blocks, such as this one you linked. Its inference engine tries to infer ex as all the possible values it can have, disregarding flow controls and whatnot. This is a known limitation and I've been saying that I am going to tackle it for a long time. I might tackling it though as soon as we release 2.0, which is scheduled around December.

@RafeSacks
Copy link

This really would solve so many situations I've encountered recently that are very frustrating. I think it would also be great to be able to use assert isinstance() to type hint.

There are times where we need to type hint for no-member issues without changing code (we should never change code just to "fool" lint results) and this is where real type hinting would be awesome. We use restructuredText for documentation so I selfishly would love pylint to understand doc comments like pycharm does, but even pure pyline type hinting would be great (e.g. # pylint type=Foo). Is there a place this subject is being discussed?

@rogalski
Copy link
Contributor

rogalski commented Aug 7, 2017

@RafeSacks
In terms of comments that can help inference, we'd likely opt for PEP484 instead of custom Pylint comment syntax.

About flow analysis, there is rogalski/flow but it's still in phase of very early drafts and not really active recently.

I'm not sure if there's much to discuss to be honest. Of course it would be awesome to have those advanced features in Pylint, main obstacle is simply lack of man-hours.

@ngrilly
Copy link

ngrilly commented Oct 19, 2017

@RafeSacks What about mypy, which uses assert isinstance() as a type hint?

@RafeSacks
Copy link

Thanks for the replies to my comment.

@ngrilly: Does pylint take advantage of mypy? The issue I have is that I have to pepper code with pylint ignores.

@rogalski: Totally agree re PEP484.

The only reason I brought up something for pylint is for users that lag behind the feature. The visual effects industry is mostly on Python 2.7. Python 3.x isn't likely to be widely adopted until VFX Platform 2019 (http://www.vfxplatform.com). Even then we aren't sure yet which version of Python 3.x, so it could be a long wait.

I do like the idea of Doc strings as a source of type hinting since this is widely used already (Sphinx and PyCharm for example). and it is mentioned in PEP484 in "Other backwards compatible conventions" (https://www.python.org/dev/peps/pep-0484/#other-backwards-compatible-conventions). I have no idea if existing code can be easily mined for this feature (https://github.com/agronholm/sphinx-autodoc-typehints) though and I understand a lack of man-hours. If I had time I would take a stab.

@dstromberg
Copy link

dstromberg commented Oct 23, 2017 via email

@dstromberg
Copy link

dstromberg commented Oct 23, 2017 via email

@ngrilly
Copy link

ngrilly commented Oct 23, 2017

@RafeSacks I have a limited understanding of pylint, but it looks like it doesn't use mypy.

@dstromberg The recommended way to annotate types for mypy is PEP 484, but I have noticed mypy uses assert isinstance(...) as a type hint, even if it's not the recommended notation.

@cdce8p
Copy link
Member

cdce8p commented Jul 28, 2021

The specific issue will be addressed by #4764. However, I'll leave this issue open as the PR doesn't implement control-flow logic.

Edit: The fix will only work for if statements. assert won't be fixed with it. See #4693 here as well.

@jacobtylerwalls jacobtylerwalls changed the title pylint might consider understand isinstance pylint lacks type-narrowing control-flow knowledge from isinstance Jun 4, 2022
@jacobtylerwalls
Copy link
Member

However, I'll leave this issue open as the PR doesn't implement control-flow logic.

I'm going to suggest treating this issue as the top-level duplicate for requests to understand type-narrowing with isinstance().

@jacobtylerwalls
Copy link
Member

pylint-dev/astroid#1189 proposes to introduce Constraint. See:

⚠️ If there is at least one inferred value, but all inferred values are filtered out, a new Uninferable value is yielded, rather than yielding nothing at all. I think this is the conservative approach, to avoid errors from the raise_if_nothing_inferred decorator.

For that case, if we return, say, a subclass of Uninferable that's more specific, e.g. UninferableDueToConstraints, then we should be able to use that to quiet most of the false positives in no-member and unsubscriptable-object.

@jacobtylerwalls
Copy link
Member

pylint-dev/astroid#1189 landed. We should welcome a PR that adds isinstance constraints to nodes. This has the ability to address both false positives and false negatives (see #9405 for the negative).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Control flow Requires control flow understanding
Projects
None yet
Development

No branches or pull requests

8 participants