[ty] Respect property deleters in attribute deletion checks#24500
[ty] Respect property deleters in attribute deletion checks#24500charliermarsh merged 4 commits intomainfrom
Conversation
Typing conformance results improved 🎉The percentage of diagnostics emitted that were expected errors increased from 87.79% to 87.80%. The percentage of expected errors that received a diagnostic increased from 82.81% to 82.91%. The number of fully passing files improved from 75/132 to 76/132. SummaryHow are test cases classified?Each test case represents one expected error annotation or a group of annotations sharing a tag. Counts are per test case, not per diagnostic — multiple diagnostics on the same line count as one. Required annotations (
Test file breakdown1 file altered
True positives added (1)1 diagnostic
|
Memory usage reportSummary
Significant changesClick to expand detailed breakdownprefect
sphinx
trio
flake8
|
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-assignment |
1 | 0 | 0 |
| Total | 1 | 0 | 0 |
Raw diff:
pytest (https://github.com/pytest-dev/pytest)
+ src/_pytest/unittest.py:493:21 error[invalid-assignment] Cannot delete read-only property `result` on object of type `CallInfo[None]`: Attempted deletion of `CallInfo[None].result` here
charliermarsh
force-pushed
the
charlie/deleter-i
branch
2 times, most recently
from
8086d7c to
e012b55
Compare
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager,
ibraheemdev and
sharkdp
as code owners
oconnor663
left a comment
oconnor663
left a comment
There was a problem hiding this comment.
LGTM with mostly some style nits and questions I'm not sure about. The one thing that looks like it might need a more substantial changes is del.md:215, if you meant for that case to fail?
| x: int = 1 | ||
|
|
||
| # error: [invalid-assignment] "Cannot delete attribute `x` on type `<class 'RejectsClassDelete'>` whose `__delattr__` method returns `Never`/`NoReturn`" | ||
| del RejectsClassDelete.x |
There was a problem hiding this comment.
Worth including a SupportsClassDelete case in this section that tests def __delattr__(...) -> None on the metaclass?
| if !self.validate_attribute_deletion(target, *element_ty, attribute, false) { | ||
| if emit_diagnostics { | ||
| self.validate_attribute_deletion(target, *element_ty, attribute, true); |
There was a problem hiding this comment.
What's the reason for doing it this way instead of passing emit_diagnostics as an argument to a single call? I don't notice any test failures when I do
diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs
index 3c426815bb..3c4958cef2 100644
--- a/crates/ty_python_semantic/src/types/infer/builder.rs
+++ b/crates/ty_python_semantic/src/types/infer/builder.rs
@@ -2730,10 +2730,12 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> {
match object_ty {
Type::Union(union) => {
for element_ty in union.elements(db) {
- if !self.validate_attribute_deletion(target, *element_ty, attribute, false) {
- if emit_diagnostics {
- self.validate_attribute_deletion(target, *element_ty, attribute, true);
- }
+ if !self.validate_attribute_deletion(
+ target,
+ *element_ty,
+ attribute,
+ emit_diagnostics,
+ ) {
return false;
}
}| } else { | ||
| if emit_diagnostics && let Some(element_ty) = intersection.positive(db).first() | ||
| { | ||
| self.validate_attribute_deletion(target, *element_ty, attribute, true); |
There was a problem hiding this comment.
This one makes more sense to me though, compared to the union one above. The deletion here needs to be valid on any intersection member, so we can't emit diagnostics until we know that all of them fail to validate. 👍
My question here is, is intersection.positive(db).first() always going to give us the same element? positive is an FxOrderSet, so if it's always constructed in the same order then I think the first element will always be the same, but I don't know if there are cases where "something something multiple files multiple threads" could make the construction order nondeterministic. If so, could this maybe produce flaky test cases?
In retrospect I guess I have the same question about union.elements above.
There was a problem hiding this comment.
I think it should be okay. It's an ordered set, and union.elements is a boxed slice.
There was a problem hiding this comment.
Yeah it's clear to me that iterating over those containers is deterministic. What I'm not entirely sure about is whether we can rely on those containers to always be constructed in the same order from one run to the next.
There was a problem hiding this comment.
If we can't, then the blame for the non-determinism would fall on whatever is causing them to be constructed in different orders. The fact that the sets are ordered is sufficient to make this code OK.
| alias.value_type(db), | ||
| attribute, | ||
| emit_diagnostics, | ||
| ), |
There was a problem hiding this comment.
I think(?) this is its own arm because the alias could be a union or intersection underneath, and we want to hit those arms above if so? If that's right, do we need Type::NewTypeInstance to similarly have its own arm? In the "NewType of float or complex" cases, it's also a union. On the other hand, maybe we know that there are no relevant deleters in that case? Probably worth a comment either way.
| match &delattr_dunder_call_result { | ||
| Ok(result) if result.return_type(db).is_never() => { | ||
| if emit_diagnostics | ||
| && let Some(builder) = | ||
| self.context.report_lint(&INVALID_ASSIGNMENT, target) | ||
| { | ||
| builder.into_diagnostic(format_args!( | ||
| "Cannot delete attribute `{attribute}` on type `{}` \ | ||
| whose `__delattr__` method returns `Never`/`NoReturn`", | ||
| object_ty.display(db), | ||
| )); | ||
| } | ||
| return false; | ||
| } | ||
| Err(err) if err.return_type(db).is_some_and(|ty| ty.is_never()) => { | ||
| if emit_diagnostics | ||
| && let Some(builder) = | ||
| self.context.report_lint(&INVALID_ASSIGNMENT, target) | ||
| { | ||
| builder.into_diagnostic(format_args!( | ||
| "Cannot delete attribute `{attribute}` on type `{}` \ | ||
| whose `__delattr__` method returns `Never`/`NoReturn`", | ||
| object_ty.display(db), | ||
| )); | ||
| } | ||
| return false; | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
| match &delattr_dunder_call_result { | |
| Ok(result) if result.return_type(db).is_never() => { | |
| if emit_diagnostics | |
| && let Some(builder) = | |
| self.context.report_lint(&INVALID_ASSIGNMENT, target) | |
| { | |
| builder.into_diagnostic(format_args!( | |
| "Cannot delete attribute `{attribute}` on type `{}` \ | |
| whose `__delattr__` method returns `Never`/`NoReturn`", | |
| object_ty.display(db), | |
| )); | |
| } | |
| return false; | |
| } | |
| Err(err) if err.return_type(db).is_some_and(|ty| ty.is_never()) => { | |
| if emit_diagnostics | |
| && let Some(builder) = | |
| self.context.report_lint(&INVALID_ASSIGNMENT, target) | |
| { | |
| builder.into_diagnostic(format_args!( | |
| "Cannot delete attribute `{attribute}` on type `{}` \ | |
| whose `__delattr__` method returns `Never`/`NoReturn`", | |
| object_ty.display(db), | |
| )); | |
| } | |
| return false; | |
| } | |
| _ => {} | |
| } | |
| let returns_never = match &delattr_dunder_call_result { | |
| Ok(result) => result.return_type(db).is_never(), | |
| Err(err) => err.return_type(db).is_some_and(|ty| ty.is_never()), | |
| }; | |
| if returns_never { | |
| if emit_diagnostics | |
| && let Some(builder) = self.context.report_lint(&INVALID_ASSIGNMENT, target) | |
| { | |
| builder.into_diagnostic(format_args!( | |
| "Cannot delete attribute `{attribute}` on type `{}` \ | |
| whose `__delattr__` method returns `Never`/`NoReturn`", | |
| object_ty.display(db), | |
| )); | |
| } | |
| return false; | |
| } |
| object_type.display(db) | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
This diagnostic is great; probably wants a snapshot test?
| del supports_delete.x | ||
|
|
||
| rejects_descriptor_delete = RejectsDescriptorDelete() | ||
| del rejects_descriptor_delete.x |
There was a problem hiding this comment.
It looks like this one was intended to be an error but currently isn't for some reason?
There was a problem hiding this comment.
Gah, sorry -- this needs a TODO, it's fixed in #24510. (Originally it was part of this PR, then I broke it out.)
| }; | ||
| let db = context.db(); | ||
| let mut diagnostic = builder.into_diagnostic(format_args!( | ||
| "Cannot delete attribute `{attribute}` on type `{}` with custom `__delattr__` method.", |
There was a problem hiding this comment.
| "Cannot delete attribute `{attribute}` on type `{}` with custom `__delattr__` method.", | |
| "Cannot delete attribute `{attribute}` on type `{}` with custom `__delattr__` method", |
charliermarsh
force-pushed
the
charlie/deleter-i
branch
from
c034367 to
0a6683d
Compare
* main: Bump typing conformance suite commit to latest upstream (#24553) [ty] Reject deleting`Final` attributes (#24508) [ty] Respect property deleters in attribute deletion checks (#24500) [ty] stop unioning Unknown into types of un-annotated attributes (#24531) [ty] Fix bad diagnostic range for incorrect implicit `__init_subclass__` calls (#24541) [ty] Add a `SupportedPythonVersion` enum (#24412) [ty] Ignore unsupported editor-selected Python versions (#24498) [ty] Add snapshots for `__init_subclass__` diagnostics (#24539) [ty] Minor fix in tests (#24538) [ty] Allow `Final` variable assignments in `__post_init__` (#24529) [ty] Expand test suite for assignment errors (#24537) [ty] Use `map`, not `__map`, as the name of the mapping parameter in `TypedDict` `__init__` methods (#24535) [ty] Rework logic for synthesizing `TypedDict` methods (#24534) [flake8-bandit] Fix S103 false positives and negatives in mask analysis (#24424) [ty] mdtest.py: update dependencies (#24533) Rename patterns and arguments source order iterator method (#24532) [ty] Omit invalid keyword arguments from `TypedDict` signature (#24522) [ty] support super() in metaclass methods (#24483) [ty] Synthesize `__init__` for `TypedDict` (#24476)
| c = C() | ||
| del c.attr # error: [invalid-assignment] | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Wait, why did this test end up here? 😄 It has nothing to do with the topic of this test suite.
4 participants
Summary
This PR adds support for deleter properties, so we now error on the following:
While this looks like a lot of code, it ends up being a fairly mechanical change -- almost every line corresponds to a similar line for setters and/or getters, with the exception of
validate_attribute_deletion.