Add RUF016: Detection of invalid index types

[zanieb]

Detects invalid types for tuple, list, bytes, string indices.

For example, the following will raise a TypeError at runtime and when imported Python will display a SyntaxWarning

var = [1, 2, 3]["x"]

example.py:1: SyntaxWarning: list indices must be integers or slices, not str; perhaps you missed a comma?
  var = [1, 2, 3]["x"]
Traceback (most recent call last):
  File "example.py", line 1, in <module>
    var = [1, 2, 3]["x"]
          ~~~~~~~~~^^^^^
TypeError: list indices must be integers or slices, not str

Previously, Ruff would not report the invalid syntax but now a violation will be reported. This does not apply to cases where a variable, call, or complex expression is used in the index — detection is roughly limited to static definitions, which matches Python's warnings.

❯ ./target/debug/ruff example.py --select RUF015 --show-source --no-cache
example.py:1:17: RUF015 Indexed access to type `list` uses type `str` instead of an integer or slice.
  |
1 | var = [1, 2, 3]["x"]
  |                 ^^^ RUF015
  |

Closes #5082
xref python/cpython@ffff144

[github-actions]

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                    pr
-----                                      ----                                    --
formatter/large/dataset.py                 1.01      8.6±0.46ms     4.8 MB/sec     1.00      8.4±0.37ms     4.8 MB/sec
formatter/numpy/ctypeslib.py               1.00  1981.2±115.85µs     8.4 MB/sec    1.01  1997.7±147.35µs     8.3 MB/sec
formatter/numpy/globals.py                 1.01   227.4±14.01µs    13.0 MB/sec     1.00   225.5±13.82µs    13.1 MB/sec
formatter/pydantic/types.py                1.00      4.2±0.19ms     6.1 MB/sec     1.02      4.3±0.21ms     6.0 MB/sec
linter/all-rules/large/dataset.py          1.00     14.9±0.62ms     2.7 MB/sec     1.01     15.0±0.53ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.02      3.7±0.15ms     4.5 MB/sec     1.00      3.6±0.15ms     4.6 MB/sec
linter/all-rules/numpy/globals.py          1.00   471.5±37.62µs     6.3 MB/sec     1.02   478.9±25.68µs     6.2 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.7±0.37ms     3.8 MB/sec     1.00      6.6±0.29ms     3.8 MB/sec
linter/default-rules/large/dataset.py      1.00      7.2±0.29ms     5.6 MB/sec     1.00      7.2±0.24ms     5.6 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1583.3±67.33µs    10.5 MB/sec     1.02  1608.8±84.20µs    10.3 MB/sec
linter/default-rules/numpy/globals.py      1.04   191.8±16.00µs    15.4 MB/sec     1.00    184.2±9.93µs    16.0 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.3±0.14ms     7.7 MB/sec     1.00      3.3±0.19ms     7.7 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      9.5±0.06ms     4.3 MB/sec    1.00      9.5±0.05ms     4.3 MB/sec
formatter/numpy/ctypeslib.py               1.00      2.1±0.01ms     7.9 MB/sec    1.02      2.2±0.09ms     7.7 MB/sec
formatter/numpy/globals.py                 1.00    229.7±5.18µs    12.8 MB/sec    1.01    232.4±7.65µs    12.7 MB/sec
formatter/pydantic/types.py                1.00      4.7±0.06ms     5.4 MB/sec    1.00      4.7±0.05ms     5.4 MB/sec
linter/all-rules/large/dataset.py          1.02     15.9±0.15ms     2.6 MB/sec    1.00     15.6±0.11ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.2±0.04ms     4.0 MB/sec    1.00      4.1±0.03ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00   427.6±11.45µs     6.9 MB/sec    1.00    426.3±4.69µs     6.9 MB/sec
linter/all-rules/pydantic/types.py         1.02      7.2±0.05ms     3.6 MB/sec    1.00      7.1±0.07ms     3.6 MB/sec
linter/default-rules/large/dataset.py      1.02      8.3±0.05ms     4.9 MB/sec    1.00      8.1±0.06ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1672.7±12.11µs    10.0 MB/sec    1.00  1668.8±18.43µs    10.0 MB/sec
linter/default-rules/numpy/globals.py      1.00    180.7±1.73µs    16.3 MB/sec    1.00    180.5±4.26µs    16.3 MB/sec
linter/default-rules/pydantic/types.py     1.01      3.7±0.02ms     7.0 MB/sec    1.00      3.6±0.02ms     7.1 MB/sec

[zanieb]

In a comparison with Python's syntax warnings, we use a different message format and perform additional validation of the types used in slices.

❯ python crates/ruff/resources/test/fixtures/ruff/RUF015.py     
RUF015.py:20: SyntaxWarning: str indices must be integers or slices, not str; perhaps you missed a comma?
RUF015.py:21: SyntaxWarning: str indices must be integers or slices, not str; perhaps you missed a comma?
RUF015.py:24: SyntaxWarning: bytes indices must be integers or slices, not str; perhaps you missed a comma?
RUF015.py:27: SyntaxWarning: list indices must be integers or slices, not str; perhaps you missed a comma?
RUF015.py:28: SyntaxWarning: tuple indices must be integers or slices, not str; perhaps you missed a comma?
RUF015.py:31: SyntaxWarning: list indices must be integers or slices, not str; perhaps you missed a comma?
RUF015.py:34: SyntaxWarning: str indices must be integers or slices, not tuple; perhaps you missed a comma?
RUF015.py:37: SyntaxWarning: list indices must be integers or slices, not str; perhaps you missed a comma?
RUF015.py:40: SyntaxWarning: list indices must be integers or slices, not float; perhaps you missed a comma?
RUF015.py:43: SyntaxWarning: list indices must be integers or slices, not dict; perhaps you missed a comma?
RUF015.py:46: SyntaxWarning: list indices must be integers or slices, not dict; perhaps you missed a comma?
RUF015.py:49: SyntaxWarning: list indices must be integers or slices, not tuple; perhaps you missed a comma?
RUF015.py:52: SyntaxWarning: list indices must be integers or slices, not list; perhaps you missed a comma?
RUF015.py:55: SyntaxWarning: list indices must be integers or slices, not set; perhaps you missed a comma?
RUF015.py:58: SyntaxWarning: list indices must be integers or slices, not bytes; perhaps you missed a comma?
RUF015.py:76: SyntaxWarning: list indices must be integers or slices, not str; perhaps you missed a comma?

❯ ./target/debug/ruff check --select RUF015 crates/ruff/resources/test/fixtures/ruff/RUF015.py
RUF015.py:20:13: RUF015 Indexed access to type `str` uses type `str` instead of an integer or slice.
RUF015.py:21:14: RUF015 Indexed access to type `str` uses type `str` instead of an integer or slice.
RUF015.py:24:14: RUF015 Indexed access to type `bytes` uses type `str` instead of an integer or slice.
RUF015.py:27:17: RUF015 Indexed access to type `list` uses type `str` instead of an integer or slice.
RUF015.py:28:17: RUF015 Indexed access to type `tuple` uses type `str` instead of an integer or slice.
RUF015.py:31:30: RUF015 Indexed access to type `list comprehension` uses type `str` instead of an integer or slice.
RUF015.py:34:13: RUF015 Indexed access to type `str` uses type `tuple` instead of an integer or slice.
RUF015.py:37:14: RUF015 Indexed access to type `list` uses type `str` instead of an integer or slice.
RUF015.py:40:14: RUF015 Indexed access to type `list` uses type `float` instead of an integer or slice.
RUF015.py:43:14: RUF015 Indexed access to type `list` uses type `dict` instead of an integer or slice.
RUF015.py:46:14: RUF015 Indexed access to type `list` uses type `dict comprehension` instead of an integer or slice.
RUF015.py:49:14: RUF015 Indexed access to type `list` uses type `tuple` instead of an integer or slice.
RUF015.py:52:14: RUF015 Indexed access to type `list` uses type `list comprehension` instead of an integer or slice.
RUF015.py:55:14: RUF015 Indexed access to type `list` uses type `set` instead of an integer or slice.
RUF015.py:58:14: RUF015 Indexed access to type `list` uses type `bytes` instead of an integer or slice.
RUF015.py:61:17: RUF015 Slice in indexed access to type `list` uses type `str` as bound instead of an integer.
RUF015.py:62:17: RUF015 Slice in indexed access to type `list` uses type `str` as bound instead of an integer.
RUF015.py:63:17: RUF015 Slice in indexed access to type `list` uses type `float` as bound instead of an integer.
RUF015.py:64:17: RUF015 Slice in indexed access to type `list` uses type `set` as bound instead of an integer.
RUF015.py:67:19: RUF015 Slice in indexed access to type `list` uses type `str` as bound instead of an integer.
RUF015.py:68:19: RUF015 Slice in indexed access to type `list` uses type `str` as bound instead of an integer.
RUF015.py:69:19: RUF015 Slice in indexed access to type `list` uses type `float` as bound instead of an integer.
RUF015.py:70:19: RUF015 Slice in indexed access to type `list` uses type `set` as bound instead of an integer.
RUF015.py:73:17: RUF015 Slice in indexed access to type `list` uses type `str` as bound instead of an integer.
RUF015.py:73:21: RUF015 Slice in indexed access to type `list` uses type `str` as bound instead of an integer.
RUF015.py:76:17: RUF015 Indexed access to type `list` uses type `str` instead of an integer or slice.

I've ensured that each of the slice cases raises an error at runtime

Traceback (most recent call last):
  File "/Users/mz/eng/src/charliermarsh/ruff/example.py", line 3, in <module>
    var = [1, 2, 3]["x":2]
          ~~~~~~~~~^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

Traceback (most recent call last):
  File "/Users/mz/eng/src/charliermarsh/ruff/example.py", line 4, in <module>
    var = [1, 2, 3][f"x":2]
          ~~~~~~~~~^^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

Traceback (most recent call last):
  File "/Users/mz/eng/src/charliermarsh/ruff/example.py", line 5, in <module>
    var = [1, 2, 3][1.2:2]
          ~~~~~~~~~^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

Traceback (most recent call last):
  File "/Users/mz/eng/src/charliermarsh/ruff/example.py", line 6, in <module>
    var = [1, 2, 3][{"x"}:2]
          ~~~~~~~~~^^^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

Traceback (most recent call last):
  File "/Users/mz/eng/src/charliermarsh/ruff/example.py", line 9, in <module>
    var = [1, 2, 3][0:"x"]
          ~~~~~~~~~^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

Traceback (most recent call last):
  File "/Users/mz/eng/src/charliermarsh/ruff/example.py", line 10, in <module>
    var = [1, 2, 3][0:f"x"]
          ~~~~~~~~~^^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

Traceback (most recent call last):
  File "/Users/mz/eng/src/charliermarsh/ruff/example.py", line 11, in <module>
    var = [1, 2, 3][0:1.2]
          ~~~~~~~~~^^^^^^^
TypeError: slice indices must be integers or None or have an __index__ method

[MichaReiser]

Should we list the expressions here to force a compile error when a new expression was added.

[zanieb]

I don't think so. Unless new literal types are introduced, a new expression is unlikely to be detectable by this rule as a violation.

[zanieb]

@charliermarsh I retained this expect because I'd rather not silently return in this case. A developer has broken this rule if the type is not checkable.

[zanieb]

Note I had to rebase this due to conflict with #5549 which took the RUF015 code — some of the commits will not pass tests anymore.

[zanieb]

I narrowed this to match the pattern used in InvalidIndexType

[charliermarsh]

To me it would be a bit more natural to do this before the if !matches (like let Some(value_type) = ... else { return; };), then have an if !matches(value_type, CheckableExprType::List | ...) to enforce the further narrowing. That way the expect is unnecessary as the condition is impossible. But it's not blocking.

[zanieb]

I did this before then moved it back to an expect, I feel like it confuses the logic of the rule. Like...

If the value is a list, byte, string, or tuple then we want to enforce this rule.

To display a violation, we happen to use CheckableExprType to generate a type name for the value. However, this is just for display purposes not a limitation of the rule itself.

If someone were to extend the rule to apply to a new type, e.g. byte arrays, I would not expect this function to exit early because there is not an implementation for displaying the type name. In this case, the expect would point a developer to the next step in their implementation.

🤷‍♀️ it does feel like a subtle stylistic choice in the end — maybe you shouldn't have sent me that BurntSushi article 😝

I'm kind of curious to see what lessons I learn from making my own stylistic choices but I'm happy to follow precedent for the project too!

[charliermarsh]

This makes sense. I'd say run with what you have here! I tend to be pretty defensive around expect and unwrap (which are ~equivalent) because they're not recoverable in any way, so e.g. if we shipped a release that had an oversight here, Ruff would bail entirely on files that hit this codepath.

The "right" answer here may be to extend CheckableExprType to instead implement all expression types (so it doesn't return Option), or perhaps to use a debug assertion so that we see this failure in debug builds but fail silently in releases. But it's also ok for now, we know that it's a superset of expressions on the match anyway. (And we're obviously overdoing it on this one point just for educational / discussion purposes, it's not a sticking point in practice :))

[zanieb]

or perhaps to use a debug assertion so that we see this failure in debug builds but fail silently in releases.

I'm pretty happy with that as an approach.

Although... I'd also want a user to report a failure here because it's a bug in Ruff. I guess it's a matter of if we want to panic and make the rule entirely unusable for their code or fail silently and hope they notice that the rule isn't raising a violation. I can understand preferring the second as a safer user experience :)

[zanieb]

I wanted debug_panic!(...) but I guess debug_assert!(false, ...) will do e058e24

[charliermarsh]

I'm happy with whatever! If it were me on a desert island, I guess I'd write something like let Some(value_type) = CheckableExprType::try_from(value) else { return; }; but I'm not convinced that's the best option. This looks good to me.

(Again just to complete the picture, I think if we hit a panic here, the behavior the user would see is that we'd throw up a message asking them to file an issue and wouldn't print any violations for the failing file, but would print out violations for all other files as usual.)

[charliermarsh]

Looks great :)