Mandatory fields for Assign

[nickdrozd]

Type of Changes

	Type
✓	🐛 Bug fix
✓	🔨 Refactoring

Description

This PR re-types the required fields of the Assign node so that they are non-optional, as described in #2060.

Two existing type errors are thereby fixed, as reported by Mypy. Here is one of them:

    @cached_property
    def _assign_nodes_in_scope(self) -> list[nodes.Assign]:
        return [self, *self.value._assign_nodes_in_scope]

If Assign.value can be None, then this could fail. But if the field is required, there is no problem.

[codecov]

Codecov Report

Merging #2061 (91bd352) into main (779c3e0) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 91bd352 differs from pull request most recent head e259fed. Consider uploading reports for the commit e259fed to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2061      +/-   ##
==========================================
- Coverage   92.78%   92.78%   -0.01%     
==========================================
  Files          94       94              
  Lines       11046    11043       -3     
==========================================
- Hits        10249    10246       -3     
  Misses        797      797              

Flag	Coverage Δ
linux	92.54% <100.00%> (-0.01%)	⬇️
pypy	88.09% <66.66%> (-0.01%)	⬇️
windows	92.33% <100.00%> (-0.01%)	⬇️

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

Impacted Files	Coverage Δ
astroid/nodes/node_classes.py	95.03% <100.00%> (-0.01%)	⬇️

[Pierre-Sassoulas]

Shouldn't we change the constructor for _base_nodes.AssignTypeNode, _base_nodes.Statement too ?

[DanielNoord]

@Pierre-Sassoulas Are we okay with merging breaking changes into main? I'd be in favour, but want to double check.

[Pierre-Sassoulas]

Let's consider that if it's not breaking pylint then it's not a change that require a major version bump even if technically it's a breaking change. Especially if it's typing, as we never advertised astroid as typed.

[nickdrozd]

Shouldn't we change the constructor for _base_nodes.AssignTypeNode, _base_nodes.Statement too ?

It looks like those don't have their own constructors. They all just delegate to NodeNG.

[Pierre-Sassoulas]

I'd rather have the overridden constructor requiring 'targets' and remove the post init function altogether. But for that to work we would need to refactor the tree rebuilder. I suppose if we instantiate the leafs first and then recurse instead of what we're currently doing it's possible (?). What do you think @DanielNoord ?

[DanielNoord]

I'd rather have the overridden constructor requiring 'targets' and remove the post init function altogether. But for that to work we would need to refactor the tree rebuilder. I suppose if we instantiate the leafs first and then recurse instead of what we're currently doing it's possible (?). What do you think @DanielNoord ?

I mean, it's worth a shot but it would be a massive rewrite.

I would be okay with reviewing a complete rewrite and contributing but don't have the time to do to brunt of the work..

[nickdrozd]

Getting rid of postinit would be a substantial rewrite, as well as a full-on breaking change to the interface. The changes in this PR on the other hand don't really affect the interface in a meaningful way. It's only a matter of where instance variables are declared and where the __init__ work is done.

As for the two-stage initialization, it's one of many Chesterton fences in Astroid. Why is it like that? There may have been a good reason long ago, but that reason may be lost.

One good reason to keep it that way is that nodes have generic information (line numbers, etc) and type-specific information (arguments, values, etc). The two-stage initialization allows for that information to be set up separately. This way, the generic information only needs to be handled in one place. (The current arrangement gets the worst of both worlds, since the generic information is declared in every single node type, only to be passed along to NodeNG for handling. That's what this PR attempts to remedy.)

[DanielNoord]

astroid/astroid/rebuilder.py

Line 808 in 345b91c

newnode.postinit(

Could you add asserts here that the typing we add here is correct?

And could you add a changelog entry? For the rest this LGTM!

[Pierre-Sassoulas]

I don't see myself doing a massive rewrite (when there are a lot of massive rewrite we could do) so let's merge, it's an upgrade in the current state of the code.

[DanielNoord]

I don't see myself doing a massive rewrite (when there are a lot of massive rewrite we could do) so let's merge, it's an upgrade in the current state of the code.

Agreed! But let's at least make sure that all the callsites of these functions, so everywhere we do Assign(... are in line with what we merge. Otherwise we will start wondering in 3 months time if we forget something and the new typing is incorrect as well 😅

[nickdrozd]

What about the rest of the node types? Many of the rest need a similar fix. (Some require more involved changes.) Should that be done here, or separate PRs? One PR per node type, or all at once?

[DanielNoord]

Let's do separate PRs! I think I can check these fairly quickly but focusing on one node at a time makes it much easier. You can tag me for review (although please don't open 10 at once 😄 )