BUG: SWIG typechecks reject foreign wrapped types#5310
BUG: SWIG typechecks reject foreign wrapped types#5310hjmjohnson merged 1 commit intoInsightSoftwareConsortium:mainfrom
Conversation
d3f1bd0 to
f14f7d7
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a new Python test file to verify the behavior of LinearImageFunction’s various IsInsideBuffer overloads as wrapped by ITK.
- Introduces a test script that exercises list, continuous index, and point-based inputs to IsInsideBuffer.
- Ensures that the Python wrapping for LinearImageFunction works as intended.
Files not reviewed (1)
- Modules/Core/ImageFunction/wrapping/test/CMakeLists.txt: Language not supported
|
On the CI, the test fails with a different error message: |
|
Found the root cause and pushed a fix to this branch ( The bug isn't specific to The fix tightens those typechecks so they (a) accept native pointer matches, (b) reject SWIG wrappers of unrelated types via |
…reConsortium#5310, Discourse #7495) The Python overload dispatcher generated by SWIG resolves overloaded methods by running each overload's `typecheck` typemap in order and selecting the first that returns `_v=1`. The fixed-length and variable-length sequence typecheck typemaps in `pyBase.i` claimed viability whenever the input passed `PySequence_Check` and had the right length: if (SWIG_ConvertPtr(...) == -1 && (!PySequence_Check($input) || PyObject_Length($input) != dim) && !PyInt_Check($input) && !PyFloat_Check($input)) _v = 0; else _v = 1; But every wrapped ITK fixed-length type (`itk::Index`, `itk::ContinuousIndex`, `itk::Point`, `itk::Vector`, ...) exposes `__getitem__` and `__len__` for ergonomic use from Python — which makes `PySequence_Check` succeed and `PyObject_Length` return the fixed dimension. As a result, every typecheck claimed viability for every wrapped fixed-length input of the right length, regardless of the actual C++ type, and overload dispatch effectively reduced to declaration order. Concretely, `itk::ImageFunction::IsInsideBuffer` has three overloads (`IndexType`, `ContinuousIndexType`, `PointType`). Calling interp.IsInsideBuffer(itk.ContinuousIndex[itk.D, 3]()) or interp.IsInsideBuffer(itk.Point[itk.D, 3]()) both reached the `Index` overload first and failed with ValueError: Expecting a sequence of int (or long) because the wrapped `ContinuousIndex`/`Point` instance was passed into the integer-only `Index` typemap (its `__getitem__` returned floats, which the `in` typemap rejected). The other two overloads were never considered. Fix the four affected typecheck typemaps in `DECL_PYTHON_VEC_TYPEMAP` (`Vector`/`CovariantVector`/`Point`/ `ContinuousIndex`/`FixedArray`), `DECL_PYTHON_SEQ_TYPEMAP` (`Index`/`Size`/`Offset`), and `DECL_PYTHON_VARLEN_SEQ_TYPEMAP` (`Array`/`RGBPixel`/`RGBAPixel`): 1. If `SWIG_ConvertPtr` succeeds for our descriptor, accept (this is the strong, type-correct match). 2. Otherwise, if `SWIG_Python_GetSwigThis` returns non-NULL the input is a SWIG-wrapped instance of an *unrelated* type — reject, so the dispatcher can keep looking for the right overload. 3. Otherwise, fall through to the cheap scalar / sequence-length checks as before. For the integer-only `SEQ` typemap we additionally peek at element 0 and reject sequences whose first element is not a Python int / long, so a `[1.5, 2.5, 3.5]` list no longer claims viability for `Index`. After this change all four documented `IsInsideBuffer` call patterns resolve to the intended overload (verified with the new `itkLinearImageFunctionWrappingTest` regression test). Closes InsightSoftwareConsortium#5310. Co-Authored-By: Hans Johnson <hans-johnson@uiowa.edu>
…sortium#5310) The Python overload dispatcher generated by SWIG resolves overloaded methods by running each overload's `typecheck` typemap in order and selecting the first that returns `_v=1`. The fixed-length and variable-length sequence typecheck typemaps in `pyBase.i` claimed viability whenever the input passed `PySequence_Check` and had the right length: if (SWIG_ConvertPtr(...) == -1 && (!PySequence_Check($input) || PyObject_Length($input) != dim) && !PyInt_Check($input) && !PyFloat_Check($input)) _v = 0; else _v = 1; But every wrapped ITK fixed-length type (`itk::Index`, `itk::ContinuousIndex`, `itk::Point`, `itk::Vector`, ...) exposes `__getitem__` and `__len__` for ergonomic use from Python — which makes `PySequence_Check` succeed and `PyObject_Length` return the fixed dimension. As a result, every typecheck claimed viability for every wrapped fixed-length input of the right length, regardless of the actual C++ type, and overload dispatch effectively reduced to declaration order. Concretely, `itk::ImageFunction::IsInsideBuffer` has three overloads (`IndexType`, `ContinuousIndexType`, `PointType`). Calling interp.IsInsideBuffer(itk.ContinuousIndex[itk.D, 3]()) or interp.IsInsideBuffer(itk.Point[itk.D, 3]()) both reached the `Index` overload first and failed with ValueError: Expecting a sequence of int (or long) because the wrapped `ContinuousIndex`/`Point` instance was passed into the integer-only `Index` typemap (its `__getitem__` returned floats, which the `in` typemap rejected). The other two overloads were never considered. Fix the four affected typecheck typemaps in `DECL_PYTHON_VEC_TYPEMAP` (`Vector`/`CovariantVector`/`Point`/ `ContinuousIndex`/`FixedArray`), `DECL_PYTHON_SEQ_TYPEMAP` (`Index`/`Size`/`Offset`), and `DECL_PYTHON_VARLEN_SEQ_TYPEMAP` (`Array`/`RGBPixel`/`RGBAPixel`): 1. If `SWIG_ConvertPtr` succeeds for our descriptor, accept (this is the strong, type-correct match). 2. Otherwise, if `SWIG_Python_GetSwigThis` returns non-NULL the input is a SWIG-wrapped instance of an *unrelated* type — reject, so the dispatcher can keep looking for the right overload. 3. Otherwise, fall through to the cheap scalar / sequence-length checks as before. For the integer-only `SEQ` typemap we additionally peek at element 0 and reject sequences whose first element is not a Python int / long, so a `[1.5, 2.5, 3.5]` list no longer claims viability for `Index`. After this change all four documented `IsInsideBuffer` call patterns resolve to the intended overload (verified with the new `itkLinearImageFunctionWrappingTest` regression test). Closes InsightSoftwareConsortium#5310. Co-Authored-By: Hans Johnson <hans-johnson@uiowa.edu>
…sortium#5310) The Python overload dispatcher generated by SWIG resolves overloaded methods by running each overload's `typecheck` typemap in order and selecting the first that returns `_v=1`. The fixed-length and variable-length sequence typecheck typemaps in `pyBase.i` claimed viability whenever the input passed `PySequence_Check` and had the right length: if (SWIG_ConvertPtr(...) == -1 && (!PySequence_Check($input) || PyObject_Length($input) != dim) && !PyInt_Check($input) && !PyFloat_Check($input)) _v = 0; else _v = 1; But every wrapped ITK fixed-length type (`itk::Index`, `itk::ContinuousIndex`, `itk::Point`, `itk::Vector`, ...) exposes `__getitem__` and `__len__` for ergonomic use from Python — which makes `PySequence_Check` succeed and `PyObject_Length` return the fixed dimension. As a result, every typecheck claimed viability for every wrapped fixed-length input of the right length, regardless of the actual C++ type, and overload dispatch effectively reduced to declaration order. Concretely, `itk::ImageFunction::IsInsideBuffer` has three overloads (`IndexType`, `ContinuousIndexType`, `PointType`). Calling interp.IsInsideBuffer(itk.ContinuousIndex[itk.D, 3]()) or interp.IsInsideBuffer(itk.Point[itk.D, 3]()) both reached the `Index` overload first and failed with ValueError: Expecting a sequence of int (or long) because the wrapped `ContinuousIndex`/`Point` instance was passed into the integer-only `Index` typemap (its `__getitem__` returned floats, which the `in` typemap rejected). The other two overloads were never considered. Fix the four affected typecheck typemaps in `DECL_PYTHON_VEC_TYPEMAP` (`Vector`/`CovariantVector`/`Point`/ `ContinuousIndex`/`FixedArray`), `DECL_PYTHON_SEQ_TYPEMAP` (`Index`/`Size`/`Offset`), and `DECL_PYTHON_VARLEN_SEQ_TYPEMAP` (`Array`/`RGBPixel`/`RGBAPixel`): 1. If `SWIG_ConvertPtr` succeeds for our descriptor, accept (this is the strong, type-correct match). 2. Otherwise, if `SWIG_Python_GetSwigThis` returns non-NULL the input is a SWIG-wrapped instance of an *unrelated* type — reject, so the dispatcher can keep looking for the right overload. 3. Otherwise, fall through to the cheap scalar / sequence-length checks as before. For the integer-only `SEQ` typemap we additionally peek at element 0 and reject sequences whose first element is not a Python int / long, so a `[1.5, 2.5, 3.5]` list no longer claims viability for `Index`. After this change all four documented `IsInsideBuffer` call patterns resolve to the intended overload (verified with the new `itkLinearImageFunctionWrappingTest` regression test). Closes InsightSoftwareConsortium#5310. Co-Authored-By: Hans Johnson <hans-johnson@uiowa.edu>
|
| Filename | Overview |
|---|---|
| Wrapping/Generators/Python/PyBase/pyBase.i | Fixes SWIG typecheck typemaps in four macros to reject unrelated wrapped types via SWIG_Python_GetSwigThis and adds element-type peeking for integer-only (Index) typechecks; logic is correct and consistent across all four affected macros. |
| Modules/Core/ImageFunction/wrapping/test/itkLinearImageFunctionWrappingTest.py | New regression test covering all four IsInsideBuffer overload paths (raw int list, ContinuousIndex, Point, raw float list) plus out-of-buffer False cases; follows established ITK Python test patterns. |
| Modules/Core/ImageFunction/wrapping/test/CMakeLists.txt | Adds itk_python_add_test registration for the new test script; follows existing patterns in this file. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Python calls IsInsideBuffer with any input] --> B{SWIG_ConvertPtr\nsucceeds for this type?}
B -- yes --> C[_v = 1\nOverload candidate]
B -- no --> D[PyErr_Clear]
D --> E{SWIG_Python_GetSwigThis\nnot NULL?\nUnrelated wrapped type}
E -- yes --> F[_v = 0\nReject candidate]
E -- no --> G{For VEC typemap:\nPyInt/Float_Check?}
G -- yes --> H[_v = 1\nOverload candidate]
G -- no --> I{PySequence_Check\nAND length == dim?}
I -- no --> F2[_v = 0\nReject candidate]
I -- yes --> J{INT_INDEX typemap only:\nPeek element 0\nPyInt or PyLong?}
J -- yes --> K[_v = 1\nOverload candidate]
J -- no --> F3[_v = 0\nReject candidate]
Reviews (1): Last reviewed commit: "BUG: SWIG typechecks reject foreign wrap..." | Re-trigger Greptile
dzenanz
left a comment
There was a problem hiding this comment.
Looks good on a glance. I can't approve my own PR.
…sortium#5310) The Python overload dispatcher generated by SWIG resolves overloaded methods by running each overload's `typecheck` typemap in order and selecting the first that returns `_v=1`. The fixed-length and variable-length sequence typecheck typemaps in `pyBase.i` claimed viability whenever the input passed `PySequence_Check` and had the right length: if (SWIG_ConvertPtr(...) == -1 && (!PySequence_Check($input) || PyObject_Length($input) != dim) && !PyInt_Check($input) && !PyFloat_Check($input)) _v = 0; else _v = 1; But every wrapped ITK fixed-length type (`itk::Index`, `itk::ContinuousIndex`, `itk::Point`, `itk::Vector`, ...) exposes `__getitem__` and `__len__` for ergonomic use from Python — which makes `PySequence_Check` succeed and `PyObject_Length` return the fixed dimension. As a result, every typecheck claimed viability for every wrapped fixed-length input of the right length, regardless of the actual C++ type, and overload dispatch effectively reduced to declaration order. Concretely, `itk::ImageFunction::IsInsideBuffer` has three overloads (`IndexType`, `ContinuousIndexType`, `PointType`). Calling interp.IsInsideBuffer(itk.ContinuousIndex[itk.D, 3]()) or interp.IsInsideBuffer(itk.Point[itk.D, 3]()) both reached the `Index` overload first and failed with ValueError: Expecting a sequence of int (or long) because the wrapped `ContinuousIndex`/`Point` instance was passed into the integer-only `Index` typemap (its `__getitem__` returned floats, which the `in` typemap rejected). The other two overloads were never considered. Fix the four affected typecheck typemaps in `DECL_PYTHON_VEC_TYPEMAP` (`Vector`/`CovariantVector`/`Point`/ `ContinuousIndex`/`FixedArray`), `DECL_PYTHON_SEQ_TYPEMAP` (`Index`/`Size`/`Offset`), and `DECL_PYTHON_VARLEN_SEQ_TYPEMAP` (`Array`/`RGBPixel`/`RGBAPixel`): 1. If `SWIG_ConvertPtr` succeeds for our descriptor, accept (this is the strong, type-correct match). 2. Otherwise, if `SWIG_Python_GetSwigThis` returns non-NULL the input is a SWIG-wrapped instance of an *unrelated* type — reject, so the dispatcher can keep looking for the right overload. 3. Otherwise, fall through to the cheap scalar / sequence-length checks as before. For the integer-only `SEQ` typemap we additionally peek at element 0 and reject sequences whose first element is not a Python int / long, so a `[1.5, 2.5, 3.5]` list no longer claims viability for `Index`. After this change all four documented `IsInsideBuffer` call patterns resolve to the intended overload (verified with the new `itkLinearImageFunctionWrappingTest` regression test). Closes InsightSoftwareConsortium#5310. Co-Authored-By: Hans Johnson <hans-johnson@uiowa.edu>
|
I'm addressing the greptile with a minor test addition. |
…sortium#5310) The Python overload dispatcher generated by SWIG resolves overloaded methods by running each overload's `typecheck` typemap in order and selecting the first that returns `_v=1`. The fixed-length and variable-length sequence typecheck typemaps in `pyBase.i` claimed viability whenever the input passed `PySequence_Check` and had the right length: if (SWIG_ConvertPtr(...) == -1 && (!PySequence_Check($input) || PyObject_Length($input) != dim) && !PyInt_Check($input) && !PyFloat_Check($input)) _v = 0; else _v = 1; But every wrapped ITK fixed-length type (`itk::Index`, `itk::ContinuousIndex`, `itk::Point`, `itk::Vector`, ...) exposes `__getitem__` and `__len__` for ergonomic use from Python — which makes `PySequence_Check` succeed and `PyObject_Length` return the fixed dimension. As a result, every typecheck claimed viability for every wrapped fixed-length input of the right length, regardless of the actual C++ type, and overload dispatch effectively reduced to declaration order. Concretely, `itk::ImageFunction::IsInsideBuffer` has three overloads (`IndexType`, `ContinuousIndexType`, `PointType`). Calling interp.IsInsideBuffer(itk.ContinuousIndex[itk.D, 3]()) or interp.IsInsideBuffer(itk.Point[itk.D, 3]()) both reached the `Index` overload first and failed with ValueError: Expecting a sequence of int (or long) because the wrapped `ContinuousIndex`/`Point` instance was passed into the integer-only `Index` typemap (its `__getitem__` returned floats, which the `in` typemap rejected). The other two overloads were never considered. Fix the four affected typecheck typemaps in `DECL_PYTHON_VEC_TYPEMAP` (`Vector`/`CovariantVector`/`Point`/ `ContinuousIndex`/`FixedArray`), `DECL_PYTHON_SEQ_TYPEMAP` (`Index`/`Size`/`Offset`), and `DECL_PYTHON_VARLEN_SEQ_TYPEMAP` (`Array`/`RGBPixel`/`RGBAPixel`): 1. If `SWIG_ConvertPtr` succeeds for our descriptor, accept (this is the strong, type-correct match). 2. Otherwise, if `SWIG_Python_GetSwigThis` returns non-NULL the input is a SWIG-wrapped instance of an *unrelated* type — reject, so the dispatcher can keep looking for the right overload. 3. Otherwise, fall through to the cheap scalar / sequence-length checks as before. For the integer-only `SEQ` typemap we additionally peek at element 0 and reject sequences whose first element is not a Python int / long, so a `[1.5, 2.5, 3.5]` list no longer claims viability for `Index`. After this change all four documented `IsInsideBuffer` call patterns resolve to the intended overload (verified with the new `itkLinearImageFunctionWrappingTest` regression test). Closes InsightSoftwareConsortium#5310. Co-Authored-By: Hans Johnson <hans-johnson@uiowa.edu>
…deBuffer BUG: SWIG typechecks reject foreign wrapped types
Based on code provided on the forum: https://discourse.itk.org/t/improper-wrapping-of-imagefunction-isinsidebuffer/7495
PR Checklist