Skip to content

Mandatory fields for Assign #2061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

nickdrozd
Copy link
Contributor

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
Copy link

codecov bot commented Mar 16, 2023

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

Impacted file tree graph

@@            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%) ⬇️

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Pierre-Sassoulas Pierre-Sassoulas added the pylint-tested PRs that don't cause major regressions with pylint label Mar 16, 2023
@DanielNoord
Copy link
Collaborator

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

@Pierre-Sassoulas
Copy link
Member

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
Copy link
Contributor Author

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.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

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
Copy link
Contributor Author

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

parent=parent,
)
_astroid_fields = ("targets", "value")
_other_other_fields = ("type_annotation",)

def postinit(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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 πŸ˜„ )

DanielNoord
DanielNoord previously approved these changes Apr 3, 2023
@Pierre-Sassoulas Pierre-Sassoulas enabled auto-merge (squash) April 3, 2023 17:57
@DanielNoord DanielNoord disabled auto-merge April 3, 2023 18:02
@DanielNoord DanielNoord merged commit c4d1436 into pylint-dev:main Apr 3, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.16.0, 3.0 Apr 24, 2023
@nickdrozd nickdrozd deleted the assign-required-fields branch October 7, 2024 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component pylint-tested PRs that don't cause major regressions with pylint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants