[pylint] Implement PLE0302 unexpected-special-method-signature

[mccullocht]

Implement pylint unexpected-special-method-signature

This is part of issue #970

[charliermarsh]

Nice, thanks for the PR! Look forward to reviewing :)

[github-actions]

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.02     15.4±0.04ms     2.6 MB/sec    1.00     15.1±0.02ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.02      3.9±0.01ms     4.3 MB/sec    1.00      3.8±0.01ms     4.4 MB/sec
linter/all-rules/numpy/globals.py          1.03    431.4±5.62µs     6.8 MB/sec    1.00    418.3±2.56µs     7.1 MB/sec
linter/all-rules/pydantic/types.py         1.02      6.6±0.02ms     3.9 MB/sec    1.00      6.5±0.01ms     3.9 MB/sec
linter/default-rules/large/dataset.py      1.04      8.1±0.02ms     5.0 MB/sec    1.00      7.8±0.01ms     5.2 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02   1775.7±5.29µs     9.4 MB/sec    1.00   1742.1±3.33µs     9.6 MB/sec
linter/default-rules/numpy/globals.py      1.01    185.4±0.51µs    15.9 MB/sec    1.00    183.5±0.85µs    16.1 MB/sec
linter/default-rules/pydantic/types.py     1.03      3.7±0.01ms     6.8 MB/sec    1.00      3.6±0.01ms     7.0 MB/sec
parser/large/dataset.py                    1.04      6.5±0.00ms     6.3 MB/sec    1.00      6.2±0.01ms     6.5 MB/sec
parser/numpy/ctypeslib.py                  1.02   1273.9±1.44µs    13.1 MB/sec    1.00   1248.1±2.03µs    13.3 MB/sec
parser/numpy/globals.py                    1.01    127.4±0.23µs    23.2 MB/sec    1.00    125.9±0.55µs    23.4 MB/sec
parser/pydantic/types.py                   1.02      2.8±0.00ms     9.2 MB/sec    1.00      2.7±0.00ms     9.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.02     17.3±0.20ms     2.3 MB/sec    1.00     17.0±0.16ms     2.4 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.01      4.4±0.04ms     3.8 MB/sec    1.00      4.4±0.04ms     3.8 MB/sec
linter/all-rules/numpy/globals.py          1.02   542.7±13.75µs     5.4 MB/sec    1.00    534.1±5.64µs     5.5 MB/sec
linter/all-rules/pydantic/types.py         1.01      7.3±0.09ms     3.5 MB/sec    1.00      7.3±0.13ms     3.5 MB/sec
linter/default-rules/large/dataset.py      1.03      8.9±0.07ms     4.6 MB/sec    1.00      8.7±0.10ms     4.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.02  1938.6±17.24µs     8.6 MB/sec    1.00  1906.0±24.51µs     8.7 MB/sec
linter/default-rules/numpy/globals.py      1.01    217.8±3.32µs    13.5 MB/sec    1.00    216.3±7.11µs    13.6 MB/sec
linter/default-rules/pydantic/types.py     1.01      4.0±0.04ms     6.3 MB/sec    1.00      4.0±0.05ms     6.4 MB/sec
parser/large/dataset.py                    1.00      6.9±0.05ms     5.9 MB/sec    1.00      6.8±0.04ms     5.9 MB/sec
parser/numpy/ctypeslib.py                  1.00  1313.2±13.86µs    12.7 MB/sec    1.00  1308.4±13.68µs    12.7 MB/sec
parser/numpy/globals.py                    1.00    131.4±2.79µs    22.5 MB/sec    1.02    133.9±2.63µs    22.0 MB/sec
parser/pydantic/types.py                   1.01      2.9±0.03ms     8.6 MB/sec    1.00      2.9±0.03ms     8.7 MB/sec

[charliermarsh]

Nice PR! Thanks for getting involved. One question below and a couple small comments.

[charliermarsh]

Does this need to take into account positional-only and/or keyword-only arguments? (Elsewhere, we have logic like let defaults_start = args.posonlyargs.len() + args.args.len() - args.defaults.len(); and let defaults_start = args.kwonlyargs.len() - args.kw_defaults.len(); to infer some similar information from function signatures.)

[mccullocht]

I patterned off of the pylint check which did not take posonly/kwonly args into account, but I can investigate further if you'd like. I don't think we need to consider kw-only arguments as it doesn't seem that any special methods have them according to the documentation.

[charliermarsh]

Perhaps we want to extend the check with a custom message when posonly/kwonly args are included, since that itself deviates from the expected signature, albeit in a different way?

[charliermarsh]

Rather than modeling each case as RangeInclusive, and using self.expected_params.end() - self.expected_params.start() > 1-like checks to determine whether a branch should be treated as a range or not, what if we introduced an enum here to represent the two cases? E.g.:

enum ExpectedParameters {
  Fixed(usize),
  // Or RangeInclusive<usize>
  Range(usize, usize),
}

(Names etc. are of course flexible, just illustrating the concept.)

I think that would make some of the case-handling below a bit clearer by way of being more explicit.

[mccullocht]

Done.

I used the new enum in the violation so it has to be public :/. If you have a preference expected_params can be rendered into a string for formatting before creating the violation, which would allow ExpectedParams to be private again.

[charliermarsh]

All good, we follow this pattern after and the enums end up public. No worries.

[charliermarsh]

(This could even be an associated function on ExpectedParameters, to increment the count.)

[mccullocht]

ExpectedParams::from_method() takes an is_staticmethod bool and applies this internally on construction, but I got rid of the map() call.

[mccullocht]

Thank you for the prompt review!

[mccullocht]

I patterned off of the pylint check which did not take posonly/kwonly args into account, but I can investigate further if you'd like. I don't think we need to consider kw-only arguments as it doesn't seem that any special methods have them according to the documentation.

[mccullocht]

Done.

I used the new enum in the violation so it has to be public :/. If you have a preference expected_params can be rendered into a string for formatting before creating the violation, which would allow ExpectedParams to be private again.

[mccullocht]

ExpectedParams::from_method() takes an is_staticmethod bool and applies this internally on construction, but I got rid of the map() call.

[charliermarsh]

Thanks so much for the contribution!