Remove explicit type check from inspect.Signature.from_function()

[scoder]

BPO	17159
Nosy	@terryjreedy, @ncoghlan, @pitrou, @scoder, @larryhastings, @benjaminp, @merwok, @1st1
Files	inspect_sig.patch: proposed patch inspect_sig_with_docstring.patch: same patch plus updated docstring inspect_sig_2.patch: new patch that keeps the original TypeError if an attribute is not found inspect_sig_3.patch: updated patch that also fixes the lower case "python" in the docstring sig_func_ducktype_01.patch sig_func_ducktype_02.patch sig_func_ducktype_03.patch divert_from_builtin.patch: divert Signature.from_builtin() to .from_function() for complete function interfaces sig_cython_01.patch sig_cython_02.patch: Tweaked unittest a bit sig_cython_03.patch: A bit more testing, better coverage sig_cython_latest_01.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/1st1'
closed_at = <Date 2014-02-21.23:37:49.170>
created_at = <Date 2013-02-08.15:20:30.752>
labels = ['type-feature', 'library']
title = 'Remove explicit type check from inspect.Signature.from_function()'
updated_at = <Date 2014-03-17.06:31:08.352>
user = 'https://github.com/scoder'

bugs.python.org fields:

activity = <Date 2014-03-17.06:31:08.352>
actor = 'python-dev'
assignee = 'yselivanov'
closed = True
closed_date = <Date 2014-02-21.23:37:49.170>
closer = 'yselivanov'
components = ['Library (Lib)']
creation = <Date 2013-02-08.15:20:30.752>
creator = 'scoder'
dependencies = []
files = ['29003', '29004', '29007', '29096', '33813', '33814', '33837', '33846', '33847', '33849', '33854', '34072']
hgrepos = []
issue_num = 17159
keywords = ['patch', 'needs review']
message_count = 67.0
messages = ['181674', '181676', '181677', '181678', '181702', '181704', '181805', '181806', '181868', '181879', '181887', '181940', '182262', '209675', '209708', '209726', '209728', '209729', '209775', '209787', '209812', '209816', '209820', '209821', '209822', '209824', '209827', '209896', '209907', '209910', '209913', '209915', '209918', '209954', '209966', '209968', '209972', '210003', '210004', '210051', '210081', '210084', '210085', '210099', '210101', '210103', '210104', '210105', '210109', '210112', '210116', '210117', '210122', '211174', '211176', '211181', '211182', '211183', '211184', '211188', '211191', '211891', '211892', '211895', '211910', '211914', '213829']
nosy_count = 9.0
nosy_names = ['terry.reedy', 'ncoghlan', 'pitrou', 'scoder', 'larry', 'benjamin.peterson', 'eric.araujo', 'python-dev', 'yselivanov']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue17159'
versions = ['Python 3.4']

[scoder]

I can't see a reason why Signature.from_function() should explicitly check the type of the object being passed in. As long as the object has all required attributes, it should be accepted.

This is specifically an issue with Cython compiled functions, which are not Python functions but look the same.

[scoder]

This patch removes the type check from Signature.from_function() and cleans up the type tests in signature() to use whatever the inspect module defines as isfunction() and isbuiltin(), so that it becomes properly monkey-patchable.

It also adds a test that makes sure the signature relies only on the four relevant special attributes, not on the type of the object being inspected.

[merwok]

Patch looks good, but I’m worried about the change from TypeError to AttributeError in a stable version.

Could you also make clear that all function-like objects are accepted in the doc?

[scoder]

The method doesn't seem to be documented, and I'm not sure if the docstring really benefits from this lengthy addition. Anyway, here's a patch that includes the docstring update.

The exception could be kept the same if we catch an AttributeError and explicitly raise a TypeError instead.

[terryjreedy]

My first concern is whether this is a behavior issue that can be fixed in the next 3.3 release or an enhancement request that should wait for 3.4.

If Signature.from_function were documented in the manual with the current limitation, this issue would definitely be an enhancement request. But it is not even mentioned in the manual. I presume this is intentional as it is pretty clearly not meant to be called directly but only from signature(). Perhaps its should have been given a leading underscore to mark it as private.

The docstring does say 'python function'. However, its type check is redundant with the guard in signature before its call. So in normal usage, it can never fail. Moreover, the limitation is completely unnecessary even for pure Python code, as shown the the ease of creating a funclike class for the test. It is contrary to Python policy and stdlib practice. So in a sense, it is a bug.

My conclusion: the proposed change is an enhancement bugfix (or bugfix enhancement?) for an undocumented private function. So I think it ok for 3.3, but would not blame anyone who disagreed.

---
I agree with Éric that the exception should not be changed, not just because it would be a change, but because is would be a wrong change. Signature.from_function(42) *is* a type error. The fact that the type error is detected by attribute rather than by class is not relevant to external callers. So please wrap the
# Parameter information.
section of the code with try: except AttributeError, as a substitute for the current too-narrow type check. Leave the message as it, except possibly add the object that fails:
"{} is not a function".format(func)

---
The change to signature has no effect on .from_function but makes it consistent with the rest of the module.

[scoder]

Fine with me. Updated patch attached.

[scoder]

Any more comments? Any objections to applying the last patch? Anyone ready to do it?

[pitrou]

I certainly think the patch is ok on the principle. I'll let someone else pronounce on the details :-)

[terryjreedy]

I am puzzled by this part of the patch:

• with self.assertRaisesRegex(TypeError, 'is not a callable object'):
  + with self.assertRaisesRegex(TypeError, '42.*is not a callable object'):

With 3.3, I get "TypeError: 42 is not a callable object", which I consider correct, and which should not be changed by the two changes to signature(). So both the old test, missing '42' (how does it pass?) and the new version, with extraneous '.*' look wrong.

I am also puzzled by the 'from None' part in
+ raise TypeError("'{!r}' is not a Python function".format(func)) from None

While I remember that being in the pydev discussion and while "raise XyzError from None" executes, it does not seems to be documented for 3.3 in 7.8. The raise statement. (Should this be another issue?) In fact, that section says " if given, the second expression must be another exception class or instance", which excludes None.

[scoder]

1. that's a regex test, so it just looks for the text. I only extended it to include the object it's supposed to test for. It's not actually related to my changes, but I considered it the right thing to do. I used "42.*is not" in order to allow for alternative spellings like "'42' is not...".

2. "raise ... from None" is the official way to explicitly drop the exception context from a newly raised exception. Otherwise, you'd get two exceptions in this case: a TypeError (as main exception) and an AttributeError (as its context), which I do not consider helpful here as it bloats the output for an otherwise simple error. For 99% of the use cases, it won't matter which attribute was missing.

[ncoghlan]

On Mon, Feb 11, 2013 at 11:46 AM, Terry J. Reedy <report@bugs.python.org> wrote:

I am also puzzled by the 'from None' part in

• raise TypeError("'{!r}' is not a Python function".format(func)) from None

While I remember that being in the pydev discussion and while "raise XyzError from None" executes, it does not seems to be documented for 3.3 in 7.8. The raise statement. (Should this be another issue?) In fact, that section says " if given, the second expression must be another exception class or instance", which excludes None.

That's a separate docs bug - it seems we missed the necessary language
reference updates when implementing PEP-309. The relevant docs are
here: http://docs.python.org/3/library/exceptions.html#built-in-exceptions

[terryjreedy]

I opened separate issue bpo-17188:
Document 'from None' in raise statement doc.

[scoder]

Updated patch to also fix a little typo in the docstring (lower case "python").