This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Previously-working SWIG code fails in Python 3.6
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.6
process
Status: closed Resolution: third party
Dependencies: Superseder:
Assigned To: Nosy List: Tristan Croll, brett.cannon, serhiy.storchaka, vstinner
Priority: normal Keywords:

Created on 2017-03-08 16:47 by Tristan Croll, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
cell.h Tristan Croll, 2017-03-08 16:47
Messages (12)
msg289245 - (view) Author: Tristan Croll (Tristan Croll) Date: 2017-03-08 16:47
Possibly related to http://bugs.python.org/issue29327 - yields the same error message:

Objects/tupleobject.c:81: bad argument to internal function

I have a large SWIG project which was previously working well in Python 3.5. After migrating to Python 3.6.0, I find I can still create any wrapped object from Python via its constructor(s), but any internal function that returns certain objects fails with the above message. I have so far been unable to find any distinction between classes that do and don't return successfully. Take the below (and attached) headers, for example. Functions that return Spacegroup objects work, as do those that return Metric_tensor objects from the attached cell.h. On the other hand, functions returning Cell or Cell_descr objects fail with the above message. Yet in all cases I can successfully call the objects' constructors. Not ashamed to say I'm a bit lost here.


#ifndef CLIPPER_SPACEGROUP
#define CLIPPER_SPACEGROUP


#include "symop.h"
#include "spacegroup_data.h"


namespace clipper {

  // forward definitions
  class HKL;
  class HKL_class;
  class Coord_frac;


  //! spacegroup description
  /*! The spacegroup description is a compact description of a
    spacegroup. It may be initialised from Hall or H-M symbols, a
    string of symops or a number. Internally a hash code is used to
    refer to the spacegroup, so this object is only 32 bits in
    size.

    For more details of spacegroup symbols, see Sydney R. Hall & Ralf
    W. Grosse-Kunstleve 'Concise Space-Group Symbols',
    http://www.kristall.ethz.ch/LFK/software/sginfo/hall_symbols.html
  */
  class Spgr_descr
  {
  public:
    enum TYPE { Hall, HM, XHM, Symops, Number, Unknown };
    //! null constructor
    Spgr_descr();
    //! constructor: from symbol or operators.
    explicit Spgr_descr( const String& symb, TYPE type = Unknown );
    //! constructor: from number.
    explicit Spgr_descr( const int& num );
    //! return the spacegroup number
    int spacegroup_number() const;
    //! return the Hall symbol
    String symbol_hall() const;
    //! return the H-M symbol
    String symbol_hm() const;
    //! return the extended H-M symbol
    String symbol_xhm() const;
    //! return the extension H-M symbol
    String symbol_hm_ext() const;
    //! set preferred default spacegroup choice
    static void set_preferred( const char& c );

    //! Vector of symop codes and associated methods
    class Symop_codes : public std::vector<Symop_code>
    {
    public:
      //! initialise from Hall symbol
      void init_hall( const String& symb );
      //! initialise from symops
      void init_symops( const String& symb );
      //! expand (incomplete) list of symops
      Symop_codes expand() const;
      //! return primitive non-inversion ops (by computation)
      Symop_codes primitive_noninversion_ops() const;
      //! return inversion ops (by computation)
      Symop_codes inversion_ops() const;
      //! return primitive incl inversion ops (by computation)
      Symop_codes primitive_ops() const;
      //! return lattice centering ops (by computation)
      Symop_codes centering_ops() const;
      //! return Laue ops
      Symop_codes laue_ops() const;
      //! return point group ops
      Symop_codes pgrp_ops() const;
      //! return Patterson ops
      Symop_codes patterson_ops() const;
      //! return minimal list of generator ops
      Symop_codes generator_ops() const;
      //! return product of this (expanded) list by another (expanded) list
      Symop_codes product( const Symop_codes& ops2 ) const;
      //! return hash code of symop list
      unsigned int hash() const;
    };

    //! constructor: from symop list.
    explicit Spgr_descr( const Symop_codes& ops );
    //! return the generators for the spacegroup
    const Symop_codes& generator_ops() const { return generators_; }
    //! return the hash code for the spacegroup \internal
    const unsigned int& hash() const { return hash_; }

  protected:
    unsigned int hash_;       //!< hash code of spacegroup
    Symop_codes generators_;  //!< codes for symop generators

    static char pref_12, pref_hr;  //!< preferred origin and hex/romb symbols
  };


  // ObjectCache data type
  class Spgr_cacheobj
  {
  public:
    typedef Spgr_descr Key;
    Spgr_cacheobj( const Key& spgr_cachekey );  //!< construct entry
    bool matches( const Key& spgr_cachekey ) const; //!< compare entry
    String format() const;  //!< string description
    // data
    Key spgr_cachekey_;                 //!< spacegroup cachekey
    int nsym, nsymn, nsymi, nsymc, nsymp;  //!< number of syms: total, primitive
    int lgrp;                           //!< Laue group number
    std::vector<Symop>  symops;         //!< symmetry operators
    std::vector<Isymop> isymops;        //!< symmetry operators
    Vec3<> asu_min_, asu_max_;          //!< real space ASU
    static Mutex mutex;                 //!< thread safety
  };


  //! Spacegroup object
  /*! The spacegroup object is a full description of a spacegroup,
    including all the most regularly used information in an efficient
    form. It may be initialised from a clipper::Spgr_descr. This
    object.

    For more details of spacegroup symbols, see Sydney R. Hall & Ralf
    W. Grosse-Kunstleve 'Concise Space-Group Symbols',
    http://www.kristall.ethz.ch/LFK/software/sginfo/hall_symbols.html
  */
  class Spacegroup : public Spgr_descr
  {
   public:
    //! enumeration for fast construction of Null or P1 spacegroup
    enum TYPE { Null, P1 };
    //! enumeration for cell axes
    enum AXIS { A=0, B=1, C=2 };
    //! null constructor
    Spacegroup() {};
    //! constructor: fast constructor for Null or P1 spacegroup
    explicit Spacegroup( TYPE type );
    //! constructor: from spacegroup description
    explicit Spacegroup( const Spgr_descr& spgr_descr );
    //! initialiser: from spacegroup description
    void init( const Spgr_descr& spgr_descr );

    //! test if object has been initialised
    bool is_null() const;

    // methods
    //! get spacegroup description
    inline const Spgr_descr& descr() const { return (*this); }
    //! get number of symops
    inline const int& num_symops() const { return nsym; }
    //! get number of primitive symops (identical to num_primitive_symops())
    inline const int& num_primops() const { return num_primitive_symops(); }
    //! get number of primitive symops (inc identity and inversion)
    inline const int& num_primitive_symops() const { return nsymp; }
    //! get number of centering symops (inc identity)
    inline const int& num_centering_symops() const { return nsymc; }
    //! get number of inversion symops (inc identity)
    inline const int& num_inversion_symops() const { return nsymi; }
    //! get number of primitive non-inversion symops (inc identity)
    inline const int& num_primitive_noninversion_symops() const { return nsymn;}
    //! get n'th symop
    inline const Symop& symop( const int& sym_no ) const
      { return symops[sym_no]; }
    //! get n'th primitive symop (identical to symop(sym_no))
    inline const Symop& primitive_symop( const int& sym_no ) const
      { return symops[sym_no]; }
    //! get n'th inversion symop (0...1 max)
    inline const Symop& inversion_symop( const int& sym_no ) const
      { return symops[nsymn*sym_no]; }
    //! get n'th centering symop (0...3 max)
    inline const Symop& centering_symop( const int& sym_no ) const
      { return symops[nsymp*sym_no]; }
    //! get the order of rotational symmetry about a given axis
    int order_of_symmetry_about_axis( const AXIS axis ) const;

    //! get 'class' of reflection: multiplicity, allowed phase, absence
    HKL_class hkl_class( const HKL& hkl ) const;
    //! test if hkl is in default reciprocal ASU
    bool recip_asu( const HKL& hkl ) const;

    //! get symop number corresponding to the product of two symops
    int product_op( const int& s1, int& s2 ) const;
    //! get symop number corresponding to the inverse of a symop
    int inverse_op( const int& s ) const;

    //! get map ASU, upper bound
    Coord_frac asu_max() const;
    //! get map ASU, lower bound
    Coord_frac asu_min() const;

    //! test if change of hand preserves spacegroup
    bool invariant_under_change_of_hand() const;

    // inherited functions listed for documentation purposes
    //-- int spacegroup_number() const;
    //-- String symbol_hall() const;
    //-- String symbol_hm() const;
    //! return the Laue group symbol
    String symbol_laue() const;

    //! Return P1 spacegroup
    static Spacegroup p1() { return Spacegroup( P1 ); }
    //! Return null spacegroup
    static Spacegroup null() { return Spacegroup( Null ); }

    void debug() const;

  private:
    ObjectCache<Spgr_cacheobj>::Reference cacheref;  //!< object cache reference
    const Symop* symops;    //!< fast access ptr
    const Isymop* isymops;  //!< fast access ptr
    data::ASUfn asufn;      //!< fast access ptr
    int nsym, nsymn, nsymi, nsymc, nsymp; //!< fast access copies
  };


} // namespace clipper

#endif
msg289280 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-09 09:15
Which function raises this exception? Add the traceback place. It may be a
bug in your code.
msg289281 - (view) Author: Tristan Croll (Tristan Croll) Date: 2017-03-09 10:10
OK, a further clue. First, a little more detail on how my project is arranged (to re-iterate, this works without complaint in Python 3.5):

Rather than use my SWIG output directly, I've created a further wrapper layer in Python to add functions/syntactic sugar that would be difficult to achieve in SWIG, add a bit of extra documentation, and bury some classes that may be occasionally useful but don't really need to be in the primary API. The SWIG output Python library I name clipper_core, and I use the following class decorators for classes in the front-end API to reduce the amount of code (Python and SWIG) needed:

def mappedclass(old_cls):
    '''
    Ensures that objects returned from functions in the clipper_core
    library are instantiated in Python as the derived class with the
    extra functionality. 
    '''
    def decorator(cls):
        def __newnew__(thiscls, *args, **kwargs):
            if thiscls == old_cls:
                return object.__new__(cls)
            return object.__new__(thiscls)
        old_cls.__new__ = __newnew__
        
        return cls
    return decorator
 
def getters_to_properties(*funcs):
    '''
    Class decorator. Add the names of any getter functions with void 
    arguments (e.g. Coord_grid.u()) to convert them to properties. If
    you want the property name to be different from the function name,
    add the desired name and the function name as a tuple 
    (e.g. ('uvw', '_get_uvw'))
    '''
    def property_factory(func):
        def getter(self):
            return getattr(super(self.__class__, self), func)()
        prop = property(getter)
        return prop

    def decorator(cls):
        for func in funcs:
            if type(func) == tuple:
                setattr(cls, func[0], property_factory(func[1]))
            else:
                setattr(cls, func, property_factory(func)) 
        return cls
    return decorator

def format_to_string(cls):
    '''
    Class decorator to redirect the Clipper format() function to __str__,
    to provide pretty printing of the object.
    '''
    def format(self):
        return super(self.__class__,self).format()
    def __str__(self):
        return self.format
    setattr(cls, 'format', property(format))
    setattr(cls, '__str__', __str__)
    return cls

Experimenting this morning with the following two classes:

@format_to_string    
@mappedclass(clipper_core.Cell_descr)
class Cell_descr(clipper_core.Cell_descr):
    def __init__(self, abc, angles):
        '''
        __init__(self, abc, angles) -> Cell_descr
        
        Args:
            abc ([float*3]): cell dimensions in Angstroms
            angles ([float*3]): alpha, beta and gamma angles in degrees
        '''
        clipper_core.Cell_descr.__init__(self, *abc, *angles)
    
@format_to_string
@getters_to_properties('cell_descr', 'matrix_frac', 'matrix_orth', 
                       'metric_real', 'metric_reci', 'volume')
@mappedclass(clipper_core.Cell)
class Cell(clipper_core.Cell):
    '''
    Define a crystallographic unit cell using the lengths of the three
    sides a, b, c (in Angstroms) and the three angles alpha, beta, gamma
    (in degrees). 
    '''
    def __init__(self, abc, angles):
        '''
        __init__(self, abc, angles) -> Cell
        
        Args:
            abc ([float*3]): cell dimensions in Angstroms
            angles ([float*3]): alpha, beta and gamma angles in degrees
        '''
        cell_descr = Cell_descr(abc, angles)
        #cell_descr = clipper_core.Cell_descr(*abc, *angles)
        clipper_core.Cell.__init__(self, cell_descr)
    
    def __eq__(self, other):
        return self.equals(other)

Then:
import clipper
cell = clipper.cell([100,100,100],[90,90,90])
cell.cell_descr

SystemError                               Traceback (most recent call last)
<ipython-input-4-c5072ce3ae97> in <module>()
----> 1 c.cell_descr

/home/tic20/apps/chimerax/lib/python3.6/site-packages/chimerax/clipper/clipper_decorators.py in getter(self)
     87     def property_factory(func):
     88         def getter(self):
---> 89             return getattr(super(self.__class__, self), func)()
     90         prop = property(getter)
     91         return prop

/home/tic20/apps/chimerax/lib/python3.6/site-packages/chimerax/clipper/lib/clipper_python_core/clipper.py in cell_descr(self)
   8249 
   8250         """
-> 8251         return _clipper.Cell_cell_descr(self)
   8252 
   8253     __swig_destroy__ = _clipper.delete_Cell

SystemError: Objects/tupleobject.c:81: bad argument to internal function

... but if I comment out the derived Cell_descr class and switch to the alternate cell_descr constructor in Cell.__init__(), then it works as expected. I have tried commenting out the other class decorators, with no effect - so it would seem it's what's happening in the @mappedclass decorator that is causing my troubles.
msg289314 - (view) Author: Tristan Croll (Tristan Croll) Date: 2017-03-09 17:30
OK, this seems to narrow down the problem. The following was legal in Python 3.5.1, but in 3.5.3 and 3.6.1rc1 returns:

'TypeError: must be type, not classobj'

class Foo_Base:
    pass

class Bar_Base:
    def get_foo(self):
        f = Foo_Base()
        return f

class Foo(Foo_Base):
    pass

class Bar(Bar_Base):
    def get_foo2(self):
        return super(Bar, self).get_foo()
    
bar = Bar()
b = bar.get_foo2()

Is this a deliberate and permanent change? If so, it looks like I have a lot of work on my hands.
msg289317 - (view) Author: Tristan Croll (Tristan Croll) Date: 2017-03-09 17:39
Nope - belay that. Checking through the SWIG-generated Python code, all the classes correctly inherit from object, which negates that issue.
msg289318 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-09 17:50
The code in msg289314 doesn't emit any warning. I tested 3.5, 3.6 and master development branches and system Python 3.5.2.
msg289319 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-03-09 17:51
"Nope - belay that. Checking through the SWIG-generated Python code, all the classes correctly inherit from object, which negates that issue."

I don't understand your comment, on Python 3, any class inherit from object be default. There is no more old and new classes.


haypo@selma$ cat y.py 
class A: pass
class B(object): pass

print(A.__bases__)
print(B.__bases__)


haypo@selma$ python3 y.py 
(<class 'object'>,)
(<class 'object'>,)
msg289320 - (view) Author: Tristan Croll (Tristan Croll) Date: 2017-03-09 17:52
Sorry - ignore that. Brain-fart at the end of a (very) long day.
msg289322 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2017-03-09 18:39
Since it sounds like everything on the Python side is fine I'm closing this.
msg289323 - (view) Author: Tristan Croll (Tristan Croll) Date: 2017-03-09 18:51
I don't agree that it should be closed yet. I still have the issue that an approach that was perfectly legal in Python 3.5 now no longer works in Python 3.6, and I don't know why. The description in msg289281 stands, and is a real problem. Nothing has changed in my code - I compile against Python 3.5 and it works, against 3.6 it doesn't.
msg289324 - (view) Author: Tristan Croll (Tristan Croll) Date: 2017-03-09 19:11
I've cross-posted the following to the SWIG bug tracker. Hopefully someone can find an answer, because I'm getting nowhere.

If I have two classes Foo and Bar (where Bar has a function get_foo() that returns a Foo object) defined in the SWIG-generated library foobar, and wrap them as follows:

```
def mappedclass(old_cls):
    '''
    Ensures that objects returned from functions in the clipper_core
    library are instantiated in Python as the derived class with the
    extra functionality. 
    '''
    def decorator(cls):
        def __newnew__(thiscls, *args, **kwargs):
            if thiscls == old_cls:
                return object.__new__(cls)
            return object.__new__(thiscls)
        old_cls.__new__ = __newnew__
        
        return cls
    return decorator

@mappedclass(foobar.Foo)
class Foo(foobar.Foo):
    pass

@mappedclass(foobar.Bar)
class Bar(foobar.Bar):
    def get_foo(self):
        return super(Bar, self).get_foo()

```

then in Python 3.5:
```
f = Foo()
b = Bar()
b.get_foo()
```
all work. In Python 3.6:

```
f = Foo()
b = Bar()
```
... both work, but
`b.get_foo()`
yields the error as per my OP. Real-world examples:

Constructor (works without trouble in both versions)
```
SWIGINTERN PyObject *_wrap_new_Cell_descr__SWIG_0(PyObject *SWIGUNUSEDPARM(self), PyObject *args) {
  PyObject *resultobj = 0;
  clipper::Cell_descr *result = 0 ;
  
  if(!PyArg_UnpackTuple(args,(char *)"new_Cell_descr",0,0)) SWIG_fail;
  {
    try
    {
      result = (clipper::Cell_descr *)new clipper::Cell_descr();
    } catch (clipper::Message_fatal m)
    {
      SWIG_exception(SWIG_RuntimeError, m.text().c_str() );
      SWIG_fail;
    } catch (std::out_of_range e)
    {
      const char *errString;
      if ( !strcmp(e.what(), "" ) ) {
        errString = "Index out of range!";
      } else {
        errString = e.what();
      }
      SWIG_exception(SWIG_IndexError, errString );
      SWIG_fail;
    } catch (std::length_error e)
    {
      SWIG_exception(SWIG_ValueError, e.what() );
      SWIG_fail;
    } catch (std::invalid_argument e)
    {
      SWIG_exception(SWIG_ValueError, e.what() );
      SWIG_fail;
    } catch (std::exception e)
    {
      SWIG_exception(SWIG_UnknownError, e.what() );
      SWIG_fail;
    } catch (...)
    {
      SWIG_exception(SWIG_UnknownError, "Unknown error" );
      SWIG_fail;
    }
    
  }
  resultobj = SWIG_NewPointerObj(SWIG_as_voidptr(result), SWIGTYPE_p_clipper__Cell_descr, SWIG_POINTER_NEW |  0 );
  return resultobj;
fail:
  return NULL;
}
```

Getter (fails under the above conditions):
```
SWIGINTERN PyObject *_wrap_Cell_cell_descr(PyObject *SWIGUNUSEDPARM(self), PyObject *args) {
  PyObject *resultobj = 0;
  clipper::Cell *arg1 = (clipper::Cell *) 0 ;
  void *argp1 = 0 ;
  int res1 = 0 ;
  PyObject * obj0 = 0 ;
  clipper::Cell_descr result;
  
  if(!PyArg_UnpackTuple(args,(char *)"Cell_cell_descr",1,1,&obj0)) SWIG_fail;
  res1 = SWIG_ConvertPtr(obj0, &argp1,SWIGTYPE_p_clipper__Cell, 0 |  0 );
  if (!SWIG_IsOK(res1)) {
    SWIG_exception_fail(SWIG_ArgError(res1), "in method '" "Cell_cell_descr" "', argument " "1"" of type '" "clipper::Cell *""'"); 
  }
  arg1 = reinterpret_cast< clipper::Cell * >(argp1);
  {
    try
    {
      result = clipper_Cell_cell_descr(arg1);
    } catch (clipper::Message_fatal m)
    {
      SWIG_exception(SWIG_RuntimeError, m.text().c_str() );
      SWIG_fail;
    } catch (std::out_of_range e)
    {
      const char *errString;
      if ( !strcmp(e.what(), "" ) ) {
        errString = "Index out of range!";
      } else {
        errString = e.what();
      }
      SWIG_exception(SWIG_IndexError, errString );
      SWIG_fail;
    } catch (std::length_error e)
    {
      SWIG_exception(SWIG_ValueError, e.what() );
      SWIG_fail;
    } catch (std::invalid_argument e)
    {
      SWIG_exception(SWIG_ValueError, e.what() );
      SWIG_fail;
    } catch (std::exception e)
    {
      SWIG_exception(SWIG_UnknownError, e.what() );
      SWIG_fail;
    } catch (...)
    {
      SWIG_exception(SWIG_UnknownError, "Unknown error" );
      SWIG_fail;
    }
    
  }
  resultobj = SWIG_NewPointerObj((new clipper::Cell_descr(static_cast< const clipper::Cell_descr& >(result))), SWIGTYPE_p_clipper__Cell_descr, SWIG_POINTER_OWN |  0 );
  return resultobj;
fail:
  return NULL;
}
```
msg289399 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2017-03-10 18:19
Please keep this issue closed until you hear back from the SWIG team. Just because your code worked under Python 3.5 doesn't mean SWIG didn't accidentally emit something that breaks under Python 3.6 because we started being more stringent about something. Basically unless we have C code (and not SWIG or C++ code) to reproduce this then we have to go on the assumption it's a problem on SWIG's side.
History
Date User Action Args
2022-04-11 14:58:44adminsetgithub: 73944
2017-03-10 18:19:17brett.cannonsetstatus: open -> closed
resolution: third party
messages: + msg289399
2017-03-09 19:11:32Tristan Crollsetmessages: + msg289324
2017-03-09 18:51:24Tristan Crollsetstatus: closed -> open
resolution: not a bug -> (no value)
messages: + msg289323
2017-03-09 18:39:07brett.cannonsetstatus: open -> closed

nosy: + brett.cannon
messages: + msg289322

resolution: not a bug
stage: resolved
2017-03-09 17:52:57Tristan Crollsetmessages: + msg289320
versions: - Python 3.5
2017-03-09 17:51:13vstinnersetmessages: + msg289319
2017-03-09 17:50:02vstinnersetmessages: + msg289318
2017-03-09 17:39:26Tristan Crollsetmessages: + msg289317
2017-03-09 17:30:00Tristan Crollsetmessages: + msg289314
versions: + Python 3.5
2017-03-09 10:10:32Tristan Crollsetmessages: + msg289281
2017-03-09 09:15:08vstinnersetmessages: + msg289280
2017-03-09 02:27:55xiang.zhangsetnosy: + vstinner, serhiy.storchaka
2017-03-08 16:47:29Tristan Crollcreate