-
-
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
Emit warning for invalid use of quoted types in union syntax #18183
base: master
Are you sure you want to change the base?
Emit warning for invalid use of quoted types in union syntax #18183
Conversation
Conformance results diff for this PR (improves conformance in :...skipping...
diff --git a/conformance/results/mypy/annotations_forward_refs.toml b/conformance/results/mypy/annotations_forward_refs.toml
index bee7ebb..6763a18 100644
--- a/conformance/results/mypy/annotations_forward_refs.toml
+++ b/conformance/results/mypy/annotations_forward_refs.toml
@@ -5,6 +5,8 @@ Does not report error for use of quoted type with "|" operator (runtime error).
Incorrectly generates error for quoted type defined in class scope.
"""
output = """
+annotations_forward_refs.py:24: error: X | Y syntax for unions cannot use quoted operands; use quotes around the entire expression instead [misc]
+annotations_forward_refs.py:25: error: X | Y syntax for unions cannot use quoted operands; use quotes around the entire expression instead [misc]
annotations_forward_refs.py:41: error: Invalid type comment or annotation [valid-type]
annotations_forward_refs.py:42: error: Invalid type comment or annotation [valid-type]
annotations_forward_refs.py:43: error: Invalid type comment or annotation [valid-type]
@@ -33,8 +35,6 @@ conformance_automated = "Fail"
errors_diff = """
Line 22: Expected 1 errors
Line 23: Expected 1 errors
-Line 24: Expected 1 errors
-Line 25: Expected 1 errors
Line 66: Expected 1 errors
Line 87: Unexpected errors ['annotations_forward_refs.py:87: error: Function "annotations_forward_refs.ClassD.int" is not valid as a type [valid-type]']
Line 96: Unexpected errors ['annotations_forward_refs.py:96: error: Expression is of type int?, not "int" [assert-type]']
diff --git a/conformance/results/mypy/version.toml b/conformance/results/mypy/version.toml
index 51d97b4..b1035e1 100644
--- a/conformance/results/mypy/version.toml
+++ b/conformance/results/mypy/version.toml
@@ -1,2 +1,2 @@
-version = "mypy 1.14.0+dev.499adaed8adbded1a180e30d071438fef81779ec"
-test_duration = 2.4
+version = "mypy 1.14.0+dev.ce34db689d8a6cab6f540a2c38ecc32952534b0a"
+test_duration = 4.6 |
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
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 for the PR! Did a quick pass, not a full review.
|
||
T = TypeVar("T") | ||
|
||
ok1: TypeAlias = T | "int" |
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.
Would it be useful to have more tests for things like "int | str" | T
or "int | str"
, here and elsewhere?
t.original_str_expr is None | ||
and not self.api.is_stub_file | ||
and (not self.api.is_future_flag_set("annotations") or self.defining_alias) | ||
): |
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.
Style nit: This new if statement now accounts for most the code in this function. What about moving it to a helper function to make the basic logic clearer?
x3: int | str | "bytes" # E: X | Y syntax for unions cannot use quoted operands; use quotes around the entire expression instead | ||
assert_type(x1, Union[int, str]) | ||
assert_type(x2, Union[int, str]) | ||
assert_type(x3, Union[int, str, bytes]) |
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.
Test also valid cases, like Callable[[], None] | "int"
, which work at runtime? Or are these tested in existing tests?
Using quoted types in PEP 604 union syntax can be problematic, since at runtime, using
|
between a type and string produces an error:The exception is when the type is a
TypeVar
or a_UnionGenericAlias
, which can both be safely|
'd with strings:This PR makes mypy emit a warning for the former cases (where using
|
with a string will produce a runtime error) while permitting the latter cases (where using|
with a string is safe).A few notes about the implementation:
from __future__ import annotations
. Like in stubs, quoted types in these cases are redundant but mostly harmless (aside from issues with runtime inspection). It might be a good idea to emit errors for these cases too (pyright does), but I wasn't sure if the benefit is enough to justify the churn. Without this exception, this PR has ~20 mypy_primer hits across 4 projects.