[ty] Preserve pure negation types in descriptor protocol#22907
[ty] Preserve pure negation types in descriptor protocol#22907charliermarsh merged 3 commits intomainfrom
Conversation
Typing conformance resultsNo changes detected ✅ |
|
charliermarsh
force-pushed
the
charlie/negation
branch
from
cf3f1ea to
0ba19d9
Compare
charliermarsh
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
| // For pure negation types (no positive elements), there's nothing to map. | ||
| if self.positive(db).is_empty() { | ||
| return Place::Undefined; | ||
| } |
There was a problem hiding this comment.
I don't think this is right. We should implicitly include object in such an intersection, and we should still call the transform_fn on object.
| let place = if mapped.is_undefined() { | ||
| attribute.place | ||
| } else { | ||
| mapped | ||
| }; |
There was a problem hiding this comment.
If we follow the suggestion below to still call the transform_fn on object when we have a negative-only intersection, then we would never get back Place::Undefined (because our transform_fn above always returns Place::Defined), and then we wouldn't need this new case either.
|
@carljm -- I think I understood the concerns but not necessarily what you viewed as the appropriate fix? I addressed the comments and added a block where we re-add the negations. Or are you saying that changing the behavior of |
|
@charliermarsh Well, that makes sense, because looking at it again, I clearly didn't think it through all the way 😆 I correctly observed that the change originally made here wasn't correct for
On a more careful second look: yes, exactly this. If we wanted So I think the right fix here is to leave |
|
Haha okay, this makes sense. Ironically, I started with what you described, then decided it was too special-case and I should pursue a deeper fix, and here we are :) |
Retain transform_fn
This reverts commit 0f21c46.
charliermarsh
force-pushed
the
charlie/negation
branch
from
22242c5 to
c73b2f9
Compare
There was a problem hiding this comment.
orthogonal nit, but I don't think there's any reason we need to be renaming this here
| Place::Defined(DefinedPlace { | ||
| ty: Type::Intersection(intersection), | ||
| origin, | ||
| definedness: boundness, | ||
| widening, | ||
| }) | ||
| .with_qualifiers(qualifiers), | ||
| .with_qualifiers(qualifiers) |
There was a problem hiding this comment.
I think this is equivalent?
| Place::Defined(DefinedPlace { | |
| ty: Type::Intersection(intersection), | |
| origin, | |
| definedness: boundness, | |
| widening, | |
| }) | |
| .with_qualifiers(qualifiers), | |
| .with_qualifiers(qualifiers) | |
| attribute |
charliermarsh
force-pushed
the
charlie/negation
branch
from
c73b2f9 to
d9d1890
Compare
* main: (76 commits) [ty] Improve the check for `NewType`s with generic bases (#22961) [ty] Ban legacy `TypeVar` bounds or constraints from containing type variables (#22949) Bump the typing conformance suite pin (#22960) [ty] Emit an error if a TypeVarTuple is used to subscript `Generic` or `Protocol` without being unpacked (#22952) [ty] Reduce false positives when subscripting classes generic over `TypeVarTuple`s (#22950) [ty] Detect invalid attempts to subclass `Protocol[]` and `Generic[]` simultaneously (#22948) Fix suppression indentation matching (#22903) Remove hidden `--output-format` warning (#22944) [ty] Validate signatures of dataclass `__post_init__` methods (#22730) [ty] extend special-cased `numbers` diagnostic to `invalid-argument-type` errors (#22938) [ty] Avoid false positive for `not-iterable` with no-positive intersection types (#22089) [ty] Preserve pure negation types in descriptor protocol (#22907) [ty] add special-case diagnostic for `numbers` module (#22931) [ty] Move the location of more `invalid-overload` diagnostics (#22933) [ty] Fix unary and comparison operators for TypeVars with union bounds (#22925) [ty] Rule Selection: ignore/warn/select all rules (unless subsequently overriden) (#22832) [ty] Fix TypedDict construction from existing TypedDict values (#22904) [ty] fix bug in string annotations and clean up diagnostics (#22913) [ty] Improve support for goto-type, goto-declaration, hover, and highlighting of string annotations (#22878) [ty] Rename old typing imports to new on `unresolved-reference`. (#22827) ...
2 participants
Summary
When accessing an attribute with a pure negation type (an intersection with only negative contributions, like
~AlwaysFalsy), the descriptor protocol would incorrectly return object instead of preserving the negation type, becausemap_with_boundnessiterates overpositive_elements_or_object, which falls back toobjectwhen there are no positive elements.