Skip to content

Duplicate-definition gate cannot see definitions inside module-level control flow or same-name closures in one parent #321

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.

Sign up for GitHub

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

Closed
jaylfc wants to merge 1 commit into from
Closed

Duplicate-definition gate cannot see definitions inside module-level control flow or same-name closures in one parent #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog.d/tsk-xhvo4a-duplicate-definition-gate-scope.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
### Fixed
- The duplicate-definition gate now catches definitions inside module-level ``if``/``try``/``for``/``while``/``with`` blocks and same-name closures within one parent function, matching Python's actual binding rules. Same-name closures in different parents remain legal, and the ``try:`` / ``except ImportError:`` fallback pattern stays silent.
178 changes: 160 additions & 18 deletions scripts/normalise_handle_gate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
``@<name>.getter`` / ``@<name>.deleter`` accessories are legitimate same-name
pairs and are therefore excluded. Files that cannot be decoded as UTF-8 are
skipped with a warning rather than allowed to crash the gate.

Definitions inside module-level ``if`` / ``try`` / ``for`` / ``while`` /
``with`` blocks are treated as module-scope, matching Python's binding rules.
Same-name closures inside one parent function are also reported; same-name
closures in different parents remain legal. The ``try:`` / ``except
ImportError:`` fallback pattern is recognised and left silent.
"""
from __future__ import annotations

Expand All @@ -32,6 +38,15 @@ class Duplicate:
lines: list[int] = field(default_factory=list)


@dataclass
class _Def:
scope: str
name: str
lineno: int
in_try: bool = False
in_import_error_except: bool = False


def _has_overload_decorator(node: ast.FunctionDef | ast.AsyncFunctionDef) -> bool:
for decorator in node.decorator_list:
if isinstance(decorator, ast.Name):
Expand Down Expand Up @@ -69,24 +84,147 @@ def _is_exempt(node: ast.FunctionDef | ast.AsyncFunctionDef) -> bool:
return _has_overload_decorator(node) or _has_property_decorator(node)


def _is_import_error_handler(handler: ast.ExceptHandler) -> bool:
if handler.type is None:
return False
if isinstance(handler.type, ast.Name):
return handler.type.id == "ImportError"
if isinstance(handler.type, ast.Tuple):
return any(
isinstance(elt, ast.Name) and elt.id == "ImportError"
for elt in handler.type.elts
)
return False


def _is_try_import_error_fallback(defs: list[_Def]) -> bool:
if not defs:
return False
if not all(d.in_try for d in defs):
return False
try_body_defs = [d for d in defs if not d.in_import_error_except]
import_error_defs = [d for d in defs if d.in_import_error_except]
return len(try_body_defs) == 1 and len(import_error_defs) == 1

@kilo-code-bot kilo-code-bot Bot Aug 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

WARNING: _is_try_import_error_fallback does not account for definitions in else or finally blocks

_collect_definitions recurses into node.orelse (line 208) and node.finalbody (line 218) with in_try=True, so those definitions are included in try_body_defs (line 105). The fallback check requires exactly one try-body-side definition, but a legitimate try/except ImportError pattern with a same-named definition in the else block (or finally block) will have two try-body-side definitions and be incorrectly reported as a duplicate.

For example, the following valid import-fallback pattern would be flagged:

try:
    from foo import bar
except ImportError:
    from foo import baz as bar
else:
    def bar():  # same name, different purpose
    ...

_is_try_import_error_fallback should exclude else/finally definitions from the try-body count, or explicitly check that no extra definitions exist in those blocks.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.



def _collect_definitions(
body: list[ast.stmt], class_path: tuple[str, ...] = ()
) -> list[tuple[str, str, int]]:
"""Yield ``(scope, name, lineno)`` for every non-exempt function/method.
body: list[ast.stmt],
scope: str = "module",
class_path: tuple[str, ...] = (),
in_try: bool = False,
in_import_error_except: bool = False,
) -> list[_Def]:
"""Yield ``_Def`` for every non-exempt function/method.

``scope`` is ``"module"`` for a top-level function, ``"class Foo.Bar"``
for a method of ``Foo.Bar``. Nested functions (closures) are
intentionally NOT collected: they are neither top-level functions nor
methods, and same-name closures in different parents are legal.
for a method of ``Foo.Bar``, and ``"module > outer"`` for a closure
inside ``outer``. Definitions inside module-level ``if`` / ``try`` /
``for`` / ``while`` / ``with`` blocks are collected with ``"module"``
scope, matching Python's binding rules. Closures are collected with a
scope that identifies their parent function, so that same-name closures
in one parent are reported while same-name closures in different parents
remain legal.
"""
scope = "module" if not class_path else "class " + ".".join(class_path)
found: list[tuple[str, str, int]] = []
found: list[_Def] = []
for node in body:
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
if not _is_exempt(node):
found.append((scope, node.name, node.lineno))
found.append(
_Def(scope, node.name, node.lineno, in_try, in_import_error_except)
)
closure_scope = (
f"{scope} > {node.name}"
if scope != "module"
else f"module > {node.name}"
)
found.extend(
_collect_definitions(
node.body,
closure_scope,
class_path=(),
in_try=False,
in_import_error_except=False,
)
)
elif isinstance(node, ast.ClassDef):
found.extend(_collect_definitions(node.body, class_path + (node.name,)))
if scope == "module":
new_class_path = class_path + (node.name,)
new_scope = "class " + ".".join(new_class_path)
found.extend(
_collect_definitions(
node.body,
new_scope,
new_class_path,
in_try=False,
in_import_error_except=False,
)
)
elif isinstance(
node,
(ast.If, ast.For, ast.AsyncFor, ast.While, ast.With, ast.AsyncWith),
):
if scope == "module":
found.extend(
_collect_definitions(
node.body,
scope,
class_path,
in_try,
in_import_error_except,
)
)
if hasattr(node, "orelse") and node.orelse:
found.extend(
_collect_definitions(
node.orelse,
scope,
class_path,
in_try,
in_import_error_except,
)
)
elif isinstance(node, ast.Try):
if scope == "module":
found.extend(
_collect_definitions(
node.body,
scope,
class_path,
in_try=True,
in_import_error_except=in_import_error_except,
)
)
for handler in node.handlers:
is_ie = _is_import_error_handler(handler)
found.extend(
_collect_definitions(
handler.body,
scope,
class_path,
in_try=True,
in_import_error_except=is_ie,
)
)
if node.orelse:
found.extend(
_collect_definitions(
node.orelse,
scope,
class_path,
in_try=True,
in_import_error_except=in_import_error_except,
)
)
if node.finalbody:
found.extend(
_collect_definitions(
node.finalbody,
scope,
class_path,
in_try=True,
in_import_error_except=in_import_error_except,
)
)
return found


Expand All @@ -105,15 +243,19 @@ def _duplicate_definitions(file_path: Path) -> list[Duplicate]:
except SyntaxError:
return []

defs: dict[tuple[str, str], list[int]] = defaultdict(list)
for scope, name, lineno in _collect_definitions(tree.body):
defs[(scope, name)].append(lineno)
defs: dict[tuple[str, str], list[_Def]] = defaultdict(list)
for defn in _collect_definitions(tree.body):
defs[(defn.scope, defn.name)].append(defn)

duplicates: list[Duplicate] = [
Duplicate(name=name, scope=scope, lines=lines)
for (scope, name), lines in defs.items()
if len(lines) > 1
]
duplicates: list[Duplicate] = []
for (scope, name), def_list in defs.items():
if len(def_list) <= 1:
continue
if _is_try_import_error_fallback(def_list):
continue
duplicates.append(
Duplicate(name=name, scope=scope, lines=[d.lineno for d in def_list])
)
duplicates.sort(key=lambda d: d.lines[0])
return duplicates

Expand Down
111 changes: 107 additions & 4 deletions tests/test_normalise_handle_gate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,12 @@
- ``@typing.overload`` stubs, ``@property`` getters, and
``@<name>.setter`` / ``@<name>.getter`` / ``@<name>.deleter`` accessories
are legitimate same-name pairs and do not fire.
- Nested functions (closures) are not collected: they are neither top-level
functions nor methods.
- Definitions inside module-level ``if`` / ``try`` / ``for`` / ``while`` /
``with`` blocks share the module scope, matching Python's binding rules.
- Same-name closures inside one parent function are reported; same-name
closures in different parents remain legal.
- The ``try:`` / ``except ImportError:`` fallback pattern is recognised and
left silent.
- Files that cannot be decoded as UTF-8 are skipped with a warning, and
files that fail to parse are left silent.
"""
Expand Down Expand Up @@ -199,7 +203,7 @@ def test_top_level_fn_and_method_same_name_is_fine(self, tmp_path):
)
assert _duplicate_definitions(f) == []

def test_nested_functions_are_not_collected(self, tmp_path):
def test_same_name_closures_in_one_parent_are_reported(self, tmp_path):
f = tmp_path / "mod.py"
f.write_text(
"def outer():\n"
Expand All @@ -209,9 +213,108 @@ def test_nested_functions_are_not_collected(self, tmp_path):
" pass\n"
" return inner\n"
)
dups = _duplicate_definitions(f)
assert len(dups) == 1
assert dups[0].name == "inner"
assert dups[0].scope == "module > outer"
assert dups[0].lines == [2, 4]

def test_same_name_closures_in_different_parents_are_fine(self, tmp_path):
f = tmp_path / "mod.py"
f.write_text(
"def outer1():\n"
" def inner():\n"
" pass\n"
"def outer2():\n"
" def inner():\n"
" pass\n"
)
assert _duplicate_definitions(f) == []

def test_def_in_module_level_if_is_reported(self, tmp_path):
f = tmp_path / "mod.py"
f.write_text(
"if condition:\n"
" def foo():\n"
" pass\n"
"def foo():\n"
" pass\n"
)
dups = _duplicate_definitions(f)
assert len(dups) == 1
assert dups[0].name == "foo"
assert dups[0].scope == "module"
assert dups[0].lines == [2, 4]

def test_def_in_module_level_for_is_reported(self, tmp_path):
f = tmp_path / "mod.py"
f.write_text(
"for x in range(10):\n"
" def foo():\n"
" pass\n"
"def foo():\n"
" pass\n"
)
dups = _duplicate_definitions(f)
assert len(dups) == 1
assert dups[0].name == "foo"
assert dups[0].scope == "module"

def test_def_in_module_level_while_is_reported(self, tmp_path):
f = tmp_path / "mod.py"
f.write_text(
"while True:\n"
" def foo():\n"
" pass\n"
" break\n"
"def foo():\n"
" pass\n"
)
dups = _duplicate_definitions(f)
assert len(dups) == 1
assert dups[0].name == "foo"
assert dups[0].scope == "module"

def test_def_in_module_level_with_is_reported(self, tmp_path):
f = tmp_path / "mod.py"
f.write_text(
"with open('x') as f:\n"
" def foo():\n"
" pass\n"
"def foo():\n"
" pass\n"
)
dups = _duplicate_definitions(f)
assert len(dups) == 1
assert dups[0].name == "foo"
assert dups[0].scope == "module"

def test_try_except_import_error_fallback_with_defs_is_silent(self, tmp_path):
f = tmp_path / "mod.py"
f.write_text(
"try:\n"
" def foo():\n"
" pass\n"
"except ImportError:\n"
" def foo():\n"
" pass\n"
)
assert _duplicate_definitions(f) == []

def test_try_except_import_error_fallback_is_silent(self, tmp_path):
def test_try_except_value_error_fallback_is_reported(self, tmp_path):
f = tmp_path / "mod.py"
f.write_text(
"try:\n"
" def foo():\n"
" pass\n"
"except ValueError:\n"
" def foo():\n"
" pass\n"
)
dups = _duplicate_definitions(f)
assert len(dups) == 1
assert dups[0].name == "foo"
assert dups[0].scope == "module"
f = tmp_path / "mod.py"
f.write_text(
"try:\n"
Expand Down