scriptoriumadmin/GitNexus
1
0
Fork 0
mirror of https://github.com/abhigyanpatwari/GitNexus.git synced 2026-10-07 02:58:02 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 8

fix(python): model restoring helper calls in decorator identity (#3415)

Browse source 
* fix(python): model restoring helper calls in decorator identity (#3414)

A bare module-level call to a same-file helper whose every global
binding of a descriptor name is an unconditional del or builtins import
now restores the builtin at the call site, matching CPython. A nonlocal
rebind nested in the enclosing function now shadows an owned builtins
import, closing a false builtin. Unprovable call orders stay fail-closed
and are pinned against CPython 3.11.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix(review): apply review findings

Close three false-builtin paths the review found: a match-pattern
capture now counts as a binding (at any scope, including inside a
restoring helper), a call before the helper's def no longer counts as a
restore, and the helper-name uniqueness check sees match captures.
Pin the helper rejections (conditional restore, async, early and nested
return, wildcard import) against CPython 3.11.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix(cache): bump parse-cache schema to v122 for #3414

Python decorator identity verdicts changed, so warm v121 ParsedFiles
would replay stale receiver bindings.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix(python): require an argument-free call to a helper with no required parameters

A call that fails to bind the helper's parameters raises TypeError
before the body runs, so it proves no restore. Keep such calls
fail-closed.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix(python): count only real captures and module bindings for decorator identity

Match value patterns, class names and keyword keys read a name rather
than capture it, so they no longer shadow a builtin descriptor. A local
of the same name as a restoring helper no longer disqualifies the
module-level helper; only a module binding or a global rebind does.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* test(python): state the fail-closed contract of the descriptor identity table

`false` means the resolver does not prove the builtin, not that CPython
shadows it. Cases where CPython keeps the builtin but the resolver fails
closed carry a comment saying so.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Gergő Magyar 2026-09-29 12:37:27 +01:00 • committed by GitHub
parent c2fab0a8d2
commit aa0f41e853
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 578 additions and 21 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

162
gitnexus/src/core/ingestion/languages/python/builtin-descriptors.ts
View file

@ -108,6 +108,7 @@ function bindingOf(identifier: SyntaxNode): Omit<NameBinding, 'redirect'> | null
}
if (parent === null) return null;
const shadow = { node: identifier, effect: 'shadow' as const };
if (inCasePattern(identifier)) return isCaseCapture(identifier) ? shadow : null;
switch (parent.type) {
case 'assignment':
case 'augmented_assignment':
@ -144,10 +145,9 @@ function bindingOf(identifier: SyntaxNode): Omit<NameBinding, 'redirect'> | null
: null;
case 'dotted_name': {
const owner = parent.parent;
// `import a.b` binds `a`; `case name:` captures a single name.
// `import a.b` binds `a`.
if (owner?.type === 'import_statement')
return parent.firstNamedChild?.id === node.id ? shadow : null;
if (owner?.type === 'case_pattern') return parent.namedChildCount === 1 ? shadow : null;
if (owner?.type !== 'import_from_statement' || !isField(parent, 'name')) return null;
return importsFromBuiltins(owner) ? { node: identifier, effect: 'builtin' } : shadow;
}
@ -194,10 +194,140 @@ function descriptorBindings(node: SyntaxNode): ReadonlyMap<string, readonly Name
const redirect = redirected.get(`${found.text}@${scopeOf(found)?.id}`) ?? null;
add(found.text, { ...binding, redirect });
}
addHelperCalls(tree.rootNode, bindings, redirected);
bindingsByTree.set(tree, bindings);
return bindings;
}
/**
* A bare module-level call to a same-file helper that always restores a
* descriptor name through `global` runs that restore at the call. Record it
* there as a module binding, so the call's position orders it.
*/
function addHelperCalls(
root: SyntaxNode,
bindings: Map<string, NameBinding[]>,
redirected: ReadonlyMap<string, 'global' | 'nonlocal'>,
): void {
// Helper function name -> its `def` and, per descriptor name, the restore
// its call performs.
const helpers = new Map<string, { fn: SyntaxNode; restores: Map<string, BindingEffect> }>();
for (const [name, list] of bindings) {
const byHelper = new Map<number, { fn: SyntaxNode; globals: NameBinding[] }>();
for (const binding of list) {
if (binding.redirect !== 'global') continue;
const fn = scopeOf(binding.node);
if (fn === null) continue;
const entry = byHelper.get(fn.id);
if (entry === undefined) byHelper.set(fn.id, { fn, globals: [binding] });
else entry.globals.push(binding);
}
for (const { fn, globals } of byHelper.values()) {
const effect = restoreOnCall(fn, globals);
const helper = fn.childForFieldName('name')?.text;
if (effect === null || helper === undefined) continue;
const entry = helpers.get(helper) ?? { fn, restores: new Map<string, BindingEffect>() };
entry.restores.set(name, effect);
helpers.set(helper, entry);
}
}
if (helpers.size === 0) return;
// A call only denotes the helper when its `def` is the name's one module
// binding. A local of the same name elsewhere cannot rebind it.
if (root.descendantsOfType('wildcard_import').length > 0) return;
const bindingCounts = new Map<string, number>();
for (const found of root.descendantsOfType('identifier')) {
if (!helpers.has(found.text) || bindingOf(found) === null) continue;
const scope = scopeOf(found);
if (scope !== null && redirected.get(`${found.text}@${scope.id}`) !== 'global') continue;
bindingCounts.set(found.text, (bindingCounts.get(found.text) ?? 0) + 1);
}
for (const [helper, count] of bindingCounts) if (count > 1) helpers.delete(helper);
const touched = new Set<NameBinding[]>();
for (const statement of root.namedChildren) {
// Only a call that is the whole statement surely runs when reached.
if (statement.type !== 'expression_statement' || statement.namedChildCount !== 1) continue;
const call = statement.firstNamedChild;
const callee = call?.type === 'call' ? call.childForFieldName('function') : null;
const helper = callee?.type === 'identifier' ? helpers.get(callee.text) : undefined;
// The call reaches the helper only once its `def` statement has run, and
// an argument-free call binds only when every parameter is optional.
if (helper === undefined || statement.startIndex < helper.fn.endIndex) continue;
if ((call?.childForFieldName('arguments')?.namedChildCount ?? 0) > 0) continue;
for (const [name, effect] of helper.restores) {
const list = bindings.get(name);
if (list === undefined) continue;
list.push({ node: callee, effect, redirect: null });
touched.add(list);
}
}
for (const list of touched) list.sort((a, b) => a.node.startIndex - b.node.startIndex);
}
/**
* Does this name in a case pattern capture? Value patterns (`a.b`), class
* names and keyword keys are reads. Every other name is a capture.
*/
function isCaseCapture(identifier: SyntaxNode): boolean {
const parent = identifier.parent;
if (parent?.type === 'keyword_pattern') return parent.firstNamedChild?.id !== identifier.id;
if (parent?.type !== 'dotted_name') return true;
if (parent.namedChildCount > 1) return false;
return !(
parent.parent?.type === 'class_pattern' && parent.parent.firstNamedChild?.id === parent.id
);
}
/** Is `node` inside a `match` case pattern? */
function inCasePattern(node: SyntaxNode): boolean {
for (let parent = node.parent; parent !== null; parent = parent.parent) {
if (parent.type === 'case_pattern') return true;
}
return false;
}
/**
* The restore a call to `fn` always performs on a module global, or `null`.
* `fn` must be a plain module-level `def` whose body runs on call (not a
* generator or coroutine), and every `global` binding of the name in it must
* be a restore in a simple statement that no earlier `return` can skip.
*/
function restoreOnCall(fn: SyntaxNode, globals: readonly NameBinding[]): BindingEffect | null {
if (fn.type !== 'function_definition' || fn.parent?.type !== 'module') return null;
if (fn.children.some((child) => child.type === 'async')) return null;
const required = fn
.childForFieldName('parameters')
?.namedChildren.some(
(param) =>
param.type === 'identifier' ||
(param.type === 'typed_parameter' && param.firstNamedChild?.type === 'identifier'),
);
if (required === true) return null;
const body = fn.childForFieldName('body');
// `globals` is in source order, so the last restore is the one that sticks.
const first = globals[0];
const last = globals[globals.length - 1];
if (body === null || first === undefined || last === undefined) return null;
for (const binding of globals) {
const statement = statementIn(binding.node, body);
if (
binding.effect === 'shadow' ||
statement === null ||
!SIMPLE_STATEMENTS.has(statement.type)
) {
return null;
}
}
const escapes = body
.descendantsOfType(['yield', 'return_statement'])
.some(
(node) =>
scopeOf(node)?.id === fn.id &&
(node.type === 'yield' || node.startIndex < first.node.startIndex),
);
return escapes ? null : last.effect;
}
/**
* The scope that owns names bound at `node`: a function or lambda body, a
* class body, a comprehension, or the module (`null`). A walrus target skips
@ -225,6 +355,11 @@ function statementIn(node: SyntaxNode, body: SyntaxNode): SyntaxNode | null {
return current.parent === null ? null : current;
}
/** Does `outer` span `node`? */
function contains(outer: SyntaxNode, node: SyntaxNode): boolean {
return node.startIndex >= outer.startIndex && node.endIndex <= outer.endIndex;
}
/** Can `binding` run before `use` in the same scope, including an earlier
* iteration of an enclosing loop? */
function mayRunBefore(binding: SyntaxNode, use: SyntaxNode, scope: SyntaxNode | null): boolean {
@ -232,8 +367,7 @@ function mayRunBefore(binding: SyntaxNode, use: SyntaxNode, scope: SyntaxNode |
for (let loop = use.parent; loop !== null && loop.id !== scope?.id; loop = loop.parent) {
if (
(loop.type === 'for_statement' || loop.type === 'while_statement') &&
binding.startIndex >= loop.startIndex &&
binding.endIndex <= loop.endIndex
contains(loop, binding)
) {
return true;
}
@ -296,6 +430,15 @@ function lookupName(bindings: readonly NameBinding[], use: SyntaxNode): BindingE
(binding) => binding.redirect === null && scopeOf(binding.node)?.id === fn.id,
);
if (!owned) continue;
// A nested function can rebind this cell whenever it is called; that
// order is not modelled, so assume it ran.
const rebound = bindings.some(
(binding) =>
binding.effect === 'shadow' &&
binding.redirect === 'nonlocal' &&
contains(fn, binding.node),
);
if (rebound) return 'shadow';
// Any binding makes the name local to this function, so the class body
// reads that cell. An unbound cell raises NameError, not the builtin.
return namespaceState(bindings, fn, use) === 'builtin' ? 'builtin' : 'shadow';
@ -313,16 +456,13 @@ function lookupName(bindings: readonly NameBinding[], use: SyntaxNode): BindingE
// is called; that order is not modelled, so assume it ran.
if (binding.redirect === 'nonlocal') {
const outer = functions[functions.length - 1];
const inside =
outer !== undefined &&
binding.node.startIndex >= outer.startIndex &&
binding.node.endIndex <= outer.endIndex;
if (inside) return 'shadow';
if (outer !== undefined && contains(outer, binding.node)) return 'shadow';
}
if (binding.redirect === 'global') {
// The function can only be called once the top-level statement that
// defines it has run. Whether a call happens is unknown, so a restoring
// `global` delete is ignored and a rebinding one is assumed.
// defines it has run. Whether a call happens is unknown, so a rebinding
// one is assumed. A restore counts only at a proven call site (see
// `addHelperCalls`).
const top = statementIn(binding.node, binding.node.tree.rootNode);
if (deferred || (top !== null && mayRunBefore(top, use, null))) return 'shadow';
}

5
gitnexus/src/storage/parse-cache.ts
View file

@ -812,7 +812,10 @@ import { copyV8CacheIfPresent, tryLoadV8Cache, writeV8CacheFile } from './v8-sid
// comments, honors rebinding of builtin descriptor names visible where the
// decorator is evaluated, and withholds subtype capacity from descriptor
// stacks. Warm v120 captures carry the old verdicts.
const SCHEMA_BUMP = 121;
// v122 (#3414): Python decorator identity models restoring helper calls and
// treats match-pattern captures and nested nonlocal rebinds as shadowing.
// Warm v121 captures carry the old verdicts.
const SCHEMA_BUMP = 122;
const GITNEXUS_PKG_VERSION = (() => {
try {
// package.json sits at gitnexus/package.json — two levels up from

7
gitnexus/test/unit/incremental-parse-cache.test.ts
View file

@ -299,8 +299,9 @@ describe('PARSE_CACHE_VERSION', () => {
// Moved 116 -> 117 for #3390's private positional-count side-channel.
// Moved 117 -> 118 for #3398, 118 -> 119 for #3396, and 119 -> 120 for #3394.
// Moved 120 -> 121 for the #3399 decorator-identity follow-up.
it('pins SCHEMA_BUMP to 121 so concurrent bumps cannot silently collide (#2766, #3015, #3088, #2885, #3128, #2865, #3130, #1432, #3161, #3179, #3219, #3190, #3253, #3273, #3339, #3354, #3371, #2965, #3390, #3398, #3396, #3394, #3399)', () => {
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(121);
// Moved 121 -> 122 for #3414 restoring helper calls.
it('pins SCHEMA_BUMP to 122 so concurrent bumps cannot silently collide (#2766, #3015, #3088, #2885, #3128, #2865, #3130, #1432, #3161, #3179, #3219, #3190, #3253, #3273, #3339, #3354, #3371, #2965, #3390, #3398, #3396, #3394, #3399, #3414)', () => {
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).toBe(122);
expect(PARSE_CACHE_BUCKET_COUNT).toBe(128);
// The PREVIOUS version must fail the reuse gate, not merely differ from the
// current one — a hardcoded number outside the conflict hunk rebases cleanly
@ -309,7 +310,7 @@ describe('PARSE_CACHE_VERSION', () => {
for (const taken of [
59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81,
82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103,
104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120,
104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121,
]) {
expect(Number(PARSE_CACHE_VERSION.split('+', 1)[0])).not.toBe(taken);
}

425
gitnexus/test/unit/scope-resolution/python/python-builtin-descriptors.test.ts
View file

@ -16,11 +16,19 @@ const decoratesWithBuiltin = (source: string): boolean => {
};
const method = [' @staticmethod', ' def t(v):', ' return v'];
const resetHelper = ['def reset():', ' global staticmethod', ' del staticmethod'];
const nonlocalRebind = [
' def rebind():',
' nonlocal staticmethod',
' staticmethod = lambda f: f',
];
// `true` means CPython's `A().t(7)` returns 7, so the decorator evaluated to
// the builtin staticmethod. A wildcard import from a module this file cannot
// see is expected `false` because the resolver fails closed, not because
// CPython always shadows the builtin there.
// `true` means the resolver proves the decorator is the builtin staticmethod,
// and CPython's `A().t(7)` returns 7. `false` means the resolver does not
// prove it. Usually CPython shadows the builtin there too. Where CPython keeps
// the builtin but the resolver fails closed (an unknown wildcard module, an
// unproven call order), the case comment says so. `true` must never hold where
// CPython shadows, because that would be a false edge.
describe('Python builtin descriptor identity', () => {
it.each([
['a later module assignment', ['class A:', ...method, 'staticmethod = lambda f: f'], true],
@ -148,13 +156,174 @@ describe('Python builtin descriptor identity', () => {
false,
],
[
// CPython restores the builtin when reset() runs; whether a call runs is
// not modelled, so the resolver keeps the override (fail closed).
// A bare top-level call runs the helper's `global` delete at the call.
'a global del in a called helper',
['staticmethod = lambda f: f', ...resetHelper, 'reset()', 'class A:', ...method],
true,
],
[
'a global builtins import in a called helper',
[
'staticmethod = lambda f: f',
'def reset():',
' global staticmethod',
' from builtins import staticmethod',
'reset()',
'class A:',
...method,
],
true,
],
[
'a called helper before a deferred class body',
[
'staticmethod = lambda f: f',
...resetHelper,
'reset()',
'def make():',
' class A:',
...method.map((line) => ` ${line}`),
' return A',
],
true,
],
[
// CPython restores the builtin; a conditional call is not modelled.
'a called helper inside an if',
[
'staticmethod = lambda f: f',
...resetHelper,
'if True:',
' reset()',
'class A:',
...method,
],
false,
],
[
'a called helper inside a boolean expression',
[
'staticmethod = lambda f: f',
...resetHelper,
'flag = False',
'flag and reset()',
'class A:',
...method,
],
false,
],
[
// The target binds after the call returns.
'a called helper whose result rebinds the name',
[
'staticmethod = lambda f: f',
...resetHelper,
' return lambda f: f',
'staticmethod = reset()',
'class A:',
...method,
],
false,
],
[
'an override after a called helper',
[
'staticmethod = lambda f: f',
...resetHelper,
'reset()',
'staticmethod = lambda f: f',
'class A:',
...method,
],
false,
],
[
// Calling a generator function does not run its body.
'a called generator helper',
['staticmethod = lambda f: f', ...resetHelper, ' yield', 'reset()', 'class A:', ...method],
false,
],
[
'a called helper that may return first',
[
'staticmethod = lambda f: f',
'def reset(flag=True):',
' global staticmethod',
' if flag:',
' return',
' del staticmethod',
'reset()',
'class A:',
...method,
],
false,
],
[
'a called helper whose name is rebound',
[
'staticmethod = lambda f: f',
...resetHelper,
'reset = print',
'reset()',
'class A:',
...method,
],
false,
],
[
// The call runs the builtin print(); the helper is defined afterwards.
'a call before the helper is defined',
[
'staticmethod = lambda f: f',
'print()',
'class A:',
...method,
'def print():',
' global staticmethod',
' del staticmethod',
],
false,
],
[
'a helper name captured by a match pattern',
[
'staticmethod = lambda f: f',
...resetHelper,
'match 1:',
' case int() as reset:',
' pass',
'reset()',
'class A:',
...method,
],
false,
],
[
'a match capture in the helper after its restore',
[
'staticmethod = lambda f: f',
...resetHelper,
' match [1]:',
' case [*staticmethod]:',
' pass',
'reset()',
'class A:',
...method,
],
false,
],
[
'a module-level match capture',
['match 1:', ' case object() as staticmethod:', ' pass', 'class A:', ...method],
false,
],
[
// The call raises TypeError before the helper body runs.
'a call missing a required helper argument',
[
'staticmethod = lambda f: f',
'def reset(required):',
' global staticmethod',
' del staticmethod',
'reset()',
'class A:',
@ -162,6 +331,250 @@ describe('Python builtin descriptor identity', () => {
],
false,
],
[
'a call to a helper whose parameters all have defaults',
[
'staticmethod = lambda f: f',
'def reset(flag=True, *args: int, **kw):',
' global staticmethod',
' del staticmethod',
'reset()',
'class A:',
...method,
],
true,
],
[
'a match value pattern that reads the name',
[
'import builtins',
'match 1:',
' case builtins.staticmethod:',
' pass',
'class A:',
...method,
],
true,
],
[
'a match class pattern that reads the name',
['match 1:', ' case staticmethod():', ' pass', 'class A:', ...method],
true,
],
[
'a match keyword capture of the name',
['match 1:', ' case int(real=staticmethod):', ' pass', 'class A:', ...method],
false,
],
[
// A parameter named like the helper is local to its own function.
'a function parameter named like the helper',
[
'staticmethod = lambda f: f',
...resetHelper,
'def unrelated(reset):',
' pass',
'reset()',
'class A:',
...method,
],
true,
],
[
// CPython keeps the builtin because swap() is never called; the
// resolver cannot prove that, so it fails closed.
'a global rebind of the helper name in another function',
[
'staticmethod = lambda f: f',
...resetHelper,
'def swap():',
' global reset',
' reset = print',
'reset()',
'class A:',
...method,
],
false,
],
[
// CPython restores the builtin; any `global` rebind in the helper keeps
// the resolver fail-closed, even after a return.
'a helper that rebinds after returning',
[
'staticmethod = lambda f: f',
...resetHelper,
' return',
' staticmethod = 1',
'reset()',
'class A:',
...method,
],
false,
],
[
'a helper whose restore is conditional',
[
'staticmethod = lambda f: f',
'def reset(flag=False):',
' global staticmethod',
' if flag:',
' del staticmethod',
'reset()',
'class A:',
...method,
],
false,
],
[
// Calling a coroutine function does not run its body.
'a called async helper',
[
'staticmethod = lambda f: f',
'async def reset():',
' global staticmethod',
' del staticmethod',
'reset()',
'class A:',
...method,
],
false,
],
[
'a called helper that returns after restoring',
[
'staticmethod = lambda f: f',
...resetHelper,
' return',
'reset()',
'class A:',
...method,
],
true,
],
[
// A nested function's return does not end the helper.
'a called helper with a nested return before restoring',
[
'staticmethod = lambda f: f',
'def reset():',
' global staticmethod',
' def g(): return 1',
' del staticmethod',
'reset()',
'class A:',
...method,
],
true,
],
[
// CPython restores the builtin; the wildcard import could rebind the
// helper name, so the resolver fails closed.
'a called helper after a wildcard import',
[
'staticmethod = lambda f: f',
...resetHelper,
'from os.path import *',
'reset()',
'class A:',
...method,
],
false,
],
[
// The helper writes module globals, not the class namespace.
'a class-body call after a class-local override',
[
'staticmethod = lambda f: f',
...resetHelper,
'class A:',
' staticmethod = lambda f: f',
' reset()',
...method,
],
false,
],
[
'a function-body call after a function-local override',
[
'def reset():',
' global staticmethod',
' from builtins import staticmethod',
'def make():',
' staticmethod = lambda f: f',
' reset()',
' class A:',
...method.map((line) => ` ${line}`),
],
false,
],
[
// CPython restores the builtin; the helper's earlier `global` rebind
// keeps the resolver fail-closed.
'a called helper that rebinds before deleting',
[
'staticmethod = lambda f: f',
'def reset():',
' global staticmethod',
' staticmethod = 1',
' del staticmethod',
'reset()',
'class A:',
...method,
],
false,
],
[
// CPython keeps the builtin when rebind() is never called; whether it
// is called is not modelled, so the resolver fails closed.
'a global rebind helper that is never called',
[
'def rebind():',
' global staticmethod',
' staticmethod = lambda f: f',
'class A:',
...method,
],
false,
],
[
// CPython keeps the builtin; call order is not modelled (fail closed).
'an uncalled nonlocal rebind over an enclosing builtins import',
[
'def make():',
' from builtins import staticmethod',
...nonlocalRebind,
' class A:',
...method.map((line) => ` ${line}`),
],
false,
],
[
'a called nonlocal rebind over an enclosing builtins import',
[
'def make():',
' from builtins import staticmethod',
...nonlocalRebind,
' rebind()',
' class A:',
...method.map((line) => ` ${line}`),
],
false,
],
[
// CPython keeps the builtin because make() runs after the del; call
// order is not modelled (fail closed).
'a module del after a deferred class body, called afterwards',
[
'staticmethod = lambda f: f',
'def make():',
' class A:',
...method.map((line) => ` ${line}`),
' return A',
'del staticmethod',
'make()',
],
false,
],
[
// Each class statement builds a fresh namespace, so the decorator runs
// before that iteration's own class-body assignment.