Issue 9519 super init with non self arg #10190
Conversation
|
the test file lines 11-13 Running which is this: so the function actually exits earlier than what was suggested in the original issue. |
|
Current debug cruft: |
|
Just prior to turning off the debug cruft, here is the output: |
jzohrab
force-pushed
the
issue_9519_super_init_with_non_self_arg
branch
from
c4f2c85 to
e29a7c8
Compare
|
There's a bunch of cruft in the commits and the code, but the idea is there, so I'm going to mark this as ready for review. I'm sure the actual ast checking for the |
Pierre-Sassoulas
added
Work in progress
False Negative 🦋
|
As you can probably tell from my hacking, I'm very new at ast hacking, so there may be a far better way to do it than what I have. At least the test case should be good as a starting point. |
|
There are some test failures, but I'll wait for feedback/notes before I try to address them, as there will likely be changes needed. |
This comment has been minimized.
This comment has been minimized.
|
Hey, thanks for tackling this one! What do you think about something like this? diff --git a/pylint/checkers/typecheck.py b/pylint/checkers/typecheck.py
index edef5188b..afed2d3fe 100644
--- a/pylint/checkers/typecheck.py
+++ b/pylint/checkers/typecheck.py
@@ -1453,8 +1453,19 @@ accessed. Python regular expressions are accepted.",
"""Check that called functions/methods are inferred to callable objects,
and that passed arguments match the parameters in the inferred function.
"""
- called = safe_infer(node.func, compare_constructors=True)
+ is_super = False
+ if (
+ isinstance(node.func, nodes.Attribute)
+ and node.func.attrname == "__init__"
+ and isinstance(node.func.expr, nodes.Call)
+ and isinstance(node.func.expr.func, nodes.Name)
+ and node.func.expr.func.name == "super"
+ ):
+ inferred = safe_infer(node.func.expr)
+ if isinstance(inferred, astroid.objects.Super):
+ is_super = True
+ called = safe_infer(node.func, compare_constructors=True)
self._check_not_callable(node, called)
try:
@@ -1468,7 +1479,21 @@ accessed. Python regular expressions are accepted.",
if called.name == "isinstance":
# Verify whether second argument of isinstance is a valid type
self._check_isinstance_args(node, callable_name)
- # Built-in functions have no argument information.
+ # Built-in functions have no argument information, but we know
+ # super() only takes self.
+ if is_super and utils.is_builtin_object(called):
+ if (
+ (call_site := astroid.arguments.CallSite.from_call(node))
+ and call_site.positional_arguments
+ and (first_arg := call_site.positional_arguments[0])
+ and not (isinstance(first_arg, nodes.Name) and first_arg.name == "self")
+ ):
+ self.add_message(
+ "too-many-function-args",
+ node=node,
+ args=(callable_name,),
+ )
+
return
if len(called.argnames()) != len(set(called.argnames())): |
|
Hi @jacobtylerwalls - thanks very much. I confess that this is out of my range of experience and so I can't say that your suggestion is the right way to handle it! It looks right to the uninformed (aka me), but I'm flying blind. If you'd like, I can make the change you suggested in the branch, remove the cruft code comments, and push (without squashing). |
|
Hey @jzohrab that would be great. Once you get a clean test run (I think I remember seeing there were a couple functional tests that needed updating?) the CI will run a "primer" job showing results of making this change on lint runs on several python packages. That might help us see if my suggestions is missing any edge cases. All part of the fun! (I'm not saying my sketch is perfect, but I was trying to rely less on asserting on the str()ingified value of a node and things like that.) |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
jzohrab
force-pushed
the
issue_9519_super_init_with_non_self_arg
branch
from
94a32c2 to
15850fd
Compare
|
Rebased off of upstream/main, added the changes suggested by @jacobtylerwalls, moved the is_super check to a separate function for readability. Will see where that leaves us at the end of the run. I don't think there will be any new test failures introduced with this change, but I'm not sure. It's possible that there are some existing test failures (which should be deactivated/indicated in a separate PR, maybe). |
|
There are failures: but I have no idea how to approach them! Here's the metaclass_attr_access file: so maybe the super code suggested is handling super(Meta, cls) incorrectly. Not sure, and I think I might be breaking things if I hack at it further. Maybe someone with more experience with asts etc can take my work as a startisg point. |
|
Hey thanks for the push. What I can say at a glance is that the pylint annotations you see in the files tab of this PR show some false positives with my suggestion -- looks like the logic needs to be fixed to make sure that the inference result for the class being init'ed is really |
Work-in-progress for issue #9519, to see if I can crack it.
This is going to contain many junk commits that I'll rebase and get rid of later, just doing this so that people can see it and maybe comment.
The first commit adds a failing test case, then there is some debug crap just so I can understand the code as I go.