[`pylint`] Also emit `PLR0206` for properties with variadic parameters
Summary
Currently this rule checks the number of nonvariadic parameters a property has, but doesn't check how many variadic parameters the property has. That makes little sense to me: if a property has variadic non-self parameters, that seems just as invalid as if a property has nonvariadic non-self parameters.
This PR changes the rule so that it checks the total number of parameters the property has, rather than just the number of nonvariadic parameters. Following the new APIs added in https://github.com/astral-sh/ruff/commit/87929ad5f1306cb889cbc85ddfdb9404d9b5c6ad, this has the nice side effect of also simplifying the code slightly.
This means that we diverge from pylint's behaviour a little more in how we implement this rule, but we were already doing something slightly different. It looks like pylint for some reason only checks the number of positional-or-keyword nonvariadic parameters in a property function, and ignores the number of positional-only or keyword-only nonvariadic parameters: https://github.com/pylint-dev/pylint/blob/524f2561eda21b5593ab1c69963442031817883d/pylint/checkers/classes/class_checker.py#L1414. I don't understand why they would do that; I think that's buggy behaviour from pylint there.
Test Plan
cargo test. I added some new fixtures.
ruff-ecosystem results
Linter (stable)
ℹ️ ecosystem check detected linter changes. (+6 -0 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)
milvus-io/pymilvus (+6 -0 violations, +0 -0 fixes)
+ pymilvus/orm/collection.py:1114:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/collection.py:1255:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/collection.py:237:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/collection.py:259:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/collection.py:266:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/partition.py:112:9: PLR0206 Cannot have defined parameters for properties
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLR0206 | 6 | 6 | 0 | 0 | 0 |
Linter (preview)
ℹ️ ecosystem check detected linter changes. (+6 -0 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)
milvus-io/pymilvus (+6 -0 violations, +0 -0 fixes)
ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview
+ pymilvus/orm/collection.py:1114:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/collection.py:1255:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/collection.py:237:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/collection.py:259:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/collection.py:266:9: PLR0206 Cannot have defined parameters for properties + pymilvus/orm/partition.py:112:9: PLR0206 Cannot have defined parameters for properties
Changes by rule (1 rules affected)
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| PLR0206 | 6 | 6 | 0 | 0 | 0 |
ℹ️ ecosystem check detected linter changes. (+6 -0 violations, +0 -0 fixes in 1 projects; 43 projects unchanged)
These all look like true positives to me. The affected project defines a bunch of properties with **kwargs variadic parameters, but no arguments are ever passed to those parameters as far as I can see. The property definitions would make more sense without the **kwargs parameters, even if the code works fine today without any exception being raised.
I think these changes are correct but it would be useful to get a second opinion. I'd also look at why Pylint doesn't trigger on them. Is there an issue in Pylint which might give us an insight here?
(Edit: Pyright -> Pylint)
I'd also look at why Pylint doesn't trigger on them. Is there an issue in Pylint which might give us an insight here?
Yeah, I'll do some digging and check to see if there's a reason why pylint only checks for the number of positional-or-keyword parameters.
The results of digging:
- The pylint check was originally added in https://github.com/pylint-dev/pylint/commit/a636569b9cc53279b3b5f4d24b58d9bc739b05d3, a commit that was pushed straight to pylint's
mainbranch without a pull request. - The commit closed the pylint issue https://github.com/pylint-dev/pylint/issues/3006. The issue suggests that the check was intended to catch properties with variadic non-
selfparameters as well as those with nonvariadic non-selfparameters, but there's no discussion on the issue. - The only issue regarding pylint's
property-with-parameterscheck in the years since it was added that I can find is https://github.com/pylint-dev/pylint/issues/3600. That issue was fixed by the pylint PR https://github.com/pylint-dev/pylint/pull/3621.
I confirmed locally that running pylint on the original motivating example for the pylint check given in https://github.com/pylint-dev/pylint/issues/3006#issue-468017491 does not actually trigger a violation of the pylint rule (because the example has variadic non-self parameters rather than nonvariadic non-self parameters). I checked using pylint==3.1.0.
I think these changes are correct but it would be useful to get a second opinion.
@carljm, could you possibly take a look?
Thanks for the details. It's good to go from my side 👍
I opened a pylint issue here to let them know about the bug on their side: https://github.com/pylint-dev/pylint/issues/9584
The pylint maintainers have added labels to https://github.com/pylint-dev/pylint/issues/9584 which indicate that they agree there's a bug on their side, so I'll merge this now 👍
Thanks for the review @dhruvmanila!