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: sage-6.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:

Status badges

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

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, (), ())

comment:2 Changed 8 years ago by Nils Bruin

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.

comment:3 in reply to:  2 Changed 8 years ago by Simon King

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.

comment:4 Changed 8 years ago by Simon King

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!

Last edited 8 years ago by Simon King (previous) (diff)

comment:5 in reply to:  4 ; Changed 8 years ago by Nils Bruin

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=()):
        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.

Last edited 8 years ago by Nils Bruin (previous) (diff)

comment:6 in reply to:  5 Changed 8 years ago by Simon King

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

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

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?

comment:9 Changed 8 years ago by Simon King

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

Branch: u/SimonKing/ticket/16309
Created: May 8, 2014, 2:23:09 PMMay 8, 2014, 2:23:09 PM
Modified: May 8, 2014, 8:48:39 PMMay 8, 2014, 8:48:39 PM

comment:11 Changed 8 years ago by Simon King

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

55b73e1Fix sage_getargspec on static methods of Python classes in Cython code
32a374fFix parsing of Cython function definition in sageinspect

comment:12 Changed 8 years ago by Simon King

Authors: Simon King
Status: newneeds_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 Simon King

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.

comment:14 Changed 8 years ago by Simon King

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 git

Commit: 32a374ffaece155c52a21cb2cef970eee18a79e4787737761fd909e740f35440ec53ee722a0c52ec

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

7877377Get source code for nested classes defined in Cython files

comment:16 Changed 8 years ago by Simon King

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

Milestone: sage-6.2sage-6.3

comment:18 Changed 8 years ago by Travis Scrimshaw

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

Branch: u/SimonKing/ticket/16309u/tscrim/argspec_static_methods-16309
Commit: 787737761fd909e740f35440ec53ee722a0c52ec143b56779f9e69c9718765a0312fc37274bc34dc
Status: needs_reviewpositive_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:

273cb86Merge branch 'u/SimonKing/ticket/16309' of trac.sagemath.org:sage into u/tscrim/argspec_static_methods-16309
143b567Removed unneeded import from doctest.

comment:20 Changed 8 years ago by Volker Braun

Branch: u/tscrim/argspec_static_methods-16309143b56779f9e69c9718765a0312fc37274bc34dc
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.