Python: Modernize the init-calls-subclass query

[joefarebrother]

No description provided.

[github-actions]

QHelp previews:

python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.qhelp

__init__ method calls overridden method

When initializing an instance of the class in the class' __init__ method, calls tha are made using the instance may receive an instance of the class that is not yet fully initialized. When a method called in an initializer is overridden in a subclass, the subclass method receives the instance in a potentially unexpected state. Fields that would be initialized after the call, including potentially in the subclass' __init__ method, will not be initialized. This may lead to runtime errors, as well as make the code more difficult to maintain, as future changes may not be aware of which fields would not be initialized.

Recommendation

If possible, refactor the initializer method such that initialization is complete before calling any overridden methods. For helper methods used as part of initialization, avoid overriding them, and instead call any additional logic required in the subclass' __init__ method.

If the overridden method does not depend on the instance self, and only on its class, consider making it a @classmethod or @staticmethod instead.

If calling an overridden method is absolutely required, consider marking it as an internal method (by using an _ prefix) to discourage external users of the library from overriding it and observing partially initialized state, and ensure that the fact it is called during initialization is mentioned in the documentation.

Example

In the following case, the __init__ method of Super calls the set_up method that is overridden by Sub. This results in Sub.set_up being called with a partially initialized instance of Super which may be unexpected.

class Super(object):

    def __init__(self, arg):
        self._state = "Not OK"
        self.set_up(arg) # BAD: This method is overridden, so `Sub.set_up` receives a partially initialized instance.
        self._state = "OK"

    def set_up(self, arg):
        "Do some setup"
        self.a = 2

class Sub(Super):

    def __init__(self, arg):
        super().__init__(arg)
        self.important_state = "OK"

    def set_up(self, arg):
        super().set_up(arg)
        "Do some more setup"
        # BAD: at this point `self._state` is set to `"Not OK"`, and `self.important_state` is not initialized.
        if self._state == "OK":
            self.b = self.a + 2

In the following case, the initialization methods are separate between the superclass and the subclass.

class Super(object):

    def __init__(self, arg):
        self._state = "Not OK"
        self.super_set_up(arg) # GOOD: This isn't overriden. Instead, additional setup the subclass needs is called by the subclass' `__init__ method.`
        self._state = "OK"

    def super_set_up(self, arg):
        "Do some setup"
        self.a = 2


class Sub(Super):

    def __init__(self, arg):
        super().__init__(arg)
        self.sub_set_up(self, arg)
        self.important_state = "OK"


    def sub_set_up(self, arg):
        "Do some more setup"
        if self._state == "OK":
            self.b = self.a + 2

References

• CERT Secure Coding: Rule MET05-J. Reference discusses Java but is applicable to object oriented programming in many languages.
• StackOverflow: Overridable method calls in constructors.
• Python documentation: @classmethod.

[Copilot]

Pull Request Overview

This PR modernizes the py/init-calls-subclass QL query by replacing the old implementation with a new dataflow-based approach.

• Removes the legacy InitCallsSubclassMethod.ql.
• Adds a new directory and file under InitCallsSubclass/ using DataFlow predicates.
• Defines two predicates (initSelfCall and initSelfCallOverridden) to detect subclass-overridden calls in __init__.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
python/ql/src/Classes/InitCallsSubclassMethod.ql	Deleted the legacy query file
python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql	Added modernized dataflow-based query implementation

Comments suppressed due to low confidence (4)

python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql:28

• [nitpick] The parameter name override is ambiguous and matches a language keyword. Consider renaming it to something like overridingMethod for clarity.

predicate initSelfCallOverridden(Function init, DataFlow::MethodCallNode call, Function override) {

python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql:18

• [nitpick] It would be helpful to add a brief doc comment above this predicate explaining its purpose and parameters for future maintainers.

predicate initSelfCall(Function init, DataFlow::MethodCallNode call) {

python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql:38

• The new dataflow-based implementation should be accompanied by tests to validate that overridden __init__ calls are correctly detected; consider adding or updating QL tests.

from Function init, DataFlow::MethodCallNode call, Function override

python/ql/src/Classes/InitCallsSubclass/InitCallsSubclassMethod.ql:16

• This import isn't used in the new query. Consider removing it to keep dependencies minimal.

import semmle.python.dataflow.new.internal.DataFlowDispatch

[tausbn]

A few comments/suggestions, but otherwise this looks good to me.
(Feel free to postpone my suggestion about self parameters to a later PR if you just want to merge the present one.)

Also, in future PRs could you make sure that you don't rename/move and also modify files in the same commit? This really confuses the diff (as it doesn't know the two files should be linked up) and makes it difficult to see what the actual differences are.