[ty] fix unary operators on NewTypes of float and complex#22605
[ty] fix unary operators on NewTypes of float and complex#22605oconnor663 merged 3 commits intomainfrom
NewTypes of float and complex#22605Conversation
oconnor663
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Typing conformance resultsNo changes detected ✅ |
|
|
Actually this still needs some more work. from typing import reveal_type, NewType
Foo = NewType('Foo', float)
def f(x: float | Foo):
print(-x) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8537aec04d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (_, Type::NewTypeInstance(newtype)) => { | ||
| self.infer_unary_expression_type(op, newtype.concrete_base_type(self.db()), unary) |
There was a problem hiding this comment.
This new arm always delegates unary ops on NewType instances to concrete_base_type, which changes how Self is bound for dunder methods: -x now binds Self to the base class, while a direct call x.__neg__() still binds Self to the NewType instance via normal member lookup. For a NewType wrapping a class that annotates __neg__/__pos__/__invert__ with typing.Self, this makes -x return the base type instead of the NewType, diverging from x.__neg__() and from the pre-change behavior. Example: class C: def __neg__(self) -> Self: ...; NT = NewType('NT', C); NT(C()).__neg__() remains NT but -NT(C()) would now infer C.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hmm, I'm not sure Codex is correct above (need to double check), but it is pointing me to a different issue. This works, because we explicitly added binary operation support:
Foo = NewType("Foo", float)
x = Foo(3.14)
x + xBut this doesn't currently work:
x.__add__(x) # error[invalid-argument-type]: Argument to bound method `__add__` is incorrectThere was a problem hiding this comment.
Actually, this isn't a NewType issue. It doesn't work with float either on main today (playground):
def _(x: float, y: float):
# This is fine.
x + y
# error[invalid-argument-type]: Argument to bound method `__add__` is incorrect
x.__add__(y)
# ^ Expected `int`, found `int | float`
oconnor663
force-pushed
the
newtype_unary
branch
from
8537aec to
5a6b34d
Compare
| // `float` and not `int | float`). | ||
| Type::NewTypeInstance(new_type_instance) => new_type_instance | ||
| .concrete_base_type(db) | ||
| .member_lookup_with_policy(db, name, policy), |
There was a problem hiding this comment.
The previous version of this fix was in infer_unary_expression_type, but that failed to handle more complex cases like:
Foo = NewType("Foo", float)
def _(x: Foo | float):
-xThis version of the fix here in member_lookup_with_policy handles all cases I'm aware of.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a6b34d80a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Type::NewTypeInstance(new_type_instance) => new_type_instance | ||
| .concrete_base_type(db) | ||
| .member_lookup_with_policy(db, name, policy), |
There was a problem hiding this comment.
Delegating Type::NewTypeInstance member lookup to concrete_base_type means the descriptor protocol now binds the instance as the base type instead of the NewType, so methods/properties annotated with typing.Self (or TypeVar bound to Self) will now resolve to the base class. This is a regression for nominal NewType behavior: e.g., with class C: def copy(self) -> Self: ...; NT = NewType("NT", C), NT(C()).copy() will now infer C instead of NT. This only shows up when the base type uses Self-style return typing, but it changes type inference for regular attribute access beyond the float/complex unary operator fix.
Useful? React with 👍 / 👎.
|
| Lint rule | Added | Removed | Changed |
|---|---|---|---|
invalid-return-type |
1 | 4 | 5 |
invalid-argument-type |
1 | 3 | 4 |
invalid-parameter-default |
0 | 0 | 7 |
invalid-assignment |
0 | 0 | 5 |
possibly-missing-attribute |
0 | 3 | 1 |
unused-ignore-comment |
3 | 0 | 0 |
invalid-await |
0 | 2 | 0 |
unresolved-attribute |
0 | 0 | 2 |
| Total | 5 | 12 | 24 |
oconnor663
force-pushed
the
newtype_unary
branch
from
5a6b34d to
82cdbfc
Compare
|
This should be ready for proper review now :) |
| // This case needs to come before the `no_instance_fallback` catch-all, so that we | ||
| // treat `NewType`s of `float` and `complex` as their special-case union base types. | ||
| // Otherwise we'll look up e.g. `__add__` with a `self` type bound to the `NewType`, | ||
| // which will fail to match e.g. `float.__add__` (because its `self` parameter is just | ||
| // `float` and not `int | float`). However, all other `NewType` cases need to fall | ||
| // through, because we generally do want e.g. methods that return `Self` to return the | ||
| // `NewType`. | ||
| Type::NewTypeInstance(new_type_instance) if new_type_instance.base_is_union(db) => { | ||
| new_type_instance | ||
| .concrete_base_type(db) | ||
| .member_lookup_with_policy(db, name, policy) | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
(currently the only method that exists on both classes and returns Self is __new__ -- float.fromhex() returns Self, but it only exists on the float class, not the int class)
There was a problem hiding this comment.
Uff, this does seem like a very silly and unlikely edge case I'm pointing out, though. And I'm not even sure what the correct answer there even is. I think you can just ignore me here.
There was a problem hiding this comment.
Yeah I went looking for Self return types in typeshed, and it looked like they were all classmethods today, so I figured maybe I didn't need a clear answer to this 😅 (Similarly, probably obvious to you, but for completeness / my notes: if I subclass int or float, and then NewType that, the base will be NewTypeBase::ClassLiteral, and this special case won't apply.)
|
Yeah it also makes me nervous. Like how many other things (now or in the future) will "actually be unions on the inside" in some sense, and will need to go through a similarly painful process of accumulating all these special cases? |
2 participants
Closes astral-sh/ty#2499.