Skip to content

Conversation

@ntBre
Copy link
Contributor

@ntBre ntBre commented Aug 8, 2025

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.

// Always insert the diagnostic at top-of-file.
let mut diagnostic = context.report_diagnostic(
MissingRequiredImport(required_import.to_string()),
TextRange::default(),
);

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

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
@ntBre ntBre added the internal An internal refactor or improvement label Aug 8, 2025
@ntBre ntBre marked this pull request as ready for review August 8, 2025 21:43
@ntBre ntBre requested review from BurntSushi and removed request for carljm, dcreager and sharkdp August 8, 2025 21:43
@MichaReiser
Copy link
Member

Is this marked as internal because the change hasn't been released yet?

@ntBre
Copy link
Contributor Author

ntBre commented Aug 10, 2025

Is this marked as internal because the change hasn't been released yet?

Yep, that was my thinking. Otherwise it's a bug.

@ntBre ntBre merged commit c433865 into main Aug 11, 2025
36 checks passed
@ntBre ntBre deleted the brent/defuse-bom branch August 11, 2025 12:52
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

Labels

internal An internal refactor or improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants