Determine argspec of static methods with defaults in Cython files

[simon-king-jena]

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

CC: @nthiery

Component: documentation

Keywords: staticmethod cython argspec

Author: Simon King

Branch/Commit: 143b567

Reviewer: Travis Scrimshaw

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

[simon-king-jena]

comment:1

That's really silly, as the names and defaults are easy to get:

sage: Bla.join.func_code.co_varnames
('categories', 'as_list', 'ignore_axioms', 'axioms')
sage: Bla.join.func_defaults
(False, (), ())

[nbruin]

comment:2

People have complained about inspect.isfunction being to picky before and a monkey patch has been proposed: http://cython-devel.gmang.org/2013-August/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.

[simon-king-jena]

comment:3

Replying to @nbruin:

People have complained about inspect.isfunction being to picky before and a monkey patch has been proposed: http://cython-devel.gmang.org/2013-August/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.

[simon-king-jena]

comment:4

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/site-packages/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!

[nbruin]

comment:5

Replying to @simon-king-jena:

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!

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.

[simon-king-jena]

comment:6

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.

[simon-king-jena]

comment:7

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.

[simon-king-jena]

comment:8

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/linux-etl7.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?

[simon-king-jena]

comment:9

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

[simon-king-jena]

Branch: u/SimonKing/ticket/16309

[simon-king-jena]

Commit: 32a374f

[simon-king-jena]

comment:11

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)
<ipython-input-4-7be424edddca> in <module>()
----> 1 sage_getsource(Bla)

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/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/site-packages/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

[simon-king-jena]

comment:12

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.

[simon-king-jena]

Author: Simon King

[simon-king-jena]

comment:13

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)
<ipython-input-4-850f43ab2175> in <module>()
----> 1 sage_getsourcelines(B)

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/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/site-packages/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/site-packages/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 built-in class'.format(object))
    409     if ismethod(object):
    410         object = object.im_func

TypeError: <module '__builtin__' (built-in)> is a built-in 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 super-class!).

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.

[simon-king-jena]

comment:14

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!

[sagetrac-git]

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

7877377

Get source code for nested classes defined in Cython files

[sagetrac-git]

Changed commit from 32a374f to 7877377

[simon-king-jena]

comment:16

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.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:18

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.

[tscrim]

Changed branch from u/SimonKing/ticket/16309 to u/tscrim/argspec_static_methods-16309

[tscrim]

Changed commit from 7877377 to 143b567

[tscrim]

comment:19

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_methods-16309
143b567	Removed unneeded import from doctest.

[vbraun]

Changed branch from u/tscrim/argspec_static_methods-16309 to 143b567