From patchwork Thu Jun 30 16:59:05 2016
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [10, of,
 10] extensions: devel-warn ui object escaping from ui/extsetup
From: Jun Wu <quark@fb.com>
X-Patchwork-Id: 15688
Message-Id: <caf11c2daf24c7f94a50.1467305945@x1c>
To: <mercurial-devel@mercurial-scm.org>
Date: Thu, 30 Jun 2016 17:59:05 +0100

# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1467296787 -3600
#      Thu Jun 30 15:26:27 2016 +0100
# Node ID caf11c2daf24c7f94a506acb440e80ea37898870
# Parent  7c2dc6643d1c37dbc388ce4ac7112341f0926f28
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r caf11c2daf24
extensions: devel-warn ui object escaping from ui/extsetup

Storing a reference to the ui object in uisetup or extsetup can cause troubles
since the ui object can be outdated because of ui.copy()s or ui.__init__()s.
It's especially an issue with chgserver since it's designed to call uisetup
only once.

This patch adds a develwarn to notify developers this usage is discouraged.

See also https://twitter.com/durin42/status/739836476318318592 .

diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -7,8 +7,10 @@
 
 from __future__ import absolute_import
 
+import contextlib
 import imp
 import os
+import sys
 
 from .i18n import (
     _,
@@ -127,20 +129,31 @@ def load(ui, name, path):
         fn(loaded=True)
     return mod
 
+@contextlib.contextmanager
+def _checkuiescape(name, ui):
+    nref = sys.getrefcount(ui)
+    yield
+    nrefchange = sys.getrefcount(ui) - nref
+    if nrefchange > 0 and (ui.configbool('devel', 'all-warnings')
+                           or ui.configbool('devel', 'check-uiescape')):
+        ui.develwarn('"ui" escaped from %s' % name, stacklevel=None)
+
 def _runuisetup(name, ui):
-    uisetup = getattr(_extensions[name], 'uisetup', None)
-    if uisetup:
-        uisetup(ui)
+    with _checkuiescape('%s.uisetup' % name, ui):
+        uisetup = getattr(_extensions[name], 'uisetup', None)
+        if uisetup:
+            uisetup(ui)
 
 def _runextsetup(name, ui):
-    extsetup = getattr(_extensions[name], 'extsetup', None)
-    if extsetup:
-        try:
-            extsetup(ui)
-        except TypeError:
-            if extsetup.func_code.co_argcount != 0:
-                raise
-            extsetup() # old extsetup with no ui argument
+    with _checkuiescape('%s.extsetup' % name, ui):
+        extsetup = getattr(_extensions[name], 'extsetup', None)
+        if extsetup:
+            try:
+                extsetup(ui)
+            except TypeError:
+                if extsetup.func_code.co_argcount != 0:
+                    raise
+                extsetup() # old extsetup with no ui argument
 
 def loadall(ui):
     result = ui.configitems("extensions")
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -1164,6 +1164,7 @@ class ui(object):
                 return
         msg = 'devel-warn: ' + msg
         if stacklevel is None:
+            self.write_err('%s\n' % msg)
             self.log('develwarn', msg)
             return
         stacklevel += 1 # get in develwarn
diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t
+++ b/tests/test-devel-warnings.t
@@ -1,4 +1,3 @@
-
   $ cat << EOF > buggylocking.py
   > """A small extension that tests our developer warnings
   > """
@@ -74,6 +73,18 @@
   > all-warnings=1
   > EOF
 
+  $ cat << EOF > uiescape.py
+  > a = []
+  > def uisetup(ui):
+  >     ui.write('ui escaping\n')
+  >     a.append(ui)
+  > EOF
+
+  $ hg --config extensions.uiescape=uiescape.py version -q
+  ui escaping
+  devel-warn: "ui" escaped from uiescape.uisetup
+  Mercurial * (glob)
+
   $ hg init lock-checker
   $ cd lock-checker
   $ hg buggylocking