[ty] Check shifts of literals ints #19281
Draft
Conversation
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
AlexWaygood
requested review from
carljm,
sharkdp,
dcreager and
MichaReiser
as code owners
AlexWaygood
changed the title
|
MichaReiser
reviewed
Jul 11, 2025
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. |
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| @@ -5,4 +5,4 @@ | |||
| [rules] | |||
| possibly-unresolved-reference = "warn" | |||
| unused-ignore-comment = "warn" | |||
| division-by-zero = "warn" | |||
| literal-math-error = "warn" | |||
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.
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 |
|
It might be worth hearing more opinions before you spend much time splitting them |
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. |
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 😆) |
|
Also, my original PR had an issue where shifting a 1 into the sign bit, like |
|
I probably weakly favor two rules here, though I'd also be quite happy if we dropped both of the rules entirely. |
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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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]