[ty] Check shifts of Literal ints#18329
Conversation
brandtbucher
requested review from
AlexWaygood,
MichaReiser,
carljm,
dcreager and
sharkdp
as code owners
brandtbucher
changed the title
Literal ints
|
That was neat: |
|
|
Yay Rust. |
|
Just realized that this doesn't correctly handle the cases where we shift a |
There was a problem hiding this comment.
Could you also add some tests with boolean literals? E.g. True << 42, 56 >> False and True << True. Your branch gives the right answer for them, but it's good to explicitly test them, I think.
(They're covered by this fallback branch here:
ruff/crates/ty_python_semantic/src/types/infer.rs
Lines 6244 to 6257 in 8d5655a
| /// 42 >> -1 | ||
| /// 42 << -1 | ||
| /// ``` | ||
| pub(crate) static NEGATIVE_SHIFT = { |
There was a problem hiding this comment.
I wonder if it's worth this being a separate lint to DIVISION_BY_ZERO, or if it might be better to group them both under a LITERAL_MATH_ERROR lint (which, like DIVISION_BY_ZERO, should probably be disabled by default for now):
- They're conceptually pretty similar in the kind of error they detect
- I can't really think of a reason why a user would want to suppress one (either on a per-line, per-file or per-project basis) but not the other
- I think they both suffer from the same drawbacks that @carljm noted in the PR description for [ty] disable division-by-zero by default #18220 (comment)
Renaming the existing DIVISION_BY_ZERO lint might be a fair amount of busywork, though, so it's okay if you're not up to that! I think one of us could reasonably take on merging the two lints as a followup task. If you do keep this as a separate lint, though, I think it should probably be disabled by default, similar to DIVISION_BY_ZERO. (See #18220 for what the necessary changes there would look like!)
|
Thanks for your contribution. I'll close this as it seems to have gone stale. Anyone interested in picking this up. Feel free to open a new PR which addresses the feedback. |
…#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]>
3 participants
Fixes astral-sh/ty#517.
I took the liberty of adding a new
negative-shiftdiagnostic for cases when we detect shifting by a negative RHS, which is aValueErrorat runtime. This reuses some machinery that's already used to detect division byLiteral[0](division-by-zero), which felt sort of natural. I'm okay with refactoring or removing it, though... I was mostly just curious how new rules are added and used under-the-hood.