pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.31k stars 1.14k forks source link

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

Open azmeuk opened 7 years ago

azmeuk commented 7 years ago

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:
```python
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)]
PCManticore commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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

RafeSacks commented 7 years ago

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 commented 7 years ago

Does mypy really use assert isinstance?

I got the impression it was a separate check run during "build", kind of like pylint or pyflakes.

On Thu, Oct 19, 2017 at 3:55 AM, Nicolas Grilly notifications@github.com wrote:

@RafeSacks https://github.com/rafesacks What about mypy, which uses assert isinstance() as a type hint?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/PyCQA/pylint/issues/1162#issuecomment-337873040, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0yGkPLHexQuAhrfvQ6mSorvfdgDl6cks5styq_gaJpZM4Kur2t .

-- Dan Stromberg

dstromberg commented 7 years ago

On Mon, Oct 23, 2017 at 11:28 AM, RafeSacks notifications@github.com wrote:

Thanks for the replies to my comment.

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

I used both pylint and mypy on one project. Most of my projects only use pylint.

I saw a bug on pylint's issue tracker saying that types imported for use with mypy may be flagged as unused identifiers - but I didn't encounter that myself.

The project I used both on is at http://stromberg.dnsalias.org/svn/equivalence-classes/trunk/equivs3-python/

I believe pylint wants to eventually have mypy-like features, but they have to find the time to add them.

@rogalski https://github.com/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.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/PyCQA/pylint/issues/1162#issuecomment-338753198, or mute the thread https://github.com/notifications/unsubscribe-auth/AA0yGnBTYmJlwz_5Mg8wkVOWeEfK946lks5svNq-gaJpZM4Kur2t .

-- Dan Stromberg

ngrilly commented 7 years ago

@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 commented 3 years ago

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 commented 2 years ago

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 commented 2 years ago

https://github.com/PyCQA/astroid/pull/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 commented 9 months ago

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).