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: user code 'import tkinter; tkinter.font' should fail #69693

Closed
terryjreedy opened this issue Oct 29, 2015 · 22 comments
Closed

IDLE: user code 'import tkinter; tkinter.font' should fail #69693

terryjreedy opened this issue Oct 29, 2015 · 22 comments
Assignees
Labels
topic-IDLE type-bug An unexpected behavior, bug, or error

Comments

@terryjreedy
Copy link
Member

BPO 25507
Nosy @terryjreedy, @roseman
Files
  • tkimports.py
  • run_autocomplete.diff
  • tkimports.py
  • 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 2016-11-11.00:10:20.394>
    created_at = <Date 2015-10-29.10:10:42.721>
    labels = ['expert-IDLE', 'type-bug']
    title = "IDLE: user code 'import tkinter; tkinter.font' should fail"
    updated_at = <Date 2016-11-11.00:10:20.393>
    user = 'https://github.com/terryjreedy'

    bugs.python.org fields:

    activity = <Date 2016-11-11.00:10:20.393>
    actor = 'terry.reedy'
    assignee = 'terry.reedy'
    closed = True
    closed_date = <Date 2016-11-11.00:10:20.394>
    closer = 'terry.reedy'
    components = ['IDLE']
    creation = <Date 2015-10-29.10:10:42.721>
    creator = 'terry.reedy'
    dependencies = []
    files = ['43736', '43742', '43761']
    hgrepos = []
    issue_num = 25507
    keywords = ['patch']
    message_count = 22.0
    messages = ['253671', '253683', '253716', '253717', '253718', '253720', '259032', '259033', '260884', '269957', '270067', '270453', '270466', '270468', '270511', '270522', '270590', '270595', '270596', '270598', '280553', '280554']
    nosy_count = 3.0
    nosy_names = ['terry.reedy', 'markroseman', 'python-dev']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25507'
    versions = ['Python 3.5', 'Python 3.6']

    @terryjreedy
    Copy link
    Member Author

    In python, 'import tkinter; tkinter.font' fails with AttributeError. In IDLE, it does not, which is a bug, This lead to a Stackoverflow question 'Why does my code run in IDLE but not with Python'?

    The issue is that importing modules in a package has the side-effect of adding the module name to the package namespace. IDLE's user process runs idlelib/run.py. While *run* does not import tkinter submodules, it imports other modules that do, and the net effect is to add colorchooser, commondialog, dialog, filedialog, font, messagebox, and simpledialog to the tkinter namespace, linked to the corresponding module objects. None are needed in normal operation.

    My first thought was to refactor so that the additions, which run does not need, are not added. My second thought seemed simpler. Delete them (in run.py) after the imports. But it turns out that after deleting a submodule attribute re-import does not work right; the name addition only happens when a module is created, not when found in the sys.modules cache.

    >>> import tkinter; import tkinter.font  # imagine this in run.py
    >>> tkinter.font
    <module tkinter.font...>
    >>> del tkinter.font  # and this in run also, after all imports
    >>> import tkinter.font  # imagine this in user code
    >>> tkinter.font  # and then this
    Traceback (most recent call last):  # it does not work as it should
      File "<stdin>", line 1, in <module>
    AttributeError: module 'tkinter' has no attribute 'font'

    Scratch that idea, and return to refactoring. An obvious culprit in run.py is the import of PyShell. This leads to the import of nearly all of idlelib. However, there are only 4 shared objects actually used, and I believe they could just as well be defined in run (or even in rpc.py or something) and imported from run into PyShell. Then the PyShell import could be deleted. I still need to look at the other imports.

    On startup, user sys.modules also has about 50 other (non-idlelib) stdlib modules not imported by python itself. Not importing PyShell in the 2nd process should reduce this number and speed up IDLE startup, which takes several seconds, first time after boot up, on Windows. It would be good to only import into the user process what is actually needed.

    (Initially importing into the idle process only what is needed to start would also be good, but a separate issue.)

    In 2.7, Tkinter is not a package, so I do not believe it is directly affected by this issue. On the other hand, it also imports too much. So backporting changes to keep things mostly synchronized should benefit 2.7 startup time also.

    @terryjreedy terryjreedy added the type-bug An unexpected behavior, bug, or error label Oct 29, 2015
    @roseman
    Copy link
    Mannequin

    roseman mannequin commented Oct 29, 2015

    This (restructuring/refactoring to minimize the subprocess imports) does definitely sound like the right approach. There will be other benefits to breaking up PyShell a bit too..

    @terryjreedy
    Copy link
    Member Author

    bpo-8231 will benefit if indirect imports of configHandler and idleConf into the user process can be eliminated. I do not currently believe it is needed. Not importing PyShell will go part way.

    AutoComplete imports idleConf to get the popup delay. run imports AutoComplete to access fetch_completions -- in particular, the part after 'else', after the logic to determine which process the code is in and if in the idle process, whether to fetch in the idle process (-n) or user process. Fetch_completions is a function and not properly a method -- it only uses self to access get_entity. Get_entity is a two line function the also should not be a method. It is only called at the one place and the only reason to not put in inline would be for testing. The code after 'else' could be made a function in a separate module and the AutoComplete import eliminated.

    CallTip does not import idleConf, nor does CallTipWindow or HyperParser or PyParse.

    @terryjreedy
    Copy link
    Member Author

    To continue: CallTips.Calltips has a similar here-or-there logic that decides where to execute the actual get-calltip-logic. In this case, get_entity and get_argspec are already functions rather than methods (I changed get_entity from method to function in 02b4c62ce393.) The functions could be moved into a different module with the completions function.

    RemoteDebugger imports Debugger which imports WindowList, ScrolledList, and macosxSupport, all without idleConf imports.

    RemoteObjectBrowser import rpc, both without idleConf. run also imports rpc.

    StackViewer imports TreeWidget and ObjectBrowser, which are not issues. It also imports PyShell for PyShellFileList, which would be an issue, except that this is only for the htest function _stack_viewer. I will move the test-specific import into the function, as is my current policy.

    IOBinding is imported for IOBinding.encoding. The code that calculates this could be put anywhere. This is fortunate because IOBinding imports idleConf and three tkinter modules.

    Conclusion: moving about 2-300 lines of code (about 6 objects) from PyShell, AutoComplete, and IOBinding into run (currently about 400 lines) would eliminate idleConf from the user process. I did not check tkinter imports for all the above, but it might also fix the tkinter namespace problem.

    @terryjreedy
    Copy link
    Member Author

    The subprocess command is built in PyShell.ModifiedInterpreter method build_subprocess_arglist (l.404). It imports run and calls run.main(args).

    The test for this issue would be to import run and check that sys.modules does not contain configHandler and that tkinter does not contain any of its possible submodules.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Oct 30, 2015

    New changeset a37ea1d56e98 by Terry Jan Reedy in branch '2.7':
    Issue bpo-25507: move test-specific imports to test function (idlelib.IOBinding).
    https://hg.python.org/cpython/rev/a37ea1d56e98

    New changeset 38b6b7253ba1 by Terry Jan Reedy in branch '3.4':
    Issue bpo-25507: move test-specific imports to test function (idlelib.IOBinding).
    https://hg.python.org/cpython/rev/38b6b7253ba1

    @terryjreedy
    Copy link
    Member Author

    Ouch. Moving the idleConf import was a blunder. It disabled printing in 2.7.11, 3.4.4, and 3.5.1. When I revert, I will also augment the htest to test the printing and save-as functions. Still have to remember to run it though.

    This sort of functional test is not the main intended use of htests. When refactoring for this issue, automated tests should be added, with mocks used to avoid consequential actions that cannot be part of a buildbot test. For print_window, '''pipe = os.popen(command, "r")''' (https://hg.python.org/cpython/file/tip/Lib/idlelib/IOBinding.py#l463) should be replaced by '''pipe = runcommand(command)''' and 'def runcommand(command): return os.pipe(command, 'r')''' (with subprocess used instead?) added at module level. Then runcommand can be replaced by a mock when testing, and the value of the passed command checked.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 27, 2016

    New changeset 34ca24fa1b4a by Terry Jan Reedy in branch '2.7':
    Issue bpo-25507: revert incorrect movement of idleConf import in a37ea1d56e98.
    https://hg.python.org/cpython/rev/34ca24fa1b4a

    New changeset 86105a109830 by Terry Jan Reedy in branch '3.5':
    Issue bpo-25507: revert incorrect movement of idleConf import in c548ad75160c.
    https://hg.python.org/cpython/rev/86105a109830

    @terryjreedy
    Copy link
    Member Author

    'import tkinter; tkinter.messagebox' should also fail, but currently work in IDLE. See https://stackoverflow.com/questions/35619027/sub-import-issues-with-tkinter-messagebox, and my answer there.

    @terryjreedy
    Copy link
    Member Author

    According to https://stackoverflow.com/questions/38249755/tkinter-not-working-in-cmd-working-in-idle, this code

    from tkinter import filedialog
    root = Tk()

    ran in 3.4.3. After the patch for 3.5.1 (and 3.4.4, 2.7.11) it raises NameError in 3.5.2.

    @terryjreedy
    Copy link
    Member Author

    @terryjreedy
    Copy link
    Member Author

    The SO question and that brought the problem to my attention was in June 2015. https://stackoverflow.com/questions/30877112/tkinter-code-using-font-module-cant-run-from-command-line. The message I just unlinked had the wrong url.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 15, 2016

    New changeset 93d325c64104 by Terry Jan Reedy in branch 'default':
    Issue bpo-25507: Move 4 objects from pyshell to run and switch inports.
    https://hg.python.org/cpython/rev/93d325c64104

    @terryjreedy
    Copy link
    Member Author

    First step. Patch moves 4 objects from pyshell to run and reverses run importing pyshell to pyshell importing run. This initially failed because run imports stackbrowser and the stackbrowser import of something from pyshell failed. Since the import is only needed for htest, I just moved it into the htest code. Removing 'import pyshell' from run avoids 'import tkinter.messagebox' in pyshell. It also reduces len(sys.modules) in the user process by 37 from 193 to 156.

    @terryjreedy terryjreedy self-assigned this Jul 15, 2016
    @terryjreedy
    Copy link
    Member Author

    In bpo-27515, Nick Coughlin said that 'del a.b', would work if sys.modules('a.b') is also deleted -- unless a.b objects to being reloaded. This seems not a problem for the current 8 tkinter submodules. The attached tkimports.py runs without error.

    I am not going to patch 2.7, which does not have the bug, just for the import reduction. The last patch could be applied to 3.5 since it does not remove anything from PyShell. Putting the three warning functions in a class and making the warning global a class attribute (a future issue) would be a different matter.

    @terryjreedy
    Copy link
    Member Author

    run_autocomplete.diff is a preliminary patch for moving the completion list function into run.py and reversing the imports. However, the assert in test_autocomplete, line 103 newly fails because the call to open_completions in autocomplete_event (line 85) returns None. The None comes from l. 155.
    comp_lists = self.fetch_completions(comp_what, mode)
    if not comp_lists[0]:
    return None
    I do not yet know why moving the one function causes the completion lists for re to be blank. I am going to shelve this approach and return to deletion.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 16, 2016

    New changeset 8df5200064c4 by Terry Jan Reedy in branch '3.5':
    Issue bpo-25507: IDLE no longer runs buggy code because of its tkinter imports.
    https://hg.python.org/cpython/rev/8df5200064c4

    New changeset af602a891892 by Terry Jan Reedy in branch 'default':
    Issue bpo-25507: Merge from 3.5 with ttk replacing colorchooser.
    https://hg.python.org/cpython/rev/af602a891892

    @terryjreedy
    Copy link
    Member Author

    I decided to fix this issue for both 3.5 and 3.6 by deleting the submodules both from tkinter and sys.modules (as discussed in bpo-27515).
    I used a new version of tkimports.py to check the result of user imports after the patch. When this file loaded into IDLE and run with F5, the dir listing for each submodule, after previous import and deletion in run startup, is the same as when the file is run with "<python-path> tkimports".

    I opened bpo-27534 to continue run import reduction. I consider 93d325c64104 and run_autocomplete.diff to be part of that issue.

    @terryjreedy
    Copy link
    Member Author

    PS: the test suite run without error on my Win10, 32bit build.

    @terryjreedy
    Copy link
    Member Author

    I am thinking about addin a test.

    @terryjreedy terryjreedy reopened this Jul 17, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 10, 2016

    New changeset 137c7b92360e by Terry Jan Reedy in branch '2.7':
    Issue bpo-25507: Add back import needed for 2.x encoding warning box.
    https://hg.python.org/cpython/rev/137c7b92360e

    @terryjreedy
    Copy link
    Member Author

    Import is only needed for the warning issued when someone using IDLE 2.x puts a non-ascii character into code being edited and tries to save without adding an encoding declaration. https://stackoverflow.com/questions/40523932/idle-2-7-11-12-nameerror-global-name-toplevel-is-not-defined

    @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

    1 participant