BUG: SWIG typechecks must reject foreign wrapped types (fixes IsInsideBuffer, #5310)#6190
Closed
hjmjohnson wants to merge 1 commit intoInsightSoftwareConsortium:mainfrom
Closed
Conversation
…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>
4 tasks
Member
Author
|
Closing — content moved into #5310 (force-pushed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix for Discourse #7495 and supersedes #5310. SWIG's overload dispatcher was always selecting the integer-only
Indexoverload ofImageFunction::IsInsideBufferbecause the typecheck typemaps inpyBase.iclaimed viability for any object that satisfiedPySequence_Check + length == dim— including wrappedContinuousIndex/Pointinstances (they expose__getitem__/__len__).Root cause walk-through
itk::ImageFunction::IsInsideBufferhas three overloads (IndexType,ContinuousIndexType,PointType). The SWIG-generated dispatcher tries them in declaration order:For
interp.IsInsideBuffer(itk.ContinuousIndex[itk.D, 3]()):SWIG_ConvertPtrtoitkIndex3returns-1(different SWIG type).PySequence_CheckreturnsTrue(wrapped fixed-length classes expose__getitem__/__len__).PyObject_Lengthreturns3.⇒
_v = 1, the dispatcher commits to theIndexoverload, then theintypemap demands integer elements and emitsThe
ContinuousIndexandPointoverloads were never reached. The same logic brokeinterp.IsInsideBuffer(itk.Point[itk.D, 3]())andinterp.IsInsideBuffer([1.5, 2.5, 3.5]).Fix
Tighten the four affected typecheck typemaps in
Wrapping/Generators/Python/PyBase/pyBase.i:SWIG_ConvertPtrsucceeds for our descriptor → accept (strong type-correct match).SWIG_Python_GetSwigThisreturns non-NULL, the input is a SWIG-wrapped instance of an unrelated type → reject, so the dispatcher keeps looking for the right overload.SEQtypemap (Index/Size/Offset), additionally peek at element 0 and reject sequences whose first element is not a Python int — so[1.5, 2.5, 3.5]no longer claims viability for theIndexoverload.This applies to:
DECL_PYTHON_VEC_TYPEMAP—Vector,CovariantVector,Point,ContinuousIndex,FixedArrayDECL_PYTHON_SEQ_TYPEMAP—Index,Size,OffsetDECL_PYTHON_VARLEN_SEQ_TYPEMAP—Array,RGBPixel,RGBAPixelVerification (hand-traced before the fix builds)
After the fix, the dispatcher resolves each test case like this:
IndextypecheckContinuousIndextypecheckPointtypecheck (fallback)[1, 2, 3](list of ints)Indexitk.ContinuousIndexinstanceGetSwigThisnon-NULL, foreign type)ConvertPtrsucceeds)ContinuousIndexitk.PointinstancePoint[1.5, 2.5, 3.5](list of floats)ContinuousIndexTest
itkLinearImageFunctionWrappingTest.pyasserts each of these calls returnsTruefor in-buffer coordinates andFalsefor out-of-buffer coordinates.Closes #5310.