Merge pull request #80 from plasma-umass/copilot/sub-pr-77

Add defensive check to prevent double-wrapping in spec_from_file_location wrapper

Author: emeryberger

src/slipcover/importer.py

@@ -310,6 +310,10 @@ def spec_from_file_location_wrapper(name, location=None, *, loader=None, submodu
         if isinstance(spec.loader, machinery.ExtensionFileLoader):
             return spec
 
+        # Skip if already wrapped - prevent double-wrapping
+        if isinstance(spec.loader, SlipcoverLoader):
+            return spec
+
         # Check if this file should be instrumented
         origin = spec.origin or (str(location) if location else None)
         if origin and file_matcher.matches(origin):

tests/test_importer.py

@@ -450,6 +450,62 @@ def downgrade():
     assert 7 in executed_lines or 8 in executed_lines, f"upgrade() lines not executed, executed: {executed_lines}"
 
 
+def test_wrap_spec_from_file_location_no_double_wrap(tmp_path, monkeypatch):
+    """Test that spec_from_file_location wrapper doesn't double-wrap already wrapped loaders."""
+    import importlib.util
+    
+    # Create a test file
+    test_file = tmp_path / "test_module.py"
+    test_file.write_text('''
+x = 1
+y = 2
+''')
+    
+    monkeypatch.chdir(tmp_path)
+    
+    # Set up slipcover and file matcher
+    import slipcover as sc
+    sci = sc.Slipcover()
+    fm = im.FileMatcher()
+    fm.addSource(tmp_path)
+    
+    # Wrap spec_from_file_location
+    im.wrap_spec_from_file_location(sci, fm)
+    
+    # Get the current (wrapped) spec_from_file_location
+    wrapped_spec_from_file_location = importlib.util.spec_from_file_location
+    
+    # Call wrapped function to get a spec with a wrapped loader
+    spec = wrapped_spec_from_file_location("test_module", test_file)
+    assert spec is not None
+    assert spec.loader is not None
+    assert isinstance(spec.loader, im.SlipcoverLoader), \
+        f"Expected SlipcoverLoader, got {type(spec.loader)}"
+    
+    # Save the wrapped loader
+    first_wrapper = spec.loader
+    
+    # Simulate the defensive check scenario: pass the spec with already-wrapped loader
+    # back through the wrapper by manipulating what the original function returns.
+    # We do this by calling spec_from_file_location with the already-wrapped loader
+    # as the loader parameter.
+    spec2 = wrapped_spec_from_file_location(
+        "test_module2", test_file, loader=first_wrapper
+    )
+    assert spec2 is not None
+    assert spec2.loader is not None
+    
+    # The defensive check should have prevented double-wrapping
+    assert isinstance(spec2.loader, im.SlipcoverLoader), \
+        f"Expected SlipcoverLoader, got {type(spec2.loader)}"
+    assert spec2.loader is first_wrapper, \
+        "Defensive check failed: loader should not be wrapped again"
+    
+    # Verify that the original loader is not another SlipcoverLoader
+    assert not isinstance(first_wrapper.orig_loader, im.SlipcoverLoader), \
+        "Loader was double-wrapped: orig_loader should not be a SlipcoverLoader"
+
+
 @pytest.mark.skipif(sys.platform == 'win32', reason='Fails due to weird PermissionError')
 def test_wrap_spec_from_file_location_with_branch(tmp_path, monkeypatch):
     """Test that files loaded via spec_from_file_location get branch coverage."""