Repository: ipython/ipython
Pull request: Do not always import * with autoreload3
Fixes #14839 

Previously, `%autoreload 3` always had the effect of `from module import *` on reloads. This PR fixes this functionality to the expected utility: if users specifically import some symbols from a module (for example `from X import Y` or `from X import Y as Z`), only those symbols are reloaded. 

Unit tests are added to demonstrate the new behavior as well. 

Problem statement:
autoreload 3 has the effect of import *
The autoreload extension is almost magical, huge thanks to whoever's worked on it. There is one behavior that I find unexpected and I think may have been unintended.

We have a file reload.py containing:
```
n = 5
```
Then in ipython we do:
```
%load_ext autoreload
%autoreload 3
from reload import n
```
now we edit and save reload.py to be:
```
n = 5
m = 7
```
Now in the ipython repl, m is also defined, even though we just imported n. In the ipython test suite for this functionality in `test_autoload_newly_added_objects`, it demonstrates it with `from module import *`. But even if we didn't `import *` it will effectively always behave that way, as with `autoreload 3`, it sets `autoload_obj` to True, which causes it to pass shell into superreload, where it does `shell.user_ns[name] = new_obj` for every new variable, even if we didn't `import *`, but explicitly only wanted to import `n` as in this example. I'm not sure if this is intended or a bug, but it seems like it has the potential to dangerously clobber existing names in cases where users didn't `import *`

Hints from issue discussion:
Hey @sczi thanks for catching this. I put up a PR #14872  with a minor modification to `autoreload 3` so that we appropriately deal with the `from X import Y` and `from X import Y as Z` cases -- I agree that it's unlikely this is intended.

Commit message: Remove unused import

Patch to review:
diff --git a/IPython/extensions/autoreload.py b/IPython/extensions/autoreload.py
index 39ac6d52d4..ebbeab7d41 100644
--- a/IPython/extensions/autoreload.py
+++ b/IPython/extensions/autoreload.py
@@ -127,6 +127,7 @@
 # Imports
 # -----------------------------------------------------------------------------
 
+import ast
 import os
 import sys
 import traceback
@@ -247,7 +248,33 @@ def check(self, check_all=False, do_reload=True):
             modules = list(sys.modules.keys())
         else:
             modules = list(self.modules.keys())
-
+        imports_froms = None
+        symbol_map = {}
+        if self.shell and "In" in self.shell.user_ns:
+            imports_froms = {}
+            lines = self.shell.user_ns["In"]
+            sourcecode = "\n".join(lines)
+            tree = ast.parse(sourcecode)
+            for node in ast.walk(tree):
+                # Handle "from X import Y" style imports
+                if isinstance(node, ast.ImportFrom):
+                    mod = node.module
+                    if mod not in imports_froms:
+                        imports_froms[mod] = []
+
+                    for name in node.names:
+                        # name.name is going to be actual name that we want to import from module
+                        # name.asname is Z in the case of from X import Y as Z
+                        # we should update Z in the shell in this situation, so track it too.
+                        imports_froms[mod].append(name.name)
+                        if mod not in symbol_map:
+                            symbol_map[mod] = {}
+                        symbol_map[mod][name.name] = (
+                            name.asname if name.asname else name.name
+                        )
+        import_from_tracker = (
+            ImportFromTracker(imports_froms, symbol_map) if imports_froms else None
+        )
         for modname in modules:
             m = sys.modules.get(modname, None)
 
@@ -275,7 +302,13 @@ def check(self, check_all=False, do_reload=True):
                 self._report(f"Reloading '{modname}'.")
                 try:
                     if self.autoload_obj:
-                        superreload(m, reload, self.old_objects, self.shell)
+                        superreload(
+                            m,
+                            reload,
+                            self.old_objects,
+                            self.shell,
+                            import_from_tracker=import_from_tracker,
+                        )
                     # if not using autoload, check if deduperreload is viable for this module
                     elif self.deduper_reloader.maybe_reload_module(m):
                         pass
@@ -427,6 +460,12 @@ def __call__(self):
 ]
 
 
+class ImportFromTracker:
+    def __init__(self, imports_froms, symbol_map):
+        self.imports_froms = imports_froms
+        self.symbol_map = symbol_map
+
+
 def append_obj(module, d, name, obj, autoload=False):
     in_module = hasattr(obj, "__module__") and obj.__module__ == module.__name__
     if autoload:
@@ -445,7 +484,9 @@ def append_obj(module, d, name, obj, autoload=False):
     return True
 
 
-def superreload(module, reload=reload, old_objects=None, shell=None):
+def superreload(
+    module, reload=reload, old_objects=None, shell=None, import_from_tracker=None
+):
     """Enhanced version of the builtin reload function.
 
     superreload remembers objects previously in the module, and
@@ -486,17 +527,28 @@ def superreload(module, reload=reload, old_objects=None, shell=None):
         module.__dict__.update(old_dict)
         raise
 
-    # iterate over all objects and update functions & classes
     for name, new_obj in list(module.__dict__.items()):
         key = (module.__name__, name)
         if key not in old_objects:
             # here 'shell' acts both as a flag and as an output var
+            imports_froms = (
+                import_from_tracker.imports_froms if import_from_tracker else None
+            )
+            symbol_map = import_from_tracker.symbol_map if import_from_tracker else None
             if (
                 shell is None
                 or name == "Enum"
                 or not append_obj(module, old_objects, name, new_obj, True)
+                or (
+                    imports_froms
+                    and module.__name__ in imports_froms
+                    and "*" not in imports_froms[module.__name__]
+                    and name not in imports_froms[module.__name__]
+                )
             ):
                 continue
+            if symbol_map and name in symbol_map.get(module.__name__, {}):
+                name = symbol_map.get(module.__name__, {})[name]
             shell.user_ns[name] = new_obj
 
         new_refs = []
diff --git a/tests/test_zzz_autoreload.py b/tests/test_zzz_autoreload.py
index db0cb1981e..34e4930460 100644
--- a/tests/test_zzz_autoreload.py
+++ b/tests/test_zzz_autoreload.py
@@ -51,6 +51,7 @@ class FakeShell:
     def __init__(self):
         self.ns = {}
         self.user_ns = self.ns
+        self.user_ns["In"] = []
         self.user_ns_hidden = {}
         self.events = EventManager(self, {"pre_run_cell", pre_run_cell})
         self.auto_magics = AutoreloadMagics(shell=self)
@@ -81,6 +82,7 @@ def run_code(self, code):
         )
         exec(code, self.user_ns)
         self.auto_magics.post_execute_hook()
+        self.user_ns["In"].append(code)
 
     def push(self, items):
         self.ns.update(items)
@@ -417,6 +419,149 @@ def meth(self):
         self.shell.run_code("t = ExtTest(); assert t.meth() == 'ext'")
         self.shell.run_code("assert ext_int == 2")
 
+    def test_autoload3_import_Y_as_Z(self):
+        self.shell.magic_autoreload("3")
+        mod_code = """
+        def func1(): pass
+        n = 1
+        """
+        mod_name, mod_fn = self.new_module(textwrap.dedent(mod_code))
+        self.shell.run_code(f"from {mod_name} import n as foo")
+        self.shell.run_code("foo")
+        with self.assertRaises(NameError):
+            self.shell.run_code("func1()")
+
+        new_code = """
+        n = 100
+        def func2(): pass
+        def func1(): pass
+        m = 5
+        """
+        self.write_file(mod_fn, textwrap.dedent(new_code))
+
+        self.shell.run_code("foo")
+        with self.assertRaises(NameError):
+            self.shell.run_code("n")
+
+    def test_autoload3_import_Y_as_Z_overloading(self):
+        self.shell.magic_autoreload("3")
+        mod_code = """
+        def func1(): pass
+        n = 1
+        """
+        mod_name, mod_fn = self.new_module(textwrap.dedent(mod_code))
+        self.shell.run_code(f"from {mod_name} import n as foo")
+        self.shell.run_code("foo")
+        with self.assertRaises(NameError):
+            self.shell.run_code("func1()")
+
+        new_code = """
+        n = 100
+        def func2(): pass
+        def func1(): pass
+        foo = 45
+        """
+        self.write_file(mod_fn, textwrap.dedent(new_code))
+        self.shell.run_code(f"assert foo == 100")
+        self.shell.run_code(f"from {mod_name} import foo")
+        self.shell.run_code(f"assert foo == 45")
+
+    def test_autoload3_normalimport(self):
+        self.shell.magic_autoreload("3")
+        mod_code = """
+        def func1(): pass
+        n = 1
+        """
+        mod_name, mod_fn = self.new_module(textwrap.dedent(mod_code))
+        self.shell.run_code(f"import {mod_name}")
+        self.shell.run_code(f"{mod_name}.func1()")
+        self.shell.run_code(f"{mod_name}.n")
+
+        new_code = """
+        n = 100
+        def func2(): pass
+        def func1(): pass
+        m = 5
+        """
+        self.write_file(mod_fn, textwrap.dedent(new_code))
+
+        self.shell.run_code(f"{mod_name}.func1()")
+        self.shell.run_code(f"{mod_name}.n")
+        self.shell.run_code(f"{mod_name}.func2()")
+        self.shell.run_code(f"{mod_name}.m")
+
+        self.shell.run_code(f"from {mod_name} import n")
+        self.shell.run_code(f"{mod_name}.func1()")
+        self.shell.run_code(f"n")
+
+    def test_autoload3_normalimport_2(self):
+        self.shell.magic_autoreload("3")
+        mod_code = """
+        def func1(): pass
+        n = 1
+        """
+        mod_name, mod_fn = self.new_module(textwrap.dedent(mod_code))
+        self.shell.run_code(f"from {mod_name} import n")
+        with self.assertRaises(NameError):
+            self.shell.run_code("func1()")
+        self.shell.run_code("n")
+
+        new_code = """
+        n = 100
+        def func2(): pass
+        def func1(): pass
+        m = 5
+        """
+        self.shell.run_code(f"import {mod_name}")
+        self.write_file(mod_fn, textwrap.dedent(new_code))
+        self.shell.run_code(f"{mod_name}.func1()")
+        self.shell.run_code(f"{mod_name}.n")
+        self.shell.run_code(f"{mod_name}.func2()")
+        self.shell.run_code(f"{mod_name}.m")
+        self.shell.run_code(f"n")
+
+    def test_autoload_3_does_not_add_all(self):
+        # Tests that %autoreload 3 does not effectively run from X import *
+        self.shell.magic_autoreload("3")
+        mod_code = """
+        def func1(): pass
+        n = 1
+        """
+        mod_name, mod_fn = self.new_module(textwrap.dedent(mod_code))
+        self.shell.run_code(f"from {mod_name} import n")
+        self.shell.run_code("n")
+        with self.assertRaises(NameError):
+            self.shell.run_code("func()")
+
+        new_code = """
+        n = 1
+        def func2(): pass
+        def func1(): pass
+        m = 5
+        """
+        self.write_file(mod_fn, textwrap.dedent(new_code))
+        self.shell.run_code("n")
+        with self.assertRaises(NameError):
+            self.shell.run_code("func1()")
+        with self.assertRaises(NameError):
+            self.shell.run_code("func2()")
+        with self.assertRaises(NameError):
+            self.shell.run_code("m")
+
+        self.shell.run_code(f"from {mod_name} import m")
+        self.shell.run_code("n")
+        self.shell.run_code("m")
+        with self.assertRaises(NameError):
+            self.shell.run_code("func1()")
+        with self.assertRaises(NameError):
+            self.shell.run_code("func2()")
+
+        self.shell.run_code(f"from {mod_name} import func1,func2")
+        self.shell.run_code("n")
+        self.shell.run_code("m")
+        self.shell.run_code("func1()")
+        self.shell.run_code("func2()")
+
     def test_verbose_names(self):
         # Asserts correspondense between original mode names and their verbose equivalents.
         @dataclass


Code review comments:
diff_hunk: @@ -247,7 +248,33 @@ def check(self, check_all=False, do_reload=True):
             modules = list(sys.modules.keys())
         else:
             modules = list(self.modules.keys())
-
+        imports_froms = None
+        symbol_map = {}
+        if self.shell and "In" in self.shell.user_ns:
+            imports_froms = {}
+            lines = self.shell.user_ns["In"]
+            sourcecode = "\n".join(lines)
+            tree = ast.parse(sourcecode)
path: IPython/extensions/autoreload.py
text: @user1:
This is an edge case, but with this approach, defining a variable called `In` will break autoreload. An as per copilot it does a lot of the same work on each execution so it will slow down the interpreter in longer sessions.

I wonder if you could instead pass `info` object from the even hook which is here:

https://github.com/ipython/ipython/blob/6b98baf45cf30b61ec72a0a5c9b6e629b3fac5f6/IPython/extensions/autoreload.py#L781-L786

and maybe make it contain the `transformed_cell` code because as of now it only contains the `raw_cell`:

https://github.com/ipython/ipython/blob/b854379e3b801ce812c648e95f8846a1334e3195/IPython/core/interactiveshell.py#L3238-L3249

and then here just make it record all "from" imports on an instance of a class for each cell? Does it make sense?

@user1:
@author what do you think about the above suggestion? I was hoping to include this PR in 9.3 to be released tomorrow.

@user1:
Another thought here, could it to be run only when autoreload=3?

@author:
Hi @user1 Sorry for the delayed response, all of the above sounds very reasonable to me, will implement. I discovered some other regressions that this pr would cause so I'm moving it to draft while I sort these out.
We can probably aim for adding this in 9.4.

diff_hunk: @@ -247,7 +248,33 @@ def check(self, check_all=False, do_reload=True):
             modules = list(sys.modules.keys())
         else:
             modules = list(self.modules.keys())
-
+        imports_froms = None
+        symbol_map = {}
+        if self.shell and "In" in self.shell.user_ns:
+            imports_froms = {}
+            lines = self.shell.user_ns["In"]
+            sourcecode = "\n".join(lines)
+            tree = ast.parse(sourcecode)
+            for node in ast.walk(tree):
path: IPython/extensions/autoreload.py
text: @user1:
Parsing the entire input history with `ast.parse` on every reload could be expensive in large notebooks; consider caching parsed imports or incrementally parsing only new cells.
```suggestion
            if not hasattr(self, "_parsed_cache"):
                self._parsed_cache = {"lines": [], "imports_froms": {}, "symbol_map": {}}

            # Identify new or modified cells
            new_lines = lines[len(self._parsed_cache["lines"]):]
            self._parsed_cache["lines"] = lines

            imports_froms = self._parsed_cache["imports_froms"]
            symbol_map = self._parsed_cache["symbol_map"]

            if new_lines:
                sourcecode = "\n".join(new_lines)
                tree = ast.parse(sourcecode)
                for node in ast.walk(tree):
```