Conversation
…ther HOCs
`_extract_js_var_functions` only created a Function node when a variable
declarator's value was directly an arrow/function expression. A component
wrapped in a higher-order call (`forwardRef(fn)`, `memo(fn)`, `observer(fn)`,
`withRouter(fn)`, `styled(Base)(fn)`, nested `memo(forwardRef(fn))`) puts the
function inside a `call_expression`, so the declaration was skipped and every
JSX use of the component produced a CALLS edge to a node that did not exist.
callers_of / tests_for / get_impact_radius then came back empty for the most
common way to declare a ref-forwarding React component.
The extractor now looks for the function literal among a wrapper call's
arguments (unwrapping nested wrappers) and treats the variable as that
function: same node, CONTAINS edge and body recursion as a direct assignment.
Calls whose arguments carry no function (`createClient({...})`,
``styled.h1`...` ``, `sum(1, 2)`) are unchanged.
Fixes tirth8205#971
|
Changes required: stopping traversal at the first callback removes calls already present on main. Parse |
Two things the first pass got wrong, both found in review. A call that takes a callback among others computes a value, it does not define a function. `choose(() => left(), () => right())` was read as a wrapped component: the variable became a Function named after the call's result and only the first callback was walked, so `choose` and `right` disappeared from the graph. A wrapper call now has to take the function as its only argument, which is what forwardRef, memo, observer and styled(Base)(fn) do, and leaves useMemo and friends alone. The body walk started at the wrapped function, so the wrapper call was not recorded either. It starts at the declarator now, which covers the wrapper, any nested wrapper and everything else in the argument list, while the signature still comes from the function itself.
|
Fixed at Ordinary values are no longer treated as definitions. A wrapper call now has to take the function as its only argument. That is the shape of the cases this PR is for, The wrapper and its siblings are preserved. The body walk started at the wrapped function, which is why the wrapper call itself went missing even in the cases the PR handles. It starts at the declarator now, so everything in the initializer is visited, while the signature still comes from the function: Before this commit the first case reported only Three tests added for it: your snippet with the exact expected call set, the two-argument hook, and the nested wrapper asserting all three calls. All three fail if the single-argument rule is relaxed. |
|
Rechecked |
A single function argument does not establish that a call returns
something callable. `const result = evaluate(() => compute())` has the
same shape as `memo(() => paint())` and returns a number, so reading it
as a definition invented a `result` function and moved `compute`'s
caller off `setup`, the function that really makes the call.
The wrapper name is what establishes it, so unwrapping is now limited
to wrappers documented to return a component. A wrapper outside that
set is not a definition here, which is what the parser did before
wrapped components were indexed at all.
Two scope defects went with it:
The wrapper call runs where the declaration is, not inside the
component it produces. The component's body walk is now the wrapped
function alone, and every wrapper in the chain is recorded against the
enclosing function through the call extractor, which does not descend
into the wrapped function a second time.
One declaration can define a component AND call a function:
`const Button = memo(() => paint()), token = nextToken()`. The caller
skips its generic recursion over the whole declaration as soon as
anything is extracted, so declarators this pass does not own are now
walked here, in the enclosing scope, instead of being dropped.
`test_a_wrapped_component_keeps_the_wrapper_calls` asserted
("Card", "memo"), which read the defect the other way round: the
component credited with a call made before it existed. It now places
the declaration inside a function and asserts the wrappers belong to
that function.
|
Both reproduced and fixed in "Establish that the result is callable." That was the real defect, and the shape check could never do it: "Keep wrapper initialisation in its enclosing scope." The component's body walk is now the wrapped function alone, and every wrapper in the chain is recorded against the enclosing function through "Preserve every declarator." The caller skips its generic recursion over the whole declaration as soon as this pass extracts anything, so a declarator it does not own was being dropped. Those are now walked here, in the enclosing scope: One of my own tests was the defect written down. I added the two cases to |
|
This fails the checks once merged into pytest (py3.13, merged origin/staging + PR head 1d0dbb6): "3233 passed, 9 skipped, 2 xpassed, 1 warning in 115.23s". ruff: "All checks passed!". mypy 2.3.0: "code_review_graph/parser.py:8827: error: Need type annotation for "chain" (hint: "chain: list[] = ...") [var-annotated] / Found 1 error in 1 file (checked 72 source files)". Same error reproduced with mypy==1.15.0, so it is not version-dependent. To fix:
Merge PRs now target |
mypy cannot infer the element type of an empty list that is only filled by the callee, and the type-check job failed on it with var-annotated. The chain holds tree-sitter nodes, which this file types as a bare list elsewhere.
The member_expression branch of _js_wrapper_name had no test. Pin that React.memo(fn) and React.forwardRef<T, P>(fn) are unwrapped, that the signature and line range come from the declaration and the wrapped function, and that the wrapper calls stay on the enclosing function.
memo(wrap(fn)) with a wrapper outside the component set, and wrappers nested one level past _JS_WRAPPER_MAX_DEPTH, are not definitions and keep every call on the enclosing function. The depth test also checks the boundary itself and that each wrapper call is recorded once.
Only the wrapped function is the component, but the wrapper-call pass recorded the call edge alone and never visited the other parts of the call. staging records all of them, so this branch lost: - the inner call of a curried wrapper, styled(Base)(fn) and connect(mapState)(fn), with the calls in its arguments; - the references that keep Base and mapState from looking unused; - the Props reference in forwardRef<HTMLButtonElement, Props>(fn), the shape from the issue. Every child of each wrapper call except its arguments is now visited in the enclosing scope, through a stand-in parent so the generic walk treats them exactly as a walk over the declaration would.
memo(Component, arePropsEqual) is the documented React API, and the single-argument rule turned it back into a plain call. memo now accepts the comparator as a second argument; forwardRef and the other wrappers still take exactly one. The comparator is not part of the component. It joins the rest of the wrapper call in the enclosing scope, and identifier arguments keep the reference the generic walk records for them, so a named arePropsEqual does not start to look unused. Also corrects the chain docstring: wrapper calls are collected innermost first.
staging walks const Card: FC<Props> = memo(...) as a whole, so Props is referenced from the enclosing scope. Once the declaration counts as a definition, only the component and the wrapper calls were visited, and the reference was lost. Parsing 2058 React source files from four large apps with staging and with this branch, these 75 references were the last staging edges the branch dropped. The one other difference is a TESTED_BY edge that follows its call into a newly indexed component, exactly as staging already does for a directly assigned one. The annotation of a wrapped declaration now joins the rest of the wrapper calls in the enclosing scope. A directly assigned function, const Card: FC<Props> = () => ..., keeps the behaviour it has on staging.
memo accepts one props comparator besides the component, and nothing else. No test covered a third argument, so letting memo take any number of arguments went unnoticed. memo(fn, eq, extra) is not a definition, and its calls stay on the enclosing function.
tree-sitter lists comment nodes among a call's named arguments, so they counted as arguments. A lint directive above the function turned forwardRef(fn) into a two-argument call and the component was not indexed; memo(fn, /* c */ eq) and memo(/* c */ fn) failed the same way. Wrapper arguments are now counted and chosen without comment and html_comment nodes, the two comment kinds of the JavaScript and TypeScript grammars. The rest of the call, which is walked in the enclosing scope, is taken from the same list, so a comment before the function cannot make the component look like the comparator and get walked a second time.
…tion Since the enclosing-scope walk of other declarators was added, it also covers a statement that assigns a function directly. staging extracted a in const a = () => x(), b = y() and skipped the whole declaration, so the call to y was lost; it is now recorded on the enclosing function. The existing sibling test only covers the wrapped form, so dropping the walk for the direct form went unnoticed.
The type annotation of a wrapped declaration is walked in the enclosing scope because staging walked that declaration whole. A directly assigned function, const Card: FC<Props> = () => ..., only ever had the function walked, and this branch does not change that. No test covered the gate, so extending the annotation walk to the direct form went unnoticed.
|
Thanks for the list. I merged mypy. Member-expression callees.
Failure branches of nested unwrapping ( While comparing the branch with staging edge by edge, I found and fixed a few more cases:
Corpus differential. I parsed 2058 React files from cherry-studio, dify, LibreChat and grafana with staging and with the branch.
One correction to my comment of 13 Sep. I wrote that the failure direction is a component that stays unindexed rather than a function that does not exist. That is not true for member callees. The wrapper name is the last segment of the callee, so a non-React call with the same name that takes one callback, such as Checks, as CI runs them.
|
|
The PR drops 63 TESTED_BY edges on material-ui across 22 test files, not the one edge in grafana's Build a repo from material-ui's own Both tests build their probe as The false positive you disclosed is also a wrong answer: // src/db.js
import { logErr } from './log';
export function handle(pool) {
const session = pool.connect(function (err) { logErr(err); });
return session;
}
All four items from the last round are there and they are load-bearing. mypy is clean on 77 files on the merged tree. I mutated the new suite rather than trusting the count: relaxing the To land it:
|
code-review-graph reviewOverall risk: 0.40 (MEDIUM) — 31 changed function(s)/class(es), 17 affected flow(s), 5 test gap(s) Risk-scored changes
Affected execution flows
Test gaps
Token savings: this graph-backed report used ~176,816 fewer tokens (~88%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
Pull Request
Linked issue
Closes #971
What & why
_extract_js_var_functionsonly created aFunctionnode when a declarator's value was directly an arrow function or function expression. A component wrapped in a higher-order call, like the issue'sforwardRef<HTMLButtonElement, Props>((props, ref) => ...), has its function inside acall_expression, so no node was created. JSX uses still resolved to<file>::<Name>, so every use became aCALLSedge to a node that does not exist, andcallers_of,tests_forandget_impact_radiuscame back empty for these components.What counts as a wrapped component.
_js_wrapped_functionaccepts a call only when all of these hold:forwardRef,memo,observer,withRouter,withStyles,withTheme,connect,styled,inject,withTranslation,withErrorBoundary(_JS_COMPONENT_WRAPPERS). The name is the callee itself, the last segment of a member callee (React.memo), or for a curried call the name of the inner callee (styled(Base)(fn),connect(mapState)(fn)).memois the one exception: it also takes the props comparator,memo(Component, arePropsEqual), and nothing more. Comments between the arguments are not counted.memo(forwardRef(fn))).The name is needed because the shape alone does not show that the result is callable:
const result = evaluate(() => compute())has the same shape asmemo(() => paint())and returns a number. Calls that fail a check are handled exactly as before. Examples:useMemo(() => x, [dep]),useCallback(fn, deps),createClient({...}),styled.h1`...`,memo(wrap(fn))with an unknownwrap, and wrappers nested past the limit.A wrapped declaration gets the same treatment as a directly assigned function:
Functionnode with the declaration's line range, and parameters and return type from the wrapped function;CONTAINSedge;What stays in the enclosing scope. Only the wrapped function is the component. The following parts run where the declaration is, so their edges keep the enclosing function (or the file, at module scope) as the source, as on staging:
setup -> memo,setup -> forwardRef);styled(Base),connect(mapState, ...));forwardRef<HTMLButtonElement, Props>) and the type annotation of the declaration (const Card: FC<Props> = memo(...));Other declarators in the same statement are now walked in the enclosing scope, both when a declarator is wrapped and when a function is assigned directly. This is a change from staging, which skipped the whole statement whenever one declarator was a directly assigned function:
const a = () => x(), b = y()lost the call toy. The type annotation of a directly assigned function is still not walked, as on staging.Known false positives. Wrappers are recognised by name, and for a member callee by its last segment, without checking where the name comes from. So a non-React call with the same name is also read as a wrapper if it takes a single callback (for
memo, a callback plus at most one more argument). Examples arepool.connect(cb)andcache.memo(fn): the variable becomes aFunctionnode, and the calls inside the callback are attributed to it. The corpus below has no such case: all 644 wrapped declarations there arememo,React.memo,forwardRef,React.forwardRef,ReactModule.forwardReforwithErrorBoundary. Restricting member callees toReact.*would remove these false positives, but it would also miss namespace imports under other names (ReactModule.forwardRefabove), so I have left that choice to you.Corpus check. I parsed 2058 React source files from cherry-studio, dify, LibreChat and grafana with staging (
f0e4eb7) and with this branch, and compared their nodes and edges.TESTED_BYedge in grafana'svalidators.test.tsx. That test renders<Component />insidememo(() => ...). The call now belongs to the newwrappedcomponent rather than to the test, andTESTED_BYfollows the call. Staging does the same when the function is assigned directly.TestRunMenuandTestInfoTab, so the existing name rule makes themTestnodes and 36 newTESTED_BYedges point at them. Staging does the same for a directly assigned function with those names.dify/web/public/vs. There the branch now walks the other declarators of a statement that assigns a function directly, which also adds 280 call edges that staging did not record.How it was tested
These are the CI commands, run at
d26609bin a Python 3.10 virtualenv (pip install -e ".[dev]" pytest-cov, plus mypy, types-networkx and bandit):The skips are mostly the opt-in suites: 715 of 728 (cli_surface, platform_lifecycle, action_e2e, exports, concurrency, corpus, surface, determinism). The other 13 are igraph not installed (5), Playwright not installed (2), four token-budget worst cases that the suite reports instead of asserting, one Windows-only path test, and one interpreter probe that needs user-site packages.
On Python 3.13:
pytest -q tests/test_js_wrapped_functions.py tests/test_callback_context_preservation.py # 23 passedtests/test_js_wrapped_functions.py(21 tests) covers:forwardRef<HTMLButtonElement, Props>reproduction (node, line range,CONTAINSedge);React.memoandReact.forwardRef<T, P>callees;Toolbar -> Buttoncase, where theCALLStarget now exists;choose(() => left(), () => right()),useMemo(fn, [dep]),evaluate(() => compute()),memo(wrap(fn)), wrappers nested past the depth limit,forwardRef(fn, extra)andmemo(fn, eq, extra);memo(Component, arePropsEqual)with a named and an inline comparator;I also mutated the wrapper code in 25 ways (argument counts, depth limit, name check, member callees, comment filtering, which scope each part is walked in). Every mutation fails at least one test.
Checklist
ruff check code_review_graph/)mypy code_review_graph/ --ignore-missing-imports --no-strict-optional)_js_wrapped_function,_js_wrapper_nameand_extract_js_var_functions; no README ordocs/page covers this)