coerce_binop has dangerous behaviour wrt additional arguments

[johanrosenkilde]

@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).

CC: @xcaruso @miguelmarco

Component: coercion

Keywords: sd75

Author: Johan Rosenkilde, Xavier Caruso

Branch/Commit: 7344f3a

Reviewer: Miguel Marco

Issue created by migration from https://trac.sagemath.org/ticket/21322

[johanrosenkilde]

Changed author from Johan Rosenkilde to Johan Rosenkilde, Xavier Caruso

[johanrosenkilde]

Branch: u/jsrn/21322_coerce_binop

[johanrosenkilde]

Commit: f49db4a

[johanrosenkilde]

comment:3

As described.

---

New commits:

f49db4a

Rewrite coerce_binop. Fix a bug exposed by this in fields.py

[johanrosenkilde]

Changed keywords from none to sd75

[xcaruso]

comment:4

For information, the popular decorator @cached_method is also implemented by a class as uses the same hack consisting in creating on the fly a new decorator for each instance (through a __get__ method).

[johanrosenkilde]

comment:5

I got the following, possibly spurious doc-test failures:

sage -t src/sage/interfaces/sage0.py  # Timed out
sage -t src/sage/graphs/tutte_polynomial.py  # Timed out
sage -t src/sage/categories/euclidean_domains.py  # 1 doctest failed
sage -t src/sage/tests/cmdline.py  # 8 doctests failed
sage -t src/sage/interacts/debugger.py  # Timed out
sage -t src/sage/structure/element.pyx  # 6 doctests failed
sage -t src/sage/quivers/algebra_elements.pxi  # Timed out
sage -t src/sage/misc/randstate.pyx  # Timed out
sage -t src/sage/rings/polynomial/padics/polynomial_padic_capped_relative_dense.py  # 1 doctest failed
sage -t src/sage/doctest/forker.py  # Timed out
sage -t src/sage/repl/interpreter.py  # Tab character found
----------------------------------------------------------------------

[xcaruso]

Changed branch from u/jsrn/21322_coerce_binop to u/caruso/21322_coerce_binop

[sagetrac-git]

Changed commit from f49db4a to cfb5326

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

cfb5326

Small fixes. All doctests pass (hopefully)

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

7344f3a

Small change in a doctest

[sagetrac-git]

Changed commit from cfb5326 to 7344f3a

[miguelmarco]

comment:10

I don't get any of the errors pointed by jsm.

Otherwise, lgtm.

---

New commits:

7344f3a

Small change in a doctest

[miguelmarco]

Reviewer: Miguel Marco

[vbraun]

Changed branch from u/caruso/21322_coerce_binop to 7344f3a