Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDLE: calltips mishandle raw strings and other examples #56719

Closed
RoyFox mannequin opened this issue Jul 6, 2011 · 29 comments
Closed

IDLE: calltips mishandle raw strings and other examples #56719

RoyFox mannequin opened this issue Jul 6, 2011 · 29 comments
Assignees
Labels
topic-IDLE type-bug An unexpected behavior, bug, or error

Comments

@RoyFox
Copy link
Mannequin

RoyFox mannequin commented Jul 6, 2011

BPO 12510
Nosy @terryjreedy, @ned-deily, @serwy
Files
  • issue12510.patch
  • issue12510_rev1.patch
  • fix_12510.patch
  • i12510.test.diff: Improve get_argspec and tests.
  • i12510b.test.diff: Improve get_argspec and tests.
  • i12510c.test.diff: Improve get_argspec and tests.
  • issue12510_self_pat.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/terryjreedy'
    closed_at = <Date 2012-12-12.01:33:41.918>
    created_at = <Date 2011-07-06.23:27:21.963>
    labels = ['expert-IDLE', 'type-bug']
    title = 'IDLE: calltips mishandle raw strings and other examples'
    updated_at = <Date 2012-12-12.01:33:41.916>
    user = 'https://bugs.python.org/RoyFox'

    bugs.python.org fields:

    activity = <Date 2012-12-12.01:33:41.916>
    actor = 'roger.serwy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2012-12-12.01:33:41.918>
    closer = 'roger.serwy'
    components = ['IDLE']
    creation = <Date 2011-07-06.23:27:21.963>
    creator = 'Roy.Fox'
    dependencies = []
    files = ['24071', '25736', '25811', '25814', '25828', '25836', '25942']
    hgrepos = []
    issue_num = 12510
    keywords = ['patch']
    message_count = 29.0
    messages = ['139961', '140051', '140056', '150033', '161698', '161721', '161742', '161743', '161748', '161828', '161847', '162192', '162195', '162233', '162248', '162324', '162334', '162335', '162336', '162353', '162373', '162510', '162511', '162690', '165057', '165058', '165793', '165822', '177361']
    nosy_count = 5.0
    nosy_names = ['terry.reedy', 'ned.deily', 'roger.serwy', 'python-dev', 'Roy.Fox']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue12510'
    versions = ['Python 2.7', 'Python 3.2', 'Python 3.3']

    @RoyFox
    Copy link
    Mannequin Author

    RoyFox mannequin commented Jul 6, 2011

    Hi,

    When you type (not copy-paste!) into an IDLE shell a string literal followed by (
    you get a calltip.

    When the string contains a bad unicode escaping you get an error (see example below), which makes some sense.

    But when the string is raw, it isn't treated as such, and you may get the same error, though now it doesn't make any sense.

    Non-raw example:

    >> '\xyz'(

    >> *** Internal Error: rpc.py:SocketIO.localcall()

    Object: exec
    Method: <bound method Executive.get_the_calltip of <idlelib.run.Executive object at 0x021C2910>>
    Args: ("'\\xyz'",)

    Traceback (most recent call last):
      File "C:\Python32\lib\idlelib\rpc.py", line 188, in localcall
        ret = method(*args, **kwargs)
      File "C:\Python32\lib\idlelib\run.py", line 324, in get_the_calltip
        return self.calltip.fetch_tip(name)
      File "C:\Python32\lib\idlelib\CallTips.py", line 103, in fetch_tip
        entity = self.get_entity(name)
      File "C:\Python32\lib\idlelib\CallTips.py", line 112, in get_entity
        return eval(name, namespace)
      File "<string>", line 1
    SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 0-2: truncated \xXX escape

    Raw example:

    >> r'\xyz'(

    >> *** Internal Error: rpc.py:SocketIO.localcall()

    Object: exec
    Method: <bound method Executive.get_the_calltip of <idlelib.run.Executive object at 0x021C2910>>
    Args: ("'\\xyz'",)

    Traceback (most recent call last):
      File "C:\Python32\lib\idlelib\rpc.py", line 188, in localcall
        ret = method(*args, **kwargs)
      File "C:\Python32\lib\idlelib\run.py", line 324, in get_the_calltip
        return self.calltip.fetch_tip(name)
      File "C:\Python32\lib\idlelib\CallTips.py", line 103, in fetch_tip
        entity = self.get_entity(name)
      File "C:\Python32\lib\idlelib\CallTips.py", line 112, in get_entity
        return eval(name, namespace)
      File "<string>", line 1
    SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 0-2: truncated \xXX escape

    @RoyFox RoyFox mannequin added topic-IDLE type-bug An unexpected behavior, bug, or error labels Jul 6, 2011
    @terryjreedy
    Copy link
    Member

    What platform? It sometimes makes a difference with tcl/tk and hence idle.

    @ned-deily
    Copy link
    Member

    The problem is easily reproducible. Although it shouldn't give that error (and that can be fixed), it seems to me that IDLE should not be trying to give a calltip in that context. What it is trying to do is display the __doc__ attribute of the string but the __doc__ is really for the str() constructor:

    >>> 'a'.__doc__
    "str(string[, encoding[, errors]]) -> str\n\nCreate a new string object from the given encoded string.\nencoding defaults to the current default string encoding.\nerrors can be 'strict', 'replace' or 'ignore' and defaults to 'strict'."

    @serwy
    Copy link
    Mannequin

    serwy mannequin commented Dec 21, 2011

    The attached patch fixes the bug. The bug occurs in "get_entity" which is used to get the object. Then "get_argspec" determines the calltip text.

    The calltip can be prevented for strings by having get_argspec check if the object has a "__call__" method. I opted for adding "not callable" to the calltip initially for testing, but setting argspec="" will prevent the calltip altogether.

    @terryjreedy
    Copy link
    Member

    With 2.7 & 3.3 on Win 7, 'a'( brings up calltip (bad).
    With 3.3, '\xyz'( causes Idle to die (worse)
    With 2.7, the same or u'\xyz'( have no effect, just like 3(.

    There are a lot of little differences between 2.7 and 3.x, but the main one is the use of inspect in 3.x. I do not see an obvious reason for the behavior difference.

    With patch, 'a'( brings up calltip with 'Not callable' added (worse, in my opinion), while '\xyz'( does nothing, just like 3( (or 2.7, great).

    Roger, perhaps you uploaded your initial test past instead of the final patch. A string instance should act like any other non-callable instance.

    That said, the patch strikes me as attacking the problem at the wrong end. Str instances are not names and get_argspec should never be called on one (I am presuming that it is not called on other non-callables, such as int instances). Similarly, back up further in the call stack, fetch_tip(name) should only be called on names. I wonder if the problem might be back up in open_calltip(), perhaps after

            hp = HyperParser(self.editwin, "insert")
            sur_paren = hp.get_surrounding_brackets('(')
            if not sur_paren:
                return
            hp.set_index(sur_paren[0])
            name = hp.get_expression()

    (argggh! I have no idea how to debug code where I cannot just insert print statements to tell me what is getting called with what arguments.)

    @serwy
    Copy link
    Mannequin

    serwy mannequin commented May 27, 2012

    Terry, the original patch added "Not callable" because I wasn't sure what the consensus was on proper behavior. Now I know. :)

    Attached is a revision against 3.3a3+ which omits the calltip if the object is not callable.

    The behavior difference between 2.7 and 3.3 is due to a difference in "localcall" in rpc.py, where "CALLEXC" raises the exception in the IDLE front-end in 3.3 causing the crash, whereas in 2.7 the exception occurs in the subprocess. This is due to "rpc_marshal_exception.patch" for bpo-14200 not being backported to 2.7.

    Entering []( or {}( or ''( will bring up a calltip. These calltips should not appear if the object is not callable. Take as an example:

        a = "string"
        a(

    The previous code will bring up a calltip even though "a" is not callable. The HyperParser does its job by returning the string representing the object just before the paren. Checking whether this is a proper Python name won't fix this corner case. The "fetch_tip" method grabs the actual object given the string name using "get_entity" and then uses "get_argspec" to return constructor or callable arguments.

    Checking if the object is callable must be done either in "get_entity" or in "get_argspec". I think it is cleaner to check in get_argspec, as is done in the attached patch.

    @terryjreedy
    Copy link
    Member

    'a'(, [](, {}( no longer do anything.
    [int][0]( and {0:int}[0] bring up int tooltip.

    However, patch is not a complete fix yet.
    [int][1]( and {0:int}[1] both 'crash' Idle.
    So we need to add IndexError and KeyError to
    + except (NameError, AttributeError, SyntaxError):

    I think instead we should just remove the list since any uncaught error in the eval will close Idle. Do you see any reason why not? If open_calltips were called with evalfuncs True (I presume there must be a way, see below), then *any* exception could happen in the eval.

    I was thinking that eval is a bit dangerous but I see this protection in open_calltips"

            if not evalfuncs and (name.find('(') != -1):
                return

    Consequently, after "def f(): return int", f()( does not get a calltip. Neither does the tuple equivalent of the list example above, (int,)[0](.

    With the call check moved from the bottom of get_argspec to the top, I am ok with it there. If we agree on the except clause, I am ready to apply.

    @serwy
    Copy link
    Mannequin

    serwy mannequin commented May 28, 2012

    I'm ok with removing the exception list, but add a comment like "# *any* exception could happen in the eval".

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 28, 2012

    New changeset 938b12452af7 by Terry Jan Reedy in branch '2.7':
    bpo-12510: Attempting to get invalid tooltip no longer closes Idle.
    http://hg.python.org/cpython/rev/938b12452af7

    New changeset 4a7582866735 by Terry Jan Reedy in branch '3.2':
    bpo-12510: Attempting to get invalid tooltip no longer closes Idle.
    http://hg.python.org/cpython/rev/4a7582866735

    New changeset 0835bee19f86 by Terry Jan Reedy in branch 'default':
    Merge 3.2 closes bpo-12510
    http://hg.python.org/cpython/rev/0835bee19f86

    @python-dev python-dev mannequin closed this as completed May 28, 2012
    @terryjreedy terryjreedy self-assigned this May 28, 2012
    @terryjreedy
    Copy link
    Member

    I am reopening just as a reminder to revise the comment.

    @terryjreedy terryjreedy reopened this May 29, 2012
    @serwy
    Copy link
    Mannequin

    serwy mannequin commented May 29, 2012

    Which comment needs revision?

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 3, 2012

    New changeset 477508efe4ab by Terry Jan Reedy in branch '2.7':
    bpo-12510: Expand 2 bare excepts. Improve comments. Change deceptive name
    http://hg.python.org/cpython/rev/477508efe4ab

    New changeset f927a5c6e4be by Terry Jan Reedy in branch '3.2':
    bpo-12510: Expand 2 bare excepts. Improve comments. Change deceptive name
    http://hg.python.org/cpython/rev/f927a5c6e4be

    New changeset a7501ddf74ac by Terry Jan Reedy in branch 'default':
    Merge with 3.2 bpo-12510
    http://hg.python.org/cpython/rev/a7501ddf74ac

    @terryjreedy
    Copy link
    Member

    CallTips.py has a 'main' test at the end. Currently, test TC fails in 3.x but not 2.7. So either the test became invalid or get_argspec was not completely and correctly converted from get_arg_text. This should be fixed.

    int.append( does not bring up a tip on either version, but should if possible.

    The failing examples in previous messages should be added to the tests to ensure that all pass now and continue to pass in the future. So I am not closing this yet, and a new patch is needed.

    @terryjreedy terryjreedy changed the title IDLE get_the_calltip mishandles raw strings IDLE: calltips mishandle raw strings and other examples Jun 3, 2012
    @serwy
    Copy link
    Mannequin

    serwy mannequin commented Jun 3, 2012

    fix_12510.patch addresses the issue with the test.

    What do you mean by: "int.append( does not bring up a tip on either version, but should if possible." ? The "int" object does not have "append" as a method.

    @terryjreedy
    Copy link
    Member

    i12510.test.diff (for Python 3 only) does the following:

    • Turn method CallTips.get_entity into a module function as it does not use the self parameter and is therefore not really an instance method.

    • Delete the erroneous _find_constructor function. Even if it were fixed, it is unnecessary at least in 3.x. Class attribute lookup already does the superclass search duplicated in this function and should find object.__init__ if nothing else. (I defaulted to None anyway in case of an unforeseen problem, such as a type subclass that somehow disables the lookup.)

    • In get_argspec, remove the lambda stuff that resulted in all classes getting a spurious '()' line. With the fix to retrieving .__init__, it never gets used and is not needed anyway.

    • Only delete 'self' from the argument list for classes and bound methods where it is implicit and not to be explicity entered.

    • Increase tests from 13 to 37.

    • Modernize and re-organize tests with an eye to becoming part of an idlelib test suite.

    With the increased number of tests, I am pretty confident in the patch, but I would not mind if you glance at it and try it out on Linux.

    @serwy
    Copy link
    Mannequin

    serwy mannequin commented Jun 5, 2012

    The reoganization in i12510.test.diff will make back-porting to 2.7 slightly more difficult since old and new style classes exist. I do agree with your reasoning for re-factoring the code. (Also, as an extremely minor point, running reindent.py adjusts "pos = 70".)

    I applied the patch and it works under 11.04 Ubuntu. All the tests pass and the behavior of the CallTip gives reasonable results when used interactively.

    I did manage to find a corner-case where a CallTip should arise:
        >>> class A:
                def __init__(self, a=None):
                    print('init')
                def __call__(self, b=None):
                    print('call')
    		
        >>> c = A(

    Gives "(a=None)" as the call-tip. However, once the object is created, giving the __call__ argument doesn't work, i.e.

    >>> c(
    

    doesn't give a call-tip. This behavior is also the same without the patch.

    I'll keep playing with the patch to see what else needs improving with CallTips. We might as well fix it completely.

    @terryjreedy
    Copy link
    Member

    By 'Python 3 only', I meant that I am not personally planning to backport to 2.7. I do not want to edit the more complicated get_arg_text and consider other issues higher priority.

    I meant to include a test case for callable instances, but forgot. When
    I add to test class TC
    " def __call__(self, ci): "(self, ci)"
    tc( gives the inherited TC.__doc__ as the tool tip, which in this case happens to by the artificial "(ai=None, *b)".

    Adding the following to get_argspec:

        \# after current if is...type: fob = ...
        elif isinstance(ob.\_\_call__, types.MethodType):
            fob = ob.\_\_call__
    

    &
    or isinstance(ob.__call__, types.MethodType)
    # to self-strip conditional, as instance is already typed
    &
    if isinstance(ob.__call__, types.MethodType):
    doc = ob.__call__.__doc__
    else:
    # before current doc = line, which gets indented

    makes this new test pass.
    fdoc = tc.__call__.__doc__
    test('tc', fdoc + "\n" + fdoc)

    Revised patch with above attached.

    Note 1: Programming is more fun with decent tests.
    Note 2: These non-visual automatic test do not completely substitute for visual testing with the gui. When I first used a broader conditional than the 'elif' above, one that caught list.append and [].append, the tests passed, but typing list.append( resulting in weird behavior: something flashed and the typed text moved down a line.

    @terryjreedy
    Copy link
    Member

    Stephen D'Aprano just asked on Python list how to get signatures of builtins, noting that calling inspect.getargspec on a Python-coded subclass of a builtin class raises an exception. So we need another testcase, verify the behavior, and probably add try-except around the inspect call. (Perhaps inspect.py should be revised to not leak an exception when given the advertised acceptible input, but that is another issue.)

    @terryjreedy
    Copy link
    Member

    Upon further thought, not a problem. Stephen considered the case:
    class myint(int):
    # inherit __init__
    def added_func(self): "some new function of an int"

    In this case, fob = myint.__init__ would not be FunctionType and inspect would not be called. Tool tip would be myint.__doc__ and writer could follow style of builtins. Still an interesting test case to add.

    A different question is whether the default for callables (as opposed to non-callables) should be nothing or something like 'args unknown' or 'see docs'.

    @serwy
    Copy link
    Mannequin

    serwy mannequin commented Jun 5, 2012

    Callable instances now return a call tip. Good!

    I agree with your analysis of the issue raised by Stephen. The get_argspec function will not fail unexpectedly.

    but typing list.append( resulting in weird behavior: something flashed and the typed text moved down a line.

    I get "L.append(object) -> None -- append object to end" as the call tip on Linux. I'm not sure why it didn't work on Win 7. Could an unintentional enter key-press be the culprit?

    A different question is whether the default for callables (as opposed to non-callables) should be nothing or something like 'args unknown' or 'see docs'.

    I agree that if the argspec string has no contents after all the code in get_argspec, then it should have a message added. Perhaps the following would be sufficient:

        if hasattr(ob, '__call__'):
        [truncated]
    
            if not argspec:
                argspec += "Arguments unknown. See docs."
        return argspec

    @terryjreedy
    Copy link
    Member

    The weird behavior was as if the cursor were moved to the beginning of the line and <Enter> pressed. I cannot duplicate it. I had
    elif not isinstance(ob, (types.FunctionType, types.MethodType)):
    instead
    elif isinstance(ob.__call__, types.MethodType):
    but something else must have been different, as list.append( works now with above changed back. The tests pass because passing fob to inspect is guarded. But the correct conditional is needed anyway elsewhere in the file. So I will forget this until it happens again ;-).

    Since we (intend to) inspect all Python-coded callables, and since that returns at least '()' for no args (there is already a test for that), empty callable argspec should only happen for builtin and extension callables with no docstring. I added this code:

    _default_callable_argspec = "No docstring, see docs."
    # before get_argspec
    
              if not argspec:
                  argspec = _default_callable_argspec

    I added tests for this and for subclasses of builtins and uploaded a new patch. Can you think of anything else to test?

    Now that the code and test cases seem about settled, the other (big) change I would like to make is to make most of the tests into proper unittests of get_argspec (which means passing an object, not a string) and some (with some overlap) into unittests of get_entity (passing expression string, getting object). Since rpcclt is None when tests call CallTips.fetch_tip, the method reduces to get_argspec(get_entity(expression)), which just mixes tests of the two functions together.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 8, 2012

    New changeset 02b4c62ce393 by Terry Jan Reedy in branch '3.2':
    Issue bpo-12510: Revise and triple # of calltip tests, with an eye to unittest
    http://hg.python.org/cpython/rev/02b4c62ce393

    New changeset 03b5f75ddac7 by Terry Jan Reedy in branch 'default':
    Merge from 3.2, bpo-12510
    http://hg.python.org/cpython/rev/03b5f75ddac7

    @terryjreedy
    Copy link
    Member

    I decided to commit the current patch (i12510c.test.diff + NEWS changes) as is. Another change is needed to correctly delete the first arg of all methods, not just instance methods with the first arg named 'self'. (In the current new classmethod test, I just allowed non-removal of 'cls'. There should also be a test with a variant of 'self'.)

    In _self_pat = re.compile('self\,?\s*'), 'self' in the pattern should be replaced by '(xxx', where 'xxx' is the pattern that matches an identifier. (And add '(' to the replacement pattern.) If anyone can quickly tell me what that is, that would be helpful.

    @serwy
    Copy link
    Mannequin

    serwy mannequin commented Jun 12, 2012

    The _self_pat RE needs to be changed to just remove the first argument. Presently, another bug exists with the current implementation:

        >>> class A:
                def t(self, self1, self2):
                    pass
        >>> a = A()
        >>> a.t(

    gives "(1,2)" as the calltip, instead of "(self1, self2)" for 3.x. Python 2.7 gives the correct calltip.

    The attached patch modifies _self_pat to remove only the first argument, modifies the classmethod test, and adds a test for "notself", as Terry requested in msg162511.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 9, 2012

    New changeset eea379307efa by Terry Jan Reedy in branch '3.2':
    bpo-12510: Delete actual first param name for all methods; revise tests.
    http://hg.python.org/cpython/rev/eea379307efa

    New changeset 464c6a50b0ce by Terry Jan Reedy in branch 'default':
    Merge with 3.2 bpo-12510
    http://hg.python.org/cpython/rev/464c6a50b0ce

    @terryjreedy
    Copy link
    Member

    Thank you for the 'positive lookbehind assertion' for anchoring the match after '('. That is what I was missing. '[^,]' is too broad as, in particular, it matches the ')' in 'a(self)'. We don't want the ')' removed. The tests passed with a faulty re because the method test was faulty in that it only tested that something is removed, but not that the removal is correct. I fixed the tests also to really test proper removal.

    @serwy
    Copy link
    Mannequin

    serwy mannequin commented Jul 18, 2012

    Are there any other aspects of the calltip handler that requires fixing? If not, then I can start back-porting the patches.

    @terryjreedy
    Copy link
    Member

    The original patches were to all 3 versions. The later patches were to both 3.x versions and mostly involved get_argspec and expanding the tests. Get_argspec is somewhat different from get_arg_text, the 2.x function it replaced. The main difference I could see is that the latter attempt to deal with old-style classes (I think) and does not use inspect.

    I *think* I am done changing get_argspec until I try making use of the new PEP-362 function signature objects, but that can only affect 3.3+. (This would be to improve calltips for builtins). So if you want to backport further changes to 2.7, so it can pass more of the expanded test suite, now is a good time. Since the TC test class in 2.7 is derived from object, there seems to be no test of old style classes, so I don't know if just replacing get_arg_text with get_argspec would break anything that actually works, assuming that get_argspec does work ;-).

    @serwy
    Copy link
    Mannequin

    serwy mannequin commented Dec 12, 2012

    I haven't had any serious problems with calltips on the dev branches for 2.7 and 3.4, except for bpo-16630. I think this issue can be closed as being completed in order to avoid further expanding its scope. Feel free to reopen if anyone disagrees.

    @serwy serwy mannequin closed this as completed Dec 12, 2012
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-IDLE type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants