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: print "Did you mean?" for AttributeError and NameError #88192

Closed
sweeneyde opened this issue May 4, 2021 · 19 comments
Closed

IDLE: print "Did you mean?" for AttributeError and NameError #88192

sweeneyde opened this issue May 4, 2021 · 19 comments
Assignees
Labels
3.10 only security fixes 3.11 bug and security fixes topic-IDLE type-feature A feature request or enhancement

Comments

@sweeneyde
Copy link
Member

BPO 44026
Nosy @terryjreedy, @aroberge, @pablogsal, @miss-islington, @sweeneyde, @E-Paine, @shreyanavigyan
PRs
  • bpo-44026: Idle get Python's 'did you mean' #25912
  • [3.10] bpo-44026: Idle - display interpreter's 'did you mean' hints (GH-25912) #25973
  • [3.9] bpo-44026: Idle - display interpreter's 'did you mean' hints (GH-25912) #25975
  • 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 2021-05-08.01:00:19.427>
    created_at = <Date 2021-05-04.03:49:13.100>
    labels = ['expert-IDLE', 'type-feature', '3.10', '3.11']
    title = 'IDLE: print "Did you mean?" for AttributeError and NameError'
    updated_at = <Date 2021-05-08.01:00:19.426>
    user = 'https://github.com/sweeneyde'

    bugs.python.org fields:

    activity = <Date 2021-05-08.01:00:19.426>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2021-05-08.01:00:19.427>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2021-05-04.03:49:13.100>
    creator = 'Dennis Sweeney'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44026
    keywords = ['patch']
    message_count = 19.0
    messages = ['392850', '392854', '392855', '392857', '392859', '392861', '392863', '392864', '392872', '392878', '392879', '392933', '392939', '392945', '392966', '392968', '393055', '393232', '393233']
    nosy_count = 7.0
    nosy_names = ['terry.reedy', 'aroberge', 'pablogsal', 'miss-islington', 'Dennis Sweeney', 'epaine', 'shreyanavigyan']
    pr_nums = ['25912', '25973', '25975']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue44026'
    versions = ['Python 3.10', 'Python 3.11']

    @sweeneyde
    Copy link
    Member Author

    After bpo-38530, I get this in the python shell:

    Python 3.10.0b1 (tags/v3.10.0b1:ba42175, May  3 2021, 20:22:30) [MSC v.1928 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> class A:
    ...     foobar = 1
    ...
    >>> A.foocar
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    AttributeError: type object 'A' has no attribute 'foocar'. Did you mean: 'foobar'?
    >>>

    But I get this in IDLE:

    Python 3.10.0b1 (tags/v3.10.0b1:ba42175, May 3 2021, 20:22:30) [MSC v.1928 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license()" for more information.
    class A:
    foobar = 1

        
    A.foocar
    Traceback (most recent call last):
      File "<pyshell#3>", line 1, in <module>
        A.foocar
    AttributeError: type object 'A' has no attribute 'foocar'

    Can we extend this functionality to IDLE, and fix the discrepancy?

    @sweeneyde sweeneyde added 3.10 only security fixes 3.11 bug and security fixes labels May 4, 2021
    @sweeneyde sweeneyde added topic-IDLE type-feature A feature request or enhancement labels May 4, 2021
    @terryjreedy
    Copy link
    Member

    I am surprised for 2 reasons. First, I have seen other improved messages in IDLE, though maybe only for syntax errors. Second, code entered through IDLE is executed by Python, 'same' in this respect as code entered through the REPL.

    In more detail, IDLE calls
    exec(compile(code, '<pyshell>', 'single', 0x200, True))
    Still, 'int.reel' does not give the message while 'int.reel' in the exec above does. I have no idea right now why the difference.

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented May 4, 2021

    Python shell uses <stdin> to evaluate while IDLE uses <pyshell>. Is that the problem?

    @sweeneyde
    Copy link
    Member Author

    I'm not sure if this helps, but this is the relevant tree of callers:

    suggestions.c: _Py_Offer_Suggestions() (the expected behavior) is only referenced by
    pythonrun.c: print_exception(), which is only referenced by
    pythonrun.c: print_exception_recursive(), which is only referenced by itself and
    pythonrun.c: _PyErr_Display(), which is referenced by:
    - threadmodule.c: thread_excepthook_file()
    - pythonrun.c: PyErr_Display()
    pythonrun.c: PyErr_Display() is referenced by:
    - pythonrun.c: _PyErr_PrintEx()
    - pythonrun.c: _Py_FatalErr_PrintExc()
    - _testcapimodule.c: exception_print()
    - sysmodule.c: sys_excepthook_impl() (Python's sys.excepthook)
    pythonrun.c: _PyErr_PrintEx() is referenced by:
    - pythonrun.c: PyErr_PrintEx() is referenced by
    - pythonrun.c: PyErr_Print()
    - pylifecycle.c: new_interpreter()
    - pythonrun.c _PyErr_Print() is referenced by
    - ceval.c:_Py_FinishPendingCalls()
    - pylifecycle.c: init_importlib_external()
    - pylifecycle.c: init_interp_main()

    @sweeneyde
    Copy link
    Member Author

    PyErr_Display() grabs the current sys.stderr and writes to that, but it looks like IDLE never gets to call PyErr_Display().

    @sweeneyde
    Copy link
    Member Author

    It looks like Lib/idlelib/run.py : print_exception() re-implements the traceback, rather than relying on sys.excepthook

    @sweeneyde
    Copy link
    Member Author

    Indeed, this change enables the feature for IDLE, though I'm not sure what it breaks.

    diff --git a/Lib/idlelib/run.py b/Lib/idlelib/run.py
    index 07e9a2bf9c..319b16f311 100644
    --- a/Lib/idlelib/run.py
    +++ b/Lib/idlelib/run.py
    @@ -569,7 +569,7 @@ def runcode(self, code):
                 self.user_exc_info = sys.exc_info()  # For testing, hook, viewer.
                 if quitting:
                     exit()
    -            if sys.excepthook is sys.__excepthook__:
    +            if False:
                     print_exception()
                 else:
                     try:

    @shreyanavigyan
    Copy link
    Mannequin

    shreyanavigyan mannequin commented May 4, 2021

    Does the test suite run succesfully?

    @terryjreedy
    Copy link
    Member

    (Shreyan, the fake filenames are not relevant.)

    By default, sys.excepthook is sys.__excepthook is <built-in function excepthook>. So by default IDLE runcode calls print_exception, which call cleanup_traceback for each traceback printed. This function makes two important changes to the traceback.

    First, it removes traceback lines that are not present when running in the C-coded REPL, because IDLE does some functions with Python code. The result is that IDLE tracebacks nearly always equal REPL tracebacks.

    Second, for Shell code, IDLE add cached code lines. Compare

    >>> a = b
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    NameError: name 'b' is not defined

    to

    >>> a = b
    Traceback (most recent call last):
      File "<pyshell#9>", line 1, in <module>
        a = b
    NameError: name 'b' is not defined
    >>> 

    We are not going to stop using print_exception. Note that it is normal for IDEs to modify tracebacks.

    The 'is' is false when user change excepthook. The false branch was recently added to run user excepthooks.

    Pablo, I checked an AttributeError instance and the missing phrase is not present. Why is it hidden from Python code. Why not add the rest of the message to the message? Or at least add it to the tuple or an attribute. Or add a .get_suggestion method to exception objects, like .with_traceback.

    @pablogsal
    Copy link
    Member

    Pablo, I checked an AttributeError instance and the missing phrase is not present

    This cannot be included in the AttributeError because of performance reasons. These errors will be thrown naturally all over the place without this meaning that the interpreter will finish so we can only compute these safely on the except hook.

    We also discarded mutating the attribute error because it can make some stuff crash if the exception happens on a thread and that doesn't imply full program termination.

    @pablogsal
    Copy link
    Member

    I don't feel comfortable adding a public API to get the message either in the traceback or the exception, at least for the time being.

    @terryjreedy
    Copy link
    Member

    I realized after posting that looking for close matching is a performance issue and does not need to be done unless the exception is going to be displayed. But it is a shame to keep such great work away from many users.

    What would you think of either

    1. add sys.tracebackhook, to be called by the traceback display functions if not None. Then idlelib.run.print_exception would not have to duplicate the sometimes changing logic for chaining or otherwise joining multiple tracebacks into one mega-traceback. Instead, from what Dennis said above, it could set the hook and call the built-in exceptionhook.

    2. add a new optional parameter to exceptionhook and the traceback display function. This would have the same advantage as above.

    @pablogsal
    Copy link
    Member

    Those are intesting options, I will think about this. I am still afraid of adding more APIs in this area as having arbitrary Python getting call in the middle of handling the exceptions it can be quite tricky: think that we are handling an exception already so other exceptions can mess up stuff in very subtle ways or we need to ignore exceptions and that can also be dangerous.

    Another thing we could do is reproduce the feature in idle itself with a custom display hook, which would be easier as we can just use difflib. It would be slightly different than the built-in one but honestly I don't think that's a problem at all. I understand that other people may think differently so I am open minded as well. But I wanted to clarify what are my worries :)

    @sweeneyde
    Copy link
    Member Author

    Another idea: do what test_exceptions() does:

        try:
            f()
        except NameError as exc:
            with support.captured_stderr() as err:
                sys.__excepthook__(*sys.exc_info())
    
            self.assertNotIn("a1", err.getvalue())

    Then instead of the assert, do something like

        last_line = err.getvalue().rsplit("\n", 2)[-2]
        _, did_you_mean, suggestion = last_line.rpartition("Did you mean: ")
        if did_you_mean:
           print(did_you_mean + suggestion)

    This can probably be done without test.support.

    @terryjreedy
    Copy link
    Member

    Pablo, unless you are suggesting rewriting IDLE's custom exception handing in C, I don't see how making it a single function would be an improvement. As long as it is Python code, the problem is accessing the message supplement.

    After reading your comment, I agree that injecting Python code into the C exception handling is a bad idea. It also, I see now, not needed

    run.print_exception first calls traceback.extract_tb (line 238). The doc says " It is useful for alternate formatting of stack traces", which IDLE does. print_exception next calls the undocumented traceback.print_list, which is format_list + print*. Finally, print_exception calls traceback.format_exception_only (line 2440), which returns a list of lines that is immediately printed.

    So all IDLE needs is for format_exception_only to include the extra lines, either by default or by option. But it just calls TracebackException.format_exception)only, which call a private Python-coded function, which can only add the lines if accessible from Python-code.

    Dennis cleverly pointed out that such access is available, even if awkwardly, by capturing the standard excepthook outout and keeping only the missing part. (test.support.captured_stderr just wraps io.StringIO as a context manager. IDLE can borrow and edits its code.)

    Dennis, would you like to prepare a PR that follows the format_exception_only call with something like

        if isinstance(typ, (AttributeError, NameError)):
            lines.extend(extract_extra_message(<args>))

    and a definition of the new function? (and a new test)? Or perhaps better, replace format_exception_only with a function that gets all lines at once.

    Pablo, do any other exception types have such omitted help info? Do you have plans for any more?

    @terryjreedy terryjreedy changed the title IDLE doesn't offer "Did you mean?" for AttributeError and NameError IDLE: print "Did you mean?" for AttributeError and NameError May 4, 2021
    @sweeneyde
    Copy link
    Member Author

    I unfortunately don't have the time/internet access this week to do a PR.

    @terryjreedy
    Copy link
    Member

    This is only backported to 3.10 and not 3.9 because the fix recommendations were only added in 3.10.

    If the code difference became a problem, this could be backported because the roundabout method for name and attribute errors would work in 3.9.

    @terryjreedy
    Copy link
    Member

    New changeset 5a5237c by Miss Islington (bot) in branch '3.10':
    bpo-44026: Idle - display interpreter's 'did you mean' hints (GH-25912)
    5a5237c

    @terryjreedy
    Copy link
    Member

    Merged to 3.11.0a0 first, bot forget to post it.

    Dennis, thank you for the analysis and then the suggestion as to how to access the not directly accessible. It would likely have been awhile before I stumbled from 'cannot' to 'can with workaround'. Feel free to add ideas on other IDLE issues.

    Thanks EP for making the fix work even with chained exceptions *and* for providing tests. I redid half the lines, but core test logic was correct and remains. In .0b1+ repository (and future .0b2 release) IDLE:

    >>> try: abc
    ... except NameError: f"{complex.reel(1+1j)} errors occurred!"
    ... 
    Traceback (most recent call last):
      File "<pyshell#2>", line 1, in <module>
        try: abc
    NameError: name 'abc' is not defined. Did you mean: 'abs'?
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "<pyshell#2>", line 2, in <module>
        except NameError: f"{complex.reel(1+1j)} errors occurred!"
    AttributeError: type object 'complex' has no attribute 'reel'. Did you mean: 'real'?

    And thank you Pablo for making exception messages more helpful.

    @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
    3.10 only security fixes 3.11 bug and security fixes topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants