Visit the transforms before handling delayed assattr nodes

[marmarek]

Type of Changes

	Type
✓	🐛 Bug fix

Description

When importing Gtk, it looks like this:

import gi
gi.require_version('Gtk', '3.0')
from gi.repository import Gtk

It is vital that gi.require_version() is made before related 'from gi.repository import ...'. The brain_gi tries to do that using transforms. And it works unless Gtk is imported as part of delayed assattr handling.

Fix this by handling transforms earlier.

Closes #2190
Closes pylint-dev/pylint#6352

[marmarek]

My change made one test fail now:

____________________ TypingBrain.test_typing_generic_slots _____________________

self = <tests.brain.test_brain.TypingBrain testMethod=test_typing_generic_slots>

    def test_typing_generic_slots(self):
        """Test slots for Generic subclass."""
        node = builder.extract_node(
            """
        from typing import Generic, TypeVar
        T = TypeVar('T')
        class A(Generic[T]):
            __slots__ = ['value']
            def __init__(self, value):
                self.value = value
        """
        )
        inferred = next(node.infer())
        slots = inferred.slots()
>       assert len(slots) == 1
E       TypeError: object of type 'NoneType' has no len()

I don't fully understand why, yet, but I think one option to make my change less invasive is to add "early transforms" and leave the original transforms in the old place. And then make brain_gi use the early transforms for the gi.require_version thing. Is that okay?

[Pierre-Sassoulas]

Thank you for the MR. I agree with the proposed early transform, it feels like a big API decision but applying the transform twice so it's possible to apply transform after delayed attr creation is a lot worse. Let's hear another maintainer opinion before committing to it, it's not like I'm a brain expert myself.

[DanielNoord]

Have you also tried running the pylint testsuite with this version of astroid? I'm wondering if this is a fix we can indeed make, the failing test doesn't make me too hopeful.

[marmarek]

When running pylint test suite on plain main branch I got 12 failures already, but my change introduces 13th: test_functional[assigning_non_slot_4509] (assigning-non-slot message is not raised).
The failure here is also related to slots, so it's probably the same thing.

I'll change my patch to not affect other brains.

[marmarek]

While at it, I've added also some basic test for brain_gi.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.73%. Comparing base (453d307) to head (b1f540d).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2453   +/-   ##
=======================================
  Coverage   92.72%   92.73%           
=======================================
  Files          94       94           
  Lines       10993    11007   +14     
=======================================
+ Hits        10193    10207   +14     
  Misses        800      800           

Flag	Coverage Δ
linux	92.61% <100.00%> (+<0.01%)	⬆️
pypy	92.73% <100.00%> (+<0.01%)	⬆️
windows	92.71% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
astroid/brain/brain_gi.py	23.02% <100.00%> (ø)
astroid/builder.py	94.27% <100.00%> (+0.07%)	⬆️
astroid/manager.py	90.23% <100.00%> (+0.39%)	⬆️
astroid/test_utils.py	85.71% <100.00%> (+0.29%)	⬆️

[marmarek]

tests\brain\test_gi.py s [ 20%]

So, the test I added doesn't run in CI (which also explain minimal test coverage of brain_gi.py). Is it worth installing "gi" python module + Gtk3 (and maybe Gtk4 too?) in there to make the test run? Or is it enough for it to run when called manually locally (assuming those dependencies are installed)?

[DanielNoord]

Yeah I don't think we want to add those dependencies to the CI.

I do wonder if this warrants the API change. I can't really comment on that part (and I think most maintainers are a bit afraid of making such a decision). @Pierre-Sassoulas @jacobtylerwalls Do you have any opinions here?

[jacobtylerwalls]

I do wonder if this warrants the API change.

Yeah, I'm reluctant to add an early transforms API for this. We should consider alternatives:

• in _register_require_version(): manipulate sys.modules or use importlib.reload to reimport gi if necessary
• remove brain_gi in favor of a new plugin to be homed at pylint-dev/pylint-gi