From c28c1f534d13a5866b4634bd63646bf843dc2fa8 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 23 Dec 2025 22:34:20 -0500 Subject: [PATCH] [ty] Check `__delitem__` instead of `__getitem__` for `del x[k]` (#22121) ## Summary Previously, `del x[k]` incorrectly required the object to have a `__getitem__` method. This was wrong because deletion only needs `__delitem__`, which is independent of `__getitem__`. Closes https://github.com/astral-sh/ty/issues/1799. --- .../resources/mdtest/del.md | 68 ++++++++ .../src/types/diagnostic.rs | 15 +- .../src/types/infer/builder.rs | 159 +++++++++++++++++- 3 files changed, 237 insertions(+), 5 deletions(-) diff --git a/crates/ty_python_semantic/resources/mdtest/del.md b/crates/ty_python_semantic/resources/mdtest/del.md index 52b1c9c4ed..6b0676ad00 100644 --- a/crates/ty_python_semantic/resources/mdtest/del.md +++ b/crates/ty_python_semantic/resources/mdtest/del.md @@ -171,6 +171,8 @@ reveal_type(c.x) # revealed: int ## Delete items +### Basic item deletion + Deleting an item also invalidates the narrowing by the assignment, but accessing the item itself is still valid. @@ -189,3 +191,69 @@ def f(l: list[int]): del l[0] reveal_type(l[0]) # revealed: int ``` + +### `__delitem__` without `__getitem__` + +A class or protocol that only defines `__delitem__` (without `__getitem__`) should still support +item deletion. The `__delitem__` method is independent of `__getitem__`. + +```py +from typing import Protocol, TypeVar + +KT = TypeVar("KT") + +class CanDelItem(Protocol[KT]): + def __delitem__(self, k: KT, /) -> None: ... + +def f(x: CanDelItem[int], k: int): + # This should be valid - the object has __delitem__ + del x[k] + +class OnlyDelItem: + def __delitem__(self, key: int) -> None: + pass + +d = OnlyDelItem() +del d[0] # OK + +# error: [non-subscriptable] "Cannot subscript object of type `OnlyDelItem` with no `__getitem__` method" +d[0] +``` + +### `__getitem__` without `__delitem__` + +A class that only defines `__getitem__` (without `__delitem__`) should not support item deletion. + +```py +class OnlyGetItem: + def __getitem__(self, key: int) -> str: + return "value" + +g = OnlyGetItem() +reveal_type(g[0]) # revealed: str + +# error: [non-subscriptable] "Cannot delete subscript on object of type `OnlyGetItem` with no `__delitem__` method" +del g[0] +``` + +### TypedDict deletion + +Deleting a required key from a TypedDict is a type error because it would make the object no longer +a valid instance of that TypedDict type. + +```py +from typing_extensions import TypedDict + +class Movie(TypedDict): + name: str + year: int + +m: Movie = {"name": "Blade Runner", "year": 1982} + +# error: [invalid-argument-type] +del m["name"] +``` + +TODO: Deletion of `NotRequired` keys (or keys in `total=False` TypedDicts) should be allowed, but ty +currently uses the typeshed stub `__delitem__(k: Never)` which rejects all deletions. See +mypy/pyright behavior for reference. diff --git a/crates/ty_python_semantic/src/types/diagnostic.rs b/crates/ty_python_semantic/src/types/diagnostic.rs index 86e6af5613..5ec7a9efc0 100644 --- a/crates/ty_python_semantic/src/types/diagnostic.rs +++ b/crates/ty_python_semantic/src/types/diagnostic.rs @@ -2385,10 +2385,17 @@ pub(super) fn report_non_subscriptable( let Some(builder) = context.report_lint(&NON_SUBSCRIPTABLE, node) else { return; }; - builder.into_diagnostic(format_args!( - "Cannot subscript object of type `{}` with no `{method}` method", - non_subscriptable_ty.display(context.db()) - )); + if method == "__delitem__" { + builder.into_diagnostic(format_args!( + "Cannot delete subscript on object of type `{}` with no `{method}` method", + non_subscriptable_ty.display(context.db()) + )); + } else { + builder.into_diagnostic(format_args!( + "Cannot subscript object of type `{}` with no `{method}` method", + non_subscriptable_ty.display(context.db()) + )); + } } pub(super) fn report_slice_step_size_zero(context: &InferContext, node: AnyNodeRef) { diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index d3eeca11e6..76d77638ec 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -4304,6 +4304,161 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } } + /// Validate a subscript deletion of the form `del object[key]`. + fn validate_subscript_deletion( + &self, + target: &ast::ExprSubscript, + object_ty: Type<'db>, + slice_ty: Type<'db>, + ) { + self.validate_subscript_deletion_impl(target, None, object_ty, slice_ty); + } + + fn validate_subscript_deletion_impl( + &self, + target: &'ast ast::ExprSubscript, + full_object_ty: Option>, + object_ty: Type<'db>, + slice_ty: Type<'db>, + ) { + let db = self.db(); + + let attach_original_type_info = |diagnostic: &mut LintDiagnosticGuard| { + if let Some(full_object_ty) = full_object_ty { + diagnostic.info(format_args!( + "The full type of the subscripted object is `{}`", + full_object_ty.display(db) + )); + } + }; + + match object_ty { + Type::Union(union) => { + for element_ty in union.elements(db) { + self.validate_subscript_deletion_impl( + target, + full_object_ty.or(Some(object_ty)), + *element_ty, + slice_ty, + ); + } + } + + Type::Intersection(intersection) => { + // Check if any positive element supports deletion + let mut any_valid = false; + for element_ty in intersection.positive(db) { + if self.can_delete_subscript(*element_ty, slice_ty) { + any_valid = true; + break; + } + } + + // If none are valid, emit a diagnostic for the first failing element + if !any_valid { + if let Some(element_ty) = intersection.positive(db).first() { + self.validate_subscript_deletion_impl( + target, + full_object_ty.or(Some(object_ty)), + *element_ty, + slice_ty, + ); + } + } + } + + _ => { + match object_ty.try_call_dunder( + db, + "__delitem__", + CallArguments::positional([slice_ty]), + TypeContext::default(), + ) { + Ok(_) => {} + Err(err) => match err { + CallDunderError::PossiblyUnbound { .. } => { + if let Some(builder) = self + .context + .report_lint(&POSSIBLY_MISSING_IMPLICIT_CALL, target) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Method `__delitem__` of type `{}` may be missing", + object_ty.display(db), + )); + attach_original_type_info(&mut diagnostic); + } + } + CallDunderError::CallError(call_error_kind, bindings) => { + match call_error_kind { + CallErrorKind::NotCallable => { + if let Some(builder) = + self.context.report_lint(&CALL_NON_CALLABLE, target) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Method `__delitem__` of type `{}` is not callable \ + on object of type `{}`", + bindings.callable_type().display(db), + object_ty.display(db), + )); + attach_original_type_info(&mut diagnostic); + } + } + CallErrorKind::BindingError => { + if let Some(builder) = + self.context.report_lint(&INVALID_ARGUMENT_TYPE, target) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Method `__delitem__` of type `{}` cannot be called \ + with key of type `{}` on object of type `{}`", + bindings.callable_type().display(db), + slice_ty.display(db), + object_ty.display(db), + )); + attach_original_type_info(&mut diagnostic); + } + } + CallErrorKind::PossiblyNotCallable => { + if let Some(builder) = + self.context.report_lint(&CALL_NON_CALLABLE, target) + { + let mut diagnostic = builder.into_diagnostic(format_args!( + "Method `__delitem__` of type `{}` may not be callable \ + on object of type `{}`", + bindings.callable_type().display(db), + object_ty.display(db), + )); + attach_original_type_info(&mut diagnostic); + } + } + } + } + CallDunderError::MethodNotAvailable => { + report_non_subscriptable( + &self.context, + target, + object_ty, + "__delitem__", + ); + } + }, + } + } + } + } + + /// Check if a type supports subscript deletion (has `__delitem__`). + fn can_delete_subscript(&self, object_ty: Type<'db>, slice_ty: Type<'db>) -> bool { + let db = self.db(); + object_ty + .try_call_dunder( + db, + "__delitem__", + CallArguments::positional([slice_ty]), + TypeContext::default(), + ) + .is_ok() + } + /// Make sure that the attribute assignment `obj.attribute = value` is valid. /// /// `target` is the node for the left-hand side, `object_ty` is the type of `obj`, `attribute` is @@ -11385,7 +11540,9 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { Type::Never } ExprContext::Del => { - self.infer_subscript_load(subscript); + let value_ty = self.infer_expression(value, TypeContext::default()); + let slice_ty = self.infer_expression(slice, TypeContext::default()); + self.validate_subscript_deletion(subscript, value_ty, slice_ty); Type::Never } ExprContext::Invalid => {