Reexported class with same name as module treated as module

[BlueskyFR]

Environment data

• Language Server version: 2021.3.1
• OS and version: linux x64
• Python version (and distribution if applicable, e.g. Anaconda):
• python.analysis.indexing: undefined
• python.analysis.typeCheckingMode: off

Expected behaviour

Pylance should show an error if the user hasn't implemented the abstract function a class inherits.

Actual behaviour

Pylance thinks it is normal!

Code Snippet / Additional information

File 1 :

class IDataLoader(ABC):
    @abstractmethod
    def get_training_dataset(self) -> Tuple[str, Dataset]:
        pass

__init__.py file :

from .IDataLoader import IDataLoader

File 2 :

from . import IDataLoader

class ImageNet(IDataLoader):
    def __init__(self):
        pass

Here, Pylance should underline class ImageNet(IDataLoader): in red telling that implementation of IDataLoader is incorrect because the implementation of IDataLoader specifies that get_training_dataset() is required.
On top of that, Pylance should provide a "quick fix" to automatically implement all the required methods. This behavior is already implemented in PyCharm.

[erictraut]

Pylance will report a problem if you enable the type checker (set typeCheckingMode to "basic") and attempt to instantiate a class that has unimplemented abstract methods.

If you go to the child class and start typing the implementation of a function (e.g. type "def g"), pylance will suggest a completion and auto-fill an initial implementation. Of course, writing the actual implementation will require some additional coding on your part.

[BlueskyFR]

I didn't know about the typeCheckingMode: thanks for that.
However, it seems like PyLance does not read __init__ files:

In the same folder, in the __init__.py file I have:

from .ImageNet import ImageNet

So PyLance should detect that the import is not a module but a class 🤔

[BlueskyFR]

Also, no completion seems to be provided for abstract properties:

[erictraut]

Pylance handles __init__.py files fine. The statement from .ImageNet import ImageNet works only if you have a file called ImageNet.py in the same directory as the __init__.py file. Is that the case?

You are correct about completion suggestions not being offered for abstract properties. We'd appreciate it if you'd file a new issue for that.

[BlueskyFR]

Pylance handles __init__.py files fine. The statement from .ImageNet import ImageNet works only if you have a file called ImageNet.py in the same directory as the __init__.py file. Is that the case?

Yes it is: this is the problem.

You are correct about completion suggestions not being offered for abstract properties. We'd appreciate it if you'd file a new issue for that.

Sure! I'll open a new issue for that so we can focus on the other problem.

[jakebailey]

This is reproducible for me:

This seems in the vein of "module name and variable name are the same" and things break, which we've had issues with before and have fixed. Most codebases don't name modules the same as things they contain to avoid aliasing ambiguity like this.

[erictraut]

There was a bug in the handling of from . import X when used in a file other than a __init__.py file. This usage is very unusual, which is why I suspect this problem has never been reported before. When this statement is used in a file other than an __init__.py, it must resolve the import from the __init__.py even if there is a submodule by the same name. This will be fixed in the next release.

[jakebailey]

I did a test pull to see, and it does fix it for the non-__init__.py case:

However in __init__.py itself, it isn't the right type:

PS E:\work\test_projects\pylance1050> python main.py
from .IDataLoader import IDataLoader <class 'module.IDataLoader.IDataLoader'>

Note "class" at runtime, but "module" in all of the hovers. The last two should be class.

I'll also point out this edge case:

from .IDataLoader import IDataLoader <class 'module.IDataLoader.IDataLoader'>
from . import IDataLoader <class 'module.IDataLoader.IDataLoader'>

Here it says module as well, but it should also be class because in effect __init__.py is importing from itself and it should see the previous decl. But, remove the top import, and at runtime you get:

from . import IDataLoader <module 'module.IDataLoader' from 'E:\\work\\test_projects\\pylance1050\\module\\IDataLoader.py'>

Because current module's symbol table doesn't have IDataLoader in it yet, so it resolves it with a real module import.

[jakebailey]

Test repo here: https://github.com/jakebailey/pylance1050

[jakebailey]

When this statement is used in a file other than an __init__.py, it must resolve the import from the __init__.py even if there is a submodule by the same name.

I think this algorithm is actually "from . import X always pulls from __init__.py's symbol table (even if that's the current file), and if this fails, the module is imported and put into the table". It's just that __init__.py appears to always grab a module because it hasn't set anything yet. If you do X = 1234 then from . import X, you get 1234, not a submodule.

[BlueskyFR]

This is reproducible for me:

This seems in the vein of "module name and variable name are the same" and things break, which we've had issues with before and have fixed. Most codebases don't name modules the same as things they contain to avoid aliasing ambiguity like this.

Yeah actually my goal is to define one file for each class, so it makes no sense to me to have to write IDataLoader.IDataLoader everywhere, which is why I use this syntax through __init__.py.

Is there another more conventional way you would recommend to keep the code clear?

[jakebailey]

Warning, opinion ahead.

You won't like this, I'm guessing, but as far as I'm aware, it's a bit un-pythonic to stick one class in each file as though you were writing Java or C# (where you must have a file or more per class and no other code). Most codebases aren't structured like this and instead would probably put most of these types in the same file as one conceptual entity. E.g., maybe you have a loaders module that contains loaders and people write import myproject.loaders; myproject.loaders.DataLoader or something. Or, abstract classes in one common module, but then some imagenet module that contains ImageNet related functionality. Python modules are typically lowercased or unique and that ends up making people avoid these sorts of troubles, or expect people to only ever import absolutely and avoid any of this odd relative behavior.

I'll also say that in terms of styling, if you're making an abstract class, it's more common to name things something like "AbstractDataLoader" (no naming at all) rather than "IDataLoader". You may also want to consider looking into Protocol if trying to define a type by its methods for other users (like you would in TS or C# with interfaces).

This is all unrelated to the bug here, of course. Drive by code reviews... 🙂 I don't want to digress forever.

[erictraut]

It's also very unusual in the Python world to see source file names that are capitalized or make use of camel casing. They're almost always lowercase with underscores separating words. As Jake said, that's just convention.

[erictraut]

@jakebailey , I found and fixed one more issue in the hover provider that was causing the hover text to return a type that was inconsistent with the type evaluator.

The other issue you found is that the import results can be affected by the side effects of imports that happen earlier in time. As you know, this is an extremely unfortunate property of the python import system, and I consider it a bug if anyone relies on such behavior. So I don't consider that a bug in the type checker, and I don't plan to make a change to accommodate that.

[BlueskyFR]

Thank you guys for your feedback on this, I am using one file for each class because I think that having 500 lines long files is harder to maintain in my opinion; however, I will definitely look deeper in Python modules :)

Please be sure to let me know if you need additional help on the issue

[jakebailey]

I found and fixed one more issue in the hover provider that was causing the hover text to return a type that was inconsistent with the type evaluator.

Thanks, I can confirm it works.

The other issue you found is that the import results can be affected by the side effects of imports that happen earlier in time. As you know, this is an extremely unfortunate property of the python import system, and I consider it a bug if anyone relies on such behavior. So I don't consider that a bug in the type checker, and I don't plan to make a change to accommodate that.

At least in the example I've given, there isn't side effect behavior. These two examples are functionally the same to the last import statement, because from . import X in __init__.py is the same as accessing X at that point in the code flow.

from foo import X

from . import X

X = 1234

from . import X

While I think this is unlikely, doing it wrong may mean people who accidentally shadow a module name above the import statement in their __init__.py will mistakenly believe that their code works. But, thankfully, it appears to have worked fine and the hover bug was just obscuring it:

So, it worked out. 🙂

[jakebailey]

Please be sure to let me know if you need additional help on the issue

There shouldn't be anything needed anymore. I think your example should work as expected now and you can test after our release today.

[jakebailey]

This issue has been fixed in version 2021.3.2, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/main/CHANGELOG.md#202132-17-march-2021