[Python] Set memory policy to "strict"

README/ReleaseNotes/v640/index.md

@@ -39,7 +39,7 @@ The following people have contributed to this new version:
 ## Removals
 
 * The `TH1K` class was removed. `TMath::KNNDensity` can be used in its stead.
-* The `TObject` equality operator pythonization (`TObject.__eq__`) that was deprecated in ROOT 6.38 and scheduled for removal in ROOT 6.40 is removed.
+* The `TObject` equality operator Pythonization (`TObject.__eq__`) that was deprecated in ROOT 6.38 and scheduled for removal in ROOT 6.40 is removed.
 * Comparing C++ `nullptr` objects with `None` in Python now raises a `TypeError`, as announced in the ROOT 6.38 release notes. Use truth-value checks like `if not x` or `x is None` instead.
 * The `TGLIncludes.h` and `TGLWSIncludes.h` that were deprecated in ROOT 6.38 and scheduled for removal are gone now. Please include your required headers like `<GL/gl.h>` or `<GL/glu.h>` directly.
 * The GLEW headers (`GL/eglew.h`, `GL/glew.h`, `GL/glxew.h`, and `GL/wglew.h`) that were installed when building ROOT with `builtin_glew=ON` are no longer installed. This is done because ROOT is moving away from GLEW for loading OpenGL extensions.
@@ -122,6 +122,55 @@ 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)`,
+calling such method on a Python object `my_instance` of type `MyClass`
+would assume that the memory ownership of `obj` transfers 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.
+
+Now, the Python interface of ROOT will not make this assumption anymore.
+Because of this change, some double-deletes or dangling references on the C++
+side might creep up in your scripts. These need to be fixed by properly
+managing object lifetime.
+
+A dangling references on the C++ side is a reference or pointer that refers to
+an object that the Python side has already deleted.
+Possible remedies:
+
+  1. Assigning the object to a Python variable that lives at least as long as
+     the C++ reference to keep the pointed-to object alive
+  2. If the C++ reference comes from a non-owning collection, like a
+     default-constructed **TCollection** (e.g. a **TList**), you can also transfer
+     the ownership to the C++ side explicitly by calling `coll.SetOwner(True)`
+     or migrate to another owning collection type like
+     `std::vector<unique_ptr<T>>`. In the general case of owning collections,
+     you will have to relinquish ownership on the Python side with
+     `ROOT.SetOwnership(obj, False)`.
+
+**Note:** **TCollection**-derived classes are Pythonized such that when an
+object is added to an owning collection with `TCollection::Add()`, the Python
+ownership is still dropped automatically. If you see other places where ROOT
+can benefit from this Pythonization, please report them in a GitHub issue. You
+can also [Pythonize classes](https://root.cern/doc/master/group__Pythonizations.html) from outside ROOT if needed.
+
+The double-delete problems can be fixed in a similar ways, but this time it's
+not necessary to make sure that the object is owned by C++. It there was a
+double-delete, that means the object was owned by C++ already. So the possible
+solutions are:
+
+  1. Dropping the ownership on the Python side with `ROOT.SetOwnership(obj, False)`
+  3. Pythonizing the member function that drops the ownership on the Python
+     side, like the **TCollection** Pythonization explained above
+
+**Important:** You can change back to the old policy by calling
+`ROOT.SetHeuristicMemoryPolicy(True)` after importing ROOT, but this should be
+only used for debugging purposes and this function might be **removed in ROOT
+6.42**!
+
+
 ## 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

@@ -77,7 +77,6 @@ def __init__(self, module, is_ipython):
             "bind_object",
             "as_cobject",
             "addressof",
-            "SetHeuristicMemoryPolicy",
             "SetImplicitSmartPointerConversion",
             "SetOwnership",
         ]
@@ -103,6 +102,19 @@ def __init__(self, module, is_ipython):
         self.__class__.__getattr__ = self._getattr
         self.__class__.__setattr__ = self._setattr
 
+    def SetHeuristicMemoryPolicy(self, enabled):
+        import textwrap
+        import warnings
+
+        msg = """ROOT.SetHeuristicMemoryPolicy() is deprecated and will be removed in ROOT 6.42.
+
+        Since ROOT 6.40, the heuristic memory policy is disabled by default, and with
+        ROOT 6.42 it won't be possible to re-enable it with ROOT.SetHeuristicMemoryPolicy(),
+        which was only meant to be used during a transition period and will be removed.
+        """
+        warnings.warn(textwrap.dedent(msg), FutureWarning, stacklevel=0)
+        return cppyy._backend.SetHeuristicMemoryPolicy(enabled)
+
     def AddressOf(self, obj):
         # Return an indexable buffer of length 1, whose only element
         # is the address of the object.
@@ -192,11 +204,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,28 @@ 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 function 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
+
+    # 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]
+    else:
+        # This can happen if the function was called with too few arguments.
+        # In that case we should not do anything.
+        return
+
+    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,19 @@ 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 always takes ownership of the added graphs.
+    """
+    from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+    declare_cpp_owned_arg(0, "graph", args, kwargs)
+
+    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()