Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg

Daniel Shahaf-2
[hidden email] wrote on Sat, 23 Dec 2017 04:43 +0000:

> Author: troycurtisjr
> Date: Sat Dec 23 04:43:26 2017
> New Revision: 1819110
>
> URL: http://svn.apache.org/viewvc?rev=1819110&view=rev
> Log:
> On branch swig-py3: Replace hasattr check for a method with try-except.
>
> * subversion/bindings/swig/include/proxy.swg
>   (_assert_valid_deep): Replace hasattr check for the 'assert_valid' method
>    with a try-except block as some class instances can have the method but return
>    False to hasattr().

I'm not too familiar with this code; shouldn't we be fixing the original
problem, of hasattr() wrongly returning False?  I assume it predates the
branch, though?

While reviewing this I also noticed that svn_swig_py_convert_ptr() also
does this hasattr() check — not sure whether it needs to change too? —
and also has an "x | 0" construct, which seems suspicious (isn't it a
no-op?).

> Modified:
>     subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
>
> Modified: subversion/branches/swig-py3/subversion/bindings/swig/include/
> proxy.swg
> URL:
> http://svn.apache.org/viewvc/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg?rev=1819110&r1=1819109&r2=1819110&view=diff
> ==============================================================================
> --- subversion/branches/swig-py3/subversion/bindings/swig/include/
> proxy.swg (original)
> +++ subversion/branches/swig-py3/subversion/bindings/swig/include/
> proxy.swg Sat Dec 23 04:43:26 2017
> @@ -57,8 +57,11 @@
>          _assert_valid_deep(v)
>      # Ensure that the passed in value isn't a type, which could have an
>      # assert_valid attribute, but it can not be called without an
> instance.
> -    elif type(value) != type and hasattr(value, "assert_valid"):
> +    elif type(value) != type:
> +      try:
>          value.assert_valid()
> +      except AttributeError:
> +        pass

Hmm.  Strictly speaking, the equivalent form would be:

try:
  value.assert_valid
except AttributeError:
  pass
else:
  value.assert_valid()

since we don't want to mask any AttributeErrors inside assert_valid().
I'm not sure how careful we are about this distinction, though.

>  %}
>  
>  /* Default code for all wrapped proxy classes in Python.
>
>

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg

Troy Curtis Jr


On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <[hidden email]> wrote:
[hidden email] wrote on Sat, 23 Dec 2017 04:43 +0000:
> Author: troycurtisjr
> Date: Sat Dec 23 04:43:26 2017
> New Revision: 1819110
>
> URL: http://svn.apache.org/viewvc?rev=1819110&view=rev
> Log:
> On branch swig-py3: Replace hasattr check for a method with try-except.
>
> * subversion/bindings/swig/include/proxy.swg
>   (_assert_valid_deep): Replace hasattr check for the 'assert_valid' method
>    with a try-except block as some class instances can have the method but return
>    False to hasattr().

I'm not too familiar with this code; shouldn't we be fixing the original
problem, of hasattr() wrongly returning False?  I assume it predates the
branch, though?


I won't lie, I didn't like this change much, since I didn't feel that I understood exactly *why* it didn't work.  I only found info stating that hasattr effectively did a getattr, but translated the AttributeError into a boolean.  However, obviously *something* else is different.  The attribute is obviously able to be found in some scenarios, but returns false for hasattr.  So far my attempts to reproduce in a small test class haven't been successful.  Perhaps, I should continue to dig into this one to get to the bottom of what the difference actually is.
 
While reviewing this I also noticed that svn_swig_py_convert_ptr() also
does this hasattr() check — not sure whether it needs to change too? —
and also has an "x | 0" construct, which seems suspicious (isn't it a
no-op?).


I looked back through the logs in an attempt figure out the rationale for the "x | 0" construct, but it just shows up that way when it was first added [0].  I agree, it should just be changed to the define. I can't come up with a reason to 'or' with 0.
 
> Modified:
>     subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg
>
> Modified: subversion/branches/swig-py3/subversion/bindings/swig/include/
> proxy.swg
> URL:
> http://svn.apache.org/viewvc/subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg?rev=1819110&r1=1819109&r2=1819110&view=diff
> ==============================================================================
> --- subversion/branches/swig-py3/subversion/bindings/swig/include/
> proxy.swg (original)
> +++ subversion/branches/swig-py3/subversion/bindings/swig/include/
> proxy.swg Sat Dec 23 04:43:26 2017
> @@ -57,8 +57,11 @@
>          _assert_valid_deep(v)
>      # Ensure that the passed in value isn't a type, which could have an
>      # assert_valid attribute, but it can not be called without an
> instance.
> -    elif type(value) != type and hasattr(value, "assert_valid"):
> +    elif type(value) != type:
> +      try:
>          value.assert_valid()
> +      except AttributeError:
> +        pass

Hmm.  Strictly speaking, the equivalent form would be:

try:
  value.assert_valid
except AttributeError:
  pass
else:
  value.assert_valid()

since we don't want to mask any AttributeErrors inside assert_valid().
I'm not sure how careful we are about this distinction, though.


Good catch!
 
>  %}
>
>  /* Default code for all wrapped proxy classes in Python.
>
>

Cheers,

Daniel

0:  svn diff -r855533:855534 ^/subversion/branches/python-bindings-improvements/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c@855534

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg

Branko Čibej
On 23.12.2017 16:27, Troy Curtis Jr wrote:

>
> On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     While reviewing this I also noticed that svn_swig_py_convert_ptr()
>     also
>     does this hasattr() check — not sure whether it needs to change too? —
>     and also has an "x | 0" construct, which seems suspicious (isn't it a
>     no-op?).
>
>
> I looked back through the logs in an attempt figure out the rationale
> for the "x | 0" construct, but it just shows up that way when it was
> first added [0].  I agree, it should just be changed to the define. I
> can't come up with a reason to 'or' with 0.


It's not necessarily a no-op: depends on the type of 'x', if it's
smaller than an int, then 'x | 0' involves an implicit type conversion.

-- Brane
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg

Daniel Shahaf-2
In reply to this post by Troy Curtis Jr
Troy Curtis Jr wrote on Sat, 23 Dec 2017 15:27 +0000:

> On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <[hidden email]>
> wrote:
>
> > [hidden email] wrote on Sat, 23 Dec 2017 04:43 +0000:
> > > Author: troycurtisjr
> > > Date: Sat Dec 23 04:43:26 2017
> > > New Revision: 1819110
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1819110&view=rev
> > > Log:
> > > On branch swig-py3: Replace hasattr check for a method with try-except.
> > >
> > > * subversion/bindings/swig/include/proxy.swg
> > >   (_assert_valid_deep): Replace hasattr check for the 'assert_valid'
> > method
> > >    with a try-except block as some class instances can have the method
> > but return
> > >    False to hasattr().
> >
> > I'm not too familiar with this code; shouldn't we be fixing the original
> > problem, of hasattr() wrongly returning False?  I assume it predates the
> > branch, though?
> >
> >
> I won't lie, I didn't like this change much, since I didn't feel that I
> understood exactly *why* it didn't work.  I only found info stating that
> hasattr effectively did a getattr, but translated the AttributeError into a
> boolean.  However, obviously *something* else is different.  The attribute
> is obviously able to be found in some scenarios, but returns false for
> hasattr.  So far my attempts to reproduce in a small test class haven't
> been successful.  Perhaps, I should continue to dig into this one to get to
> the bottom of what the difference actually is.

I assume it could affect users' code as well, so yes, it'll be nice to get to
the bottom of it (and to confirm that it's not a regression).  What classes can
you reproduce the mismatch with?

I don't see any difference between the CPython implementations of getattr and
hasattr, but perhaps I'm overlooking something (or looking at the source of a
different CPython version to yours).

>
> > While reviewing this I also noticed that svn_swig_py_convert_ptr() also
> > does this hasattr() check — not sure whether it needs to change too? —
> > and also has an "x | 0" construct, which seems suspicious (isn't it a
> > no-op?).
> >
> >
> I looked back through the logs in an attempt figure out the rationale for
> the "x | 0" construct, but it just shows up that way when it was first
> added [0].  I agree, it should just be changed to the define. I can't come
> up with a reason to 'or' with 0.

SWIG_POINTER_EXCEPTION is defined as «0» in swig3 so the construct does appear
to be a no-op there.  I don't have older swigs to test with.

(@brane: Good catch.)

> 0:  svn diff -r855533:855534
> ^/subversion/branches/python-bindings-improvements/subversion/bindings/
> swig/python/libsvn_swig_py/swigutil_py.c@855534

Also known as
https://svn.apache.org/viewvc/subversion/branches/python-bindings-improvements/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c?r1=855534&r2=855533&pathrev=855534

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg

Troy Curtis Jr


On Sat, Dec 23, 2017 at 2:57 PM Daniel Shahaf <[hidden email]> wrote:
Troy Curtis Jr wrote on Sat, 23 Dec 2017 15:27 +0000:
> On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <[hidden email]>
> wrote:
>
> > [hidden email] wrote on Sat, 23 Dec 2017 04:43 +0000:
> > > Author: troycurtisjr
> > > Date: Sat Dec 23 04:43:26 2017
> > > New Revision: 1819110
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1819110&view=rev
> > > Log:
> > > On branch swig-py3: Replace hasattr check for a method with try-except.
> > >
> > > * subversion/bindings/swig/include/proxy.swg
> > >   (_assert_valid_deep): Replace hasattr check for the 'assert_valid'
> > method
> > >    with a try-except block as some class instances can have the method
> > but return
> > >    False to hasattr().
> >
> > I'm not too familiar with this code; shouldn't we be fixing the original
> > problem, of hasattr() wrongly returning False?  I assume it predates the
> > branch, though?
> >
> >
> I won't lie, I didn't like this change much, since I didn't feel that I
> understood exactly *why* it didn't work.  I only found info stating that
> hasattr effectively did a getattr, but translated the AttributeError into a
> boolean.  However, obviously *something* else is different.  The attribute
> is obviously able to be found in some scenarios, but returns false for
> hasattr.  So far my attempts to reproduce in a small test class haven't
> been successful.  Perhaps, I should continue to dig into this one to get to
> the bottom of what the difference actually is.

I assume it could affect users' code as well, so yes, it'll be nice to get to
the bottom of it (and to confirm that it's not a regression).  What classes can
you reproduce the mismatch with?

I don't see any difference between the CPython implementations of getattr and
hasattr, but perhaps I'm overlooking something (or looking at the source of a
different CPython version to yours).


The place I was finding the issue was on a SWIG wrapped auth baton instance used in a client context.  In particular, in bindings/swig/python/tests/pool.py:87 during the test_assert_valid function, the assert_valid on the auth_baton instance would fail.  When viewed in debugger, just before the hasattr() check, I confirmed that it returned 'False', but would actually run when called.
 
>
> > While reviewing this I also noticed that svn_swig_py_convert_ptr() also
> > does this hasattr() check — not sure whether it needs to change too? —
> > and also has an "x | 0" construct, which seems suspicious (isn't it a
> > no-op?).
> >
> >
> I looked back through the logs in an attempt figure out the rationale for
> the "x | 0" construct, but it just shows up that way when it was first
> added [0].  I agree, it should just be changed to the define. I can't come
> up with a reason to 'or' with 0.

SWIG_POINTER_EXCEPTION is defined as «0» in swig3 so the construct does appear
to be a no-op there.  I don't have older swigs to test with.

(@brane: Good catch.)

> 0:  svn diff -r855533:855534
> ^/subversion/branches/python-bindings-improvements/subversion/bindings/
> swig/python/libsvn_swig_py/swigutil_py.c@855534

Also known as
https://svn.apache.org/viewvc/subversion/branches/python-bindings-improvements/subversion/bindings/swig/python/libsvn_swig_py/swigutil_py.c?r1=855534&r2=855533&pathrev=855534

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg

Troy Curtis Jr
In reply to this post by Daniel Shahaf-2


On Sat, Dec 23, 2017 at 2:57 PM Daniel Shahaf <[hidden email]> wrote:
Troy Curtis Jr wrote on Sat, 23 Dec 2017 15:27 +0000:
> On Fri, Dec 22, 2017 at 11:11 PM Daniel Shahaf <[hidden email]>
> wrote:
>
> > [hidden email] wrote on Sat, 23 Dec 2017 04:43 +0000:
> > > Author: troycurtisjr
> > > Date: Sat Dec 23 04:43:26 2017
> > > New Revision: 1819110
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1819110&view=rev
> > > Log:
> > > On branch swig-py3: Replace hasattr check for a method with try-except.
> > >
> > > * subversion/bindings/swig/include/proxy.swg
> > >   (_assert_valid_deep): Replace hasattr check for the 'assert_valid'
> > method
> > >    with a try-except block as some class instances can have the method
> > but return
> > >    False to hasattr().
> >
> > I'm not too familiar with this code; shouldn't we be fixing the original
> > problem, of hasattr() wrongly returning False?  I assume it predates the
> > branch, though?
> >
> >
> I won't lie, I didn't like this change much, since I didn't feel that I
> understood exactly *why* it didn't work.  I only found info stating that
> hasattr effectively did a getattr, but translated the AttributeError into a
> boolean.  However, obviously *something* else is different.  The attribute
> is obviously able to be found in some scenarios, but returns false for
> hasattr.  So far my attempts to reproduce in a small test class haven't
> been successful.  Perhaps, I should continue to dig into this one to get to
> the bottom of what the difference actually is.

I assume it could affect users' code as well, so yes, it'll be nice to get to
the bottom of it (and to confirm that it's not a regression).  What classes can
you reproduce the mismatch with?

I don't see any difference between the CPython implementations of getattr and
hasattr, but perhaps I'm overlooking something (or looking at the source of a
different CPython version to yours).


In taking another look at why hasattr() seeming fails in certain cases on SWIG generated new style classes under python2, I ran across this article, https://hynek.me/articles/hasattr/, which confirms that python 2's hasattr() can misbehave in the presence of properties (which the non-classic SWIG classes use).  So this appears to be a known discrepancy.  I did change the code to no hide any AttributeError's encountered in the assert_valid() method call itself.

Troy
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1819110 - /subversion/branches/swig-py3/subversion/bindings/swig/include/proxy.swg

Daniel Shahaf-2
Troy Curtis Jr wrote on Wed, 27 Dec 2017 02:48 +0000:
> In taking another look at why hasattr() seeming fails in certain cases on
> SWIG generated new style classes under python2, I ran across this article,
> https://hynek.me/articles/hasattr/, which confirms that python 2's
> hasattr() can misbehave in the presence of properties (which the
> non-classic SWIG classes use).  So this appears to be a known discrepancy.
> I did change the code to no hide any AttributeError's encountered in the
> assert_valid() method call itself.

If I'm reading the article correctly, it didn't actually explain why
hasattr() false negatived, but it did say that hasattr() is an
antipattern, which  supports r1819110.

Fair enough :-)

Thanks,

Daniel