Closed
Conversation
AlexWaygood
requested review from
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
AlexWaygood
changed the title
Contributor
|
MichaReiser
reviewed
Jul 11, 2025
Member
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
You'll be surprised but I'm sort of favouring two rules here 😓
Comment on lines
+247
to
+249
| /// Detects runtime errors that would result from invalid math operations | ||
| /// between two objects with literal `int` types. Examples include division | ||
| /// by zero and negative bitshifts. |
Member
There was a problem hiding this comment.
I don't think it's necessary that both sides are literal ints. At least for division by zero.
def test(a: int):
return a / 0| possibly-unresolved-reference = "warn" | ||
| unused-ignore-comment = "warn" | ||
| division-by-zero = "warn" | ||
| literal-math-error = "warn" |
Member
There was a problem hiding this comment.
I don't have good recommendations other than splitting the rule but I do find the new name much less clear.
Member
Author
Haha, that does seem somewhat out of character for you 😆 I can switch it back to two rules. I think they'll need to both be disabled by default, though -- I believe this rule is going to suffer from the same issues regarding false positives as |
Member
|
It might be worth hearing more opinions before you spend much time splitting them |
Contributor
To be honest, I am unsure if this is a valuable feature to have. Is it worth the additional code and time that we need to review and maintain this? It looks like there's not a single hit in the ecosystem. We would also disable the rule by default because it's prone to false positives. And it is non-trivial to get this right (see initial PR). So I think I'm in favor of not implementing this at all. |
Member
Author
I agree that emitting a diagnostic if the r.h.s. is negative is not a very important feature at all; I'm okay with dropping it. I also don't think it's much code to maintain at all, though: we already have to check whether the r.h.s. is negative in order to get the literal math inference correct. github.com//pull/18329 has been basically mergable since it was originally filed; the only blocker was that I suggested combining the two rules (and we're now discussing whether doing that is even desirable 😆) |
Contributor
|
Also, my original PR had an issue where shifting a 1 into the sign bit, like |
Contributor
|
I probably weakly favor two rules here, though I'd also be quite happy if we dropped both of the rules entirely. |
Member
Author
Oh, don't think so. I'll check that. I'll also rip out the diagnostic for now, since we're not sure whether it should be its own diagnostic or combined with the |
Contributor
|
I'm closing this due to inactivity. Please feel free to comment in case it should be re-opened. |
AlexWaygood
pushed a commit
that referenced
this pull request
Feb 15, 2026
…rflow Rebase the inference-only parts of #19281 onto main: - Support literal int inference for `<<` and `>>` operators - Use `checked_neg()` for unary negation to handle i64::MIN overflow - Add tests for bitshift arithmetic with booleans and integers The division-by-zero diagnostic rename from #19281 is intentionally excluded per review consensus. https://claude.ai/code/session_01MPdchMiqZsZQ2Le3JV9aWf
AlexWaygood
pushed a commit
that referenced
this pull request
Feb 15, 2026
`i64::checked_shl` only validates that the shift amount is < 64 bits, but does not detect when a bit is shifted into the sign bit position. For example, `1_i64.checked_shl(63)` returns `Some(i64::MIN)`, which is negative. But Python integers have arbitrary precision, so `1 << 63 == 9223372036854775808` (positive). Fix this by adding a round-trip check: after computing `result`, verify that `result >> m == n`. If not, the shift overflowed and we fall back to inferring `int`. This was flagged by @brandtbucher in #19281 (comment) https://claude.ai/code/session_01MPdchMiqZsZQ2Le3JV9aWf
AlexWaygood
added a commit
that referenced
this pull request
Feb 16, 2026
…#23301) ## Summary Fixes astral-sh/ty#517. This PR revives #18329 / #19281, but without the controversial bit of those PRs (the new diagnostic). Rather than adding a new diagnostic if an integer is left-shifted or right-shifted by a negative number, we just ignore that and infer `int`. We can always add a new diagnostic later, if we find consensus on it. Along the way, Claude also discovered that `reveal_type(-(-9223372036854775807 - 1))` currently reveals `Literal[-9223372036854775808]` on `main`, which is... not correct. So this PR fixes that too. ### Test Coverage mdtests Co-authored-by: Brandt Bucher <[email protected]>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Summary
This is a rebased version of #18329 by @brandtbucher, with some additional changes from me. Fixes astral-sh/ty#517
With this PR, we now infer
Literalint values from left-shift and right-shift operations between literal-int types, literal-bool types, or combinations of the two. We also attempt to detect invalid bitshift operations (where the r.h.s. is<0), in the same way as we attempt to detectZeroDivisionErrors.I renamed the existing
division-by-zeroerror toliteral-math-error, and expanded it to cover negative bitshifts, rather than introducing a new rule. The reasons for this are:[ty] disable division-by-zero by default #18220 (comment)
Test Plan
mdtests
Co-authored-by: Brandt Bucher [email protected]