-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Avoid underflow in default ranges before a BOM #19839
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
Merged
Merged
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
Summary -- This fixes a regression caused by the BOM handling in #19806. Most diagnostics already account for the BOM in their ranges, but those that use `TextRange::default` to mean the beginning of the file do not, causing an underflow in `RenderableAnnotation::new` when subtracting the BOM-shifted `snippet_start` from the annotation range. I ran into this when trying to run benchmarks on CPython in preparation for caching work. The file `cpython/Lib/test/bad_coding2.py` was causing a crash because it had a default-range `I002` diagnostic, with a BOM. https://github.com/astral-sh/ruff/blob/7cc3f1ebe9386e77e7009bc411fc6480d3851015/crates/ruff_linter/src/rules/isort/rules/add_required_imports.rs#L122-L126 The fix here is just to saturate to zero instead of panicking. I considered adding a `TextRange::saturating_sub` method, but I wasn't sure it was worth it for this one use. I'm happy to do that if preferred, though. Saturating seemed easier than shifting the affected annotations over, but that could be another solution. Test Plan -- A new `ruff_db` test that reproduced the issue and manual testing against the CPython file mentioned above
MichaReiser
approved these changes
Aug 10, 2025
Member
|
Is this marked as internal because the change hasn't been released yet? |
Contributor
Author
Yep, that was my thinking. Otherwise it's a bug. |
BurntSushi
approved these changes
Aug 11, 2025
dcreager
added a commit
that referenced
this pull request
Aug 11, 2025
* main: (31 commits) Add AIR301 rule (#17707) Avoid underflow in default ranges before a BOM (#19839) Update actions/download-artifact digest to de96f46 (#19852) Update docker/login-action action to v3.5.0 (#19860) Update rui314/setup-mold digest to 7344740 (#19853) Update cargo-bins/cargo-binstall action to v1.14.4 (#19855) Update actions/cache action to v4.2.4 (#19854) Update Rust crate hashbrown to v0.15.5 (#19858) Update Rust crate camino to v1.1.11 (#19857) Update Rust crate proc-macro2 to v1.0.96 (#19859) Update dependency ruff to v0.12.8 (#19856) SIM905: Fix handling of U+001C..U+001F whitespace (#19849) RUF064: offer a safe fix for multi-digit zeros (#19847) Clean up unused rendering code in `ruff_linter` (#19832) [ty] Add Salsa caching to `TupleType::to_class_type` (#19840) [ty] Handle cycles when finding implicit attributes (#19833) [ty] fix goto-definition on imports (#19834) [ty] Implement stdlib stub mapping (#19529) [`flake8-comprehensions`] Fix false positive for `C420` with attribute, subscript, or slice assignment targets (#19513) [ty] Implement module-level `__getattr__` support (#19791) ...
dcreager
added a commit
that referenced
this pull request
Aug 11, 2025
* dcreager/bound-typevar: (41 commits) [ty] Use separate Rust types for bound and unbound type variables (#19796) fix ide tests better unbound typevar rendering Apply suggestions from code review [ty] Add `static-frame` as a walltime benchmark (#19844) add explanatory comment [ty] Update goto range for attribute access to only target the attribute (#19848) remove unneeded ord add TODO for broken hover test better PEP 695 binding context Add AIR301 rule (#17707) Avoid underflow in default ranges before a BOM (#19839) Update actions/download-artifact digest to de96f46 (#19852) Update docker/login-action action to v3.5.0 (#19860) Update rui314/setup-mold digest to 7344740 (#19853) Update cargo-bins/cargo-binstall action to v1.14.4 (#19855) Update actions/cache action to v4.2.4 (#19854) Update Rust crate hashbrown to v0.15.5 (#19858) Update Rust crate camino to v1.1.11 (#19857) Update Rust crate proc-macro2 to v1.0.96 (#19859) ...
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 fixes a regression caused by the BOM handling in #19806. Most diagnostics already account for the BOM in their ranges, but those that use
TextRange::defaultto mean the beginning of the file do not, causing an underflow inRenderableAnnotation::newwhen subtracting the BOM-shiftedsnippet_startfrom the annotation range.I ran into this when trying to run benchmarks on CPython in preparation for caching work. The file
cpython/Lib/test/bad_coding2.pywas causing a crash because it had a default-rangeI002diagnostic, with a BOM.ruff/crates/ruff_linter/src/rules/isort/rules/add_required_imports.rs
Lines 122 to 126 in 7cc3f1e
The fix here is just to saturate to zero instead of panicking. I considered adding a
TextRange::saturating_submethod, but I wasn't sure it was worth it for this one use. I'm happy to do that if preferred, though.Saturating seemed easier than shifting the affected annotations over, but that could be another solution.
Test Plan
A new
ruff_dbtest that reproduced the issue and manual testing against the CPython file mentioned above