Resolve an object lifetime bug in add_polyline

[eivindjahren]

No description provided.

[Copilot]

Pull request overview

This PR resolves a lifetime/ownership issue when adding polylines between collections by moving the underlying “polyline/polygon” implementation to a C++ rd::Polygon type stored and shared via std::shared_ptr, and by updating both C++ and Python bindings/callers to use that shared ownership model.

Changes:

• Replace the legacy C-style geo_polygon_type API with a C++ rd::Polygon class and propagate that through region/grid selection and polygon collections.
• Switch Python CPolyline from a pure-Python cwrap wrapper to a dedicated pybind11 extension module (resdata.geometry.cpolyline) plus a .pyi stub.
• Simplify CPolylineCollection.addPolyline()/shallowCopy() to rely on shared ownership instead of reference/wrapper mechanics.

Reviewed changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tests/geometry_tests/test_cpolyline_collection.py	Renames/adjusts a collection-to-collection add test for the new ownership model.
python/resdata/geometry/cpolyline.pyi	Adds typing stubs for the new CPolyline extension module.
python/resdata/geometry/cpolyline.py	Removes the old pure-Python CPolyline wrapper implementation.
python/resdata/geometry/cpolyline_collection.py	Updates collection add/copy logic to use the new _add_polyline signature and shared ownership.
lib/tests/test_geometry.cpp	Migrates polygon tests to use rd::Polygon APIs instead of legacy C functions.
lib/resdata/rd_region.cpp	Updates region polygon selection internals to accept/use rd::Polygon.
lib/resdata/rd_region_pybind.cpp	Updates Python bindings for region polygon selection to take rd::Polygon.
lib/resdata/rd_grid.cpp	Updates XY-in-cell helpers to build/query rd::Polygon instead of C polygons.
lib/resdata/fault_block.cpp	Updates fault block polyline intersection checks to use rd::Polygon.
lib/resdata/cwrap_pybind.cpp	Removes CPolyline-specific from_cwrap<geo_polygon_type> conversion now that the C type is removed.
lib/private-include/detail/resdata/cwrap_pybind.hpp	Removes the CPolyline() helper declaration.
lib/include/resdata/rd_region.hpp	Updates the public C API signatures to take rd::Polygon.
lib/include/ert/geometry/geo_region.hpp	Updates geo_region polygon selection signatures to take rd::Polygon.
lib/include/ert/geometry/geo_polygon.hpp	Replaces the old C polygon API declarations with the rd::Polygon class interface.
lib/include/ert/geometry/geo_polygon_collection.hpp	Updates polygon collection APIs to store/return std::shared_ptr<rd::Polygon>.
lib/geometry/geo_region.cpp	Updates region selection implementation to call rd::Polygon::contains_point.
lib/geometry/geo_region_pybind.cpp	Updates geo_region Python bindings to accept rd::Polygon.
lib/geometry/geo_polygon.cpp	Ports polygon algorithms (contains/intersects/length/load) onto rd::Polygon.
lib/geometry/geo_polygon_pybind.cpp	Introduces the new cpolyline pybind module exposing CPolyline backed by rd::Polygon.
lib/geometry/geo_polygon_collection.cpp	Refactors polygon collection storage to std::vector + std::map of shared_ptrs.
lib/geometry/geo_polygon_collection_pybind.cpp	Updates polyline-collection bindings to return/store shared_ptr polygons directly.
lib/CMakeLists.txt	Renames the built pybind module from _cpolyline to cpolyline.

Suppressed comments (3)

lib/geometry/geo_polygon_pybind.cpp:81

• __getitem__ uses an undefined variable (i) for bounds checking and indexing, which will fail to compile and also breaks negative-index handling. Convert the Python index to a signed integer, validate against the container size, and then index the C++ vector using a size_t cast.

                 if (index < py::int_{0} || i >= size)
                     throw py::index_error(fmt::format(
                         "Invalid index:{} valid range: [0,{})", i, size));

                 return self[i];

lib/geometry/geo_polygon_pybind.cpp:132

• The py::class_ definition chain is accidentally terminated after __radd__ (});), so the following .def(...) calls are no longer attached to any object and will not compile. Keep the chain alive here so later .def(...) calls remain on the same py::class_ expression.

        });
    .def(py::self == py::self)
        .def("segmentLength",

lib/geometry/geo_polygon_pybind.cpp:222

• connect() previously returned a Python list of two points, but the new binding returns a tuple via py::make_tuple(...). That is a user-visible API change and also disagrees with the new stub (cpolyline.pyi declares connect(...) -> list[tuple[float, float]]). Return a py::list (and also terminate the .def(...) chain with a semicolon).

            if (d1.cast<double>() < d2.cast<double>())
                return py::make_tuple(end1_tup, p1);
            else
                return py::make_tuple(end2_tup, p2);
        })

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.

Suppressed comments (3)

lib/resdata/rd_region_pybind.cpp:374

• The keyword argument name for the polygon parameter is set to region, which is misleading and breaks keyword-based calls. It should be polygon (matching the lambda parameter).

            rd_region_select_outside_polygon(from_cwrap<rd_region_type>(self),
                                             polygon);
        },
        py::arg("self"), py::arg("region").none(false));

lib/resdata/rd_region_pybind.cpp:381

• The keyword argument name for the polygon parameter is set to region, which is misleading and breaks keyword-based calls. It should be polygon (matching the lambda parameter).

            rd_region_deselect_inside_polygon(from_cwrap<rd_region_type>(self),
                                              polygon);
        },
        py::arg("self"), py::arg("region").none(false));

lib/resdata/rd_region_pybind.cpp:388

• The keyword argument name for the polygon parameter is set to region, which is misleading and breaks keyword-based calls. It should be polygon (matching the lambda parameter).

            rd_region_deselect_outside_polygon(from_cwrap<rd_region_type>(self),
                                               polygon);
        },
        py::arg("self"), py::arg("region").none(false));

[Copilot]

Pull request overview

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

Suppressed comments (2)

lib/geometry/geo_polygon_collection.cpp:82

• polygon_list.at() takes a size_t index; passing an int relies on an implicit signed-to-unsigned conversion that can trigger sign-conversion/narrowing diagnostics. Casting explicitly (and/or validating index >= 0) avoids new warnings and makes the intent clear.

    return polygons->polygon_list.at(index);

tests/geometry_tests/test_cpolyline_collection.py:167

• This test exercises cross-collection addPolyline(), but it doesn’t assert the lifetime behavior implied by the PR title (i.e., that the polyline remains valid after the source collection and temporary references are released). Adding an explicit del/gc.collect() and then accessing the polyline from target would guard against regressions of the original use-after-free.

    target = CPolylineCollection()
    target.addPolyline(reference)

    assert "border" in target
    assert len(target) == 1

[ajaust]

Nice work. I left some comments.

[ajaust]

Typo? The commit message refers to "safe case", but we remove a "save cast".

[ajaust]

If length() is called on an empty Polygon we access the points vector even though it is empty.

rd::Polygon polygon;
std::cout << polygon.length() << std::endl; // Out-of-bounds access to points[0], but points.size() is 0

[ajaust]

Can we add any tests that would trigger the lifetime bug to ensure that it is not introduced again in the future?

[ajaust]

Why can't we match against the error message anymore?