-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add interactions between Literal and Final #6081
Add interactions between Literal and Final #6081
Conversation
Before doing the full review, I wanted to point out an important thing. A pattern like this (using a constant as a default value) is very common: BORDER_SIZE: Final = 10
class Window:
def __init__(self) -> None:
self.border = BORDER_SIZE
w = Window()
w.border = 20
START: Final = 42
def count() -> int:
counter = START
while True:
counter += 1
... Currently this works, but with this PR it will emit (quite mysterious) errors. I think the logic of interactions between NUM: Final = 42
x = NUM # type of x is just int, because there is no literal context
def test(x: Literal[42]) -> None:
...
test(NUM) # this works however This can be summarized in short as everything should work as if final names are replaced with their values (including chaining). Note that this cannot be achieved by simply "erasing" the literal type if there is no literal context, because explicit literal types should "propagate": small: Literal[1, 2, 3] = 1
smalls = [small] # type here is List[Literal[1, 2, 3]] I think this should be implemented very roughly like this: final assignments still infer instance types, then This may be more tricky with "intelligent indexing", unless we set a literal context there (more like in your option 2, that we recently discussed). |
Also note that all your three questions may become solved/irrelevant if you implement this PR the way I propose. |
Hmm, I'll try experimenting with that idea. I agree that preserving the use-case you pointed out is important: I wasn't very happy about having to make changes to mypy itself after breaking that use-case. I do have some new questions about this though:
|
OK, here are my thoughts:
As a general comment, I am not worried about interference between literals and explicit types, since their use cases are almost non-overlapping. A typical use case for explicit type is a "global container": |
This pull request adds logic to handle interactions between Literal and Final: for example, inferring that `foo` has type `Literal[3]` when doing `foo: Final = 3`. A few additional notes: 1. This unfortunately had the side-effect of causing some of the existing tests for `Final` become noiser. I decided to mostly bias towards preserving the original error messages by modifying many of the existing variable assignments to explicitly use things like `Final[int]`. I left in the new error messages in a few cases -- mostly in cases where I was able to add them in a relatively tidy way. Let me know if this needs to be handled differently. 2. Since mypy uses 'Final', this means that once this PR lands, mypy itself will actually be using Literal types (albeit somewhat indirectly) for the first time. I'm not fully sure what the ramifications of this are. For example, do we need to detour and add support for literal types to mypyc? 3. Are there any major users of `Final` other then mypy? It didn't seem like we were really using it in our internal codebase at least, but I could be wrong about that. If there *are* some people who have already started depending on 'Final', maybe we should defer landing this PR until Literal types are more stable to avoid disrupting them. I had to make a few changes to mypy's own source code to get it to type check under these new semantics, for example.
This commit modifies this PR to make selecting the type of final variables context-sensitive. Now, when we do: x: Final = 1 ...the variable `x` is normally inferred to be of type `int`. However, if that variable is used in a context which expects `Literal`, we infer the literal type. This commit also removes some of the hacks to mypy and the tests that the first iteration added.
304f4c7
to
7335991
Compare
# Conflicts: # test-data/unit/check-literal.test
Ok, this should be ready for a second look whenever. @ilevkivskyi -- I basically went with the approach you suggested (and removed the hacks I added to mypy + the tests.) |
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.
Thanks! Overall looks good, I have few minor comments.
if t.final_value is not None: | ||
raw_final_value = t.final_value.accept(self) | ||
assert isinstance(raw_final_value, LiteralType) | ||
final_value = raw_final_value |
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.
Why can't you just write:
final_value = t.final_value.accept(self)
assert isinstance(final_value, LiteralType)
thus avoiding the extra variable.
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.
It's because t.final_value.accept(self)
has a return type of Type
, which means we wouldn't be able to assign it to final_value
without raising an error or without casting.
I can replace this entire if statement with a cast if you want, similar to what we're doing in visit_tuple_type
or visit_typeddict_type
below, but I wanted to add a runtime check mostly for peace of mind.
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.
Will my version work if you remove the type comment on final_value
?
(This is not important however.)
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.
Alas, it doesn't: I get a 'Argument "final_value" to "Instance" has incompatible type "Optional[Type]"; expected "Optional[LiteralType]"` error a little later on when we use the variable.
__main__.C.z | ||
__main__.C.zi | ||
__main__.w | ||
__main__.x |
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.
Hm, I am not sure I totally understand why x: Final = 1
and x: Final[int] = 1
appear in the diff. Shouldn't we infer exactly the same type Instance('builtins.int', final_value=Literal[1])
for both of them?
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.
No -- we infer Instance('builtins.int', final_value=None)
for the latter. So, since those two declarations have subtly different behavior, I thought it would be important to test both.
(I also went ahead and expanded this test a little. I forgot to update it to test the new semantics earlier.)
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.
What is the reason for inferring different type? Is this because of situations like x: Final[float] = 42
? It looks like this is similar to #2008, so I am not going to argue 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.
Basically, yeah.
In short, I thought it was more important to make sure that statements like x: Final[float] = 42
and x: float = 42
behaves in the same way as much as possible. This helps ensure that mypy's behavior stays consistent no matter what, even if we lose track of the original literal value (for example, if the user is messing around with generics).
This does mean that x: Final[int] = 9
and x: Final = 9
appear to behave "inconsistently", but I thought that would be less surprising then if x: Final[int] = 9
and x: int = 9
were to behave "inconsistently".
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.
On second thought though, I wonder if it might be worth making x: Final[int] = 4
and x: Final = 4
behave in the same way only if the declared and inferred types are equivalent. That way, we don't run into any issues with expressions like x: Final[float] = 4
since we'd be disabling this "substitution" behavior in that case.
That might be a little too magical though, not sure. LMK if this is an idea you think might be worth exploring.
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.
LMK if this is an idea you think might be worth exploring.
Maybe just open an issue about this. We can decide later when we will have more experience.
from typing_extensions import Final, Literal | ||
|
||
a: Final = 1 | ||
b: Final = (1, 2) |
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.
In principle we can support this at some point, but not now. IIRC there is already an issue about this (for common fields in named tuples).
# Conflicts: # test-data/unit/check-literal.test
…2a/mypy into add-literal-final-interactions
@ilevkivskyi -- ok, this should be ready for a second look whenever! I made the changes you asked and also added in a few more tests. |
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 think this LG now, just a couple of optional questions.
if t.final_value is not None: | ||
raw_final_value = t.final_value.accept(self) | ||
assert isinstance(raw_final_value, LiteralType) | ||
final_value = raw_final_value |
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.
Will my version work if you remove the type comment on final_value
?
(This is not important however.)
__main__.C.z | ||
__main__.C.zi | ||
__main__.w | ||
__main__.x |
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.
What is the reason for inferring different type? Is this because of situations like x: Final[float] = 42
? It looks like this is similar to #2008, so I am not going to argue here.
This pull request adds logic to handle interactions between Literal and Final: for example, inferring that
foo
has typeLiteral[3]
when doingfoo: Final = 3
.A few additional notes:
This unfortunately had the side-effect of causing some of the existing tests for
Final
become noiser. I decided to mostly bias towards preserving the original error messages by modifying many of the existing variable assignments to explicitly use things likeFinal[int]
.I left in the new error messages in a few cases -- mostly in cases where I was able to add them in a relatively tidy way.
Let me know if this needs to be handled differently.
Since mypy uses 'Final', this means that once this PR lands, mypy itself will actually be using Literal types (albeit somewhat indirectly) for the first time.
I'm not fully sure what the ramifications of this are. For example, do we need to detour and add support for literal types to mypyc?
Are there any major users of
Final
other then mypy? It didn't seem like we were really using it in our internal codebase at least, but I could be wrong about that.If there are some people who have already started depending on 'Final', maybe we should defer landing this PR until Literal types are more stable to avoid unnecessarily disrupting them. I had to make a few changes to mypy's own source code to get it to type check under these new semantics, for example.