Deepen: one binding-and-use model behind a single interface, instead of six visitors and three copies of the same knowledge #166

Recommendation strength: Strong

Modules

src/messpy/cli.py — _record_pattern_binding, _named_binding_targets, _FunctionLocalBindings, _ScopeBindingCollector, _ScopedNameUseVisitor, _ModuleBindingCollector, _ScopeRebindingCollector, _NamingRoleCollector, _target_names, _assigned_names, _direct_bindings.

Problem

"What does this Python construct bind, and where is that name used?" is the single most load-bearing fact in messpy, and it currently has no module. It is re-derived independently in six AST visitors plus a handful of free functions, so the knowledge has no locality: each answer is only as correct as the visitor that happens to be asking.

The duplication is verbatim. Pattern-and-handler binding is spelled three times. Once as a recorder:

def _record_pattern_binding(names: set[str], node: ast.AST) -> None:
    if isinstance(node, (ast.MatchAs, ast.MatchStar, ast.ExceptHandler)):
        if node.name is not None:
            names.add(node.name)
    elif isinstance(node, ast.MatchMapping) and node.rest is not None:
        names.add(node.rest)

Once as a naming-target collector over the same four node types:

def _named_binding_targets(tree: ast.Module) -> list[NamingTarget]:
    targets: list[NamingTarget] = []
    for node in ast.walk(tree):
        if isinstance(node, ast.ExceptHandler) and node.name is not None:
            targets.append(NamingTarget(node.name, node.lineno, "variable"))
        elif isinstance(node, (ast.MatchAs, ast.MatchStar)) and node.name is not None:
            targets.append(NamingTarget(node.name, node.lineno, "variable"))
        elif isinstance(node, ast.MatchMapping) and node.rest is not None:
            targets.append(NamingTarget(node.rest, node.lineno, "variable"))
    return targets

And once more as four visit_* methods on _FunctionLocalBindings. Target destructuring is spelled twice, identically but for the return type:

def _target_names(node: ast.AST) -> list[ast.Name]:
    if isinstance(node, ast.Name):
        return [node]
    if isinstance(node, ast.Starred):
        return _target_names(node.value)
    if isinstance(node, (ast.Tuple, ast.List)):
        return [name for element in node.elts for name in _target_names(element)]
    return []

def _assigned_names(target: ast.AST) -> list[str]:
    if isinstance(target, ast.Name):
        return [target.id]
    if isinstance(target, ast.Starred):
        return _assigned_names(target.value)
    if isinstance(target, (ast.Tuple, ast.List)):
        return [name for element in target.elts for name in _assigned_names(element)]
    return []

The cost is measurable in this repository's own history: roughly 17 of the closed bug issues are binding-, use-, or scope-resolution defects. The recurring shape is a construct handled in one visitor and missed in another, which is exactly what you would predict from three independent implementations of the same rule. Adding support for a new binding form today means finding all six places and hoping.

Proposed deepening

Extract a bindings module whose interface is small and whose implementation absorbs every construct-specific isinstance chain now scattered through cli.py. Something in the shape of:

• bindings(tree) -> BindingTable, where a BindingTable answers "which names does this scope bind", "what kind of binding is each" (parameter, local, comprehension, handler, pattern, module-level, class field), "where is each bound", and "is this name used inside this scope".

Callers stop walking the AST for names. UnusedLocalVariable, UnusedFormalParameter, GlobalVariable, ShortVariable/LongVariable/CamelCaseVariableName, and the shadowing-sensitive rules (ExitExpression, DevelopmentCodeFragment) all consume the same table.

Deletion test: deleting this module does not move the complexity, it restores it to six places. That is the signal we want.

Benefits

Locality. New Python binding syntax is taught once. The next match-statement or comprehension-scope defect is fixed once, and the fix lands in every rule that reads names — instead of being fixed in _FunctionLocalBindings and quietly left broken in _named_binding_targets.

Leverage. Rule authors learn one interface (BindingTable) rather than choosing correctly between six near-identical visitors. A rule that needs name information goes from "write an ast.NodeVisitor" to "ask the table", so the per-rule cost of getting scoping right drops to near zero.

Test impact

Today, every one of these ~17 historical defects could only be expressed as an end-to-end CLI test: write a temp .py file containing the construct, run the whole analyzer under some ruleset that happens to include a name-sensitive rule, and string-match the rendered text report. That is why tests/test_cli.py reaches behind the seam at all — its only two non-run() tests exist precisely because this concern has no interface:

from messpy.cli import run, _direct_bindings, _is_protocol, _protocol_base_names

With a bindings module the construct-coverage matrix becomes a table test over (source snippet) -> BindingTable, asserting the bound names directly. That is still a real acceptance test in the sense CODING_STANDARDS.md requires — real source, real parse, no mocks, no injected fakes — just observed at a narrower and more honest seam. The end-to-end tests that should remain end-to-end (exit codes, report composition) are unaffected, and _direct_bindings stops being a test backdoor because it becomes part of a published interface.