Repository: sphinx-gallery/sphinx-gallery

Issue:
Searching for short module names in backreferences has side effects
This came up via a weird problem we were having building [napari docs](https://github.com/napari/docs) on certain platforms. I think this is a pretty niche problem, and in our case was caused partly by [setuptools allowing case-insensitive imports from editable installations](https://github.com/pypa/setuptools/issues/3994) on certain platforms (in our case macOS). I think that will eventually be fixed, but regardless it suggests a possible improvement in sphinx-gallery.

The basic problem is that sphinx-gallery does some importing when searching for short(er) module names in backreferences:
https://github.com/sphinx-gallery/sphinx-gallery/blob/2e1e2a8a23ed8becb737dac15a733f8b93318cda/sphinx_gallery/backreferences.py#L165

This has potential side effects that may affect examples, scrapers, and reset functions.

Unfortunately I don't really know a good way to clean up after such imports or perform them in isolation (subprocesses would be way too slow). You can remove things from `sys.modules`, but imports also get inserted into the namespace of the parent module. There are also ways to use `importlib` to execute the module without importing it, but none of this prevents the module itself from importing others.

All this said, my current best idea is to _only import modules that are already imported_ when looking at backreferences. That is, something like this:
```diff
diff --git a/sphinx_gallery/backreferences.py b/sphinx_gallery/backreferences.py
index 2d070f5..dfc2d9d 100644
--- a/sphinx_gallery/backreferences.py
+++ b/sphinx_gallery/backreferences.py
@@ -14,6 +14,7 @@ from html import escape
 import inspect
 import os
 import re
+import sys
 import warnings
 
 from sphinx.errors import ExtensionError
@@ -163,6 +164,7 @@ class NameFinder(ast.NodeVisitor):
 
 
 def _from_import(a, b):
+    assert a in sys.modules, f'{a} not in sys.modules, not importing it'
     imp_line = f'from {a} import {b}'
     scope = dict()
     with warnings.catch_warnings(record=True):  # swallow warnings
```

This should be safer as it leaves all initial importing up to the user/example code. I think the cost is that it may be a little less aggressive at finding shorter names. I don't think this will generally make a big (or any) difference, but I am also not that familiar with the feature. I did a build with and without this change on the napari docs and couldn't find a meaningful difference in the backrefs. If needed it could be added as a configuration, or maybe an existing configuration already covers this behavior?

I'm happy to hear other ideas here or thoughts on whether this is a real problem at all 😅.

See https://github.com/napari/docs/issues/214 for more discussion - also thanks to @lucyleeow for listening to all my wild theories as I investigated this.

Discussion:
larsoner: I think in principle we shouldn't need a config option for this. I'd rather try using it globally and if we find problems with it fix them (maybe by use of a config var but hopefully not)

Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 4d95321..efa5edd 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -1,12 +1,12 @@
 repos:
   - repo: https://github.com/psf/black
-    rev: 23.7.0
+    rev: 23.9.1
     hooks:
       - id: black
         args: [--quiet]
         exclude: plot_syntaxerror
   - repo: https://github.com/astral-sh/ruff-pre-commit
-    rev: v0.0.285
+    rev: v0.0.290
     hooks:
       - id: ruff
   - repo: https://github.com/codespell-project/codespell
diff --git a/sphinx_gallery/backreferences.py b/sphinx_gallery/backreferences.py
index 55c185b..3b1ce53 100644
--- a/sphinx_gallery/backreferences.py
+++ b/sphinx_gallery/backreferences.py
@@ -12,7 +12,7 @@ from html import escape
 import inspect
 import os
 import re
-import warnings
+import sys
 
 from sphinx.errors import ExtensionError
 import sphinx.util
@@ -178,42 +178,34 @@ class NameFinder(ast.NodeVisitor):
         return options
 
 
-def _from_import(a, b):
-    imp_line = f"from {a} import {b}"
-    scope = dict()
-    with warnings.catch_warnings(record=True):  # swallow warnings
-        warnings.simplefilter("ignore")
-        exec(imp_line, scope, scope)
-    return scope
-
-
 def _get_short_module_name(module_name, obj_name):
     """Get the shortest possible module name."""
     if "." in obj_name:
         obj_name, attr = obj_name.split(".")
     else:
         attr = None
-    scope = {}
+
     try:
-        # Find out what the real object is supposed to be.
-        scope = _from_import(module_name, obj_name)
-    except Exception:  # wrong object
+        # look only in sys.modules to avoid importing the module, which may
+        # otherwise have side effects
+        real_obj = getattr(sys.modules[module_name], obj_name)
+        if attr is not None:
+            getattr(real_obj, attr)
+    except (AttributeError, KeyError):
+        # AttributeError: wrong class
+        # KeyError: wrong object or module not previously imported
         return None
-    else:
-        real_obj = scope[obj_name]
-        if attr is not None and not hasattr(real_obj, attr):  # wrong class
-            return None  # wrong object
 
     parts = module_name.split(".")
     short_name = module_name
     for i in range(len(parts) - 1, 0, -1):
         short_name = ".".join(parts[:i])
-        scope = {}
         try:
-            scope = _from_import(short_name, obj_name)
-            # Ensure shortened object is the same as what we expect.
-            assert real_obj is scope[obj_name]
-        except Exception:  # libraries can throw all sorts of exceptions...
+            assert real_obj is getattr(sys.modules[short_name], obj_name)
+        except (AssertionError, AttributeError, KeyError):
+            # AssertionError: shortened object is not what we expect
+            # KeyError: short module name not previously imported
+            # AttributeError: wrong class or object
             # get the last working module name
             short_name = ".".join(parts[: (i + 1)])
             break
diff --git a/sphinx_gallery/py_source_parser.py b/sphinx_gallery/py_source_parser.py
index a7b401d..59fd8d7 100644
--- a/sphinx_gallery/py_source_parser.py
+++ b/sphinx_gallery/py_source_parser.py
@@ -212,7 +212,7 @@ def remove_ignore_blocks(code_block):
     """
     Return the content of *code_block* with ignored areas removed.
 
-    An ignore block starts with # sphinx_gallery_begin_ignore, and ends with
+    An ignore block starts with # sphinx_gallery_start_ignore, and ends with
     # sphinx_gallery_end_ignore. These lines and anything in between them will
     be removed, but surrounding empty lines are preserved.
 
@@ -221,8 +221,8 @@ def remove_ignore_blocks(code_block):
     code_block : str
         A code segment.
     """
-    num_start_flags = len(re.findall(START_IGNORE_FLAG, code_block))
-    num_end_flags = len(re.findall(END_IGNORE_FLAG, code_block))
+    num_start_flags = len(re.findall(START_IGNORE_FLAG, code_block, re.MULTILINE))
+    num_end_flags = len(re.findall(END_IGNORE_FLAG, code_block, re.MULTILINE))
 
     if num_start_flags != num_end_flags:
         raise ExtensionError(


Test patch:
diff --git a/sphinx_gallery/tests/test_py_source_parser.py b/sphinx_gallery/tests/test_py_source_parser.py
index afc37e3..15a8280 100644
--- a/sphinx_gallery/tests/test_py_source_parser.py
+++ b/sphinx_gallery/tests/test_py_source_parser.py
@@ -76,7 +76,7 @@ def test_remove_ignore_comments():
     normal_code = "# Regular code\n# should\n# be untouched!"
     assert sg.remove_ignore_blocks(normal_code) == normal_code
 
-    mismatched_code = "# sphinx_gallery_start_ignore"
+    mismatched_code = "x=5\n# sphinx_gallery_start_ignore\ny=4"
     with pytest.raises(ExtensionError) as error:
         sg.remove_ignore_blocks(mismatched_code)
     assert "must have a matching" in str(error)