Trac #21322: coerce_binop has dangerous behaviour wrt additional arguments

`@coerce_binop` is an important decorator used on binary operators to
automagically apply the coercion framework on its arguments. It's widely
used on e.g. `gcd` operations (`_add_`, `_sub_` etc. have coercion
handled with another mechanism).

The current `@coerce_binop` has the following dangerous behaviour,
where, if the decorated method is given an extra unnamed argument, the
`self`-argument is swallowed:

{{{
    sage: P.<x> = GF(5)[]
    sage: f = x^2
    sage: g = x
    sage: f.gcd(g)
    x
    sage: f.gcd(g, 1)
    1
}}}

The reason is that `@coerce_binop` included a hack to allow calls such
as
`Polynomial.gcd(f,g)` where `f` and `g` are polynomials and the `gcd`
method is
decorated with `@coerce_binop`. The reason that this required a hack at
all is that `@coerce_binop` was implemented as a class: it seems that
creating decorators as classes implies a very unfortunate behaviour:

{{{
class MyDec:

   def __init__(self, f):
       self.f = f

   def __call__(self, x):
       print "decorated: ", self , x
       self.f(x)

mydec = MyDec

class A:

    def __init__(self, a):
        self.a = a

    @mydec
    def met(self, x):
        print "x:", x

myA = A(1)
myA.met(5)

}}}

the above prints something like

{{{
    decorated:  <__main__.MyDec instance at 0x7f63c5ab6c20> 5
}}}

and then crashes with

{{{
    AttributeError: MyDec instance has no attribute 'a'
}}}

This is because the `self` in `__call__` of `MyDec` becomes the `MyDec`
instance
-- the `A` instance is never passed to the decorating class instance!

The solution here is to rewrite `coerce_binop` as a function. At the
same time, we can use `@sage_wraps` which ensures proper documentation
(e.g. source file and line which was not retained in the previous
`coerce_binop`).

URL: https://trac.sagemath.org/21322
Reported by: jsrn
Ticket author(s): Johan Rosenkilde, Xavier Caruso
Reviewer(s): Miguel Marco

Author: Release Manager

src/sage/categories/euclidean_domains.py

@@ -250,8 +250,9 @@ def gcd(self, other):
 
             EXAMPLES::
 
-                sage: EuclideanDomains().ElementMethods().gcd(6,4)
-                2
+                sage: R.<x> = PolynomialRing(QQ, sparse=True)
+                sage: EuclideanDomains().element_class.gcd(x,x+1)
+                -1
             """
             A = self
             B = other

src/sage/categories/fields.py

@@ -235,7 +235,7 @@ def _gcd_univariate_polynomial(self, f, g):
                 x
 
             """
-            ret = EuclideanDomains().ElementMethods().gcd(f,g)
+            ret = EuclideanDomains().element_class.gcd(f,g)
             c = ret.leading_coefficient()
             if c.is_unit():
                 return (1/c)*ret

src/sage/rings/polynomial/padics/polynomial_padic_capped_relative_dense.py

@@ -1139,8 +1139,7 @@ def xgcd(self, right):
         from sage.misc.stopgap import stopgap
         stopgap("Extended gcd computations over p-adic fields are performed using the standard Euclidean algorithm which might produce mathematically incorrect results in some cases.", 13439)
 
-        from sage.rings.polynomial.polynomial_element_generic import Polynomial_generic_field
-        return Polynomial_generic_field.xgcd(self,right)
+        return Polynomial_generic_cdv.xgcd(self,right)
 
     #def discriminant(self):
     #    raise NotImplementedError

src/sage/structure/element.pyx

@@ -154,6 +154,7 @@ from sage.misc import sageinspect
 from sage.misc.classcall_metaclass cimport ClasscallMetaclass
 from sage.misc.superseded import deprecated_function_alias
 from sage.arith.numerical_approx cimport digits_to_bits
+from sage.misc.decorators import sage_wraps
 
 # Create a dummy attribute error, using some kind of lazy error message,
 # so that neither the error itself not the message need to be created
@@ -3329,170 +3330,115 @@ def coercion_traceback(dump=True):
         return coercion_model.exception_stack()
 
 
-cdef class NamedBinopMethod:
-    """
-    A decorator to be used on binary operation methods that should operate
-    on elements of the same parent. If the parents of the arguments differ,
-    coercion is performed, then the method is re-looked up by name on the
-    first argument.
-
-    In short, using the ``NamedBinopMethod`` (alias ``coerce_binop``) decorator
-    on a method gives it the exact same semantics of the basic arithmetic
-    operations like ``_add_``, ``_sub_``, etc. in that both operands are
-    guaranteed to have exactly the same parent.
-    """
-    cdef _self
-    cdef _func
-    cdef _name
-
-    def __init__(self, func, name=None, obj=None):
-        """
-        TESTS::
-
-            sage: from sage.structure.element import NamedBinopMethod
-            sage: NamedBinopMethod(gcd)(12, 15)
-            3
-        """
-        self._func = func
-        if name is None:
-            if isinstance(func, types.FunctionType):
-                name = func.__name__
-            if isinstance(func, types.UnboundMethodType):
-                name = func.__func__.__name__
-            else:
-                name = func.__name__
-        self._name = name
-        self._self = obj
-
-    def __call__(self, x, y=None, **kwds):
-        """
-        TESTS::
-
-            sage: from sage.structure.element import NamedBinopMethod
-            sage: test_func = NamedBinopMethod(lambda x, y, **kwds: (x, y, kwds), '_add_')
-            sage: class test_class(Rational):
-            ....:     def __init__(self,value):
-            ....:         self.v = value
-            ....:     @NamedBinopMethod
-            ....:     def test_add(self, other, keyword='z'):
-            ....:         return (self.v, other, keyword)
-
-        Calls func directly if the two arguments have the same parent::
-
-            sage: test_func(1, 2)
-            (1, 2, {})
-            sage: x = test_class(1)
-            sage: x.test_add(1/2)
-            (1, 1/2, 'z')
-            sage: x.test_add(1/2, keyword=3)
-            (1, 1/2, 3)
-
-        Passes through coercion and does a method lookup if the
-        left operand is not the same::
-
-            sage: test_func(0.5, 1)
-            (0.500000000000000, 1.00000000000000, {})
-            sage: test_func(1, 2, algorithm='fast')
-            (1, 2, {'algorithm': 'fast'})
-            sage: test_func(1, 1/2)
-            3/2
-            sage: x.test_add(2)
-            (1, 2, 'z')
-            sage: x.test_add(2, keyword=3)
-            (1, 2, 3)
-
-        A real example::
-
-            sage: R1=QQ['x,y']
-            sage: R2=QQ['x,y,z']
-            sage: f=R1(1)
-            sage: g=R1(2)
-            sage: h=R2(1)
-            sage: f.gcd(g)
-            1
-            sage: f.gcd(g,algorithm='modular')
-            1
-            sage: f.gcd(h)
-            1
-            sage: f.gcd(h,algorithm='modular')
-            1
-            sage: h.gcd(f)
-            1
-            sage: h.gcd(f,algorithm='modular')
-            1
-        """
-        if y is None:
-            if self._self is None:
-                self._func(x, **kwds)
-            else:
-                x, y = self._self, x
-        if not have_same_parent(x, y):
-            old_x = x
-            x,y = coercion_model.canonical_coercion(x, y)
-            if old_x is x:
-                return self._func(x,y, **kwds)
-            else:
-                return getattr(x, self._name)(y, **kwds)
-        else:
-            return self._func(x,y, **kwds)
-
-    def __get__(self, obj, objtype):
-        """
-        Used to transform from an unbound to a bound method.
-
-        TESTS::
-            sage: from sage.structure.element import NamedBinopMethod
-            sage: R.<x> = ZZ[]
-            sage: isinstance(x.quo_rem, NamedBinopMethod)
-            True
-            sage: x.quo_rem(x)
-            (1, 0)
-            sage: type(x).quo_rem(x,x)
-            (1, 0)
-        """
-        return NamedBinopMethod(self._func, self._name, obj)
-
-    def _sage_doc_(self):
-        """
-        Return the docstring of the wrapped object for introspection.
-
-        EXAMPLES::
-
-            sage: from sage.structure.element import NamedBinopMethod
-            sage: g = NamedBinopMethod(gcd)
-            sage: from sage.misc.sageinspect import sage_getdoc
-            sage: sage_getdoc(g) == sage_getdoc(gcd)
-            True
-        """
-        return sageinspect._sage_getdoc_unformatted(self._func)
+def coerce_binop(method):
+    r"""
+    Decorator for a binary operator method for applying coercion to the
+    arguments before calling the method.
+
+    Consider a parent class in the category framework, `S`, whose element class
+    expose a method `binop`. If `a` and `b` are elements of `S`, then
+    `a.binop(b)` behaves as expected. If `a` and `b` are not elements of `S`,
+    but rather have a common parent `T` whose element class also exposes
+    `binop`, we would rather expect `a.binop(b)` to compute `aa.binop(bb)`,
+    where `aa = T(a)` and `bb = T(b)`. This decorator ensures that behaviour
+    without having to otherwise modify the implementation of `binop` on the
+    element class of `A`.
+
+    Since coercion will be attempted on the arguments of the decorated method, a
+    `TypeError` will be thrown if there is no common parent between the
+    elements. An `AttributeError` or `NotImplementedError` or similar will be
+    thrown if there is a common parent of the arguments, but its element class
+    does not implement a method of the same name as the decorated method.
+
+    EXAMPLES:
+
+    Sparse polynomial rings uses `@coerce_binop` on `gcd`::
+
+        sage: S.<x> = PolynomialRing(ZZ,sparse=True)
+        sage: f = x^2
+        sage: g = x
+        sage: f.gcd(g)  #indirect doctest 
+        x
+        sage: T = PolynomialRing(QQ, name='x', sparse=True)
+        sage: h = 1/2*T(x)
+        sage: u = f.gcd(h); u  #indirect doctest 
+        x
+        sage: u.parent() == T
+        True
 
-    def _sage_src_(self):
-        """
-        Return the source of the wrapped object for introspection.
+    Another real example::
 
-        EXAMPLES::
+        sage: R1=QQ['x,y']
+        sage: R2=QQ['x,y,z']
+        sage: f=R1(1)
+        sage: g=R1(2)
+        sage: h=R2(1)
+        sage: f.gcd(g)
+        1
+        sage: f.gcd(g,algorithm='modular')
+        1
+        sage: f.gcd(h)
+        1
+        sage: f.gcd(h,algorithm='modular')
+        1
+        sage: h.gcd(f)
+        1
+        sage: h.gcd(f,'modular')
+        1
 
-            sage: from sage.structure.element import NamedBinopMethod
-            sage: g = NamedBinopMethod(gcd)
-            sage: 'def gcd(' in g._sage_src_()
-            True
-        """
-        return sageinspect.sage_getsource(self._func)
+    We demonstrate a small class using `@coerce_binop` on a method::
+
+        sage: from sage.structure.element import coerce_binop
+        sage: class MyRational(Rational):
+        ....:     def __init__(self,value):
+        ....:         self.v = value
+        ....:     @coerce_binop
+        ....:     def test_add(self, other, keyword='z'):
+        ....:         return (self.v, other, keyword)
+
+    Calls func directly if the two arguments have the same parent::
+
+        sage: x = MyRational(1)
+        sage: x.test_add(1/2)
+        (1, 1/2, 'z')
+        sage: x.test_add(1/2, keyword=3)
+        (1, 1/2, 3)
+
+    Passes through coercion and does a method lookup if the left operand is not
+    the same. If the common parent's element class does not have a method of the
+    same name, an exception is raised::
+
+        sage: x.test_add(2)
+        (1, 2, 'z')
+        sage: x.test_add(2, keyword=3)
+        (1, 2, 3)
+        sage: x.test_add(CC(2))
+        Traceback (most recent call last):
+        ...
+        AttributeError: 'sage.rings.complex_number.ComplexNumber' object has no attribute 'test_add'
 
-    def _sage_argspec_(self):
-        """
-        Return the argspec of the wrapped object for introspection.
+    TESTS:
 
-        EXAMPLES::
+    Test that additional arguments given to the method does not override the
+    `self` argument, see #21322::
 
-            sage: from sage.structure.element import NamedBinopMethod
-            sage: g = NamedBinopMethod(gcd)
-            sage: g._sage_argspec_()
-            ArgSpec(args=['a', 'b'], varargs=None, keywords='kwargs', defaults=(None,))
-        """
-        return sageinspect.sage_getargspec(self._func)
+        sage: f.gcd(g, 1)
+        Traceback (most recent call last):
+        ...
+        TypeError: algorithm 1 not supported
+    """
+    @sage_wraps(method)
+    def new_method(self, other, *args, **kwargs):
+        if have_same_parent(self, other):
+            return method(self, other, *args, **kwargs)
+        else:
+            a, b = coercion_model.canonical_coercion(self, other)
+            if a is self:
+                return method(a, b, *args, **kwargs)
+            else:
+                return getattr(a, method.__name__)(b, *args, **kwargs)
+    return new_method
 
-coerce_binop = NamedBinopMethod
 
 ###############################################################################