-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Fix subtraction overflow bug #21570
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
Conversation
|
Before: 2025-11-21T13.58.50-05.00.mp4After: 2025-11-21T14.01.48-05.00.mp4 |
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
PR #21549 introduced a subtle overflow bug that seemed impossible, but can empirically happen. This PR fixes it by saturating to zero. I did try to write a regression test for this, but couldn't manage it. Instead, I'll attach before-and-after screen recordings.
3016e44 to
f57c8ae
Compare
|
|
Gankra
left a comment
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.
Hmm I wonder if this is getting confused about utf8 vs utf16... 😨
|
Oh that was sloppy by me, not sure how I didn't see that in the playground. 🤔 This seems to (at the very least) happen when the cursor is in the middle of a token at the start of the file. This seems to reproduce it: let builder = completion_test_builder(
"\
f<CURSOR>oo = 1
",
);
builder.build(); |
|
Oh I see, are we subtracting the size of the whole token from the cursor, rather than token-up-to-the-cursor? |
|
We do get the range for the entire token ruff/crates/ty_ide/src/completion.rs Line 1266 in 438ef33
The subtraction is probably a bit off in general then and there might be some other scenarios where we don't suppress suggestions (even though we should) if the cursor is not at the end of the current token. Had a quick check but couldn't figure out a test case that trigger such a scenario even though I feel like they must exist. I'll have a think. |
PR #21549 introduced a subtle overflow bug that seemed impossible, but
can empirically happen. This PR fixes it by saturating to zero.
I did try to write a regression test for this, but couldn't manage it.
Instead, I'll attach before-and-after screen recordings.