Safeguard _sage_getargspec_cython#39776
Conversation
49cdfd6 to
d87a60c
Compare
|
Documentation preview for this PR (built with commit 21dd822; changes) is ready! 🎉 |
src/sage/misc/sageinspect.py
Outdated
| defaults=('a string', {(1, 2, 3): True}), | ||
| kwonlyargs=[], kwonlydefaults=None, annotations={}) | ||
| """ | ||
| if not isinstance(source, str): |
There was a problem hiding this comment.
what do you think about instead of raising a TypeError to use assert not None, and then properly fix the cases in the caller that may pass None?
For example, further below one finds
try:
source = sage_getsource(obj)
except TypeError: # happens for Python builtins
source = ''
if source:
return inspect.FullArgSpec(*_sage_getargspec_cython(source))
with a fall-back if source == None. This appears to me like a good pattern that one should encourage.
There was a problem hiding this comment.
I mean in principle it seems like TypeError is the morally correct error to raise (instead of AssertionError).
But then if previously TypeError is raised for Python builtins, maybe we should change that to… ValueError or RuntimeError? (which one?)
Edit: never mind, TypeError is raised for built-in
File …/inspect.py:905, in getfile(object)
903 if object.__module__ == '__main__':
904 raise OSError('source code not available')
--> 905 raise TypeError('{!r} is a built-in class'.format(object))
906 if ismethod(object):
907 object = object.__func__
TypeError: <class 'object'> is a built-in class
Maybe AssertionError is acceptable.
Still, TypeError is raised when wrong argument type is passed sometimes
sage: len(None)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
Cell In[54], line 1
----> 1 len(None)
TypeError: object of type 'NoneType' has no len()
Compromising on API just so that caller can distinguish between two kinds of error is… I don't know.
There was a problem hiding this comment.
TypeError is also raised by Python's inspect in case of Python builtins, so we probably want to keep it.
On the other hand, I agree with
I mean in principle it seems like TypeError is the morally correct error to raise (instead of AssertionError).
(My reason for the assertion was that there is already one two lines below, and this is a private helper method anyway.)
tobiasdiez
left a comment
There was a problem hiding this comment.
Thanks!
Question: would the underlying issue be fixed by activating binding=true? My hope would be that in this case the inspection module is able to extract the signature without looking at the source code.
should be yes, but I don't know if there's any other side effects (supposedly it's slower…? It's not my immediate concern and I never benchmarked but inefficient code will hurt all of us in the long run when there's an occasional thing that requires a large amount of computation) another potential problem is with An alternative is to set |
sagemathgh-39776: Safeguard _sage_getargspec_cython Fix sagemath#39735 Actually just a workaround, this sweeps the fundamental problem under the rug. Without source available: ``` sage: from sage.structure.category_object import CategoryObject sage: CategoryObject._CategoryObject__gens_dict Cached version of <method '_CategoryObject__gens_dict' of 'sage.structure.category_object.CategoryObject' objects> sage: type(CategoryObject._CategoryObject__gens_dict) <class 'sage.misc.cachefunc.CachedMethodCaller'> ``` With source available (specialized version `CachedMethodCallerNoArgs` is used, thus more efficient): ``` sage: from sage.structure.category_object import CategoryObject sage: CategoryObject._CategoryObject__gens_dict Cached version of <method '_CategoryObject__gens_dict' of 'sage.structure.category_object.CategoryObject' objects> sage: type(CategoryObject._CategoryObject__gens_dict) <class 'sage.misc.cachefunc.CachedMethodCallerNoArgs'> ``` The proper fix should be to either somehow make `sage_getargspec` always return the correct value, or to somehow make the cache function mechanism know when to use specialization. In any case, it can't hurt to make the function raise the correct exception type. ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39776 Reported by: user202729 Reviewer(s): Tobias Diez, user202729
|
The underlying problem should be partially fixed after cython/cython#6764 which embeds the signature into (some) docstring, thus not rely on reading the source code. |
Fix #39735
Actually just a workaround, this sweeps the fundamental problem under the rug.
Without source available:
With source available (specialized version
CachedMethodCallerNoArgsis used, thus more efficient):The proper fix should be to either somehow make
sage_getargspecalways return the correct value,or to somehow make the cache function mechanism know when to use specialization.
In any case, it can't hurt to make the function raise the correct exception type.
📝 Checklist
⌛ Dependencies