Patchwork [1,of,4,py3] error: wrap super() init call in try/except

login
register
mail settings
Submitter Yuya Nishihara
Date Nov. 10, 2016, 1:09 p.m.
Message ID <20161110220923.c6b21520077a9fed63cb869e@tcha.org>
Download mbox | patch
Permalink /patch/17445/
State Not Applicable
Headers show

Comments

Yuya Nishihara - Nov. 10, 2016, 1:09 p.m.
On Wed, 09 Nov 2016 11:23:38 -0500, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1476019730 14400
> #      Sun Oct 09 09:28:50 2016 -0400
> # Node ID 6f2a1367baa59f33fcbc328aea1a637658ce345e
> # Parent  c9313a5b8e602b6b3b9a4427e5c2f452a711dd73
> error: wrap super() init call in try/except
> 
> This is how we have to handle object's pickiness while still correctly
> handling multiple inheritance MRO nonsense.
> 
> diff --git a/mercurial/error.py b/mercurial/error.py
> --- a/mercurial/error.py
> +++ b/mercurial/error.py
> @@ -23,7 +23,10 @@ class Hint(object):
>      """
>      def __init__(self, *args, **kw):
>          self.hint = kw.pop('hint', None)
> -        super(Hint, self).__init__(*args, **kw)
> +        try:
> +            super(Hint, self).__init__(*args, **kw)
> +        except TypeError:
> +            pass

I don't fully understand the MRO, but I believe what we need is u'hint'.
Martijn Pieters - Nov. 10, 2016, 5:01 p.m.
> On 10 Nov 2016, at 13:09, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Wed, 09 Nov 2016 11:23:38 -0500, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1476019730 14400
>> #      Sun Oct 09 09:28:50 2016 -0400
>> # Node ID 6f2a1367baa59f33fcbc328aea1a637658ce345e
>> # Parent  c9313a5b8e602b6b3b9a4427e5c2f452a711dd73
>> error: wrap super() init call in try/except
>> 
>> This is how we have to handle object's pickiness while still correctly
>> handling multiple inheritance MRO nonsense.
>> 
>> diff --git a/mercurial/error.py b/mercurial/error.py
>> --- a/mercurial/error.py
>> +++ b/mercurial/error.py
>> @@ -23,7 +23,10 @@ class Hint(object):
>>     """
>>     def __init__(self, *args, **kw):
>>         self.hint = kw.pop('hint', None)
>> -        super(Hint, self).__init__(*args, **kw)
>> +        try:
>> +            super(Hint, self).__init__(*args, **kw)
>> +        except TypeError:
>> +            pass
> 
> I don't fully understand the MRO, but I believe what we need is u'hint'.

No, Hint is not a string. It is a direct reference to the class object in which this method is defined (it is looked up as a global each time __init__ is run).

super(Hint, self).__init__ will find the *next class* after Hint in the MRO for type(self). Depending on how Hint was used as a base class in *another class*, the next class in the MRO for self may or may not be object. Since object.__init__() doesn't take any arguments (not even *args or **kwargs), if either args or kw is non-empty you'd get a TypeError here.

Because you can't know for certain that the next class in the MRO is *not* object, it is prudent to use a try...except TypeError: pass here.

> --- a/mercurial/error.py
> +++ b/mercurial/error.py
> @@ -22,7 +22,7 @@ class Hint(object):
>     pass remaining arguments to the exception class.
>     """
>     def __init__(self, *args, **kw):
> -        self.hint = kw.pop('hint', None)
> +        self.hint = kw.pop(u'hint', None)
>         super(Hint, self).__init__(*args, **kw)
> 
> class RevlogError(Hint, Exception):
Yuya Nishihara - Nov. 11, 2016, 12:54 p.m.
On Thu, 10 Nov 2016 17:01:22 +0000, Martijn Pieters wrote:
> > On 10 Nov 2016, at 13:09, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Wed, 09 Nov 2016 11:23:38 -0500, Augie Fackler wrote:
> >> # HG changeset patch
> >> # User Augie Fackler <augie@google.com>
> >> # Date 1476019730 14400
> >> #      Sun Oct 09 09:28:50 2016 -0400
> >> # Node ID 6f2a1367baa59f33fcbc328aea1a637658ce345e
> >> # Parent  c9313a5b8e602b6b3b9a4427e5c2f452a711dd73
> >> error: wrap super() init call in try/except
> >> 
> >> This is how we have to handle object's pickiness while still correctly
> >> handling multiple inheritance MRO nonsense.
> >> 
> >> diff --git a/mercurial/error.py b/mercurial/error.py
> >> --- a/mercurial/error.py
> >> +++ b/mercurial/error.py
> >> @@ -23,7 +23,10 @@ class Hint(object):
> >>     """
> >>     def __init__(self, *args, **kw):
> >>         self.hint = kw.pop('hint', None)
> >> -        super(Hint, self).__init__(*args, **kw)
> >> +        try:
> >> +            super(Hint, self).__init__(*args, **kw)
> >> +        except TypeError:
> >> +            pass
> > 
> > I don't fully understand the MRO, but I believe what we need is u'hint'.
> 
> No, Hint is not a string. It is a direct reference to the class object in which this method is defined (it is looked up as a global each time __init__ is run).

I mean this try-except hides the real bug caused by kw[b'hint'] != kw[u'hint']
on Python 3. kw.pop('hint') is noop.

> super(Hint, self).__init__ will find the *next class* after Hint in the MRO for type(self). Depending on how Hint was used as a base class in *another class*, the next class in the MRO for self may or may not be object. Since object.__init__() doesn't take any arguments (not even *args or **kwargs), if either args or kw is non-empty you'd get a TypeError here.

In which case, I think the previous class of Hint must consume *args and **kw.
Martijn Pieters - Nov. 11, 2016, 1:29 p.m.
> On 11 Nov 2016, at 12:54, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Thu, 10 Nov 2016 17:01:22 +0000, Martijn Pieters wrote:
>>> On 10 Nov 2016, at 13:09, Yuya Nishihara <yuya@tcha.org> wrote:
>>> On Wed, 09 Nov 2016 11:23:38 -0500, Augie Fackler wrote:
>>>> # HG changeset patch
>>>> # User Augie Fackler <augie@google.com>
>>>> # Date 1476019730 14400
>>>> #      Sun Oct 09 09:28:50 2016 -0400
>>>> # Node ID 6f2a1367baa59f33fcbc328aea1a637658ce345e
>>>> # Parent  c9313a5b8e602b6b3b9a4427e5c2f452a711dd73
>>>> error: wrap super() init call in try/except
>>>> 
>>>> This is how we have to handle object's pickiness while still correctly
>>>> handling multiple inheritance MRO nonsense.
>>>> 
>>>> diff --git a/mercurial/error.py b/mercurial/error.py
>>>> --- a/mercurial/error.py
>>>> +++ b/mercurial/error.py
>>>> @@ -23,7 +23,10 @@ class Hint(object):
>>>>    """
>>>>    def __init__(self, *args, **kw):
>>>>        self.hint = kw.pop('hint', None)
>>>> -        super(Hint, self).__init__(*args, **kw)
>>>> +        try:
>>>> +            super(Hint, self).__init__(*args, **kw)
>>>> +        except TypeError:
>>>> +            pass
>>> 
>>> I don't fully understand the MRO, but I believe what we need is u'hint'.
>> 
>> No, Hint is not a string. It is a direct reference to the class object in which this method is defined (it is looked up as a global each time __init__ is run).
> 
> I mean this try-except hides the real bug caused by kw[b'hint'] != kw[u'hint']
> on Python 3. kw.pop('hint') is loop.

Ah! Yes, good catch again.

>> super(Hint, self).__init__ will find the *next class* after Hint in the MRO for type(self). Depending on how Hint was used as a base class in *another class*, the next class in the MRO for self may or may not be object. Since object.__init__() doesn't take any arguments (not even *args or **kwargs), if either args or kw is non-empty you'd get a TypeError here.
> 
> In which case, I think the previous class of Hint must consume *args and **kw.

Probably, yes, still having a non-empty args / kw dataset by the time you reach `object` could be a bug.

But might there be a situation where extensions may have to deal with *potential* mixins? There are scenarios possible where one extension can't know what other extensions are installed and must always assume that certain arguments are needed.

I'm not familiar enough with where `Hint` is used, and it is probably better to only plug in a `try:...except:` at the end when the situation actually arises.

Martijn
Yuya Nishihara - Nov. 11, 2016, 2:52 p.m.
On Fri, 11 Nov 2016 13:29:31 +0000, Martijn Pieters wrote:
> > On 11 Nov 2016, at 12:54, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Thu, 10 Nov 2016 17:01:22 +0000, Martijn Pieters wrote:
> >> super(Hint, self).__init__ will find the *next class* after Hint in the MRO for type(self). Depending on how Hint was used as a base class in *another class*, the next class in the MRO for self may or may not be object. Since object.__init__() doesn't take any arguments (not even *args or **kwargs), if either args or kw is non-empty you'd get a TypeError here.
> > 
> > In which case, I think the previous class of Hint must consume *args and **kw.
> 
> Probably, yes, still having a non-empty args / kw dataset by the time you reach `object` could be a bug.
> 
> But might there be a situation where extensions may have to deal with *potential* mixins? There are scenarios possible where one extension can't know what other extensions are installed and must always assume that certain arguments are needed.
> 
> I'm not familiar enough with where `Hint` is used, and it is probably better to only plug in a `try:...except:` at the end when the situation actually arises.

I hope there isn't a use of the Hint mixin in complex MRO tree. If we hit the
problem in future, I'd rather wanna remove the Hint class entirely. It's no
more than saving 3 lines of code.

Patch

--- a/mercurial/error.py
+++ b/mercurial/error.py
@@ -22,7 +22,7 @@  class Hint(object):
     pass remaining arguments to the exception class.
     """
     def __init__(self, *args, **kw):
-        self.hint = kw.pop('hint', None)
+        self.hint = kw.pop(u'hint', None)
         super(Hint, self).__init__(*args, **kw)
 
 class RevlogError(Hint, Exception):