From 02394b8049b52836ae7daca7132fab93031d1162 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Wed, 21 May 2025 15:38:56 -0400 Subject: [PATCH] [ty] Improve `invalid-type-form` diagnostic where a module-literal type is used in a type expression and the module has a member which would be valid in a type expression (#18244) --- .../resources/mdtest/annotations/invalid.md | 32 ++++++++++ ..._Module-literal_used_…_(652fec4fd4a6c63a).snap | 62 +++++++++++++++++++ crates/ty_python_semantic/src/types.rs | 49 ++++++++++++--- 3 files changed, 136 insertions(+), 7 deletions(-) create mode 100644 crates/ty_python_semantic/resources/mdtest/snapshots/invalid.md_-_Tests_for_invalid_ty…_-_Diagnostics_for_comm…_-_Module-literal_used_…_(652fec4fd4a6c63a).snap diff --git a/crates/ty_python_semantic/resources/mdtest/annotations/invalid.md b/crates/ty_python_semantic/resources/mdtest/annotations/invalid.md index 96a9dfe32d..ed4571330e 100644 --- a/crates/ty_python_semantic/resources/mdtest/annotations/invalid.md +++ b/crates/ty_python_semantic/resources/mdtest/annotations/invalid.md @@ -113,3 +113,35 @@ def _( reveal_type(f) # revealed: Unknown reveal_type(g) # revealed: Unknown ``` + +## Diagnostics for common errors + + + +### Module-literal used when you meant to use a class from that module + +It's pretty common in Python to accidentally use a module-literal type in a type expression when you +*meant* to use a class by the same name that comes from that module. We emit a nice subdiagnostic +for this case: + +`foo.py`: + +```py +import datetime + +def f(x: datetime): ... # error: [invalid-type-form] +``` + +`PIL/Image.py`: + +```py +class Image: ... +``` + +`bar.py`: + +```py +from PIL import Image + +def g(x: Image): ... # error: [invalid-type-form] +``` diff --git a/crates/ty_python_semantic/resources/mdtest/snapshots/invalid.md_-_Tests_for_invalid_ty…_-_Diagnostics_for_comm…_-_Module-literal_used_…_(652fec4fd4a6c63a).snap b/crates/ty_python_semantic/resources/mdtest/snapshots/invalid.md_-_Tests_for_invalid_ty…_-_Diagnostics_for_comm…_-_Module-literal_used_…_(652fec4fd4a6c63a).snap new file mode 100644 index 0000000000..abe619e045 --- /dev/null +++ b/crates/ty_python_semantic/resources/mdtest/snapshots/invalid.md_-_Tests_for_invalid_ty…_-_Diagnostics_for_comm…_-_Module-literal_used_…_(652fec4fd4a6c63a).snap @@ -0,0 +1,62 @@ +--- +source: crates/ty_test/src/lib.rs +expression: snapshot +--- +--- +mdtest name: invalid.md - Tests for invalid types in type expressions - Diagnostics for common errors - Module-literal used when you meant to use a class from that module +mdtest path: crates/ty_python_semantic/resources/mdtest/annotations/invalid.md +--- + +# Python source files + +## foo.py + +``` +1 | import datetime +2 | +3 | def f(x: datetime): ... # error: [invalid-type-form] +``` + +## PIL/Image.py + +``` +1 | class Image: ... +``` + +## bar.py + +``` +1 | from PIL import Image +2 | +3 | def g(x: Image): ... # error: [invalid-type-form] +``` + +# Diagnostics + +``` +error[invalid-type-form]: Variable of type `` is not allowed in a type expression + --> src/foo.py:3:10 + | +1 | import datetime +2 | +3 | def f(x: datetime): ... # error: [invalid-type-form] + | ^^^^^^^^ + | +info: Did you mean to use the module's member `datetime.datetime` instead? +info: rule `invalid-type-form` is enabled by default + +``` + +``` +error[invalid-type-form]: Variable of type `` is not allowed in a type expression + --> src/bar.py:3:10 + | +1 | from PIL import Image +2 | +3 | def g(x: Image): ... # error: [invalid-type-form] + | ^^^^^ + | +info: Did you mean to use the module's member `Image.Image` instead? +info: rule `invalid-type-form` is enabled by default + +``` diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index 733628624e..6c9123acae 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -4821,7 +4821,7 @@ impl<'db> Type<'db> { pub fn in_type_expression( &self, db: &'db dyn Db, - scope_id: ScopeId, + scope_id: ScopeId<'db>, ) -> Result, InvalidTypeExpressionError<'db>> { match self { // Special cases for `float` and `complex` @@ -4872,7 +4872,9 @@ impl<'db> Type<'db> { | Type::BoundSuper(_) | Type::ProtocolInstance(_) | Type::PropertyInstance(_) => Err(InvalidTypeExpressionError { - invalid_expressions: smallvec::smallvec![InvalidTypeExpression::InvalidType(*self)], + invalid_expressions: smallvec::smallvec![InvalidTypeExpression::InvalidType( + *self, scope_id + )], fallback_type: Type::unknown(), }), @@ -4910,7 +4912,7 @@ impl<'db> Type<'db> { return Err(InvalidTypeExpressionError { fallback_type: Type::unknown(), invalid_expressions: smallvec::smallvec![ - InvalidTypeExpression::InvalidType(*self) + InvalidTypeExpression::InvalidType(*self, scope_id) ], }); }; @@ -5037,7 +5039,7 @@ impl<'db> Type<'db> { )), _ => Err(InvalidTypeExpressionError { invalid_expressions: smallvec::smallvec![InvalidTypeExpression::InvalidType( - *self + *self, scope_id )], fallback_type: Type::unknown(), }), @@ -5751,7 +5753,8 @@ impl<'db> InvalidTypeExpressionError<'db> { let Some(builder) = context.report_lint(&INVALID_TYPE_FORM, node) else { continue; }; - builder.into_diagnostic(error.reason(context.db())); + let diagnostic = builder.into_diagnostic(error.reason(context.db())); + error.add_subdiagnostics(context.db(), diagnostic); } } fallback_type @@ -5778,7 +5781,7 @@ enum InvalidTypeExpression<'db> { /// and which would require exactly one argument even if they appeared in an annotation expression TypeQualifierRequiresOneArgument(KnownInstanceType<'db>), /// Some types are always invalid in type expressions - InvalidType(Type<'db>), + InvalidType(Type<'db>, ScopeId<'db>), } impl<'db> InvalidTypeExpression<'db> { @@ -5822,7 +5825,7 @@ impl<'db> InvalidTypeExpression<'db> { "Type qualifier `{q}` is not allowed in type expressions (only in annotation expressions, and only with exactly one argument)", q = qualifier.repr(self.db) ), - InvalidTypeExpression::InvalidType(ty) => write!( + InvalidTypeExpression::InvalidType(ty, _) => write!( f, "Variable of type `{ty}` is not allowed in a type expression", ty = ty.display(self.db) @@ -5833,6 +5836,38 @@ impl<'db> InvalidTypeExpression<'db> { Display { error: self, db } } + + fn add_subdiagnostics(self, db: &'db dyn Db, mut diagnostic: LintDiagnosticGuard) { + let InvalidTypeExpression::InvalidType(ty, scope) = self else { + return; + }; + let Type::ModuleLiteral(module_type) = ty else { + return; + }; + let module = module_type.module(db); + let Some(module_name_final_part) = module.name().components().next_back() else { + return; + }; + let Some(module_member_with_same_name) = ty + .member(db, module_name_final_part) + .symbol + .ignore_possibly_unbound() + else { + return; + }; + if module_member_with_same_name + .in_type_expression(db, scope) + .is_err() + { + return; + } + + // TODO: showing a diff (and even having an autofix) would be even better + diagnostic.info(format_args!( + "Did you mean to use the module's member \ + `{module_name_final_part}.{module_name_final_part}` instead?" + )); + } } /// Whether this typecar was created via the legacy `TypeVar` constructor, or using PEP 695 syntax.