Clarify on non-Module roots

[temyurchenko]

The PR fixes a missing parent on the "main" node and weakens the typing of "NodeNG.root" to better reflect the reality

I don't know what entry to fill in in the ChangeLog.

Type of Changes

	Type
✓	🐛 Bug fix
✓	📜 Docs

Description

Refs #120, partially.

[temyurchenko]

The reason the CI is failing is that at the class construction stage classes have "Unknown" for a parent. On the previous revision, the result was just that all builtin (or not just?) classes have "Unknown" as their locals['__module__'].
I'm thinking of a better way to approach it in this version.

[DanielNoord]

I think this will cause a lot of type errors in pylint as we assume root() always returns... Can we get away by asserting?

[temyurchenko]

I think this will cause a lot of type errors in pylint as we assume root() always returns... Can we get away by asserting?

I'm currently in the process of eliminating non-module roots. Without that, the assert is going to fail a loot of times.

[temyurchenko]

I think this will cause a lot of type errors in pylint as we assume root() always returns... Can we get away by asserting?

What is the difference between Uninferable and InferenceError? Which one should be used and in what cases? Is there some documentation on it? Or a specific person to ask?

[DanielNoord]

I think this will cause a lot of type errors in pylint as we assume root() always returns... Can we get away by asserting?

I'm currently in the process of eliminating non-module roots. Without that, the assert is going to fail a loot of times.

I don't understand what you mean with this?

I think this will cause a lot of type errors in pylint as we assume root() always returns... Can we get away by asserting?

What is the difference between Uninferable and InferenceError? Which one should be used and in what cases? Is there some documentation on it? Or a specific person to ask?

It depends on the situation. Ideally InferenceError isn't exposed to the users of astroid such as pylint so that they only need to handle Uninferable.

[temyurchenko]

I don't understand what you mean with this?

The root() currently returns non-Module nodes, often. So, if we just assert that the return is a Module, that assert is going to fail often.

It depends on the situation. Ideally InferenceError isn't exposed to the users of astroid such as pylint so that they only need to handle Uninferable.

What is the logic for preferring one or the other internally? I often see lines like:

            if not name.startswith("__") and self.has_dynamic_getattr(context):
                # class handle some dynamic attributes, return a Uninferable object
                yield util.Uninferable
            else:
                raise InferenceError(
                    str(error), target=self, attribute=name, context=context
                ) from error

(this one is from scoped_nodes.ClassDef.igetattr)

[temyurchenko]

I don't understand what you mean with this?

The root() currently returns non-Module nodes, often. So, if we just assert that the return is a Module, that assert is going to fail often.

It depends on the situation. Ideally InferenceError isn't exposed to the users of astroid such as pylint so that they only need to handle Uninferable.

What is the logic for preferring one or the other internally? I often see lines like:

            if not name.startswith("__") and self.has_dynamic_getattr(context):
                # class handle some dynamic attributes, return a Uninferable object
                yield util.Uninferable
            else:
                raise InferenceError(
                    str(error), target=self, attribute=name, context=context
                ) from error

(this one is from scoped_nodes.ClassDef.igetattr)

What is the logic for preferring one or the other internally? I often see lines like:

I see, raising the InferenceError doesn't let adding new inference results, while just yielding uninferrable does allow it.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.24%. Comparing base (d3df248) to head (5e77e13).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2536      +/-   ##
==========================================
- Coverage   93.25%   93.24%   -0.01%     
==========================================
  Files          93       93              
  Lines       11050    11049       -1     
==========================================
- Hits        10305    10303       -2     
- Misses        745      746       +1     

Flag	Coverage Δ
linux	93.13% <100.00%> (-0.01%)	⬇️
pypy	93.24% <100.00%> (-0.01%)	⬇️
windows	93.23% <100.00%> (-0.01%)	⬇️

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

Files with missing lines	Coverage Δ
astroid/brain/brain_argparse.py	95.83% <ø> (ø)
astroid/brain/brain_builtin_inference.py	92.06% <100.00%> (ø)
astroid/brain/brain_namedtuple_enum.py	93.86% <ø> (ø)
astroid/manager.py	90.00% <100.00%> (ø)
astroid/nodes/node_ng.py	93.05% <100.00%> (+0.04%)	⬆️
astroid/nodes/scoped_nodes/scoped_nodes.py	93.33% <ø> (-0.01%)	⬇️
astroid/objects.py	94.26% <ø> (ø)
astroid/raw_building.py	94.55% <ø> (-0.04%)	⬇️

... and 1 file with indirect coverage changes

[temyurchenko]

Now, this should also close #1490.

I tried my best to give complete descriptions to the commits, because they are lengthy. I also changed the annotation of root() back to just Module (with an added assert).

I'm happy to elaborate more on changes, since there are quite a few of them.

As a side note, I still feel that it's not a very honest type signature, given how many nodes we are creating out-of-tree and thus without a parent (see const_factory). Furthermore, there is no enforcement of the parent is not None invariant anywhere in the codebase, so the accidents are bound to creep in.

[temyurchenko]

Also fixes #2517

[DanielNoord]

Awesome changes! But also pretty hard to review.

Maintainer time on astroid is sparse and nobody has actively worked on its internals for about a year.

I'd be open to spending some time of the next days/weeks to get this merged but I an only approve smaller bite sized PRs. If you're op for that let's go down that route!

For example, the first commit could be a separate PR to declutter this PR.
The second commit does a lot of things at once (as indicated by the commit message). I think a lot of changes in there could be separated into separate PRs, also to show their effects on the tests more clearly. That helps with reviewing them.

[DanielNoord]

Not sure about the name of adhoc. But I also don't have better suggestions

[temyurchenko]

I can come up with other options. Please, tell what you are unsure about it, to guide the thought process for new names.

[DanielNoord]

@Pierre-Sassoulas Any suggestions here?

[Pierre-Sassoulas]

Maybe synthetic_root_node, placeholder_root_node, placeholder_parent_node, placeholder_module_node ? wasn't sure what adhoc meant exactly before checking the definition to be honest.

[temyurchenko]

I'd be open to spending some time of the next days/weeks to get this merged but I an only approve smaller bite sized PRs. If you're op for that let's go down that route!

For sure.

For example, the first commit could be a separate PR to declutter this PR.

Will do.

The second commit does a lot of things at once (as indicated by the commit message). I think a lot of changes in there could be separated into separate PRs, also to show their effects on the tests more clearly. That helps with reviewing them.

Will do.

Regarding isort. I've grepped the project and found a few similar isort comment. Based on that I concluded that the project uses isort to sort imports. However, when I ran isort on it, it resulted in quite a few changes, thus the "isort: off" comments. Could you please tell me if I should use isort? If so, is there a special config for the project? I wasn't able to find one.

[DanielNoord]

astroid/pyproject.toml

Line 146 in 7954bac

"I", # isort

We use ruff to do our import sorting now. Any comments you found are probably outdated (and could be removed in a separate PR)

[temyurchenko]

I think a lot of changes in there could be separated into separate PRs, also to show their effects on the tests more clearly.

Is it possible to have something like an merge train on github? That is, for one PR to depend on some previous PR? It's possible to emulate with merging into branches, but then you'd have to do reviews on my local fork, since I can't create branches in this repository.

If not, the simplest way would probably be do just breakup commits within this PR. Would you be open to that?

[DanielNoord]

I think a lot of changes in there could be separated into separate PRs, also to show their effects on the tests more clearly.

Is it possible to have something like an merge train on github? That is, for one PR to depend on some previous PR? It's possible to emulate with merging into branches, but then you'd have to do reviews on my local fork, since I can't create branches in this repository.

If not, the simplest way would probably be do just breakup commits within this PR. Would you be open to that?

Sadly it doesn't.

I'm okay with splitting up in this PR but I think that increases the risk of me (and other maintainers) finding it hard to approve of the full PR and merge it.

Separate PRs make it much easier to spend 15-20 minutes during lunch to review and merge the smaller commits. From experience I know although it involves more rebase work in the end it significantly increases the chances of astroid PRs landing on main.

Edit: I wanted to add that I'm mainly saying this because I really think the changes in this PR are super valuable and should be merged. I'd hate for this to die down due to maintainer fatigue and want to do anything to prevent that from happening :)

[temyurchenko]

I'm okay with splitting up in this PR but I think that increases the risk of me (and other maintainers) finding it hard to approve of the full PR and merge it.

Sure, I'll do my best to achieve that.

[DanielNoord]

To give an update: changes coming a long nicely. @temyurchenko has been very helpful in rebasing and responding to comments.

#2563 is on my radar, but after a long week at work I don't really have the capacity to have a good look at it. I hope to do so this weekend or on Monday.

[DanielNoord]

Awesome! Great job working with us to get this merged. Working with you was a joy!

I would be okay with adding a Changelog for this as I think you work should be recognized!

[Pierre-Sassoulas]

Thank you for your work on this @temyurchenko ! There's some warning in the CI job that need to be fixed before merging and we could rename "adhoc_module" if it makes sense, but LGTM.

[temyurchenko]

Awesome! Great job working with us to get this merged. Working with you was a joy!

I would be okay with adding a Changelog for this as I think you work should be recognized!

Thank you, same for you and @Pierre-Sassoulas!

A bit later, I will do one final pass over the changes here, fix the commit message and fix the the linting errors. Not sure, if the changelog is needed, since this is mostly an internal change.

[temyurchenko]

we could rename "adhoc_module" if it makes sense

Okay, I'll go with "synthetic_root" or "synthetic_module".