Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Aug 17, 2025

This fixes the issue here: misrasaurabh1/recce@16f0638

Basically, from recce.adapter.base import BaseAdapter got added even though it was already in the file.

my guess is that AddImportsVisitor.add_needed_import stops looking for imports once it hits something that’s not an import. In this case, it hit the try/except block within lines 39:46, so it never saw the existing BaseAdapter import at line 50 and just added it again.

The fix was to manually collect all top-level imports in a normalized dotted format, so duplicates don’t slip through anymore.


Description

  • Replace conditional import collector with dotted importer

  • Prevent adding duplicate imports in extraction logic

  • Update import normalization and alias handling

  • Add test for duplicate import removal


Diagram Walkthrough

flowchart LR
  parse["Parse destination module"] --> collect["DottedImportCollector.collect imports"]
  gather["Gather needed imports"] --> filter["Filter out existing imports"]
  filter --> add["AddImportsVisitor.add_needed_import"]
  filter --> remove["RemoveImportsVisitor.remove_unused_import"]
Loading

File Walkthrough

Relevant files
Bug fix
code_extractor.py
Refactor import collector and dedupe logic                             

codeflash/code_utils/code_extractor.py

  • Rename ConditionalImportCollector to DottedImportCollector
  • Normalize top-level imports to dotted format
  • Replace collector usage and invert duplicate import checks
  • Invoke block import collection in visit_Module
+31/-17 
Tests
test_add_needed_imports_from_module.py
Add test for duplicate import removal                                       

tests/test_add_needed_imports_from_module.py

  • Add import re for regex assertions
  • Introduce test_duplicated_imports for dedupe check
  • Assert only one BaseAdapter import remains
+127/-0 

@github-actions
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Type mismatch

The check isinstance(asname, cst.Attribute) will never be true because asname is assigned a Python string. Update the logic to correctly detect alias nodes or remove the unreachable branch.

if isinstance(asname, cst.Attribute):
    self.imports.add(module)
else:
    self.imports.add(module if module == asname else f"{module}.{asname}")
Collector invocation

visit_Module calls _collect_imports_from_block(node) but _collect_imports_from_block expects a cst.IndentedBlock. Ensure top-level imports are collected by iterating over node.body or updating the helper signature.

def visit_Module(self, node: cst.Module) -> None:
    self.depth = 0
    self._collect_imports_from_block(node)

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Correct alias name extraction

The current asname is always a string, so the isinstance(asname, cst.Attribute)
branch never executes
and will raise when alias.name is an Attribute. Derive the
alias name from the AST node for both
Name and Attribute and then build the fully
qualified import string.

codeflash/code_utils/code_extractor.py [231-234]

-if isinstance(asname, cst.Attribute):
-    self.imports.add(module)
-else:
-    self.imports.add(module if module == asname else f"{module}.{asname}")
+alias_name = (
+    alias.asname.name.value
+    if alias.asname
+    else (
+        alias.name.attr.value
+        if isinstance(alias.name, cst.Attribute)
+        else alias.name.value
+    )
+)
+import_fqn = module if module == alias_name else f"{module}.{alias_name}"
+self.imports.add(import_fqn)
Suggestion importance[1-10]: 9

__

Why: The branch using isinstance(asname, cst.Attribute) never executes because asname is always a string, so the proposed extraction fixes a real bug by handling AST attributes correctly.

High

new_module = add_needed_imports_from_module(src_module, dst_module, src_path, dst_path, project_root)

matches = re.findall(r"^\s*from\s+recce\.adapter\.base\s+import\s+BaseAdapter\s*$", new_module, re.MULTILINE)
assert len(matches) == 1, f"Expected 1 match for BaseAdapter import, but found {len(matches)}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would prefer a direct string comparison over the entirety of the newly replaced string. this way we have more of a guarantee than searching for particular patterns.

misrasaurabh1
misrasaurabh1 previously approved these changes Aug 17, 2025
@mohammedahmed18 mohammedahmed18 merged commit b349a21 into main Aug 17, 2025
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants