Opened 3 years ago

Closed 3 years ago

#21322 closed defect (fixed)

coerce_binop has dangerous behaviour wrt additional arguments

Reported by: jsrn Owned by:
Priority: major Milestone: sage-7.4
Component: coercion Keywords: sd75
Cc: caruso, mmarco Merged in:
Authors: Johan Rosenkilde, Xavier Caruso Reviewers: Miguel Marco
Report Upstream: N/A Work issues:
Branch: 7344f3a (Commits) Commit: 7344f3a7a1eaf9b9bd35f37556f7bac2614364c0
Dependencies: Stopgaps:

Description

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

Change History (11)

comment:1 Changed 3 years ago by jsrn

  • Authors changed from Johan Rosenkilde to Johan Rosenkilde, Xavier Caruso

comment:2 Changed 3 years ago by jsrn

  • Branch set to u/jsrn/21322_coerce_binop

comment:3 Changed 3 years ago by jsrn

  • Commit set to f49db4ab66d2e6c693981ef7fd5c5552a94629c2
  • Keywords sd75 added

As described.


New commits:

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

comment:4 Changed 3 years ago by caruso

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

comment:5 Changed 3 years ago by jsrn

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
----------------------------------------------------------------------

comment:6 Changed 3 years ago by caruso

  • Branch changed from u/jsrn/21322_coerce_binop to u/caruso/21322_coerce_binop

comment:7 Changed 3 years ago by git

  • Commit changed from f49db4ab66d2e6c693981ef7fd5c5552a94629c2 to cfb532633e7b7df58b8b4ec00c7dc1e0b99c90aa

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

cfb5326Small fixes. All doctests pass (hopefully)

comment:8 Changed 3 years ago by caruso

  • Status changed from new to needs_review

comment:9 Changed 3 years ago by git

  • Commit changed from cfb532633e7b7df58b8b4ec00c7dc1e0b99c90aa to 7344f3a7a1eaf9b9bd35f37556f7bac2614364c0

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

7344f3aSmall change in a doctest

comment:10 Changed 3 years ago by mmarco

  • Reviewers set to Miguel Marco
  • Status changed from needs_review to positive_review

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

Otherwise, lgtm.


New commits:

7344f3aSmall change in a doctest

comment:11 Changed 3 years ago by vbraun

  • Branch changed from u/caruso/21322_coerce_binop to 7344f3a7a1eaf9b9bd35f37556f7bac2614364c0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.