[Python] Set memory policy to "strict"

README/ReleaseNotes/v640/index.md

@@ -113,6 +113,61 @@ As part of this migration, the following build options are deprecated. From ROOT
 
 ROOT dropped support for Python 3.9, meaning ROOT now requires at least Python 3.10.
 
+### Changed ownership policy for non-`const` pointer member function parameters
+
+If you have a member function taking a raw pointer, like `MyClass::foo(T *obj)`,
+PyROOT was so far assuming that calling this method on `my_instance`
+transfers the ownership of `obj` to `my_instance`.
+
+However, this resulted in many memory leaks, since many functions with such a
+signature actually don't take ownership of the object.
+
+To avoid such memory leaks, PyROOT now doesn't make this guess anymore as of
+ROOT 6.32. Because of this change, some double-deletes or dangling references
+might creep up in your scripts. These need to be fixed by properly managing
+object lifetime with Python references.
+
+You can fix the dangling references problem for example via:
+
+  1. Assigning the object to a python variable
+  2. Creating an owning collection that keeps the objects alive
+  3. Writing a pythonization for the member function that does the ownership
+     transfer if needed
+
+The double-delete problems can be fixed via:
+
+  1. Drop the ownership on the Python side with `ROOT.SetOwnership(obj, False)`
+  3. Writing a pythonization for the member function that drops the ownership on the Python side as above
+
+This affects for example the `TList::Add(TObject *obj)` member function, which
+will not transfer ownership from PyROOT to the TList anymore. The new policy
+fixes a memory leak, but at the same time it is not possible anymore to create
+the contained elements in place:
+
+```python
+# A TList is by default a non-owning container
+my_list = ROOT.TList()
+
+
+# This is working, but resulted in memory leak prior to ROOT 6.32:
+obj_1 = ROOT.TObjString("obj_1")
+my_list.Add(obj_1)
+
+
+# This is not allowed anymore, as the temporary would be
+# deleted immediately leaving a dangling pointer:
+my_list.Add(ROOT.TObjString("obj_2"))
+
+# Python reference count to contained object is now zero,
+# TList contains dangling pointer!
+```
+
+**Note:** You can change back to the old policy by calling
+`ROOT.SetMemoryPolicy(ROOT.kMemoryHeuristics)` after importing ROOT, but this
+should be only used for debugging purposes and this function might be removed
+in the future!
+
+
 ## Command-line utilities
 
 ## JavaScript ROOT

bindings/pyroot/pythonizations/CMakeLists.txt

@@ -38,6 +38,7 @@ if(roofit)
         ROOT/_pythonization/_roofit/_roojsonfactorywstool.py
         ROOT/_pythonization/_roofit/_roomcstudy.py
         ROOT/_pythonization/_roofit/_roomsgservice.py
+        ROOT/_pythonization/_roofit/_rooplot.py
         ROOT/_pythonization/_roofit/_rooprodpdf.py
         ROOT/_pythonization/_roofit/_roorealvar.py
         ROOT/_pythonization/_roofit/_roosimultaneous.py

bindings/pyroot/pythonizations/python/ROOT/_facade.py

@@ -192,11 +192,6 @@ def _finalSetup(self):
         if not self.gROOT.IsBatch() and self.PyConfig.StartGUIThread:
             self.app.init_graphics()
 
-        # Set memory policy to kUseHeuristics.
-        # This restores the default in PyROOT which was changed
-        # by new Cppyy
-        self.SetHeuristicMemoryPolicy(True)
-
         # The automatic conversion of ordinary obejcts to smart pointers is
         # disabled for ROOT because it can cause trouble with overload
         # resolution. If a function has overloads for both ordinary objects and

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py

@@ -63,3 +63,26 @@ def _SetDirectory_SetOwnership(self, dir):
         import ROOT
 
         ROOT.SetOwnership(self, False)
+
+
+def declare_cpp_owned_arg(position, name, positional_args, keyword_args, condition=lambda _: True):
+    """
+    Helper function to drop Python ownership of a specific funciton argument
+    with a given position and name, referring to the C++ signature.
+
+    positional_args and keyword_args should be the usual args and kwargs passed
+    to the function, and condition is an optional condition on which the Python
+    ownership is dropped.
+    """
+    import ROOT
+
+    arg = None
+
+    # has to match the C++ argument name
+    if name in keyword_args:
+        arg = keyword_args[name]
+    elif len(positional_args) > position:
+        arg = positional_args[0]
+
+    if condition(arg):
+        ROOT.SetOwnership(arg, False)

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/__init__.py

@@ -40,6 +40,7 @@
 from ._roojsonfactorywstool import RooJSONFactoryWSTool
 from ._roomcstudy import RooMCStudy
 from ._roomsgservice import RooMsgService
+from ._rooplot import RooPlot
 from ._rooprodpdf import RooProdPdf
 from ._roorealvar import RooRealVar
 from ._roosimultaneous import RooSimultaneous
@@ -69,6 +70,7 @@
     RooJSONFactoryWSTool,
     RooMCStudy,
     RooMsgService,
+    RooPlot,
     RooProdPdf,
     RooRealVar,
     RooSimultaneous,

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py

@@ -31,13 +31,19 @@ def _pack_cmd_args(*args, **kwargs):
     assert len(kwargs) == 0
 
     # Put RooCmdArgs in a RooLinkedList
-    cmdList = ROOT.RooLinkedList()
+    cmd_list = ROOT.RooLinkedList()
     for cmd in args:
         if not isinstance(cmd, ROOT.RooCmdArg):
             raise TypeError("This function only takes RooFit command arguments.")
-        cmdList.Add(cmd)
+        cmd_list.Add(cmd)
 
-    return cmdList
+    # The RooLinkedList passed to functions like fitTo() is expected to be
+    # non-owning. To make sure that the RooCmdArgs live long enough, we attach
+    # then as an attribute of the output list, such that the Python reference
+    # counter doesn't hit zero.
+    cmd_list._owning_pylist = args
+
+    return cmd_list
 
 
 class RooAbsPdf(RooAbsReal):

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooplot.py

@@ -0,0 +1,20 @@
+# Authors:
+# * Jonas Rembser 09/2023
+
+################################################################################
+# Copyright (C) 1995-2020, Rene Brun and Fons Rademakers.                      #
+# All rights reserved.                                                         #
+#                                                                              #
+# For the licensing terms see $ROOTSYS/LICENSE.                                #
+# For the list of contributors see $ROOTSYS/README/CREDITS.                    #
+################################################################################
+
+
+class RooPlot(object):
+    def addObject(self, *args, **kwargs):
+        from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+        # Python should transfer the ownership to the RooPlot
+        declare_cpp_owned_arg(0, "obj", args, kwargs)
+
+        return self._addObject(*args, **kwargs)

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tcollection.py

@@ -93,11 +93,26 @@ def _iter_pyz(self):
         yield o
 
 
+def _TCollection_Add(self, *args, **kwargs):
+    from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+    def condition(_):
+        return self.IsOwner()
+
+    declare_cpp_owned_arg(0, "obj", args, kwargs, condition=condition)
+
+    self._Add(*args, **kwargs)
+
+
 @pythonization('TCollection')
 def pythonize_tcollection(klass):
     # Parameters:
     # klass: class to be pythonized
 
+    # Pythonize Add()
+    klass._Add = klass.Add
+    klass.Add = _TCollection_Add
+
     # Add Python lists methods
     klass.append = klass.Add
     klass.remove = _remove_pyz

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tgraph.py

@@ -181,6 +181,9 @@
 
 import cppyy
 
+from . import pythonization
+
+
 def set_size(self, buf):
     # Parameters:
     # - self: graph object
@@ -213,3 +216,26 @@ def set_size(self, buf):
 # Add the composite to the list of pythonizors
 cppyy.py.add_pythonization(comp)
 
+def _TMultiGraph_Add(self, *args, **kwargs):
+    """
+    The TMultiGraph takes ownership of the added graphs, unless we're using the
+    Add(TMultiGraph*) overload, in which cases it adopts the graphs inside the
+    other TMultiGraph.
+    """
+    from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+    def condition(gr):
+        import ROOT
+
+        return gr and not isinstance(gr, ROOT.TMultiGraph)
+
+    declare_cpp_owned_arg(0, "graph", args, kwargs, condition=condition)
+
+    self._Add(*args, **kwargs)
+
+
+@pythonization("TMultiGraph")
+def pythonize_tmultigraph(klass):
+    # Pythonizations for TMultiGraph::Add
+    klass._Add = klass.Add
+    klass.Add = _TMultiGraph_Add

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tseqcollection.py

@@ -200,7 +200,10 @@ def _reverse_pyz(self):
         return
 
     t = tuple(self)
+    was_owner = self.IsOwner()
+    self.SetOwner(False)
     self.Clear()
+    self.SetOwner(was_owner)
     for elem in t:
         self.AddAt(elem, 0)
 
@@ -222,7 +225,10 @@ def _sort_pyz(self, *args, **kwargs):
         # Sort in a Python list copy
         pylist = list(self)
         pylist.sort(*args, **kwargs)
+        was_owner = self.IsOwner()
+        self.SetOwner(False)
         self.Clear()
+        self.SetOwner(was_owner)
         self.extend(pylist)
 
 
@@ -241,11 +247,30 @@ def _index_pyz(self, val):
     return idx
 
 
+def _TSeqCollection_AddAt(self, *args, **kwargs):
+    from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+    def condition(_):
+        return self.IsOwner()
+
+    declare_cpp_owned_arg(0, "obj", args, kwargs, condition=condition)
+
+    self._AddAt(*args, **kwargs)
+
+
 @pythonization('TSeqCollection')
 def pythonize_tseqcollection(klass):
+    from ROOT._pythonization._tcollection import _TCollection_Add
+
     # Parameters:
     # klass: class to be pythonized
 
+    # Pythonize Add() methods
+    klass._Add = klass.Add
+    klass.Add = _TCollection_Add
+    klass._AddAt = klass.AddAt
+    klass.AddAt = _TSeqCollection_AddAt
+
     # Item access methods
     klass.__getitem__ = _getitem_pyz
     klass.__setitem__ = _setitem_pyz
@@ -257,3 +282,17 @@ def pythonize_tseqcollection(klass):
     klass.reverse = _reverse_pyz
     klass.sort    = _sort_pyz
     klass.index   = _index_pyz
+
+
+@pythonization("TList")
+def pythonize_tlist(klass):
+    from ROOT._pythonization._tcollection import _TCollection_Add
+
+    # Parameters:
+    # klass: class to be pythonized
+
+    # Pythonize Add() methods
+    klass._Add = klass.Add
+    klass.Add = _TCollection_Add
+    klass._AddAt = klass.AddAt
+    klass.AddAt = _TSeqCollection_AddAt

bindings/pyroot/pythonizations/test/tcollection_listmethods.py

@@ -14,10 +14,9 @@ class TCollectionListMethods(unittest.TestCase):
     # Helpers
     def create_tcollection(self):
         c = ROOT.TList()
+        c.SetOwner(True)
         for _ in range(self.num_elems):
             o = ROOT.TObject()
-            # Prevent immediate deletion of C++ TObjects
-            ROOT.SetOwnership(o, False)
             c.Add(o)
 
         return c
@@ -76,6 +75,7 @@ def test_extend(self):
         len1 = c1.GetEntries()
         len2 = c2.GetEntries()
 
+        c2.SetOwner(False)
         c1.extend(c2)
 
         len1_final = c1.GetEntries()
@@ -105,6 +105,8 @@ def test_count(self):
         self.assertEqual(c.count(o1), 2)
         self.assertEqual(c.count(o2), 1)
 
+        c.Clear()
+
 
 if __name__ == '__main__':
     unittest.main()