Opened 7 years ago

Closed 7 years ago

#14017 closed defect (fixed)

Determine the correct argspec for python functions defined in cython files

Reported by: SimonKing Owned by: jason
Priority: major Milestone: sage-5.7
Component: misc Keywords: introspection cython argspec
Cc: Merged in: sage-5.7.beta4
Authors: Simon King Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

In the following simple example, sage_getargspec does not correctly determine the arguments of a python function:

sage: cython("""
....: def f_cy(*args,**kwds):
....:     pass
....: """)
sage: sage_getargspec(f_cy)
ArgSpec(args=['*args', '**kwds'], varargs=None, keywords=None, defaults=None)

Writing the same in Python, one gets

sage: def f_py(*args,**kwds):
....:     pass
....: 
sage: sage_getargspec(f_py)
ArgSpec(args=[], varargs='args', keywords='kwds', defaults=None)

This bug is what currently prevents as to put UniqueRepresentation into a cython file.

Attachments (1)

trac14017_sage_getargspec_cython.patch (18.8 KB) - added by SimonKing 7 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 7 years ago by SimonKing

Here is our problem:

sage: obj = UniqueRepresentation.__classcall__.f
sage: from sage.misc.sageinspect import sage_getsource, _sage_getargspec_from_ast, _sage_getargspec_cython
sage: source = sage_getsource(obj, is_binary=True)
sage: _sage_getargspec_cython(source)   # wrong
(['cls', '*args', '**options'], None, None, None)
sage: _sage_getargspec_from_ast(source) # correct
ArgSpec(args=['cls'], varargs='args', keywords='options', defaults=None)

Perhaps _sage_getargspec_cython (which is called if it is suspected that the function is defined in Cython) should first check whether _sage_getargspec_from_ast works.

comment:2 Changed 7 years ago by SimonKing

Actually a part of the current doctests expect wrong answers:

        sage: from sage.misc.sageinspect import _sage_getargspec_cython as sgc

# That is ok:
        sage: sgc("cpdef double abc(self, x=None, base=0):")
        (['self', 'x', 'base'], None, None, (None, 0))
        sage: sgc("def __init__(self, x=None, unsigned int base=0):")
        (['self', 'x', 'base'], None, None, (None, 0))

# The following is wrong, and is the main reason why we couldnt' simply
# put UniqueRepresentation into a Cython file to make it faster:
        sage: sgc('def o(p, *q, r={}, **s) except? -1:')
        (['p', '*q', 'r', '**s'], None, None, ({},))

# That is ok:
        sage: sgc('cpdef how(r=(None, "u:doing?")):')
        (['r'], None, None, ((None, 'u:doing?'),))
        sage: sgc('def _(x="):"):')
        (['x'], None, None, ('):',))
        sage: sgc('def f(z = {(1,2,3): True}):\n    return z')
        (['z'], None, None, ({(1, 2, 3): True},))

# The following gives a valid function definition in a cython file,
# hence, there must be no error when determining the argspec
        sage: sgc('def f(double x, z = {(1,2,3): True}):\n    return z')
        Traceback (most recent call last):
        ...
        ValueError: Could not parse cython argspec

Last night I have coded something which I think is a finite state automaton that correctly parses any argspec, provided that the given input is valid in Cython. Hence, I do not claim that my code would detect all possible syntax errors; but if there is no syntax error, then the output should be the correct argspec.

In the above doctests, my code would give the correct results. But I need some more tests at this point, and also I am not sure whether I should try to detect some more syntactical problems.

For example, I currently have the reasonable answer

sage: sgc('def o(int * bla p, *q, r={}, **s) except? -1:')
(['p', 'r'], 'q', 's', ['{}'])

(here, int * bla is thought of as a type definition in Cython, which is of course wrong but doesn't matter), but I also get something that could easily be avoided:

sage: sgc('def o(int * bla p, *q, r={}, , **s) except? -1:')
(['p', 'r', 's'], 'q', None, ['{}'])

(here, , ** is thought of as a type definition in Cython, but of course it would be straight forward to recognise that there is one illegal comma inserted).

comment:3 Changed 7 years ago by SimonKing

I just realise that there is one more wrong expected result in the current doctests:

        sage: sgc('def _(x="):"):')
        (['x'], None, None, ('):',))

The function specification states that the string representation of the default values is returned. But of course '):' is not the string representation of "):". Instead, we should have

sage: sgc('def _(x="):"):')
(['x'], None, None, ['"):"'])

and this is actually done by my experimental code.

comment:4 Changed 7 years ago by SimonKing

Here is some other doctest that currently has a very odd result. The docs say that "In the case of a class or a class instance, the ArgSpec? of the new or init methods are returned". The example is

        sage: P.<x,y> = QQ[]
        sage: sage_getargspec(P)
        ArgSpec(args=['self', 'element'], varargs=None, keywords=None, defaults=None)
        sage: sage_getargspec(P.__class__)
        ArgSpec(args=['self', 'element'], varargs=None, keywords=None, defaults=None)

but in fact we have

sage:  sage_getargspec(P.__init__)
ArgSpec(args=['self', 'base_ring', 'n', 'names', 'order'], varargs=None, keywords=None, defaults=("'degrevlex'",))

and thus the expected answer looks suspicious to me.

comment:5 Changed 7 years ago by SimonKing

Probably it is better to have the defaults not as string, because _sage_getargspec_from_ast doesn't return strings either.

comment:6 follow-up: Changed 7 years ago by SimonKing

Here is another example I am currently not happy with:

sage: cython("""
def test():
    return 1
def f(x,y={test():2},**kwds):pass 
"""
)
sage: sage_getargspec(f)
ArgSpec(args=['x', 'y'], varargs=None, keywords='kwds', defaults=({None: 2},))

Hence, the default for 'y' is given as {None: 2}, but in fact it should be {1: 2}.

As much as I know, Cython does not make the default values available via attributes of a function, unlike Python. And even if one would try to find the actual value by evaluating the string '{test():2}' in the module's name space, things could go terribly wrong.

Is someone reading this? How should we proceed in this situation?

comment:7 Changed 7 years ago by SimonKing

  • Authors set to Simon King
  • Status changed from new to needs_review

At sage-devel, Robert Bradshaw confirmed that it is probably best to pre-process the source code of a cython function definition, by letting _sage_getargspec_cython first remove the argument's type definition and then call _sage_getargspec_from_ast.

In addition to that, I think for consistency _sage_getargspec_cython should return an ArgSpec and not a plain tuple.

Needs review!

Last edited 7 years ago by SimonKing (previous) (diff)

comment:8 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by tscrim

Hey Simon,

Replying to SimonKing:

Here is another example I am currently not happy with:

sage: cython("""
def test():
    return 1
def f(x,y={test():2},**kwds):pass 
"""
)
sage: sage_getargspec(f)
ArgSpec(args=['x', 'y'], varargs=None, keywords='kwds', defaults=({None: 2},))

Hence, the default for 'y' is given as {None: 2}, but in fact it should be {1: 2}.

As much as I know, Cython does not make the default values available via attributes of a function, unlike Python. And even if one would try to find the actual value by evaluating the string '{test():2}' in the module's name space, things could go terribly wrong.

Is someone reading this? How should we proceed in this situation?

I need a few questions answered before I can have a reasonable answer:

  1. When are the argspec functions called, during compile, and roughly by what?
  2. Would this be caught as an error by the cython compiler (or any other bad syntax for that matter)?
  3. Would _sage_getargspec_from_ast would parse it correctly?

As a general design note, I would think all sage_getargspec* functions should return the same data in the same format (in regard to your comment here).

As for the patch, two quick things. Is there any reason to have that large commented out code block? Also I believe we should try to be python 3 compliant as much as possible, so the exceptions should be raise ExceptionType("msg").

Thank you,
Travis

comment:9 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by SimonKing

Hi Travis,

Replying to tscrim:

  1. When are the argspec functions called, during compile, and roughly by what?

The argspec functions have of course nothing to do with compilation. Their main use cases seem to be:

  • Creation of the docs. If you look into the Sage reference, you will see that any function or method shows its arguments. I think the documentation would not change with my patch, because from the point of view of documentation, it simply does not matter whether the method
      def foo(self, int n, *args):
          ...
    
    has an argument name *args (including the *!!) or has varargs named args (ecluding the *).
  • Decorators. The reason for creating this patch is the cached_method decorator. Let foodec be obtained by applying the cached_method decorator to a method foo. When calling foodec, it is needed to determine the names of the arguments of foo, including varargs and keywords. This is done only once, of course.
  1. Would this be caught as an error by the cython compiler (or any other bad syntax for that matter)?

What would be caught as an error? It really seems you are confusing some things. The compiler did its work already. sage_getargspec (or inspect.getargspec) is called on functions or methods. In the case of Python functions, inspect.getargspec can determine the arguments of a function by certain attributes that the function provides.

But in the case of a Cython function (def or cpdef), these attributes do not exist. Hence, sage_getargspec will try to find the source code (via sage_getsource) and determine the argument list from the source.

Before my patch, the parsing process used to fail on typical function definitions in Cython -- as shown in the ticket description. With my patch, the arguments are correctly determined from the source code.

Relation with the compiler:

Let s be a string, which is some code supposed to define a function.

If _sage_getargspec_cython(s) raises a syntax error, then Cython is supposed to refuse compilation of s. Hence, _sage_getargspec_cython(s) will never raise an error on the source code of a successfully compiled function (otherwise it would be a bug).

  1. Would _sage_getargspec_from_ast would parse it correctly?

Cython code can look like "def foo(b, int a=1, *args)". _sage_getargspec_from_ast would not know what to do with that string. As I have explained in some post above, the new _sage_getargspec_cython will strip the type definitions; hence, it would first transform it into "def dummy(b,a=1,*args)" and then call _sage_getargspec_from_ast.

As a general design note, I would think all sage_getargspec* functions should return the same data in the same format (in regard to your comment here).

That's what my patch does.

As for the patch, two quick things. Is there any reason to have that large commented out code block?

Oops, sorry, I'll remove that block and will submit a new patch after dinner.

Also I believe we should try to be python 3 compliant as much as possible, so the exceptions should be raise ExceptionType("msg").

Never heard of that rule. Could you ask on sage-devel whether this rule should be encouraged?

comment:10 in reply to: ↑ 9 ; follow-up: Changed 7 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

Hey Simon,

Replying to SimonKing:

Replying to tscrim:

  1. When are the argspec functions called, during compile, and roughly by what?

[...] But in the case of a Cython function (def or cpdef), these attributes do not exist. Hence, sage_getargspec will try to find the source code (via sage_getsource) and determine the argument list from the source.

Before my patch, the parsing process used to fail on typical function definitions in Cython -- as shown in the ticket description. With my patch, the arguments are correctly determined from the source code.

Ah I see. That clarifies things for me. Thank you.

Relation with the compiler:

Let s be a string, which is some code supposed to define a function.

If _sage_getargspec_cython(s) raises a syntax error, then Cython is supposed to refuse compilation of s. Hence, _sage_getargspec_cython(s) will never raise an error on the source code of a successfully compiled function (otherwise it would be a bug).

So it's not a requirement to actually check bad syntax, but only needs to guarantee that it will parse correct syntax. Correct?

  1. Would _sage_getargspec_from_ast would parse it correctly?

Cython code can look like "def foo(b, int a=1, *args)". _sage_getargspec_from_ast would not know what to do with that string. As I have explained in some post above, the new _sage_getargspec_cython will strip the type definitions; hence, it would first transform it into "def dummy(b,a=1,*args)" and then call _sage_getargspec_from_ast.

Sorry I was unclear; I was thinking of if the function was in python:

sage: def test(): return 1
....: 
sage: def f(x,y={test():2},**kwds): pass
....: 
sage: sage_getargspec(f)
ArgSpec(args=['x', 'y'], varargs=None, keywords='kwds', defaults=({1: 2},))

I think I'm now starting I understand what the issue is. However I don't see how evaluating the default argument string in the modules namespace could cause an issue unless the user directly calls _sage_getargspec_cython() as you are in the tests. (I guess there is also some difficulty in parsing out functions foo(args) and replacing it with module.foo(args)...)

As a general design note, I would think all sage_getargspec* functions should return the same data in the same format (in regard to your comment here).

That's what my patch does.

:D

As for the patch, two quick things. Is there any reason to have that large commented out code block?

Oops, sorry, I'll remove that block and will submit a new patch after dinner.

Thanks.

Also I believe we should try to be python 3 compliant as much as possible, so the exceptions should be raise ExceptionType("msg").

Never heard of that rule. Could you ask on sage-devel whether this rule should be encouraged?

Here's the topic: https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/P35X-WjDuK4

Thank you for clarifying things and answering all of my (crazy/misguided) questions,
Travis

Edit - The reason for the drop in coverage is because sage -coverage doesn't correctly parse that the functions in the doctests are not "real" functions.

Last edited 7 years ago by tscrim (previous) (diff)

comment:11 in reply to: ↑ 10 Changed 7 years ago by SimonKing

Hi Travis,

Replying to tscrim:

So it's not a requirement to actually check bad syntax, but only needs to guarantee that it will parse correct syntax. Correct?

Correct. The patch fixes cases in which this used not to be the case. But I think it is a bug if correct syntax is not parsed.

Sorry I was unclear; I was thinking of if the function was in python:

sage: def test(): return 1
....: 
sage: def f(x,y={test():2},**kwds): pass
....: 
sage: sage_getargspec(f)
ArgSpec(args=['x', 'y'], varargs=None, keywords='kwds', defaults=({1: 2},))

That's indeed tricky.

This example does work for Python functions, because inspect.getargspec can read off the default values from some attribute of the function -- hence, reading source code is not involved for Python.

But if you put the same definition in a .pyx file, then inspect.getargspec does not work, unless one compiles everything with the "binding" option (which is probably bad for performance). Hence, source code parsing probably is the only way to find the arguments. But as soon as you use external functions to define default values, source code parsing must fail sooner or later.

In this example, _sage_getargspec_from_ast (and thus _sage_getargspec_cython) would return {None: 2} and not {1: 2} as default value.

I see no safe way to overcome this limitation. But note that knowing the correct default values is less critical than knowing the argument names. The bug that made me create this ticket was: Currently the argument names are not correctly determined, in some situations.

I think I'm now starting I understand what the issue is. However I don't see how evaluating the default argument string in the modules namespace could cause an issue

No, in examples of that kind, source code parsing must fail at some point. Two examples:

  • If test() in the example above is a cdef function that is not declared in the .pxd file, then _sage_getargspec_from_ast (which returns the incorrect default) can not import it. Hence, it would be impossible to simply call test() and see what it returns.
  • It is easy to write a function whose return value changes over time. If test() is such function, then the value used to define the default argument of f would differ from the value deduced by _sage_getargspec_from_ast.

As for the patch, two quick things. Is there any reason to have that large commented out code block?

Oops, sorry, I'll remove that block and will submit a new patch after dinner.

Thanks.

This will be my next post - dinner was quite long...

Also I believe we should try to be python 3 compliant as much as possible, so the exceptions should be raise ExceptionType("msg").

Never heard of that rule. Could you ask on sage-devel whether this rule should be encouraged?

Here's the topic: https://groups.google.com/forum/?fromgroups=#!topic/sage-devel/P35X-WjDuK4

Thanks!

comment:12 Changed 7 years ago by SimonKing

The patch is updated - the uncommented chunk of code is removed, and I raise Exception("message").

comment:13 Changed 7 years ago by tscrim

  • Status changed from needs_review to positive_review

Looks good to me now. Thank you Simon.

Best,
Travis

comment:14 follow-up: Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Building the documentation fails badly:

/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_element_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_element_letterplace.poly_reduce: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_element_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_element_letterplace.singular_system: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_letterplace.poly_reduce: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_letterplace.singular_system: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/letterplace_ideal.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.letterplace_ideal.poly_reduce: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/letterplace_ideal.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.letterplace_ideal.singular_system: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.problem_name: Pointer types not allowed in def or cpdef functions
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.write_lp: Pointer types not allowed in def or cpdef functions
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.write_mps: Pointer types not allowed in def or cpdef functions
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.print_ranges: Pointer types not allowed in def or cpdef functions
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.problem_name: Pointer types not allowed in def or cpdef functions
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.write_lp: Pointer types not allowed in def or cpdef functions
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.write_mps: Pointer types not allowed in def or cpdef functions
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/rings/polynomial/multi_polynomial_ring_generic.rst:11: WARNING: error while formatting signature for sage.rings.polynomial.multi_polynomial_ring_generic.MPolynomialRing_generic.remove_var: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/rings/polynomial/pbori.rst:11: WARNING: error while formatting signature for sage.rings.polynomial.pbori.BooleanPolynomialRing.remove_var: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/rings/polynomial/pbori.rst:11: WARNING: error while formatting signature for sage.rings.polynomial.pbori.mod_mon_set: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/structure/sage_object.rst:11: WARNING: error while formatting signature for sage.structure.sage_object.load: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/symbolic/expression.rst:11: WARNING: error while formatting signature for sage.symbolic.expression.Expression.add: invalid syntax (<unknown>, line 1)
/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/symbolic/expression.rst:11: WARNING: error while formatting signature for sage.symbolic.expression.Expression.mul: invalid syntax (<unknown>, line 1)

comment:15 in reply to: ↑ 14 Changed 7 years ago by SimonKing

Replying to jdemeyer:

Building the documentation fails badly:

/release/merger/sage-5.7.beta3/devel/sage/doc/en/reference/sage/algebras/letterplace/free_algebra_element_letterplace.rst:11: WARNING: error while formatting signature for sage.algebras.letterplace.free_algebra_element_letterplace.poly_reduce: invalid syntax (<unknown>, line 1)

OK, let's see what is happening.

sage: from sage.misc.sageinspect import sage_getsource, sage_getargspec
sage: from sage.algebras.letterplace.free_algebra_element_letterplace import poly_reduce
sage: source = sage_getsource(poly_reduce.__call__)
sage: print source
    def __call__(self, *args, ring=None, bint interruptible=True, attributes=None):
...

This actually looks fine, and rather easy to parse.

But in fact, _sage_getargspec_from_ast is to blame, it seems:

sage: from sage.misc.sageinspect import _sage_getargspec_cython, _sage_getargspec_from_ast
sage: _sage_getargspec_from_ast("def __call__(self, *args, ring,  interruptible, attributes):")
------------------------------------------------------------
   File "<unknown>", line 1
     def __call__(self, *args, ring,  interruptible, attributes):
                                  ^
SyntaxError: invalid syntax

Aha!!! That's very interesting!! Apparently Cython accepts this argument definition, but it is indeed a syntax error in Python!

sage: def dummy(self,*args,ring=None,interruptible=True,attributes=None): pass
------------------------------------------------------------
   File "<ipython console>", line 1
     def dummy(self,*args,ring=None,interruptible=True,attributes=None): pass
                             ^
SyntaxError: invalid syntax
sage: def dummy(self,ring=None,interruptible=True,attributes=None, *args): pass
....: 
sage:

So, what can one do? I could try to let _sage_getargspec_cython determine the varargs and keywords name, and put them to the very end of the argument list. The list of arguments/argspec would not change, but it would be correct syntax in Python.

comment:16 follow-up: Changed 7 years ago by jdemeyer

I would say it's a bug in Cython for allowing that syntax, but that doesn't help obviously...

I could try to let _sage_getargspec_cython determine the varargs and keywords name, and put them to the very end of the argument list. The list of arguments/argspec would not change, but it would be correct syntax in Python.

Sounds reasonable.

Alternatively, actually change

def __call__(self, *args, ring=None, bint interruptible=True, attributes=None)

to the Python-compatible

def __call__(self, ring=None, bint interruptible=True, attributes=None, *args)
Last edited 7 years ago by jdemeyer (previous) (diff)

comment:17 in reply to: ↑ 16 Changed 7 years ago by SimonKing

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

I could try to let _sage_getargspec_cython determine the varargs and keywords name, and put them to the very end of the argument list. The list of arguments/argspec would not change, but it would be correct syntax in Python.

Sounds reasonable.

I have updated my patch accordingly

Alternatively, actually change

def __call__(self, *args, ring=None, bint interruptible=True, attributes=None)

to the Python-compatible

def __call__(self, ring=None, bint interruptible=True, attributes=None, *args)

But similar syntax occurs all over the place. I think that should be done on a separate ticket, if people think it is a problem.

Needs review, including a look at the outcome of the documentation as mentioned in comment:14.

comment:18 Changed 7 years ago by SimonKing

PS: I have added more doctests, dealing with corner cases (no positional arguments, but only varargs or only keywords) and the good-in-Cython-but-bad-in-Python syntax.

comment:19 Changed 7 years ago by SimonKing

  • Status changed from needs_review to needs_work
  • Work issues set to Fix variable name in pbori, accept pointer types in function definitions

Here is the outcome of building the reference guide from scratch, with the new patch:

/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.problem_name: Pointer types not allowed in def or cpdef functions
/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.write_lp: Pointer types not allowed in def or cpdef functions
/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/generic_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.generic_backend.GenericBackend.write_mps: Pointer types not allowed in def or cpdef functions
/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.print_ranges: Pointer types not allowed in def or cpdef functions
/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.problem_name: Pointer types not allowed in def or cpdef functions
/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.write_lp: Pointer types not allowed in def or cpdef functions
/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/numerical/backends/glpk_backend.rst:11: WARNING: error while formatting signature for sage.numerical.backends.glpk_backend.GLPKBackend.write_mps: Pointer types not allowed in def or cpdef functions
/home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference/sage/rings/polynomial/pbori.rst:11: WARNING: error while formatting signature for sage.rings.polynomial.pbori.mod_mon_set: invalid syntax (<unknown>, line 1)

So, in most cases, there is complaint about a '*' being inside of the type definition, making it a pointer type. Let's have a look:

cpdef problem_name(self, char * name = NULL)

Arrgh. I thought no pointer was allowed, but char* is an exception.

Since the only requirement is that sage_getargspec correctly parses correct syntax, a potential solution is to simply accept a "pointed" type definition, whether it is legal or not.

The last problem is different:

def mod_mon_set(BooleSet as, BooleSet vs):

as is a reserved name in Python -- so, it is hard to believe that Cython accepts it as a variable name!

But in this case, I suggest to simply change the name in pbori.pyx, rather than trying to make _sage_getargspec_cython swallow it.

I'll do the two changes tomorrow.

Changed 7 years ago by SimonKing

comment:20 Changed 7 years ago by SimonKing

  • Status changed from needs_work to needs_review
  • Work issues Fix variable name in pbori, accept pointer types in function definitions deleted

It was a rather easy change, so, I updated the patch already. _sage_getargspec_cython is still entitled to complain about pointer types, but an exception from raising an exception is made (and doctested) for pointers to char. And I changed the critical variable name in pbori.pyx.

Needs review again (including docs).

comment:21 Changed 7 years ago by SimonKing

For the record:

simon@linux-sqwp:~/SAGE/debug/sage-5.6.rc0/devel/sage-main> rm -r doc/output/
simon@linux-sqwp:~/SAGE/debug/sage-5.6.rc0/devel/sage-main> ../../sage -docbuild reference html
sphinx-build -b html -d /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/output/doctrees/en/reference    /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/en/reference /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/output/html/en/reference
Running Sphinx v1.1.2
loading pickled environment... not yet created
loading intersphinx inventory from /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/common/python.inv...
building [html]: targets for 1089 source files that are out of date
updating environment: 1089 added, 0 changed, 0 removed
reading sources... [100%] todolist                                                                                                                                                                        
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] todolist                                                                                                                                                                         
writing additional files... genindex py-modindex search
copying images... [100%] sage/combinat/../../media/combinat/complete-binary-trees-4.png                                                                                                                   
copying static files... done
dumping search index... done
dumping object inventory... done
build succeeded.
Build finished.  The built documents can be found in /home/simon/SAGE/debug/sage-5.6.rc0/devel/sage/doc/output/html/en/reference

comment:22 Changed 7 years ago by tscrim

  • Status changed from needs_review to positive_review

Documentation rebuilt cleanly for me as well. Looks good to me.

comment:23 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.7.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.