Ensure the `*_fields` attributes of `init`/`postinit` are cohesive #1218

thejcannon · 2021-10-22T21:19:10Z

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

It was discovered (and the fix is needed by) #1194 that there isn't strict cohesion between the *_fields attributes and the parameters in __init__ and postinit, so I've added tests which enforces this.

Through testing a handful of violators have been found, so a deprecation warning decorator has been added, and parameters have been updated accordingly.

Type of Changes

	Type
✓	🐛 Bug fix
	✨ New feature
✓	🔨 Refactoring
	📜 Docs

Related Issue

for more information, see https://pre-commit.ci

Pierre-Sassoulas

Thank you for taking the time to review this part of the code and for undergoing the huge work of making it coherent. I have some small suggestions.

astroid/decorators.py

astroid/nodes/node_classes.py

Pierre-Sassoulas · 2021-10-23T19:41:21Z

astroid/nodes/scoped_nodes.py

@@ -2105,8 +2108,7 @@ def postinit(
        :param keywords: The keywords given to the class definition.
        :type keywords: list(Keyword) or None
        """
-        if keywords is not None:


This was a very strange way to do it.

What is strange about it? The default for self.keywords is an empty list. If I remember correctly, this change was added recently to fix a crash in pylint.

Well usually you use if keyword is None: to apply the default value when the default value is a mutable like [] in order to avoid the problem of mutable default argument being function attribute, so doing the opposite where you initialize only if it's not None "feel" wrong.

See the relevant PR for this change here:
#1181

tests/unittest_nodes.py

Co-authored-by: Pierre Sassoulas <[email protected]>

cdce8p

IMO we should not do these changes. For most, if not all, there is a good reason why it was designed that way. Some (limited) areas can certainly be improved, but these are changes that are more likely to introduce bugs than fix anything.

As for why some attributes are added with postinit and other in __init__, I don't know the full answer to that. I believe the idea was to pass most with __init__ but that isn't possible for children nodes. Thus they are added in postinit. Besides that, whatever makes most sense.

Moving some, just for the point of doing it, will add more potential for errors that is IMO just unnecessary.

cdce8p · 2021-10-23T22:37:59Z

astroid/nodes/node_classes.py

+        lineno: Optional[int] = None,
+        col_offset: Optional[int] = None,


The ast node arguments doesn't have lineno or col_offset information. IMO it doesn't make sense to add them. https://docs.python.org/3.10/library/ast.html

cdce8p · 2021-10-23T22:40:14Z

astroid/nodes/node_classes.py

@@ -1328,7 +1332,7 @@ def postinit(
        self.target = target
        self.annotation = annotation
        self.value = value
-        self.simple = simple
+        self.simple = simple or self.simple


This can easily break. simple is a boolean integer so either 0 or 1.

Yeah this was a forehead-slapper.

cdce8p · 2021-10-23T22:41:21Z

astroid/nodes/node_classes.py

+        lineno: Optional[int] = None,
+        col_offset: Optional[int] = None,


Same here, the ast Comprehension node doesn't have lineno or col_offset information.
https://docs.python.org/3.10/library/ast.html

cdce8p · 2021-10-23T22:41:40Z

astroid/nodes/node_classes.py

@@ -1800,7 +1811,7 @@ def postinit(
        self.iter = iter
        if ifs is not None:
            self.ifs = ifs
-        self.is_async = is_async
+        self.is_async = is_async or self.is_async


Can break too!

cdce8p · 2021-10-23T22:44:35Z

astroid/nodes/node_classes.py

+        self.fromname: Optional[str] = fromname  # can be None
+        self.modname = self.fromname  # For backwards compatibility


This shouldn't be part of this PR. IMO it's not worth it to do this change in isolation, there is no really need for it.

cdce8p · 2021-10-23T22:47:10Z

astroid/nodes/node_classes.py

@@ -4076,6 +4088,7 @@ class FormattedValue(NodeNG):
    """

    _astroid_fields = ("value", "format_spec")
+    _other_other_fields = ("conversion",)


Why should this be in _other_other_fields and not _other_fields? It's just an optional integer.

cdce8p · 2021-10-23T22:48:03Z

astroid/nodes/node_classes.py

@@ -4252,7 +4265,7 @@ class EvaluatedObject(NodeNG):

    name = "EvaluatedObject"
    _astroid_fields = ("original",)
-    _other_fields = ("value",)
+    _other_other_fields = ("value",)


cdce8p · 2021-10-23T22:48:28Z

astroid/nodes/node_classes.py

+        lineno: Optional[int] = None,
+        col_offset: Optional[int] = None,


MatchCase doesn't have lineno or col_offset information.

cdce8p · 2021-10-23T22:50:01Z

astroid/nodes/node_classes.py

-        kwd_attrs: typing.List[str],
        kwd_patterns: typing.List[Pattern],
+        kwd_attrs: typing.Optional[typing.List[str]] = None,


IMO it does make more sense to keep kwd_attrs and kwd_patterns together in postinit. No need to move one to __init__.

cdce8p · 2021-10-23T22:53:29Z

astroid/nodes/scoped_nodes.py

@@ -2105,8 +2108,7 @@ def postinit(
        :param keywords: The keywords given to the class definition.
        :type keywords: list(Keyword) or None
        """
-        if keywords is not None:


What is strange about it? The default for self.keywords is an empty list. If I remember correctly, this change was added recently to fix a crash in pylint.

thejcannon · 2021-10-29T20:44:27Z

It says in https://github.com/PyCQA/astroid/blob/3f98fa0be2279ff422a0e157705f4e3ea1e4d48f/doc/extending.rst that

When instantiating a node, the non-AST parameters are usually passed via the constructor, while the AST parameters are provided via the postinit() method. The only exception is that the parent is also passed via the constructor. Instantiating a new node might look as in:

So in addition to having these follow a convention for developer's sake conventions also helps allow code to make assumptions.

So I wouldn't say it's "just for the point of doing it". It's to address some tech debt / code uncleanliness.

cdce8p · 2021-11-01T13:45:21Z

It says in https://github.com/PyCQA/astroid/blob/3f98fa0be2279ff422a0e157705f4e3ea1e4d48f/doc/extending.rst that

When instantiating a node, the non-AST parameters are usually passed via the constructor, while the AST parameters are provided via the postinit() method. The only exception is that the parent is also passed via the constructor. Instantiating a new node might look as in:

So in addition to having these follow a convention for developer's sake conventions also helps allow code to make assumptions.

So I wouldn't say it's "just for the point of doing it". It's to address some tech debt / code uncleanliness.

We might need to update the documentation then 😉
I still don't think we should change it. Even if it addresses some tech debt, there are also good arguments to keep it the way it's now. The most important is the risk of regressions: Just from the current changes, we would have introduced three new regressions that could each crash astroid and by extension pylint. It's just not worth it!

Pierre-Sassoulas · 2021-11-03T09:12:05Z

We've done some pretty heavy refactor and breaking change in 3.0 for consistency's sake, Marc, what is different here ? It think having a clear rule like:

non-AST parameters are passed via the constructor # notice I removed "usually"
AST parameters are provided via the postinit()
except for "parents" which are passed to constructor

Would make astroid development easier and reduce mistakes then the need to retro-engineer classes to see how they were implemented if they do not follow the general "rule". Is a breaking change in 3.0 really that much more impacting than what we already did ?

cdce8p · 2021-11-03T13:09:37Z

We've done some pretty heavy refactor and breaking change in 3.0 for consistency's sake, Marc, what is different here ?

The breaking changes so far have been because we moved code around. So changing the import is usually enough. Then there are the cases which shouldn't happen anyway, like passing None as argument in cases where it should never be None. Those are bugs that we previously handled gracefully but to improve typing, we deprecate it now.

This change in contrast will break code just because it "might" improve consistency. IMO this is dangerous and not worth it. As I've pointed out already, just from a few lines, there would have been multiple regressions. True, we can fix them but who is to say those don't introduce new bugs in plugins. Usually I think it's better to limit breaking changes to an absolute minimum and only do them when there is a clear benefit.

To highlight something different. Although it might fix inconsistencies, it also introduces new ones. All node types have lineno, col_offset, and parent as their last arguments for __init__. This PR creates an exceptions just for a few. Should we mention that in the docs, then?

A last point, the ast changes with every Python release. These inconsistencies are a direct result of that. We can easily add new arguments to postinit whereas with __init__ we would create breaking changes or change the order, i.e. parent isn't the last anymore. One of the major things astroid does so well, is hide the ast behind a somewhat stable interface pylint and plugins can work with. As for the need to "retro-engineer" classes, I would be surprised if anyone is able to write plugins without at least looking at the documentation, sometimes even that isn't enough. This PR won't change that.

Pierre-Sassoulas · 2021-11-03T19:05:57Z

Ok, I'm convinced, it seemed really nice on paper @thejcannon thank you for the proposal. But the inconsistencies coming from the AST that will keep on coming and the bigger changes required for lib that are not us (pylint) are pretty convincing reasons to not fix this.

thejcannon · 2021-11-04T21:37:26Z

Bummer. Cheers anyways.

* Based on suggestions from pylint-dev#1218

* Based on suggestions from #1218

thejcannon and others added 11 commits October 21, 2021 07:57

node changes

92580dc

init test

89a5a98

Ensure the _fields attributes and __init__/postinit are cohesive

5c91e84

changelog

21d3496

Fixes from self-review

ac264e4

[pre-commit.ci] auto fixes from pre-commit.com hooks

1eb489c

for more information, see https://pre-commit.ci

Fix syntax typo

445220c

flake8

720b7d8

fix lineno

1bcdb30

no more foo/bar

607cc43

baz

3dd7c72

Pierre-Sassoulas reviewed Oct 23, 2021

View reviewed changes

thejcannon and others added 4 commits October 23, 2021 15:53

Update astroid/decorators.py

1fbfe2b

Co-authored-by: Pierre Sassoulas <[email protected]>

Update astroid/decorators.py

481805c

Co-authored-by: Pierre Sassoulas <[email protected]>

Update astroid/nodes/node_classes.py

b5af638

Co-authored-by: Pierre Sassoulas <[email protected]>

Update tests/unittest_nodes.py

db0ea1c

Co-authored-by: Pierre Sassoulas <[email protected]>

cdce8p requested changes Oct 23, 2021

View reviewed changes

thejcannon closed this Nov 4, 2021

cdce8p added a commit to cdce8p/astroid that referenced this pull request Nov 6, 2021

Small changes

ddf44ce

* Based on suggestions from pylint-dev#1218

cdce8p added a commit to cdce8p/astroid that referenced this pull request Nov 6, 2021

Small changes

d91bb71

* Based on suggestions from pylint-dev#1218

cdce8p mentioned this pull request Nov 6, 2021

Small changes #1232

Merged

cdce8p added a commit that referenced this pull request Nov 6, 2021

Small changes (#1232)

62f3cfb

* Based on suggestions from #1218

This was referenced Jan 12, 2022

Implementation question #1336

Closed

Should astroid adhere to ast's module API ? #1361

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the `*_fields` attributes of `init`/`postinit` are cohesive #1218

Ensure the `*_fields` attributes of `init`/`postinit` are cohesive #1218

thejcannon commented Oct 22, 2021 •

edited

Loading

Pierre-Sassoulas left a comment

Pierre-Sassoulas Oct 23, 2021

cdce8p Oct 23, 2021

Pierre-Sassoulas Oct 24, 2021

DanielNoord Oct 24, 2021

cdce8p left a comment

cdce8p Oct 23, 2021

cdce8p Oct 23, 2021

thejcannon Oct 29, 2021

cdce8p Oct 23, 2021

cdce8p Oct 23, 2021

cdce8p Oct 23, 2021

cdce8p Oct 23, 2021

cdce8p Oct 23, 2021

cdce8p Oct 23, 2021

cdce8p Oct 23, 2021

cdce8p Oct 23, 2021

thejcannon commented Oct 29, 2021

cdce8p commented Nov 1, 2021

Pierre-Sassoulas commented Nov 3, 2021

cdce8p commented Nov 3, 2021 •

edited

Loading

Pierre-Sassoulas commented Nov 3, 2021

thejcannon commented Nov 4, 2021

		lineno: Optional[int] = None,
		col_offset: Optional[int] = None,

		self.fromname: Optional[str] = fromname # can be None
		self.modname = self.fromname # For backwards compatibility

Ensure the *_fields attributes of __init__/postinit are cohesive #1218

Ensure the *_fields attributes of __init__/postinit are cohesive #1218

Conversation

thejcannon commented Oct 22, 2021 • edited Loading

Steps

Description

Type of Changes

Related Issue

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejcannon commented Oct 29, 2021

cdce8p commented Nov 1, 2021

Pierre-Sassoulas commented Nov 3, 2021

cdce8p commented Nov 3, 2021 • edited Loading

Pierre-Sassoulas commented Nov 3, 2021

thejcannon commented Nov 4, 2021

Ensure the `*_fields` attributes of `init`/`postinit` are cohesive #1218

Ensure the `*_fields` attributes of `init`/`postinit` are cohesive #1218

thejcannon commented Oct 22, 2021 •

edited

Loading

cdce8p commented Nov 3, 2021 •

edited

Loading