This is an archive of the discontinued Mercurial Phabricator instance.

Differential D632

wrapfunction: use functools.partial if possible
ClosedPublic

Authored by quark on Sep 5 2017, 7:48 PM.

Details

Reviewers
phillco
Group Reviewers
hg-reviewers
Commits
rHG5361771f9714: wrapfunction: use functools.partial if possible
Summary

Every extensions.bind call inserts a frame in traceback:

... in closure
  return func(*(args + a), **kw)

which makes traceback noisy.

The Python stdlib has a functools.partial which is backed by C code and
does not pollute traceback. However it does not support instancemethod and
sets args attribute which could be problematic for alias handling.

This patch makes wrapfunction use functools.partial if we are wrapping a
function directly exported by a module (so it's impossible to be a class or
instance method), and special handles wrapfunction results so alias
handling code could handle args just fine.

As an example, hg rebase -s . -d . --traceback got 6 lines removed in my
setup:

 File "hg/mercurial/dispatch.py", line 898, in _dispatch
   cmdpats, cmdoptions)
-File "hg/mercurial/extensions.py", line 333, in closure
-  return func(*(args + a), **kw)
 File "hg/hgext/journal.py", line 84, in runcommand
   return orig(lui, repo, cmd, fullargs, *args)
-File "hg/mercurial/extensions.py", line 333, in closure
-  return func(*(args + a), **kw)
 File "fb-hgext/hgext3rd/fbamend/hiddenoverride.py", line 119, in runcommand
   result = orig(lui, repo, cmd, fullargs, *args)
 File "hg/mercurial/dispatch.py", line 660, in runcommand
   ret = _runcommand(ui, options, cmd, d)
-File "hg/mercurial/extensions.py", line 333, in closure
-  return func(*(args + a), **kw)
 File "hg/hgext/pager.py", line 69, in pagecmd
   return orig(ui, options, cmd, cmdfunc)
 ....

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Sep 5 2017, 7:48 PM
quark edited the summary of this revision. (Show Details)Sep 5 2017, 7:51 PM
phillco accepted this revision.Sep 6 2017, 6:03 PM
phillco added a subscriber: phillco.

Nice.

martinvonz added a subscriber: martinvonz.Sep 7 2017, 4:53 PM
martinvonz added inline comments.
mercurial/extensions.py
464

This patch makes test-py3-commands.t fail. I suspect it's because py3 has no "im_self".

quark edited the summary of this revision. (Show Details)Sep 7 2017, 6:08 PM
quark updated this revision to Diff 1678.
quark marked an inline comment as done.EditedSep 7 2017, 6:10 PM

Fixed. It's tricker than I thought - Python 3 does not have "unbound function" concept and there is no easy way to detect "unbound function". Python 3 descriptor protocol seems to treat function and partial differently. So I changed approach to check ismodule instead, which seems to be more conservative and correct.

Closed by commit rHG5361771f9714: wrapfunction: use functools.partial if possible (authored by quark). · Explain WhySep 8 2017, 12:21 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/dispatch.py (5 lines)
Mmercurial/extensions.py (11 lines)

Diff 1619

mercurial/dispatch.py

raise raise
except: # probably re-raises except: # probably re-raises
if not handlecommandexception(ui): if not handlecommandexception(ui):
raise raise
return -1 return -1
def aliasargs(fn, givenargs): def aliasargs(fn, givenargs):
args = getattr(fn, 'args', []) args = []
# only care about alias 'args', ignore 'args' set by extensions.wrapfunction
if not util.safehasattr(fn, '_origfunc'):
args = getattr(fn, 'args', args)
if args: if args:
cmd = ' '.join(map(util.shellquote, args)) cmd = ' '.join(map(util.shellquote, args))
nums = [] nums = []
def replacer(m): def replacer(m):
num = int(m.group(1)) - 1 num = int(m.group(1)) - 1
nums.append(num) nums.append(num)
if num < len(givenargs): if num < len(givenargs):

mercurial/extensions.py

# extensions.py - extension handling for mercurial # extensions.py - extension handling for mercurial
# #
# Copyright 2005-2007 Matt Mackall <mpm@selenic.com> # Copyright 2005-2007 Matt Mackall <mpm@selenic.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import functools
import imp import imp
import inspect import inspect
import os import os
from .i18n import ( from .i18n import (
_, _,
gettext, gettext,
) )
f(1, 2, bar=3) === bind(f, 1)(2, bar=3)''' f(1, 2, bar=3) === bind(f, 1)(2, bar=3)'''
assert callable(func) assert callable(func)
def closure(*a, **kw): def closure(*a, **kw):
return func(*(args + a), **kw) return func(*(args + a), **kw)
return closure return closure
def _updatewrapper(wrap, origfn, unboundwrapper): def _updatewrapper(wrap, origfn, unboundwrapper):
'''Copy and add some useful attributes to wrapper''' '''Copy and add some useful attributes to wrapper'''
wrap.__name__ = origfn.__name__
wrap.__module__ = getattr(origfn, '__module__') wrap.__module__ = getattr(origfn, '__module__')
wrap.__doc__ = getattr(origfn, '__doc__') wrap.__doc__ = getattr(origfn, '__doc__')
wrap.__dict__.update(getattr(origfn, '__dict__', {})) wrap.__dict__.update(getattr(origfn, '__dict__', {}))
wrap._origfunc = origfn wrap._origfunc = origfn
wrap._unboundwrapper = unboundwrapper wrap._unboundwrapper = unboundwrapper
def wrapcommand(table, command, wrapper, synopsis=None, docstring=None): def wrapcommand(table, command, wrapper, synopsis=None, docstring=None):
'''Wrap the command named `command' in table '''Wrap the command named `command' in table
work. Since you cannot control what other extensions are loaded by work. Since you cannot control what other extensions are loaded by
your end users, you should play nicely with others by using the your end users, you should play nicely with others by using the
subclass trick. subclass trick.
''' '''
assert callable(wrapper) assert callable(wrapper)
origfn = getattr(container, funcname) origfn = getattr(container, funcname)
assert callable(origfn) assert callable(origfn)
if util.safehasattr(origfn, 'im_self'):
martinvonzUnsubmitted
Done

This patch makes test-py3-commands.t fail. I suspect it's because py3 has no "im_self".

martinvonz: This patch makes test-py3-commands.t fail. I suspect it's because py3 has no "im_self".
# traditional bind, work with bound and unbound instancemethod, but
# will insert a frame in traceback output
wrap = bind(wrapper, origfn) wrap = bind(wrapper, origfn)
else:
# functools.partial, does not work with instancemethod, but won't
# insert a frame in traceback output
wrap = functools.partial(wrapper, origfn)
_updatewrapper(wrap, origfn, wrapper) _updatewrapper(wrap, origfn, wrapper)
setattr(container, funcname, wrap) setattr(container, funcname, wrap)
return origfn return origfn
def unwrapfunction(container, funcname, wrapper=None): def unwrapfunction(container, funcname, wrapper=None):
'''undo wrapfunction '''undo wrapfunction
If wrappers is None, undo the last wrap. Otherwise removes the wrapper If wrappers is None, undo the last wrap. Otherwise removes the wrapper