Add typing to NodeNG.statement

[DanielNoord]

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

statement() is only defined twice. Once on NodeNG and once on Module. The one on NodeNG refers to self.is_statement which is only set to True on Statement. As a result statement() can either return Module or Statement.

Added because of comment by @cdce8p here:
pylint-dev/pylint#5163 (review)

I think I will not wait for this version to be added to pylint to update the typing there.

Type of Changes

	Type
✓	🔨 Refactoring

Related Issue

[DanielNoord]

We're actually catching the AttributeError when a Node does not have a parent here:
https://github.com/PyCQA/pylint/blob/259f7a381d19bdbfaa84343825e0638323aed55f/pylint/checkers/typecheck.py#L1019-L1028

This does seem like something we want to fix, or at least document better. Using the ParentMissingError would be much better imo.

However, how do we go about fixing a bug simultaneously in astroid and pylint?

[Pierre-Sassoulas]

There is a really strange fail in the pre-commit continuous integration. Maybe we need to not use the current astroid with pylint's in pre-commit ?

[DanielNoord]

@Pierre-Sassoulas No the pre-commit warns us of a crucial regression here!
The weird errors that occur within the pylint step follow from an earlier crash within that step. This is strange behaviour that we might want to look at at some point.

pylint's typecheck class is dependent on NodeNG.statement() sometimes raising an AttributeError (see the code I linked). Without pre-commit I wouldn't have noticed this.

We will thus need to change pylint and astroid at the same time, to make pylint react to the more specific ParentMissingError and "fix" astroid.

However, @cdce8p also mentioned that he wondered if it is correct for Modules to return self. So I guess we will also need to wait for his judgement there!

[cdce8p]

Technically, you are correct that statement can return nodes.Statement or nodes.Module. I was just wondering if it's of any use for us to return Module or if we should raise an exception instead. Module itself isn't a Statement so from that perspective I would imaging it's special cased in pylint already.

In that sense, a general StatementMissing, or similar, error might make sense. This could also be raised if a node doesn't have a parent (instead of ParentMissingError).

[DanielNoord]

In that sense, a general StatementMissing, or similar, error might make sense. This could also be raised if a node doesn't have a parent (instead of ParentMissingError).

Wouldn't using a ParentMissingError be more specific? It conveys more information about why the statement is missing which might be relevant for any function handling the error or calling statement().

[cdce8p]

In that sense, a general StatementMissing, or similar, error might make sense. This could also be raised if a node doesn't have a parent (instead of ParentMissingError).

Wouldn't using a ParentMissingError be more specific? It conveys more information about why the statement is missing which might be relevant for any function handling the error or calling statement().

Not really sure we need more information. It works just fine with the AttributeError we have now.
What would you suggest to emit if statement is called on nodes.Module then? A module shouldn't have a parent, so ParentMissingError doesn't really fit IMO.

[DanielNoord]

Wouldn't using a ParentMissingError be more specific? It conveys more information about why the statement is missing which might be relevant for any function handling the error or calling statement().

Not really sure we need more information. It works just fine with the AttributeError we have now.

I do think we should at least show that this is expected behaviour of the method. The fact that we are actually handling AttributeError in pylint came as a total surprise now and I think the current method also does not indicate that you are expected to handle that.
Adding a specifically raised error solves that problem by making it more visible.

What would you suggest to emit if statement is called on nodes.Module then? A module shouldn't have a parent, so ParentMissingError doesn't really fit IMO.

I'm not sure if I agree with ParentMissingError not fitting. I think it depends on how you "read" missing, which could mean either 1) supposed to be present but not present or 2) not present. I read it as the second option, but I can see why with an interpretation of the first kind it doesn't fit for nodes.Module.
Perhaps a sub-classed error called something like ModuleDoesNotHaveParent? Users of the method would then still be fine by handling ParentMissingError, but could also focus on the nodes.Module specific case.
Btw, we might want to create a completely different error called ModuleIsTopLevel or something similar, as I expect this problem will also occur with .frame() and .scope()

I think raising more specific errors in the long run will be better as it makes the code more maintainable/readable. Not only can we give mypy more specific information, but users also get more information about errors they can expect and are supposed to handle.

[cdce8p]

I think raising more specific errors in the long run will be better as it makes the code more maintainable/readable. Not only can we give mypy more specific information, but users also get more information about errors they can expect and are supposed to handle.

I agree with that. Just not sure ParentMissingError is the right choice. It's currently used for scope() (and should probably be added to frame(), but those end with module. For statement() everything except from nodes.Module should return something.

At the moment, I just find StatementMissing more fitting 🤷🏻‍♂️ What about if that inherits from ParentMissingError?

[DanielNoord]

I agree with that. Just not sure ParentMissingError is the right choice. It's currently used for scope() (and should probably be added to frame(), but those end with module. For statement() everything except from nodes.Module should return something.

At the moment, I just find StatementMissing more fitting 🤷🏻‍♂️ What about if that inherits from ParentMissingError?

Do you then want to raise StatementMissing for non-Module nodes without parent as well? Perhaps you were already thinking like this, but I would suggest the following patterns:

1. For all non-Module nodes without parent raise ParentMissingError
2. For Module raise a new StatementMissing error inherited from ParentMissingError

[cdce8p]

Do you then want to raise StatementMissing for non-Module nodes without parent as well? Perhaps you were already thinking like this, but I would suggest the following patterns:

1. For all non-Module nodes without parent raise ParentMissingError
2. For Module raise a new StatementMissing error inherited from ParentMissingError

I would recommend to raise StatementMissing in both cases. Devs are still able to use except ParentMissingError.

[Pierre-Sassoulas]

👍

[DanielNoord]

2 questions:

• Is the pylint message about self = cast("nodes.Statement", self) correct? -> Make self-cls-assignment consider tuple assign. & ignore typing.cast() pylint#5254
  - How should I overload nodes.Module.statement()? I thought this was correct but both pylint and mypy complain..

[cdce8p]

I've seen that pytest will emit DeprecationWarnings for astroid itself. Could you add future=True to existing statement in astroid, too?
https://github.com/PyCQA/astroid/runs/4126645345?check_suite_focus=true#step:6:81

[DanielNoord]

I've seen that pytest will emit DeprecationWarnings for astroid itself. Could you add future=True to existing statement in astroid, too?
https://github.com/PyCQA/astroid/runs/4126645345?check_suite_focus=true#step:6:81

This started to fail some of the tests. I wanted to do this in a separate PR because I wondered if some of the functions/methods calling statement() are considered public and should thus keep raising AttributeError or returning Module.

[cdce8p]

Overall, LGTM! Thanks @DanielNoord 🚀
A bit unfortunate, that we came across multiple false-positives in the process.

@Pierre-Sassoulas Do you want to take a final look at it? Maybe with focus on the Exception and deprecation message?

[DanielNoord]

Thanks for all of your help here @cdce8p!

Indeed a bit unfortunate, but I learned a lot from this PR and it should help with some of the future PR's and issues we might come across.

[Pierre-Sassoulas]

I only have a small remark regarding the "bad super call" but I don't see anything wrong with the MR otherwise. To be fair I did not find the time to read the whole conversation and this seem really polished :) Look like you took care of a lot of problems, thanks a lot for handling this !

[Pierre-Sassoulas]

I think the spirit of the check is to signal the problem to people that don't really know how to use super(). Here it's a false positive because calling the father's function is what you really want to do, but maybe in most case and especially for beginner the check is right. If we remove this "false positive" the check will only have really obvious cases where the class called is not even an ancestor ?

[cdce8p]

🤔 Not sure about that. What it comes done to is if we want to emit errors for valid Python code. Even if it's probably not what was intended, there are still situations where it's useful and has it's place, especially with multi-inheritance.

The check also emits errors in cases that could cause recursion loops:

class SuperWithType(object):
    """type(self) may lead to recursion loop in derived classes"""
    def __init__(self):
        super(type(self), self).__init__()  # [bad-super-call]

class SuperWithSelfClass(object):
    """self.__class__ may lead to recursion loop in derived classes"""
    def __init__(self):
        super(self.__class__, self).__init__()  # [bad-super-call]

Any modifications should also make sure to still emit errors if the class is not part of the MRO.

class Getattr(object):
    """ crash """
    name = NewAaaa

class CrashSuper(object):
    """ test a crash with this checker """
    def __init__(self):
        super(Getattr.name, self).__init__()  # [bad-super-call]

Test cases taken from the pylint testsuit: super_checks.py

[Pierre-Sassoulas]

OK, I was trying to search for the initial issue where this was implemented for the rational but I can't find it. It makes sense to not warn for valid code, and the check has a lot of valid test cases, so let's fix this false positive.

[cdce8p]

Suggested change

	# pylint: disable-next=signature-differs
	# https://github.com/PyCQA/pylint/issues/5264

It seems like our luck didn't outran us yet. This actual now triggers a useless-suppression message. Guess we found a false-negative by adding * 😅 pylint-dev/pylint#5270

[DanielNoord]

Is useless-suppression not enabled for astroid? CI is passing.

We might want to enable that in another PR.

[cdce8p]

It's enabled, but not a failure condition! I don't think we should change that.

[DanielNoord]

Sure? Seems like something that will be easily missed in review

[Pierre-Sassoulas]

I also think we should make it a failure condition so it's handled in the proper MR each time.

[cdce8p]

We might want to enable that in another PR.

It would be as easy as adding --fail-on=useless-suppression to the pylint command.

The issue is that some errors might be Python version specific. It could create a situation where only one, pre-commit or CI, will succeed when tested with different versions. The py-version option should help, but I don't know if it's enough.

Additionally, this would prevent us from having the latest pylint version installed in a local env as this is likely to have fixed some false-positives. -> Which would then trigger useless-suppression.

[DanielNoord]

That last point will indeed be difficult to avoid. I'll try and think of something to fix that, but in the meantime let's let this rest!

[cdce8p]

Do you maybe want to track this as issue in pylint?

in the meantime let's let this rest!

I completely agree!

[DanielNoord]

Shouldn't this be tracked in astroid? pylint has its own issues with useless-suppression (pylint-dev/pylint#5040) 😄

[cdce8p]

😂 Though if we enable it, we'll likely want to do it for pylint too. Tbh I don't really care about it due to the issues mentioned earlier.

[cdce8p]

Let's (squash) merge it!