-
Notifications
You must be signed in to change notification settings - Fork 259
Proposed clarification of spec for int/float/complex promotion #1748
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
Draft
JelleZijlstra
wants to merge
5
commits into
python:main
Choose a base branch
from
JelleZijlstra:intfloat
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
d2a3bdf
Proposed clarification of spec for int/float/complex promotion
JelleZijlstra cfefa40
typo
JelleZijlstra cf3486f
remove incorrect case
JelleZijlstra 6155dbd
Extend test
JelleZijlstra a442bfe
Guard against unreachability
JelleZijlstra 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
remove incorrect case
- Loading branch information
commit cf3486fef90e730e2dc3a364c378677da794e086
There are no files selected for viewing
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 |
---|---|---|
@@ -1,2 +1,2 @@ | ||
version = "pyre 0.9.21" | ||
test_duration = 1.7 | ||
test_duration = 2.3 |
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 |
---|---|---|
@@ -1,2 +1,2 @@ | ||
version = "pytype 2024.04.11" | ||
test_duration = 27.6 | ||
test_duration = 27.3 |
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
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
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.
I think worth having an unguarded
f.hex()
as wellUh 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.
This one is interesting, because it's not totally clear to me what behavior we should specify for an unguarded
.hex()
call.The proposed spec wording, as is, would mean that this should be an error, right? Because "member
int
offloat | int
has no methodhex
."builtins.int
in typeshed doesn't have thehex
method. So if you want to callhex
you have to guard it withisinstance(float)
.But this doesn't seem to be an error in either mypy or pyright. The latter is particularly interesting, since my understanding was that pyright already used the
float | int
interpretation offloat
annotations.@erictraut What's the explanation of pyright's behavior here? Is this special-cased in some way?
I think if the conformance suite shows
f.hex()
here to not be an error, then that needs to be justified with some additional wording in the spec.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 pyright's current implementation, I have a function called expandPromotionTypes that is responsible for taking the type
float
and expanding it toFloat | int
andcomplex
intoComplex | Float | int
. (I use a capitalized name here to indicate the "real" type.)There are three places where I initially added calls to this function:
isinstance
type narrowingmatch
statement equivalent ofisinstance
checks)I also 4) changed the inference logic for float literals to be inferred as
Float
rather thanfloat
.I ended up backing out 3 and 4 because these two cases produced too much noise, including some false positive errors. See this issue, which shows some of the pain this caused pyright users.
I think it's reasonable to add 3 back, but doing so requires an additional change to avoid some of the noise. Namely, a call to
float()
orcomplex()
constructors needs to evaluate toFloat
andComplex
, respectively.I think that adding 4 back would be problematic, especially in situations where float literals are used in list, set and dict expressions. These are problematic because the types are invariant. Consider the following code:
I think the expression
[3.1]
should continue to be inferred aslist[float]
, which probably means that3.1
should continue to be inferred asfloat
and notFloat
. I think that's OK, but it does lead to an apparent inconsistency because[float(3.1)]
is evaluated aslist[Float]
. The typing spec doesn't dictate type inference rules (currently), so these are not in scope for the spec, but this issue is something that type checker maintainers / authors will need to consider.Here's a PR (and "mypy_primer" run) that adds back 3 from my list above. This makes pyright conformant with the proposed language in this typing spec update (i.e. it now generates an error for
f.numerator
in the conformance test case). The "mypy_primer" run shows one change in thedd_trace
library. It's in code that looks like this:This change is not without consequences, but I think the impact is relatively minimal and new type errors are straightforward to fix.
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 see, so pyright's implementation doesn't actually expand
float
tofloat | int
when it's encountered in an annotation, but rather at certain points where the type is used.I guess with #3 added back, pyright would also error on
f: float; f.hex()
. Thehex/fromhex
case is similar to theis_integer
case encountered in microsoft/pyright#6032, but I expectis_integer
is more commonly used. Andis_integer
was added to theint
type in Python 3.12 to avoid this problem. It doesn't seem like your mypy primer run encountered any issues with calls to.hex
or.fromhex
, which doesn't surprise me.One thing that does surprise me on that mypy primer run is that the two new errors don't appear to involve attribute access (your (3)), but rather inferred types for containers (your (4)). E.g. one of the errors is on this line: https://github.com/DataDog/dd-trace-py/blob/main/ddtrace/llmobs/_integrations/bedrock.py#L43
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.
That's correct. It would be confusing for pyright & pylance users to see
float | int
in hover text, inlined type annotations, completion suggestions,reveal_type
text, etc. if they usefloat
in a type annotation. And likewise, if they specifyfloat | int
in a type annotation, it would be confusing to reduce that to justfloat
. I try to retain the same form used in the annotation where possible.