Acceptance test fail on "typing.py"

[Pierre-Sassoulas]

The followint test: pytest -m acceptance -k "test_libmodule[typing.py]"

Fail with the following stacktrace:

    def child_sequence(self, child):
        """Search for the sequence that contains this child.
    
        :param child: The child node to search sequences for.
        :type child: NodeNG
    
        :returns: The sequence containing the given child node.
        :rtype: iterable(NodeNG)
    
        :raises AstroidError: If no sequence could be found that contains
            the given child.
        """
        for field in self._astroid_fields:
            node_or_sequence = getattr(self, field)
            if node_or_sequence is child:
                return [node_or_sequence]
            # /!\ compiler.ast Nodes have an __iter__ walking over child nodes
            if (
                isinstance(node_or_sequence, (tuple, list))
                and child in node_or_sequence
            ):
                return node_or_sequence
    
        msg = "Could not find %s in %s's children"
>       raise exceptions.AstroidError(msg % (repr(child), repr(self)))
E       astroid.exceptions.AstroidError: Could not find 
<ClassDef.Hashable_typing l.0 at 0x7fa3ceac21f0> in <Module._collections_abc l.0 at 0x7fa3cea020a0>'s children

[Pierre-Sassoulas]

This is a problem introduced in astroid 2.5.1 apparentely.

[cdce8p]

The error is a direct result of pylint-dev/astroid#916. To make it work, I needed to insert intermediate classes that don't exist, like Hashable_typing. See explanation here: pylint-dev/astroid#905 (comment)

[Pierre-Sassoulas]

Probably linked but the error seems different: pylint /usr/lib/python3.8/typing.py

Traceback (most recent call last):
  File "/home/pierre/pylint/venv/bin/pylint", line 7, in <module>
    exec(compile(f.read(), __file__, 'exec'))
  File "/home/pierre/pylint/bin/pylint", line 5, in <module>
    run_pylint()
  File "/home/pierre/pylint/pylint/__init__.py", line 23, in run_pylint
    PylintRun(sys.argv[1:])
  File "/home/pierre/pylint/pylint/lint/run.py", line 358, in __init__
    linter.check(args)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 862, in check
    self._check_files(
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 896, in _check_files
    self._check_file(get_ast, check_astroid_module, name, filepath, modname)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 922, in _check_file
    check_astroid_module(ast_node)
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1054, in check_astroid_module
    retval = self._check_astroid_module(
  File "/home/pierre/pylint/pylint/lint/pylinter.py", line 1099, in _check_astroid_module
    walker.walk(ast_node)
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 75, in walk
    self.walk(child)
  [Previous line repeated 3 more times]
  File "/home/pierre/pylint/pylint/utils/ast_walker.py", line 72, in walk
    callback(astroid)
  File "/home/pierre/pylint/pylint/extensions/check_elif.py", line 67, in visit_if
    if not self._elifs[self._if_counter]:
IndexError: list index out of range

[cdce8p]

Continuing the discussion from pylint-dev/astroid#919 here:

Hmm, no I think it's a blocker, it means a crash is happening when we analyses typing.py from the standard lib.

@Pierre-Sassoulas I understand the issue. I just don't how to to fix it. As far as I know we can only choose between the acceptance test passing OR Generic Type Alias working at the moment. We had many open issue for the later whereas I've seen only this one for the acceptance test. What do you have in mind to fix it?

[Pierre-Sassoulas]

I did not follow that very closely, could you explain why we can't have both ? Trying to reproduce the crash with a minimal example proved to be too hard for the time I had.

[cdce8p]

For pylint-dev/astroid#916 to work, I needed to create and insert custom classes that don't exist into the MRO. This was because in the stdlib the typing classes (Iterable, Container, Generator, ...) are basically just aliases to the corresponding class in the collections and collections.abc package with some added sugar (begin subscriptable in Python 3.7 and 3.8). Due to the way caching works in astroid, without the custom classes, we would need to add the __getitem__ method to the ABCMeta class. This would mean that suddenly all classes that have it set as a metaclass would be subscriptable too, which isn't the case.

To be perfectly honest, I wasn't sure it would work out completely perfect. However besides for your issue, I haven't seen any.

[Pierre-Sassoulas]

@AWhetter @hippo91 do you think we can ignore this acceptance test and document the problem in 2.7.3 before releasing ? 2.7.2 also have this problem, so it might be an argument for ignoring, as we're not adding a regression by releasing, it already exists.

[cdce8p]

[...] so it might be an argument for ignoring, as we're not adding a regression by releasing, it already exists.

I would add that the next release will solve some highly requested bugs: #3167 and #3202 and others. Just noticed a duplicate today as well #4219

[hippo91]

We could release without the bug fix and make a new release as soon as it is fixed.
What do you think about it?

[cdce8p]

@Pierre-Sassoulas As things stand at the moment, the fix will probably not be ready soon. What do you think about releasing 2.7.3 now, without it, and do a 2.7.4 once it's done?

I haven't seen any issues pop-up other than this one regarding the acceptance test, so it should be ok. 2.7.3 together with a new astroid release (current master) would solve a lot of issues that are now fixed. I have soft deadline Tuesday to get new changes into https://github.com/home-assistant/core, otherwise I would need to wait another month. Just to give you a perspective, the change would fix over 150 # pylint: disable=invalid-name comments alone (with #4207).

[Pierre-Sassoulas]

Yeap, I was going to suggest the same thing, as we must handle duplicate of issue already fixed in pylint's master.