Fix unicode handling in PLE2515 by konstin · Pull Request #3898 · astral-sh/ruff

konstin · 2023-04-06T10:59:06Z

Previously, we used byte indices when we should have used char indices, causing crashes when the were non-ascii characters before our replaces

Fixes #3716

Previously, we used byte indices when we should have used char indices, causing crashes when the were non-ascii characters before our replaces Fixes #3716

konstin · 2023-04-06T10:59:45Z

crates/ruff/src/rules/pylint/rules/invalid_string_characters.rs

                _ => unreachable!(),
            };
-            let location = helpers::to_absolute(Location::new(row + 1, column), start);
+            let column_chars = line[..column_bytes].chars().count();


@MichaReiser I've found this by guessing, i'm not sure if that's the correct way of computing offsets

It's the correct way for computing char offsets, which is slow. That's why I want to change Location to use byte offsets directly so that it simply becomes line.len() which is O(1)

The current solution has a worst-case complexity of O(n^2) because line[..column_bytes].chars().count() iterates from the beginning of the text for every match. I would recommend not using match_indices and iterate over chars directly.

let mut char_offset = 0; for (byte_offset, char) in line.char_indices() { let replacement = match char { ...patterns _ => { char_offset += 1; continue; } }; // .. rest char_offset += 1; }

updated, char() works

konstin · 2023-04-06T11:07:31Z

crates/ruff/resources/test/fixtures/pylint/invalid_characters.py

i also hope that this is the right terminology (github doesn't like the weird bytes inside)

github-actions · 2023-04-06T11:10:05Z

PR Check Results

Ecosystem

ℹ️ ecosystem check detected changes. (+0, -11, 0 error(s))

airflow (+0, -9)

- dev/breeze/src/airflow_breeze/params/build_prod_params.py:47:27: RUF201 Do not perform function call `get_airflow_extras` in dataclass defaults
- dev/breeze/src/airflow_breeze/params/common_build_params.py:42:27: RUF201 Do not perform function call `os.environ.get` in dataclass defaults
- dev/breeze/src/airflow_breeze/params/common_build_params.py:43:39: RUF201 Do not perform function call `os.environ.get` in dataclass defaults
- dev/breeze/src/airflow_breeze/params/common_build_params.py:54:27: RUF201 Do not perform function call `os.environ.get` in dataclass defaults
- dev/breeze/src/airflow_breeze/params/common_build_params.py:56:25: RUF201 Do not perform function call `os.environ.get` in dataclass defaults
- dev/breeze/src/airflow_breeze/params/shell_params.py:70:27: RUF201 Do not perform function call `os.environ.get` in dataclass defaults
- dev/breeze/src/airflow_breeze/params/shell_params.py:71:39: RUF201 Do not perform function call `os.environ.get` in dataclass defaults
- dev/breeze/src/airflow_breeze/params/shell_params.py:87:27: RUF201 Do not perform function call `os.environ.get` in dataclass defaults
- dev/breeze/src/airflow_breeze/params/shell_params.py:89:25: RUF201 Do not perform function call `os.environ.get` in dataclass defaults

scikit-build-core (+0, -2)

- src/scikit_build_core/file_api/model/toolchains.py:41:27: RUF201 Do not perform function call `APIVersion` in dataclass defaults
- tests/test_settings.py:23:17: RUF201 Do not perform function call `Path` in dataclass defaults

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.02     15.2±0.08ms     2.7 MB/sec    1.00     15.0±0.06ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.8±0.01ms     4.4 MB/sec    1.00      3.8±0.01ms     4.4 MB/sec
linter/all-rules/numpy/globals.py          1.01    412.0±2.41µs     7.2 MB/sec    1.00    407.4±1.97µs     7.2 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.5±0.03ms     3.9 MB/sec    1.00      6.5±0.03ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.02      7.9±0.03ms     5.2 MB/sec    1.00      7.7±0.01ms     5.3 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02   1748.4±6.68µs     9.5 MB/sec    1.00   1713.4±1.76µs     9.7 MB/sec
linter/default-rules/numpy/globals.py      1.00    181.6±0.58µs    16.2 MB/sec    1.00    181.5±0.55µs    16.3 MB/sec
linter/default-rules/pydantic/types.py     1.02      3.6±0.01ms     7.0 MB/sec    1.00      3.6±0.01ms     7.1 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     14.5±0.26ms     2.8 MB/sec    1.00     14.5±0.18ms     2.8 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.7±0.09ms     4.5 MB/sec    1.00      3.7±0.06ms     4.5 MB/sec
linter/all-rules/numpy/globals.py          1.01    441.2±8.40µs     6.7 MB/sec    1.00    437.7±7.99µs     6.7 MB/sec
linter/all-rules/pydantic/types.py         1.01      6.2±0.15ms     4.1 MB/sec    1.00      6.1±0.12ms     4.2 MB/sec
linter/default-rules/large/dataset.py      1.00      7.3±0.08ms     5.6 MB/sec    1.00      7.3±0.11ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1589.0±27.12µs    10.5 MB/sec    1.01  1607.6±30.01µs    10.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    172.8±7.51µs    17.1 MB/sec    1.01    174.0±5.52µs    17.0 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.3±0.08ms     7.7 MB/sec    1.01      3.3±0.06ms     7.6 MB/sec

MichaReiser · 2023-04-06T11:18:36Z

crates/ruff/src/rules/pylint/rules/invalid_string_characters.rs

                _ => unreachable!(),
            };
-            let location = helpers::to_absolute(Location::new(row + 1, column), start);
+            let column_chars = line[..column_bytes].chars().count();


It's the correct way for computing char offsets, which is slow. That's why I want to change Location to use byte offsets directly so that it simply becomes line.len() which is O(1)

The current solution has a worst-case complexity of O(n^2) because line[..column_bytes].chars().count() iterates from the beginning of the text for every match. I would recommend not using match_indices and iterate over chars directly.

let mut char_offset = 0; for (byte_offset, char) in line.char_indices() { let replacement = match char { ...patterns _ => { char_offset += 1; continue; } }; // .. rest char_offset += 1; }

Fix unicode handling in PLE2515

de7e379

Previously, we used byte indices when we should have used char indices, causing crashes when the were non-ascii characters before our replaces Fixes #3716

konstin requested a review from MichaReiser April 6, 2023 10:59

konstin commented Apr 6, 2023

View reviewed changes

MichaReiser approved these changes Apr 6, 2023

View reviewed changes

Use .chars()

c1e22c8

charliermarsh merged commit cae5503 into main Apr 6, 2023

charliermarsh deleted the fix_unicode_in_PLE2515 branch April 6, 2023 17:54

charliermarsh added the bug Something isn't working label Apr 6, 2023

p-linnane mentioned this pull request Apr 20, 2023

ruff 0.0.262 Homebrew/homebrew-core#128839

Merged

renovate bot mentioned this pull request Jun 7, 2023

chore(pre-commit): update ruff to v0.0.286 guludo/venv-run#28

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unicode handling in PLE2515#3898

Fix unicode handling in PLE2515#3898
charliermarsh merged 2 commits intomainfrom
fix_unicode_in_PLE2515

konstin commented Apr 6, 2023

Uh oh!

konstin Apr 6, 2023

Uh oh!

MichaReiser Apr 6, 2023

Uh oh!

konstin Apr 6, 2023

Uh oh!

konstin Apr 6, 2023

Uh oh!

github-actions bot commented Apr 6, 2023 •

edited

Loading

Uh oh!

MichaReiser Apr 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

konstin commented Apr 6, 2023

Uh oh!

konstin Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

MichaReiser Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

konstin Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

konstin Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Check Results

Ecosystem

Benchmark

Linux

Windows

Uh oh!

MichaReiser Apr 6, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Apr 6, 2023 •

edited

Loading