ruff icon indicating copy to clipboard operation
ruff copied to clipboard

[`pylint`] Also emit `PLR0206` for properties with variadic parameters

Open AlexWaygood opened this issue 1 year ago • 2 comments

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.

AlexWaygood avatar Apr 29 '24 11:04 AlexWaygood

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

github-actions[bot] avatar Apr 29 '24 11:04 github-actions[bot]

ℹ️ 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.

AlexWaygood avatar Apr 29 '24 11:04 AlexWaygood

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)

dhruvmanila avatar Apr 30 '24 08:04 dhruvmanila

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.

AlexWaygood avatar Apr 30 '24 08:04 AlexWaygood

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 main branch 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-self parameters as well as those with nonvariadic non-self parameters, but there's no discussion on the issue.
  • The only issue regarding pylint's property-with-parameters check 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.

AlexWaygood avatar Apr 30 '24 08:04 AlexWaygood

I think these changes are correct but it would be useful to get a second opinion.

@carljm, could you possibly take a look?

AlexWaygood avatar Apr 30 '24 08:04 AlexWaygood

Thanks for the details. It's good to go from my side 👍

dhruvmanila avatar Apr 30 '24 08:04 dhruvmanila

I opened a pylint issue here to let them know about the bug on their side: https://github.com/pylint-dev/pylint/issues/9584

AlexWaygood avatar Apr 30 '24 09:04 AlexWaygood

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!

AlexWaygood avatar Apr 30 '24 10:04 AlexWaygood