You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.name> on 2017/12/30 05:46:06 UTC
Re: svn commit: r1818995 - in /subversion/branches/swig-py3:
build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py
subversion/bindings/swig/include/proxy.swg
Good morning Troy,
troycurtisjr@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.
> 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
Re: svn commit: r1818995 - in /subversion/branches/swig-py3:
build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py
subversion/bindings/swig/include/proxy.swg
Posted by Branko Čibej <br...@apache.org>.
On 04.01.2018 02:48, Troy Curtis Jr wrote:
>
>
> On Tue, Jan 2, 2018 at 3:12 AM Branko Čibej <brane@apache.org
> <ma...@apache.org>> wrote:
>
> On 31.12.2017 03:05, Troy Curtis Jr wrote:
> >
> > This all makes sense and seems nice on the surface, but I'm not
> > sure we
> > can just change the behaviour of the bindings from old-style to
> > new-style classes in a Python 2.x build. There are enough subtle
> > differences in behaviour between the two that existing
> scripts could
> > break after an upgrade of the bindings.
> >
> > Python 3.x has only new-style (or rather, even-newer-style)
> > classes and
> > there's no backward-compatibility consideration, since our
> bindings
> > currently don't work with Python3.
> >
> >
> > That is a reasonable concern. I definitely preferred the cleaner
> > single implementation, but honestly the code necessary to
> continue to
> > use classic classes in python 2 is not large. I've attached a
> working
> > patch for reference/discussion. It is a bit more code and some
> > conditional definitions, but perhaps it is the more preferred course
> > to take?
> >
> > [[[
> > On branch swig-py3: Go back to using classic classes for Python
> 2 swig
> > bindings.
> >
> > Add some additional clarifying comments for the reasons behind
> overriding
> > __getattr__ and __getattribute__.
> >
> > * build/ac-macros/swig.m4
> > (SVN_FIND_SWIG): Add the '-classic' flag to swig when python 2 is
> > detected.
> >
> > * subversion/bindings/swig/include/proxy.py
> > (_retrieve_swig_value): Factor out metadata retrieval from
> > __getattribute__ to a new function.
> > (__getattribute__): Only define __getattribute__ for new
> style classes.
> > (__getattr__): Add back implementation for classic classes.
> > ]]]
> >
> > Troy
>
> [...]
>
> > +· # SWIG classes generated with -classic do not define this
> variable,
> > +· # so set it to 0 when it doesn't exist
> > +· try: _newclass
> > +· except NameError: _newclass = 0
>
> I prefer to break the try/except blocks onto separate lines, and
> to use
> None for the tristate idiom value:
>
> try:
> _newclass
> except NameError:
> _newclass = None
>
>
> Using None here is certainly more Pythonic, but in this case I was
> trying to match up with what swig generates:
>
> try:
> _object = object
> _newclass = 1
> except __builtin__.Exception:
> class _object:
> pass
> _newclass = 0
>
> In this case we only need the _newclass variable defined, and not the
> "empty" class definition. In all the conditional cases which use that
> value, either way should work, but I think it is likely better to
> stick with 0 for consistency in this case.
>
> However, I can understand the formatting request.
>
> Other than that, is the consensus that we should continue with classic
> classes in Python 2 with the conditional logic, or use a common
> implementation for the python2/python3 code like is currently in the
> swig-py3 branch?
The differences between old-style and new-style classes is tricky.
Offhand I can think of differences in invocation of base class
constructors, for example. I'd say leave old style classes for Python 2
precisely because the change would not be 100% backwards compatible.
-- Brane
Re: svn commit: r1818995 - in /subversion/branches/swig-py3:
build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Tue, Jan 2, 2018 at 3:12 AM Branko Čibej <br...@apache.org> wrote:
> On 31.12.2017 03:05, Troy Curtis Jr wrote:
> >
> > This all makes sense and seems nice on the surface, but I'm not
> > sure we
> > can just change the behaviour of the bindings from old-style to
> > new-style classes in a Python 2.x build. There are enough subtle
> > differences in behaviour between the two that existing scripts could
> > break after an upgrade of the bindings.
> >
> > Python 3.x has only new-style (or rather, even-newer-style)
> > classes and
> > there's no backward-compatibility consideration, since our bindings
> > currently don't work with Python3.
> >
> >
> > That is a reasonable concern. I definitely preferred the cleaner
> > single implementation, but honestly the code necessary to continue to
> > use classic classes in python 2 is not large. I've attached a working
> > patch for reference/discussion. It is a bit more code and some
> > conditional definitions, but perhaps it is the more preferred course
> > to take?
> >
> > [[[
> > On branch swig-py3: Go back to using classic classes for Python 2 swig
> > bindings.
> >
> > Add some additional clarifying comments for the reasons behind overriding
> > __getattr__ and __getattribute__.
> >
> > * build/ac-macros/swig.m4
> > (SVN_FIND_SWIG): Add the '-classic' flag to swig when python 2 is
> > detected.
> >
> > * subversion/bindings/swig/include/proxy.py
> > (_retrieve_swig_value): Factor out metadata retrieval from
> > __getattribute__ to a new function.
> > (__getattribute__): Only define __getattribute__ for new style
> classes.
> > (__getattr__): Add back implementation for classic classes.
> > ]]]
> >
> > Troy
>
> [...]
>
> > +· # SWIG classes generated with -classic do not define this variable,
> > +· # so set it to 0 when it doesn't exist
> > +· try: _newclass
> > +· except NameError: _newclass = 0
>
> I prefer to break the try/except blocks onto separate lines, and to use
> None for the tristate idiom value:
>
> try:
> _newclass
> except NameError:
> _newclass = None
>
>
Using None here is certainly more Pythonic, but in this case I was trying
to match up with what swig generates:
try:
_object = object
_newclass = 1
except __builtin__.Exception:
class _object:
pass
_newclass = 0
In this case we only need the _newclass variable defined, and not the
"empty" class definition. In all the conditional cases which use that
value, either way should work, but I think it is likely better to stick
with 0 for consistency in this case.
However, I can understand the formatting request.
Other than that, is the consensus that we should continue with classic
classes in Python 2 with the conditional logic, or use a common
implementation for the python2/python3 code like is currently in the
swig-py3 branch?
Troy
Re: svn commit: r1818995 - in /subversion/branches/swig-py3:
build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py
subversion/bindings/swig/include/proxy.swg
Posted by Branko Čibej <br...@apache.org>.
On 31.12.2017 03:05, Troy Curtis Jr wrote:
>
> This all makes sense and seems nice on the surface, but I'm not
> sure we
> can just change the behaviour of the bindings from old-style to
> new-style classes in a Python 2.x build. There are enough subtle
> differences in behaviour between the two that existing scripts could
> break after an upgrade of the bindings.
>
> Python 3.x has only new-style (or rather, even-newer-style)
> classes and
> there's no backward-compatibility consideration, since our bindings
> currently don't work with Python3.
>
>
> That is a reasonable concern. I definitely preferred the cleaner
> single implementation, but honestly the code necessary to continue to
> use classic classes in python 2 is not large. I've attached a working
> patch for reference/discussion. It is a bit more code and some
> conditional definitions, but perhaps it is the more preferred course
> to take?
>
> [[[
> On branch swig-py3: Go back to using classic classes for Python 2 swig
> bindings.
>
> Add some additional clarifying comments for the reasons behind overriding
> __getattr__ and __getattribute__.
>
> * build/ac-macros/swig.m4
> (SVN_FIND_SWIG): Add the '-classic' flag to swig when python 2 is
> detected.
>
> * subversion/bindings/swig/include/proxy.py
> (_retrieve_swig_value): Factor out metadata retrieval from
> __getattribute__ to a new function.
> (__getattribute__): Only define __getattribute__ for new style classes.
> (__getattr__): Add back implementation for classic classes.
> ]]]
>
> Troy
[...]
> +· # SWIG classes generated with -classic do not define this variable,
> +· # so set it to 0 when it doesn't exist
> +· try: _newclass
> +· except NameError: _newclass = 0
I prefer to break the try/except blocks onto separate lines, and to use
None for the tristate idiom value:
try:
_newclass
except NameError:
_newclass = None
-- Brane
Re: svn commit: r1818995 - in /subversion/branches/swig-py3:
build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Sat, Dec 30, 2017 at 12:49 PM Branko Čibej <br...@apache.org> wrote:
> On 30.12.2017 16:11, Troy Curtis Jr wrote:
> >
> >
> > On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <d.s@daniel.shahaf.name
> > <ma...@daniel.shahaf.name>> wrote:
> >
> > Good morning Troy,
> >
> > troycurtisjr@apache.org <ma...@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'.
>
> This all makes sense and seems nice on the surface, but I'm not sure we
> can just change the behaviour of the bindings from old-style to
> new-style classes in a Python 2.x build. There are enough subtle
> differences in behaviour between the two that existing scripts could
> break after an upgrade of the bindings.
>
> Python 3.x has only new-style (or rather, even-newer-style) classes and
> there's no backward-compatibility consideration, since our bindings
> currently don't work with Python3.
>
>
That is a reasonable concern. I definitely preferred the cleaner single
implementation, but honestly the code necessary to continue to use classic
classes in python 2 is not large. I've attached a working patch for
reference/discussion. It is a bit more code and some conditional
definitions, but perhaps it is the more preferred course to take?
[[[
On branch swig-py3: Go back to using classic classes for Python 2 swig
bindings.
Add some additional clarifying comments for the reasons behind overriding
__getattr__ and __getattribute__.
* build/ac-macros/swig.m4
(SVN_FIND_SWIG): Add the '-classic' flag to swig when python 2 is
detected.
* subversion/bindings/swig/include/proxy.py
(_retrieve_swig_value): Factor out metadata retrieval from
__getattribute__ to a new function.
(__getattribute__): Only define __getattribute__ for new style classes.
(__getattr__): Add back implementation for classic classes.
]]]
Troy
Re: svn commit: r1818995 - in /subversion/branches/swig-py3:
build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py
subversion/bindings/swig/include/proxy.swg
Posted by Branko Čibej <br...@apache.org>.
On 30.12.2017 16:11, Troy Curtis Jr wrote:
>
>
> On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <d.s@daniel.shahaf.name
> <ma...@daniel.shahaf.name>> wrote:
>
> Good morning Troy,
>
> troycurtisjr@apache.org <ma...@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'.
This all makes sense and seems nice on the surface, but I'm not sure we
can just change the behaviour of the bindings from old-style to
new-style classes in a Python 2.x build. There are enough subtle
differences in behaviour between the two that existing scripts could
break after an upgrade of the bindings.
Python 3.x has only new-style (or rather, even-newer-style) classes and
there's no backward-compatibility consideration, since our bindings
currently don't work with Python3.
-- Brane
Re: svn commit: r1818995 - in /subversion/branches/swig-py3:
build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Good morning Troy,
>
> troycurtisjr@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
>
>
Re: svn commit: r1818995 - in /subversion/branches/swig-py3:
build/ac-macros/swig.m4 subversion/bindings/swig/include/proxy.py subversion/bindings/swig/include/proxy.swg
Posted by Troy Curtis Jr <tr...@gmail.com>.
On Fri, Dec 29, 2017 at 11:46 PM Daniel Shahaf <d....@daniel.shahaf.name>
wrote:
> Good morning Troy,
>
> troycurtisjr@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
>
>