gh-139933: correctly suggest attributes for classes with a custom __dir__

[picnixz]

This is based on the observation by @Locked-chess-official about obj.__dir__ returning the unsorted and unfiltered list of names that could be returned by dir. I misassumed that obj.__dir__ had some magic applied onto it so thanks for the idea.

cc @ambv@devdanzin

• Issue: Safer attribute suggestion for classes with custom "bad" __dir__ #139933

[picnixz]

Using a isinstance(obj, type) test would not be good in the first place because we could try to get the suggestions of a metaclass itself, so we should first try obj.__dir__ anyway.

[Locked-chess-official]

Add this test:

import sys
sys.modules['intmodule'] = int
from intmodule import ...

The name is pending.

[picnixz]

sys.modules['intmodule'] = int

I wouldn't count this as a legitimate use. Imports should only return modules. I don't want to support a bad sys.modules here.

[picnixz]

Or did I misunderstand and you want to test that there is indeed nothing good here?

[picnixz]

Note: the exception would occur in mod = __import__(exc_value.name) because it would be int in this case. So I don't think we should do anything more here.

[Locked-chess-official]

Note: the exception would occur in mod = __import__(exc_value.name) because it would be int in this case. So I don't think we should do anything more here.

If the modulename in sys.modules, the exception will be raised only if it is None and message is "ModuleNotFoundError: import if $name halted, None in sys.modules".

[picnixz]

I don't understand. Your example already fails at the __import__(exc_value.name), not when computing the suggestion:

importsyssys.modules["foo"] =intfromfooimportrea

This will raise an ImportError and we won't have a suggestion here at all. For the runtime completion suggestions, this is something different. The ImportError is:

ImportError: cannot import name 'rea' from 'int' (unknown location)

[Locked-chess-official]

If it is exception in __import__, it is strange. I'll take more time to read the code.

[picnixz]

That's because you're importing intmodule. exc_value.name here is int, not intmodule (and that's because it's not a module I guess).

EDIT: when you write from intmodule import X, what happens is that we try to import sys.modules["intmodule"].__name__ which is int and not intmodule.

For instance:

classFakeMeta(type):
__name__="re"classFakeName(metaclass=FakeMeta):
passimportsyssys.modules["fake"] =FakeNametry:
fromfakeimportescapeexceptImportErrorasexc:
print(exc.name) # prints "re"print(exc.name_from) # prints "escape"

IOW, when writing from X import Y, the X is not relevant as we actually use the name given by the module. This is so that we can also write sys.modules[fancy_name] = types.ModuleName("other_fancy_name").

[Locked-chess-official]

If it is because that, the question is exist:

If the name is the class name?
If the module name "int" is strange, the question will also occurs on the custom class.

[picnixz]

Ok, so this happens when we reuse the same name. But I don't think it's ok to mess things like that. My point is: what do you want me to do here? the behavior is somehow correct but if we want to have the same behavior for from fake import rea when sys.modules["fake"] = int and sys.modules["int"] = int, then I don't think it's a good idea. We could check if sys.modules[exc_value.name] already exists without trying to do __import__ but this is really for niche cases. We should not support random classes in sys.modules except None.

[Locked-chess-official]

It is a behavior change. I think that it is needed to be recorded.

[picnixz]

(I deleted a duplicated comment).

What do you mean it's a behavior change? what is changing from before? I did document the change but I will not document a change affecting an incorrect usage of sys.modules.

[picnixz]

I think I understand what you mean. But since __import__ is already failing when trying to import the fake module, this is something that I cannot change in this PR (it seems something different). I'll try to bisect this.

[picnixz]

Ok, I finally understood what you wanted to tell me. Indeed, with my PR, if by chance we store some weird object when importing, then we would indeed have a change of behavior. Now, the existing behavior in 3.13 and 3.14 is "working" but I would say that we're in the garbage-in garbage-out situation where, by chance, it also works.

I don't know if it's preferrable to keep the behavior or actually fix it. Because I don't think it's actually correct to suggest something when the module to import is a class and not a module. OTOH, modules can be simply considered as special namespaces and someone could use classes to emulate singletons as we have for modules (something that is a namespace-like with some internal state emulated as class variables).

Anyway, for now I've kept the existing behavior although I think it's partially incorrect.

[miss-islington-app]

Thanks @picnixz for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

[miss-islington-app]

Thanks @picnixz for the PR, and @ambv for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

[miss-islington-app]

Sorry, @picnixz and @ambv, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 4722202a1a81974089801e6173d269836b6a074f 3.14

[miss-islington-app]

Sorry, @picnixz and @ambv, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 4722202a1a81974089801e6173d269836b6a074f 3.13

[ambv]

I'll deal with the backports, we gotta remove lazy import handling.

[bedevere-app]

GH-145827 is a backport of this pull request to the 3.14 branch.

[picnixz]

Thanks for the review & merge!