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:  sage7.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
comment:2 Changed 3 years ago by
 Branch set to u/jsrn/21322_coerce_binop
comment:3 Changed 3 years ago by
 Commit set to f49db4ab66d2e6c693981ef7fd5c5552a94629c2
 Keywords sd75 added
comment:4 Changed 3 years ago by
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
I got the following, possibly spurious doctest 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
 Branch changed from u/jsrn/21322_coerce_binop to u/caruso/21322_coerce_binop
comment:7 Changed 3 years ago by
 Commit changed from f49db4ab66d2e6c693981ef7fd5c5552a94629c2 to cfb532633e7b7df58b8b4ec00c7dc1e0b99c90aa
Branch pushed to git repo; I updated commit sha1. New commits:
cfb5326  Small fixes. All doctests pass (hopefully)

comment:8 Changed 3 years ago by
 Status changed from new to needs_review
comment:9 Changed 3 years ago by
 Commit changed from cfb532633e7b7df58b8b4ec00c7dc1e0b99c90aa to 7344f3a7a1eaf9b9bd35f37556f7bac2614364c0
Branch pushed to git repo; I updated commit sha1. New commits:
7344f3a  Small change in a doctest

comment:10 Changed 3 years ago by
 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:
7344f3a  Small change in a doctest

comment:11 Changed 3 years ago by
 Branch changed from u/caruso/21322_coerce_binop to 7344f3a7a1eaf9b9bd35f37556f7bac2614364c0
 Resolution set to fixed
 Status changed from positive_review to closed
As described.
New commits:
Rewrite coerce_binop. Fix a bug exposed by this in fields.py