-
Notifications
You must be signed in to change notification settings - Fork 216
Description
Summary
Consider this situation:
module.py:
import typingmain.py:
from ty_extensions import is_subtype_of, is_equivalent_to, TypeOf
import typing
from module import typing as other_typing
reveal_type(is_subtype_of(TypeOf[typing], TypeOf[other_typing]))
reveal_type(is_equivalent_to(TypeOf[typing], TypeOf[other_typing]))The reveal_type calls both reveal Literal[False] here, even though typing refers to the same module as other_typing. This is because our implementation of equivalence and subtyping for module-literal types just does a naive == comparison between two ModuleLiteralTypes to determine whether one ModuleLiteralType is equivalent to another. But these two "copies" of the typing module in our representation were originally imported by different modules, so they do not compare equal, because we record the importing module on the type itself in the importing_file field on ModuleLiteralType: https://github.com/astral-sh/ruff/blob/f32f7a3b48ff11351c709c60109fafd5ca18ec6c/crates/ty_python_semantic/src/types.rs#L7499-L7510
The reason why we store the importing_file field on ModuleLiteralTypes at all is so that we can check whether any submodules of that module have been imported in the same file that imported the module. If there were, then we recognize those submodules as being available as attributes on the ModuleLiteralType. That means that for something like this, we would recognize an abc attribute as being available on other_importlib but not importlib:
module.py:
import importlib
import importlib.abcmain.py:
import importlib
from module import importlib as other_importlib
# error: Type `<module 'importlib'>` has no attribute `abc` (unresolved-attribute)
reveal_type(importlib.abc) # revealed: Unknown
reveal_type(other_importlib.abc) # revealed: <module 'importlib.abc'>So perhaps there is an argument that our current implementation is actually correct -- we could treat the two ModuleLiteralTypes differently in some situations, so they shouldn't be seen as equivalent types, even if they point to the same underlying module?
In any case, we should add some tests for this, and some comments to the test explaining why this is currently the case -- I don't think we have any at the moment. No tests fail if I apply this diff to main, which changes the result of the reveal_type calls in my original snippet from Literal[False] to Literal[True]
diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs
index 71e7ab5798..1b3fe9ab5d 100644
--- a/crates/ty_python_semantic/src/types.rs
+++ b/crates/ty_python_semantic/src/types.rs
@@ -1355,6 +1355,9 @@ impl<'db> Type<'db> {
(Type::MethodWrapper(self_method), Type::MethodWrapper(target_method)) => {
self_method.has_relation_to(db, target_method, relation)
}
+ (Type::ModuleLiteral(self_module), Type::ModuleLiteral(target_module)) => {
+ self_module.module(db).file() == target_module.module(db).file()
+ }
// No literal type is a subtype of any other literal type, unless they are the same
// type (which is handled above). This case is not necessary from a correctness
@@ -1630,6 +1633,9 @@ impl<'db> Type<'db> {
| (nominal @ Type::NominalInstance(n), Type::ProtocolInstance(protocol)) => {
n.class.is_object(db) && protocol.normalized(db) == nominal
}
+ (Type::ModuleLiteral(first), Type::ModuleLiteral(second)) => {
+ first.module(db).file() == second.module(db).file()
+ }
_ => false,
}
}