-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Don't special-case class instances in unary expression inference #15045
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
534255d
Add test case for classes
dcreager e22902b
Don't special-case class instances
dcreager 7e0cd2d
Add a bunch of tests
dcreager 6c0ee5f
Add metaclass tests
dcreager 7f30089
Use call_dunder
dcreager a05646e
Add other special cases
dcreager f0ec0a3
List unary dunder variants explicitly
dcreager File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
165 changes: 165 additions & 0 deletions
165
crates/red_knot_python_semantic/resources/mdtest/unary/custom.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
# Custom unary operations | ||
|
||
## Class instances | ||
|
||
```py | ||
class Yes: | ||
def __pos__(self) -> bool: | ||
return False | ||
|
||
def __neg__(self) -> str: | ||
return "negative" | ||
|
||
def __invert__(self) -> int: | ||
return 17 | ||
|
||
class Sub(Yes): ... | ||
class No: ... | ||
|
||
reveal_type(+Yes()) # revealed: bool | ||
reveal_type(-Yes()) # revealed: str | ||
reveal_type(~Yes()) # revealed: int | ||
|
||
reveal_type(+Sub()) # revealed: bool | ||
reveal_type(-Sub()) # revealed: str | ||
reveal_type(~Sub()) # revealed: int | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `No`" | ||
reveal_type(+No()) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `No`" | ||
reveal_type(-No()) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `No`" | ||
reveal_type(~No()) # revealed: Unknown | ||
``` | ||
|
||
## Classes | ||
|
||
```py | ||
class Yes: | ||
def __pos__(self) -> bool: | ||
return False | ||
|
||
def __neg__(self) -> str: | ||
return "negative" | ||
|
||
def __invert__(self) -> int: | ||
return 17 | ||
|
||
class Sub(Yes): ... | ||
class No: ... | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `Literal[Yes]`" | ||
reveal_type(+Yes) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `Literal[Yes]`" | ||
reveal_type(-Yes) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `Literal[Yes]`" | ||
reveal_type(~Yes) # revealed: Unknown | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `Literal[Sub]`" | ||
reveal_type(+Sub) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `Literal[Sub]`" | ||
reveal_type(-Sub) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `Literal[Sub]`" | ||
reveal_type(~Sub) # revealed: Unknown | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `Literal[No]`" | ||
reveal_type(+No) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `Literal[No]`" | ||
reveal_type(-No) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `Literal[No]`" | ||
reveal_type(~No) # revealed: Unknown | ||
``` | ||
|
||
## Function literals | ||
|
||
```py | ||
def f(): | ||
pass | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `Literal[f]`" | ||
reveal_type(+f) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `Literal[f]`" | ||
reveal_type(-f) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `Literal[f]`" | ||
reveal_type(~f) # revealed: Unknown | ||
``` | ||
|
||
## Subclass | ||
|
||
```py | ||
class Yes: | ||
def __pos__(self) -> bool: | ||
return False | ||
|
||
def __neg__(self) -> str: | ||
return "negative" | ||
|
||
def __invert__(self) -> int: | ||
return 17 | ||
|
||
class Sub(Yes): ... | ||
class No: ... | ||
|
||
def yes() -> type[Yes]: | ||
return Yes | ||
|
||
def sub() -> type[Sub]: | ||
return Sub | ||
|
||
def no() -> type[No]: | ||
return No | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `type[Yes]`" | ||
reveal_type(+yes()) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `type[Yes]`" | ||
reveal_type(-yes()) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `type[Yes]`" | ||
reveal_type(~yes()) # revealed: Unknown | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `type[Sub]`" | ||
reveal_type(+sub()) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `type[Sub]`" | ||
reveal_type(-sub()) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `type[Sub]`" | ||
reveal_type(~sub()) # revealed: Unknown | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `type[No]`" | ||
reveal_type(+no()) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `type[No]`" | ||
reveal_type(-no()) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `type[No]`" | ||
reveal_type(~no()) # revealed: Unknown | ||
``` | ||
|
||
## Metaclass | ||
|
||
```py | ||
class Meta(type): | ||
def __pos__(self) -> bool: | ||
return False | ||
|
||
def __neg__(self) -> str: | ||
return "negative" | ||
|
||
def __invert__(self) -> int: | ||
return 17 | ||
|
||
class Yes(metaclass=Meta): ... | ||
class Sub(Yes): ... | ||
class No: ... | ||
|
||
reveal_type(+Yes) # revealed: bool | ||
reveal_type(-Yes) # revealed: str | ||
reveal_type(~Yes) # revealed: int | ||
|
||
reveal_type(+Sub) # revealed: bool | ||
reveal_type(-Sub) # revealed: str | ||
reveal_type(~Sub) # revealed: int | ||
|
||
# error: [unsupported-operator] "Unary operator `+` is unsupported for type `Literal[No]`" | ||
reveal_type(+No) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `-` is unsupported for type `Literal[No]`" | ||
reveal_type(-No) # revealed: Unknown | ||
# error: [unsupported-operator] "Unary operator `~` is unsupported for type `Literal[No]`" | ||
reveal_type(~No) # revealed: Unknown | ||
``` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know there's a lot of variants in the
Type
enum, but I'd prefer it if we could explicitly list all the remaining ones in thismatch
arm rather than using_
for the second tuple element. The advantage is that if we add anotherType
variant in the future that we want to special-case in unary expressions (similar to the special-casing we already do forType::IntLiteral
andType::BooleanLiteral
above), there's no chance of us forgetting to add that special-casing. We'll be forced to explicitly consider whether the new variant should be handled in its ownmatch
arm or be handled as part of the catch-all fallback case here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so sure explicitly listing all variants is always the best answer. In this case, the handling in this arm really is a generic implementation of the semantics in terms of other type semantics. It should always be correct for any type (even the ones we special case above here), assuming a correct implementation of calling dunder methods (
to_meta_type
,member
, etc -- all of which do use exhaustive matches and we'll be forced to consider for any new Type variant.) Thus, a special case for a new Type variant here would not be required for correctness, it would just potentially offer more precision. I kind of think it's fine, possibly even good, for that "more precision" to be driven only by actual user needs, and not something we are forced to consider.But I'm really fine with it either way.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right that there isn't a correctness issue here. But I would expect more precision here to be both trivial to accomplish for any new variants and simpler for us to compute. The
IntLiteral()
branches above really involve much less work for us than doing the full lookup for an instance type's dunder method in typeshed or user code (which could involve materialising an MRO!). I don't really see a reason not for us to infer the more precise type whenever possible if it's trivial to do so, and I'd at least like us to be forced to consider whether it would be better to do soThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like there's a -0 vote from Carl and a +1 from Alex, so I went ahead and expanded the clauses out. We can always put it back to
_
in the future if we find this more unwieldy than the benefit.