On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <d.s_at_daniel.shahaf.name>
wrote:
> Good morning Troy,
>
> troycurtisjr_at_apache.org wrote on Fri, Dec 22, 2017 at 03:52:59 -0000:
> > On branch swig-py3: Correctly manage swig objects with Python new style
> classes.
> >
> > * build/ac-macros/swig.m4
> > (SVN_FIND_SWIG):
> > Request that swig generate new style classes, even for Python 2, to
> reduce
> > differences with Python 3.
> >
> > Modified: subversion/branches/swig-py3/build/ac-macros/swig.m4
> > URL:
> http://svn.apache.org/viewvc/subversion/branches/swig-py3/build/ac-macros/swig.m4?rev=1818995&r1=1818994&r2=1818995&view=diff
> >
> ==============================================================================
> > --- subversion/branches/swig-py3/build/ac-macros/swig.m4 (original)
> > +++ subversion/branches/swig-py3/build/ac-macros/swig.m4 Fri Dec 22
> 03:52:59 2017
> > @@ -143,7 +143,7 @@ AC_DEFUN(SVN_FIND_SWIG,
> > if test "$ac_cv_python_is_py3" = "yes"; then
> > SWIG_PY_OPTS="-python -py3"
> > else
> > - SWIG_PY_OPTS="-python -classic"
> > + SWIG_PY_OPTS="-python"
> > fi
>
> What are the compatibility implications of this change? I couldn't find
> any
> documentation for the "-classic" option.
>
> > * subversion/bindings/swig/include/proxy.py
> > (__getattr__): Replace __getattr__ with __getattribute__ to correctly
> > intercept attribute access, even when swig uses descriptors for new
> style
> > classes (which are the only type available in Python 3).
>
> Could you help me understand what's going on here and why it's correct?
> I've
> read the documentation of object.__getattr__ and object.__getattribute__¹
> but I
> don't follow how the fact that the class into which this code is included
> became a new-style class (rather than a classic class) brought on the need
> to
> migrate from __getattr__ to __getattribute__, nor why the order of
> precedence
> of lookup (first __dict__, then object.__getattribute__, then
> _swig_getattr) is
> correct.
>
> I'm asking because I'm trying to review the branch (to +1 its merge) and
> this
> is one of the open questions I have.
>
> ¹ https://docs.python.org/3/reference/datamodel.html#object.__getattr__
> et seq.
>
Sure thing. This is the change that took me so long to find, and then fix
the right way, so I certainly understand the non-obviousness of it.
Perhaps some portion of this explanation could go in an appropriate place
in the code for future reference? Though much of it only applies to the
motivation for the change and probably wouldn't be beneficial just given
the final implementation.
Giving swig the '-classic' option forces it to generate only classic
classes. Without that flag it will produce code that works as either an
old style or new style class, dependent on a try/except block around
'import object'. In practice, this means that for any Python past 2.2
(when new-style classes were introduced), the classes are in the new style
and inherit from 'object'.
Just the move to the new style classes did not require the move from
__getattr__ to __getattribute__, but rather the swig implementation
together with the pool/metadata management requirements of the subversion
interfaces did. For old style classes, swig uses __getattr__ together with
a private set of attributes holding getter and setter methods to provide
the natural attribute access pattern of 'foo.bar', but without 'bar'
actually existing as an attribute. This way swig can correctly forward to
underlying getter/setter functions of the wrapped code. The subversion
bindings then make use of this by overriding __setattr__ and __getattr__ to
more perfectly construct returned python objects. First, it saves off the
original object in __setattr__ (which also saves it, along with the
associated pool, from getting garbage collected). Then in __getattr__ it
uses attributes from the original object to fill out metadata on the newly
constructed object returned from the underlying swig getter methods.
When '-classic' is not used in SWIG, and you are on anything newer than
Python 2.2, you get new style classes. A really nice feature of new style
classes are descriptors, or properties. This allows easy specification of
getter/setter code for individual attributes built-in to the class syntax
[2]. As this is exactly what swig is trying to accomplish, it uses
descriptors where available (in the generated code you can see all the 'if
_newclass' constructs accomplishing this). However, by doing this,
__getattr__ is now no longer called for any of those attributes, which
breaks the pool/metadata management code!
The alternative is __getattribute__, which gets *every* attribute access,
unlike __getattr__ which is only invoked when all standard lookup fails.
That gets us what we need for the pool management code, but it means there
is additional responsibility to mimic the standard behavior as well. I
know from personal experience how easy it is to get into infinite recursion
loops. ;) Mimicing the standard attribute lookup behavior is the rationale
behind first resolving from __dict__, then using object.__getattribute__,
then using swig's interface. Without the initial __dict__ lookup, you get
into infinite recursion trying to lookup the other basic attributes. Next,
object.__getattribute__ does a standard lookup, and finally the swig lookup
picks up any internal swig specific values that aren't accessible by
standard means. Now, with a reference to the returned value, the existing
pool/metadata code can be applied properly.
On the compatibility front, the notable change is the move to new style
classes. For code using instances of the objects there should be no
functional change. The __getattr__/__getattribute__ using
swig-magic-methods/descriptors are only implementation details. If for
some reason users of the bindings inherited from those classes, then there
might be a noticeable change in the move, especially if they try
overloading internal features. I assume that in doing so, you are already
asking for trouble. It would also make those derived classes go from
classic to new style, but the distinction between the classic vs new
classes really come down to:
1. Method resolution order (only change relative to multiple
inheritance),
2. Instances of the class have a real type() and isn't always 'instance'
like old-style classes
3. You can use descriptors/properties
Which I would say is fairly safe compatibility wise, since again this would
only impact users that subclassed these generated classes, and tried to dig
into internal implementation details. This seems fairly unlikely (since all
the important bits have leading underscores indicating internal use only).
Though I suppose that without documentation stating that you *shouldn't*
derive from the generated swig classes, there is always the possibility
that users could have done so, and thus be surprised at suddenly having
new-style classes.
On a final note, I believe that we could use a conditional on _newclass to
use __getattr__ and __getattribute__ (and return the -classic option in the
python 2 swig invocation), and as long as we called into an appropriately
common function, the code would not be particularly repetitive. However, I
thought this unnecessarily complicated the code and reduced commonality
between py2 and py3. If you think the compatibility concerns are too big,
then I could explore that conditional implementation.
Hope this helps, and isn't too long winded. ;)
Troy
2: https://docs.python.org/3.4/reference/datamodel.html#descriptors
> > Modified:
> subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py
> > URL:
> http://svn.apache.org/viewvc/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py?rev=1818995&r1=1818994&r2=1818995&view=diff
> >
> ==============================================================================
> > ---
> subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py
> (original)
> > +++
> subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.py Fri
> Dec 22 03:52:59 2017
> > @@ -12,17 +12,36 @@
> > if "_is_valid" in self.__dict__:
> > assert self.__dict__["_is_valid"](), "Variable has already been
> deleted"
> >
> > - def __getattr__(self, name):
> > - """Get an attribute from this object"""
> > - self.assert_valid()
> > + def __getattribute__(self, name):
> > + """Manage access to all attributes of this object."""
> >
> > - value = _swig_getattr(self, self.__class__, name)
> > + # Start by mimicing __getattr__ behavior: immediately return
> __dict__ or
> > + # items directly present in __dict__
> > + mydict = object.__getattribute__(self, '__dict__')
> > + if name == "__dict__":
> > + return mydict
> > +
> > + if name in mydict:
> > + return mydict[name]
> > +
> > + object.__getattribute__(self, 'assert_valid')()
> > +
> > + try:
> > + value = object.__getattribute__(self, name)
> > + except AttributeError:
> > + value = _swig_getattr(self,
> > + object.__getattribute__(self, '__class__'),
> > + name)
> >
> > # If we got back a different object than we have, we need to copy
> all our
> > # metadata into it, so that it looks identical
> > - members = self.__dict__.get("_members")
> > - if members is not None:
> > - _copy_metadata_deep(value, members.get(name))
> > + try:
> > + members = object.__getattribute__(self, '_members')
> > + if name in members:
> > + _copy_metadata_deep(value, members[name])
> > + # Verify that the new object is good
> > + except AttributeError:
> > + pass
> >
> > # Verify that the new object is good
> > _assert_valid_deep(value)
>
> Cheers,
>
> Daniel
>
>
Received on 2017-12-30 16:12:20 CET