Opened 8 years ago
Closed 8 years ago
#16309 closed defect (fixed)
Determine argspec of static methods with defaults in Cython files
Reported by:  Simon King  Owned by:  

Priority:  major  Milestone:  sage6.3 
Component:  documentation  Keywords:  staticmethod cython argspec 
Cc:  Nicolas M. Thiéry  Merged in:  
Authors:  Simon King  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  143b567 (Commits, GitHub, GitLab)  Commit:  143b56779f9e69c9718765a0312fc37274bc34dc 
Dependencies:  Stopgaps: 
Description
Fix this:
sage: cython(""" ....: class Bla: ....: @staticmethod ....: def join(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): ....: pass ....: """) sage: from sage.misc.sageinspect import sage_getargspec sage: sage_getargspec(Bla.join) File "<string>", line unknown SyntaxError: Unexpected EOF while parsing argument list
This does not happen if join
is a bound or unbound (not static) method.
Since this will affect building the docs (see #16296), I'd say the component is "documentation".
Change History (20)
comment:1 Changed 8 years ago by
comment:2 followup: 3 Changed 8 years ago by
People have complained about inspect.isfunction
being to picky before and a monkey patch has been proposed: http://cythondevel.gmang.org/2013August/003804.html
We wouldn't need to hack that badly. We can just go with ducktyping: if hasattr(obj,'func_code')
we're probably looking at a function (line 1292 of sageinspect.py).
It's been noted that testing for <type 'cython_function_or_method'>
cannot be done via isinstance
, since every cython module makes a separate version of that type (which is considered a bug but unlikely to be fixed (soon)). By the time you're checking for the name of a type you're probably better off looking for a telltale attribute.
comment:3 Changed 8 years ago by
Replying to nbruin:
People have complained about
inspect.isfunction
being to picky before and a monkey patch has been proposed: http://cythondevel.gmang.org/2013August/003804.html
Indeed it is a bit odd that the inspect module does type checks rather than duck typing.
We wouldn't need to hack that badly. We can just go with ducktyping: if
hasattr(obj,'func_code')
we're probably looking at a function (line 1292 of sageinspect.py).
That's an important information, that I will use in #16296 for the @abstract_method decorator.
Anyway, I suppose we need to somehow copy what is done in the inspect module, replacing the type check by duck typing.
comment:4 followup: 5 Changed 8 years ago by
It gets worse.
sage: cython(""" ....: class Bla: ....: @staticmethod ....: def join(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): ....: pass ....: """) sage: from sage.misc.sageinspect import sage_getsource sage: sage_getsource(Bla) Traceback (most recent call last): ... /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/sage/misc/sageinspect.pyc in sage_getsourcelines(obj, is_binary) 1836 if B is not None and B is not obj: 1837 return sage_getsourcelines(B) > 1838 if obj.__class__ != type: 1839 return sage_getsourcelines(obj.__class__) 1840 raise AttributeError: class Bla has no attribute '__class__'
However, the source of Bla.join
is almost correctly determined:
sage: print sage_getsource(Bla.join) def join(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): pass
This should include the decorator!
comment:5 followup: 6 Changed 8 years ago by
Replying to SimonKing:
However, the source of
Bla.join
is almost correctly determined:sage: print sage_getsource(Bla.join) def join(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): passThis should include the decorator!
Are you sure? Getsource does exactly what __doc__
indicates. It would be @staticmethod
's responsibility to modify that. However, I think cython processes @staticmethod
specially, so you'd need to file a bug to cython for that. I do agree it would be nice to see that a method is static, but presently you'd need to write that in the docstring yourself.
You are correct in noting that sage_getsource
for @staticmethod
in a .py
file does include the decorator. That suggests that cython's @staticmethod
processing should perhaps adjust Bla.join.func_code.co_firstlineno
and/or Bla.join.__doc__
, where it writes file and line number info too.
comment:6 Changed 8 years ago by
Replying to nbruin:
This should include the decorator!
Are you sure? Getsource does exactly what
__doc__
indicates. It would be@staticmethod
's responsibility to modify that.
OK, good point. I think it would be odd to fix that on our side.
comment:7 Changed 8 years ago by
Inserting the following
if hasattr(obj, 'func_code'): try: args, varargs, varkw = inspect.getargs(obj.func_code) return inspect.ArgSpec(args, varargs, varkw, obj.func_defaults) except (TypeError, AttributeError): pass
then the problem is solved for static methods of Python classes defined in Cython code, however it is not solved for static methods of cdef classes and cpdef methods of cdef classes.
comment:8 Changed 8 years ago by
Next problem:
sage: cython(""" ....: class Foo: ....: @staticmethod ....: def join(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): pass ....: cdef class Bar: ....: @staticmethod ....: def join(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): pass ....: cpdef meet(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): pass ....: """) sage: from sage.misc.sageinspect import sage_getargspec, sage_getsource, sage_getsourcelines sage: sage_getfile(Foo) '/home/king/.sage/temp/linuxetl7.site/3169/spyx/_home_king__sage_temp_linux_etl7_site_3169_tmp_qz2nuW_spyx/_home_king__sage_temp_linux_etl7_site_3169_tmp_qz2nuW_spyx_0.so'
Shouldn't the file end with .pyx
?
comment:9 Changed 8 years ago by
Problem, that is probably the reason why sage_getargspecs has problems:
sage: source = ' def join(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): pass\n\n' sage: from sage.misc.sageinspect import _sage_getargspec_cython sage: _sage_getargspec_cython(source) File "<string>", line unknown SyntaxError: Unexpected EOF while parsing argument list
and this (as I just found) boils down to this:
sage: from sage.misc.sageinspect import _split_syntactical_unit sage: _split_syntactical_unit('()): pass') ('())', ': pass')
The expected result is
('()', '): pass')
comment:10 Changed 8 years ago by
Branch:  → u/SimonKing/ticket/16309 

Created:  May 8, 2014, 2:23:09 PM → May 8, 2014, 2:23:09 PM 
Modified:  May 8, 2014, 8:48:39 PM → May 8, 2014, 8:48:39 PM 
comment:11 Changed 8 years ago by
Commit:  → 32a374ffaece155c52a21cb2cef970eee18a79e4 

With the new branch, most problems mentioned here are fixed (and tested against), but the following remains a problem
sage: cython(''' ....: class Bla: ....: @staticmethod ....: def join(categories, bint as_list = False, tuple ignore_axioms=(), tuple axioms=()): pass ....: ''') sage: from sage.misc.sageinspect import sage_getsource sage: sage_getsource(Bla)  AttributeError Traceback (most recent call last) <ipythoninput47be424edddca> in <module>() > 1 sage_getsource(Bla) /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/sage/misc/sageinspect.pyc in sage_getsource(obj, is_binary) 1610 pass 1611 > 1612 t = sage_getsourcelines(obj, is_binary) 1613 if not t: 1614 return None /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/sage/misc/sageinspect.pyc in sage_getsourcelines(obj, is_binary) 1877 if B is not None and B is not obj: 1878 return sage_getsourcelines(B) > 1879 if obj.__class__ != type: 1880 return sage_getsourcelines(obj.__class__) 1881 raise AttributeError: class Bla has no attribute '__class__'
New commits:
55b73e1  Fix sage_getargspec on static methods of Python classes in Cython code

32a374f  Fix parsing of Cython function definition in sageinspect

comment:12 Changed 8 years ago by
Authors:  → Simon King 

Status:  new → needs_review 
Hmm. At the moment, I tend to say that this already needs review. The problem described in the previous comment only occurs of Bla
has no docstring, since otherwise Cython adds embedding information to the docstring.
Hence, as long as all classes in Sage get a docstring, we will have no problem. And the fix should be enough for #16296.
comment:13 Changed 8 years ago by
The following should be fixed, since it causes other trouble with #16296:
sage: cython(''' ....: class A: ....: def __init__(self): ....: "some init doc" ....: pass ....: from sage.misc.nested_class import NestedClassMetaclass ....: class B: ....: __metaclass__ = NestedClassMetaclass ....: class A(A): ....: pass ....: ''') sage: from sage.misc.sageinspect sage_getsourcelines sage: sage_getsourcelines(B.A) (['class A:\n', ' def __init__(self):\n', ' "some init doc"\n', ' pass\n'], 7) sage: sage_getsourcelines(B)  TypeError Traceback (most recent call last) <ipythoninput4850f43ab2175> in <module>() > 1 sage_getsourcelines(B) /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/sage/misc/sageinspect.pyc in sage_getsourcelines(obj, is_binary) 1876 B = None 1877 if B is not None and B is not obj: > 1878 return sage_getsourcelines(B) 1879 if obj.__class__ != type: 1880 return sage_getsourcelines(obj.__class__) /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/sage/misc/sageinspect.pyc in sage_getsourcelines(obj, is_binary) 1863 if pos is None: 1864 try: > 1865 return inspect.getsourcelines(obj) 1866 except IOError: 1867 if (not hasattr(obj, '__class__')) or hasattr(obj,'__metaclass__'): /home/king/Sage/git/sage/local/lib/python/inspect.pyc in getsourcelines(object) 688 original source file the first line of code was found. An IOError is 689 raised if the source code cannot be retrieved.""" > 690 lines, lnum = findsource(object) 691 692 if ismodule(object): return lines, 0 /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/IPython/core/ultratb.pyc in findsource(object) 149 FIXED version with which we monkeypatch the stdlib to work around a bug.""" 150 > 151 file = getsourcefile(object) or getfile(object) 152 # If the object is a frame, then trying to get the globals dict from its 153 # module won't work. Instead, the frame object itself has the globals /home/king/Sage/git/sage/local/lib/python/inspect.pyc in getsourcefile(object) 442 Return None if no way can be identified to get the source. 443 """ > 444 filename = getfile(object) 445 if string.lower(filename[4:]) in ('.pyc', '.pyo'): 446 filename = filename[:4] + '.py' /home/king/Sage/git/sage/local/lib/python/inspect.pyc in getfile(object) 406 if hasattr(object, '__file__'): 407 return object.__file__ > 408 raise TypeError('{!r} is a builtin class'.format(object)) 409 if ismethod(object): 410 object = object.im_func TypeError: <module '__builtin__' (builtin)> is a builtin class
So, it detects B.A
in the wrong location (reason: It tries to find the embedding information from what Cython inserted into the docstring of the class, and if this has none, then it considers the docstring of __init__
even if this is not defined in the class but in a superclass!).
Moreover, it cannot get the source lines of B
, which I think is because we catch an IOError
but not a TypeError
raised by inspect.getsourcelines
.
comment:14 Changed 8 years ago by
Catching the TypeError
does not solve the example above, but it does solve a modified example: If B
is provided with a docstring, then its source can be detected from the embedding information.
So, what we should learn: Always provide a docstring!
comment:15 Changed 8 years ago by
Commit:  32a374ffaece155c52a21cb2cef970eee18a79e4 → 787737761fd909e740f35440ec53ee722a0c52ec 

Branch pushed to git repo; I updated commit sha1. New commits:
7877377  Get source code for nested classes defined in Cython files

comment:16 Changed 8 years ago by
Cc:  Nicolas M. Thiéry added 

So far, I have no idea how to find the sources of a class that is defined in a Cython file, has no docstring and inherits a documented __init__
method from some other class. However, if the class has a documentation, then we can now correctly detect a nested class.
Note that a nested class defined in a Cython file does not have a dot in its name. However, Cython provides an attribute __qualname__
, and there one does find the full name, including the dot. I have changed sageinspect to use this (new?) feature.
I suppose now it can really be reviewed. It will be essential for Cythoning sage.categories.category.
comment:17 Changed 8 years ago by
Milestone:  sage6.2 → sage6.3 

comment:18 Changed 8 years ago by
Reviewers:  → Travis Scrimshaw 

Hey Simon,
Once you remove line 1643:
....: from sage.misc.nested_class import NestedClassMetaclass
as it is unused in the doctest, LGTM and you can set a positive review.
comment:19 Changed 8 years ago by
Branch:  u/SimonKing/ticket/16309 → u/tscrim/argspec_static_methods16309 

Commit:  787737761fd909e740f35440ec53ee722a0c52ec → 143b56779f9e69c9718765a0312fc37274bc34dc 
Status:  needs_review → positive_review 
I've removed the line from the doctest and since it is a trivial change, I'm going to set this to positive review.
sage t sageinspect.py [263 tests, 39.52 s]  All tests passed!  Total time for all tests: 39.9 seconds cpu time: 5.0 seconds cumulative wall time: 39.5 seconds
New commits:
273cb86  Merge branch 'u/SimonKing/ticket/16309' of trac.sagemath.org:sage into u/tscrim/argspec_static_methods16309

143b567  Removed unneeded import from doctest.

comment:20 Changed 8 years ago by
Branch:  u/tscrim/argspec_static_methods16309 → 143b56779f9e69c9718765a0312fc37274bc34dc 

Resolution:  → fixed 
Status:  positive_review → closed 
That's really silly, as the names and defaults are easy to get: