fix(python): suppress bare calls shadowed by an enclosing parameter - #1912
Open
DeusData wants to merge 3 commits into
Open
fix(python): suppress bare calls shadowed by an enclosing parameter#1912DeusData wants to merge 3 commits into
DeusData wants to merge 3 commits into
Conversation
A Python `foo()` whose callee identifier is bound as a parameter of an
enclosing scope cannot be the module-level `foo` -- the parameter shadows it
for the whole body -- so resolving the call to a project Function/Method by a
weak short-name strategy fabricates the edge by construction:
def _run_with_heavy_slot(run):
return run() # bound an unrelated SatoriLive.run
The receiver-aware weak-member guard (#1276) cannot see this class at all: a
bare call has no receiver, so is_method is false and the guard never fires.
This is the bare-call counterpart of python_receiver_is_exempt.
Keyed on the SCOPE FACT, not on the callee's spelling. A list of
generic-looking names (get / run / execute) asserts that certain spellings are
usually noise, which is a claim about corpus fashion rather than about what the
resolver knew, and it ages invisibly: nothing fails when the distribution
shifts, the graph just quietly loses different edges. A parameter binding is
decidable from this file's AST outright.
Parameters only, deliberately. A parameter is in scope for the entire body
regardless of position and Python forbids `global` on one, so no flow analysis
is needed. Local assignments are flow- and binding-form-sensitive (`for`,
`with as`, `except as`, `:=`, unpacking, plus global/nonlocal overrides); a
partial body scan would suppress the wrong edges invisibly -- the same failure
mode that rules out the name list. Enclosing scopes are walked to the file root
so a closure over an outer parameter counts.
Wired at both pass_calls.c and pass_parallel.c with an identical language gate:
a guard on one resolver only diverges the sequential and parallel paths. The
weak-strategy drop-list is now a single shared predicate used by both the member
guard and this one, so they cannot disagree about what "weak" means; a unit test
pins that agreement.
Tests pin both directions. The pipeline positive control is a cross-file bare
call with NO import, so it resolves by a weak strategy this guard could have
killed -- asserting a same_module edge would prove nothing, since no guard
touches same_module for any input. Verified by breaking the guard in both
directions: under-suppressing fails the negatives and the extraction flag;
over-suppressing fails the positives, so they are not vacuous. Disabling only
the pass_parallel.c gate fails only the >=50-file test and nothing else.
805 passed / 0 failed across pipeline registry parallel extraction complexity
lsp_resolution_probe.
The defect class was identified by Joseph-MingEn in #1386, which proposed a
name-keyed shape; the diagnosis is theirs.
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
Co-authored-by: Joseph-MingEn <125283161+Joseph-MingEn@users.noreply.github.com>
Changed-range clang-format (Homebrew LLVM 22.1.8, the build CI's lint-ci uses) flagged two alignment violations in the tests added by the previous commit. Whitespace only; 805 passed / 0 failed re-run after the reformat. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
#1912 hung stack_overflow_b on all seven CI legs: rc=124 at 900s with zero tests completed, on lsp_python_deep_nesting_no_crash. The bare-call scope walk used ts_node_parent(), which is NOT O(1) -- it restarts at the tree root and descends to find the parent (vendored ts_runtime/src/node.c). Each hop therefore costs O(depth), a parent chain is O(depth^2) per call, and since every level of f(f(f(...))) is itself a bare call, O(depth^3) across the file. The fixture nests 30,000 deep, so this is a hang rather than a slowdown. A hop-count cap alone is NOT sufficient, and this was measured rather than assumed: with a 64-hop cap the suite still timed out, because 64 x O(30,000) x 30,000 calls is still ~5.7e10. When the cost is per hop, bounding the number of hops cannot fix it -- the walk itself has to be cheap. Use the unified walk's own cursor. WalkState.current_cursor is already parked on the current node and ts_tree_cursor_goto_parent() IS O(1), because the cursor carries its path stack, so ascending costs nothing per hop. Same current_cursor idiom, including the ts_node_eq identity guard before trusting the shared cursor, as usage_current_field_name in extract_usages.c. This is the same lesson CBMWalkScope already records: carry walk state, never recompute it per node. The 64-hop cap is retained and now does real work, bounding the remaining O(depth) per call. Precedent for the shape: CBM_LSP_PERL_MAX_WALK_DEPTH and LEAN_MAX_PARENT_DEPTH in this file. Both the cap and a missing or mismatched cursor FAIL OPEN -- they can only ever cost a suppression, never a true edge, which is the safe direction for a guard whose justification is precision. lsp_python_deep_nesting_no_crash: 900s timeout, 0 completions -> PASS. stack_overflow_b overall: 6 passed in 46s. The regression test pins the cap's contract deterministically rather than by wall clock: within the cap a shadowed callee is flagged; past 64 ancestors the guard fails open and leaves it unflagged. Raising the cap flips the deep case and fails the test, verified. 826 passed / 0 failed across stack_overflow_a stack_overflow_b stack_overflow_c pipeline registry parallel extraction complexity lsp_resolution_probe. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com> Co-authored-by: Joseph-MingEn <125283161+Joseph-MingEn@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Distilled from #1386 with
Co-authored-by:credit to @Joseph-MingEn, who identified this defect class. The mechanism differs from theirs — see below.The gap
After #1903 landed Python member-call suppression, this still resolves wrongly:
#1903 structurally cannot catch it — there is no receiver, so its guard never fires. #1647's cross-language suffix guard only fires when caller and target languages differ, and both are Python here. So the class survives everything currently on main.
Keyed on local binding, not on a name list
#1386 keyed this on
{get, run, execute}. That is a claim about spellings, not about whether the resolver knew anything — and it would age invisibly: nothing fails when the generic-name distribution shifts, the graph just quietly loses different edges. It is also open-ended (why notstart,send,handle,process?) and corpus-dependent.This keys on the fact that decides the question: the callee identifier is bound as a parameter of an enclosing function, so it shadows any project function and short-name resolution is fabricated by construction. No list, decidable outright, and it covers
handler(),callback(),fn()and every Callable-parameter shape.Parameters only, deliberately. A parameter is in scope for the entire body regardless of position, and Python forbids
globalon a parameter (SyntaxError: name is parameter and global), so no flow analysis is needed. Local assignments are flow- and binding-form-sensitive acrossfor,with…as,except…as,:=, unpacking, nesteddef/class,import, plusglobal/nonlocal— a partial version would be incomplete invisibly, which is the failure mode the name list was rejected for. That wants a real scope analyser and its own evidence.Enclosing scopes are walked to any depth, so
def outer(run): def inner(): return run()— a common decorator/callback shape — is covered. The one knowingly over-flagged shape (global runin a nested function whose outer scope has arunparameter) is documented in the helper rather than left silent.A new field rather than overloading
is_methodcallee_is_locally_boundis a distinct field onCBMCall.is_methodmeans "member call with unresolved receiver" and is read by the pxc synthetic-carrier dedup key (pass_lsp_cross.c:801/:815); overloading it for bare calls would corrupt that key and make the field's documented contract untrue.One drop-list, shared
The weak-strategy list (
suffix_match/unique_name/field_type_hint/fuzzy) is now a singleweak_short_name_strategy()used by both the member guard and this one. Two copies would have let the guards silently disagree about what "weak" means.weak_call_guards_share_one_drop_listpins that agreement across 15 strategies — and it fired correctly during revert-check.Python-gated, wired at both
pass_calls.candpass_parallel.cwith textually identical gates. ArkTS preserved in both member gates.Evidence — three revert-check rounds, each isolating one claim
pass_parallel.cgate onlypass_parallel.crather than falling back to the sequential pathRound C is the one that matters. It proves the positive controls are not vacuous:
uses_free_function → compute_widget_totalis a cross-file bare call with no import, so it resolves by a weak strategy this guard can drop — and an over-suppressing guard does drop it. #1386's positive asserted asame_moduleedge that no guard touches for any input, so it would have passed even if the change dropped every other Python edge. Both pipeline tests also carryASSERT_GTE(CALLS, 1)anti-vacuity.The extraction test pins all 7 parameter binding forms (bare, typed, default, keyword-only,
*args,**kwargs, lambda, closure) plus 3 negatives (unbound, imported, nesteddef), each asserted to appear exactly once.Verification
pipeline registry parallel extraction complexity lsp_resolution_probe→ 805 passed, 0 failed. Built in a fresh worktree with no priorbuild/, so no stale-binary exposure. All 6 new tests confirmed by name in output, andRUN_TESTs verified insideSUITE(pipeline)rather than thepipeline_semantic_manifest_reprodecoy that has swallowed tests before.Cost is O(depth × params) per bare call — never corpus-coupled, so no complexity-guard interaction.
CBMCallis not serialized, so no cache or index-format bump.