scan: nest the children a legacy parser maps off its own node - #38
Conversation
Thirteen `WADeprecatedWapParser`s call a child method straight on the parser's
node — `incomingMsgParser` writes `e.mapChildrenWithTag("enc", …)`. `handle_call`
had a branch for the chained form (`param.child("tag").<childMethod>()`) and one
for a tracked child var, but none for this, so the repeated element was never
built at all.
Those callbacks reach the IR only by accident: minified code names the callback's
parameter after the parser's, and `obj_is_param` is a bare name comparison, so the
reads land at the ROOT — flat, unrepeated, one level too high. `incoming/0`
published `type`, `mediatype`, `decrypt-fail`… as attributes of `<message>` when
the wire has them on a repeated `<enc>`.
Two changes, in this order, because either alone is wrong:
1. The missing branch. Adds the child where it belongs; `incoming` gains
`message → enc (repeats) → [type, mediatype, content, count, decrypt-fail,
state, session_type]`, `notif` gains 54 nested reads, nothing is removed.
2. Scope tracking for the parser's parameter. Now that the reads have a home, the
root copies go: 25 in `iq`, 22 in `notif`, 13 in `incoming`. `privacyParser`'s
response root goes from twelve fields to one — `privacy` — with the ten
`value`s nested under `category` as the source has them.
Doing 2 first was tried and rejected: it deletes the `incoming` reads outright,
because without 1 there is nothing nested to keep. The floor guard caught the
shrink, and only because I had not silenced its stderr.
`w:gp2` keeps a flattened shape inside its new `children` node — that is the
attribute-discriminated union, unchanged by this and visible in the linter
baseline instead of hidden at the root.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4a456808f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.param_shadow_depth += u32::from(shadows); | ||
| walk::walk_function(self, func, flags); |
There was a problem hiding this comment.
When a newly supported callback reuses the parser parameter and aliases a descendant, such as function(e) { var x = e.child("inner"); x.attrString("id") }, this shadow counter suppresses only direct calls on e. The outer analyzer's visit_variable_declaration still records x, so its later accessor emits an extra inner field at the response root in addition to the correctly nested copy. Scope or suspend child_vars while visiting the shadowed function; the example currently produces root fields row and inner instead of only row.
Useful? React with 👍 / 👎.
| let shadows = params_bind(&func.params, self.param); | ||
| self.param_shadow_depth += u32::from(shadows); | ||
| walk::walk_arrow_function_expression(self, func); |
There was a problem hiding this comment.
For a direct child map written as e.mapChildrenWithTag("enc", e => e.attrString("id")), this visitor now suppresses the accessor during the outer walk, while process_child_method accepts only FunctionExpression callbacks and therefore builds no enc child. Consequently the parser shape becomes completely empty, whereas the same callback written with function(e) is extracted. Add arrow-function handling to the recursive child analysis before treating its parameter as shadowed.
Useful? React with 👍 / 👎.
| if is_child_method(method) && obj_is_param { | ||
| process_child_method(method, call, "", &mut self.fields, self.code, self.module); |
There was a problem hiding this comment.
When a direct mapped child contains an unresolved constraint, for example e.mapChildrenWithTag("enc", function(e) { e.contentBytesRange(min, max) }), this recursive call keeps only child_result.fields; its unresolved entries never reach the enclosing ParserResult. Because the new shadow tracking also suppresses the callback during the outer walk, the previously observed loss is no longer recorded in pending_drops, allowing extraction coverage to shrink without the intended diagnostic. Pass the child result's unresolved reasons back to the outer analyzer.
Useful? React with 👍 / 👎.
Three defects in the child nesting, all from deciding by name what only the
walk knows.
`process_child_method` accepted `function (n) {…}` and nothing else, so an
arrow callback built no child. Paired with the parameter-shadow check, its
reads stopped reaching the root as well: the whole shape went empty.
The shadow counter only silenced calls on the re-bound name. A descendant
bound beside it — `var t = e.maybeChild("id")` inside a callback of a parser
that already holds `var t = e.child("product_list")` — still registered in
the outer map, rebinding the outer name, and `t.contentString()` published a
field against whichever node happened to be there.
Suppress by span instead: skip what `process_child_method` demonstrably read
in its own scope, rather than what shares a parameter name with it. A
callback the recursion declined to descend into is a different case, and a
name test cannot tell the two apart. `digestResponseParser` writes
`t.child("list").mapChildren(…)` — chained off a tracked var, a form the
recursion does not follow — and its reads were landing at the response root:
`content` beside `digest`. Those are now counted under `shadowedCallbackRead`
rather than published at the wrong level or dropped in silence.
What a re-analysed callback could not resolve also stayed in the child's own
result and never reached the parser's diagnostics, so a loss inside a mapped
child could shrink coverage without a number moving.
Nothing new is extracted; 18 duplicate copies at the root go away — the
second `participant` on `w:gp2`, `error` on the two jobs that read it twice,
`media` on `psa`. The four reads that have no correct level to sit at are in
`dropsByReason`.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a90287511
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.inside_recursed(call.span) { | ||
| // Already read in its own scope by `process_child_method`. | ||
| } else if self.param_shadow_depth > 0 { |
There was a problem hiding this comment.
When a mapped-child callback captures an outer tracked node, such as var x = e.child("meta"); e.mapChildrenWithTag("row", function(row) { x.attrString("id"); row.attrString("v") }), the recursive analysis has no copy of the outer child_vars, so it ignores the read through x; this branch then suppresses the same call during the outer walk because its span is inside the recursed callback. The resulting meta field therefore loses its id child. Carry captured child bindings into the recursive analyzer or allow the outer walk to process calls whose receiver is not owned by the callback.
Useful? React with 👍 / 👎.
Reading a callback in its own scope and skipping it in the outer walk is
right only for the names that callback binds. A node captured from outside
is still the parser's: `var x = e.child("meta")` read inside
`mapChildrenWithTag("row", …)` is a read of `meta`, and the recursive
analyser cannot place it — it starts with no bindings at all. Suppressing by
span alone lost it with no diagnostic.
The mirror case was already wrong in the other direction. `usyncParser`
binds `r` to `<usync>`, then re-binds `r` inside a `forEach` to a child of
`<result>`; the walk resolved the inner name against the outer binding and
published `usync/refresh` and an `<error>` at the response root. Neither is
where the wire puts them. A parameter-name test could not see this: the
callback's parameter is `e`, and the collision is on a `var`.
So track what each enclosing function binds — parameters and its own locals,
not those of functions nested deeper — and suppress a read only when the name
it goes through belongs to one of them. Reads through a captured node are
handled as before. The five fields usync was publishing at the wrong level go
away; the reads behind them join `dropsByReason` rather than disappearing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5800dc6515
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.inside_recursed(call.span) { | ||
| // Read in its own scope by `process_child_method` — unless it reaches back | ||
| // through a node that scope never bound, which only this walk can resolve. | ||
| if self.reads_a_captured_child(call) { | ||
| self.handle_call(call); |
There was a problem hiding this comment.
When a child callback uses a different parameter while capturing the enclosing parser node, such as e.mapChildrenWithTag("row", function(row) { e.attrString("status"); row.attrString("v"); }), the recursive analyzer only recognizes row, while this branch suppresses the e.attrString("status") call because e is not a tracked child variable. The root status field therefore disappears without an unresolved diagnostic; allow the outer walk to retain reads through an unshadowed parser parameter.
Useful? React with 👍 / 👎.
| if let Some(body) = func.body.as_ref() { | ||
| bound.visit_function_body(body); | ||
| } |
There was a problem hiding this comment.
When a callback has a block-scoped declaration matching a captured child variable, collecting every binding from the whole function makes that declaration shadow the capture outside its block. For example, in a callback that reads captured x before an if block containing let x = row.child("inner"), the first read legally targets the outer child, but bound_by_inner_scope treats it as callback-owned and the field is lost. Track let/const bindings by their lexical extent rather than assigning all body bindings to the function scope.
Useful? React with 👍 / 👎.
Two gaps left by deciding ownership per function.
A callback with its own parameter shadows nothing, so the parser's node is
still reachable through it: `mapChildrenWithTag("row", function (row) {
e.attrString("status") })` reads the parser's `status`. The exception for
captured nodes covered tracked child vars but not the parameter itself, so
that read was suppressed with no diagnostic. `groupNotificationParser` does
exactly this — an inner function reads `participant` off the notification a
second time, for `oldOwner` — and the previous commit had dropped it. That
correction also revises what I said about the earlier one: the second
`participant` was never a duplicate copy of a nested field, it was a second
read of the same attribute, and the extractor does not merge those (the root
`t` has always appeared twice for the same reason). The linter baseline goes
back to tracking both names.
Collecting bindings per function is also wrong for `let`/`const`, which reach
only to the end of their block. A declaration inside an `if` was shadowing a
captured name for the whole body, so a read before that block looked
callback-owned and was lost. Record the lexical ones against the block that
bounds them and resolve ownership at the reading site. No parser body in the
pinned bundles uses `let`/`const` today, so this changes nothing in
generated/ — but they appear elsewhere in the bundles, and the failure mode
is a silent drop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9b7214e2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.inside_recursed(call.span) { | ||
| // Read in its own scope by `process_child_method` — unless it reaches back | ||
| // through a node that scope never bound, which only this walk can resolve. | ||
| if self.reads_an_outer_node(call) { | ||
| self.handle_call(call); |
There was a problem hiding this comment.
When a recursively analyzed child callback passes a captured outer child variable to a module-scope helper, such as var x=e.child("meta"); e.mapChildrenWithTag("row", row => helper(x)), the nested analyzer has no binding for x, while this branch neither handles the bare helper call nor invokes try_helper_descent. The outer analyzer previously recovered the helper's fields, but they now disappear from meta without an unresolved diagnostic; retain helper descent for calls whose arguments include an unshadowed captured child.
Useful? React with 👍 / 👎.
| // `let`/`const` reach only to the end of their block, so record the extent instead | ||
| // of letting the name stand for the whole function. | ||
| let block = self.blocks.last().copied().unwrap_or(decl.span); | ||
| for d in &decl.declarations { | ||
| for id in d.id.get_binding_identifiers() { | ||
| self.lexical.push((block, id.name.as_str().to_string())); |
There was a problem hiding this comment.
Fresh evidence in this revision is that only block statements push a lexical extent, so a let/const declared in a for header is recorded against the surrounding callback body. If the name matches a captured child variable, reads before or after the loop are consequently treated as callback-owned and dropped; for example, outer x.attrString("before") and x.attrString("after") reads around for (let x=0; ...) both vanish. Record loop-header bindings with the loop's span rather than the enclosing function or block.
Useful? React with 👍 / 👎.
Two leftovers from the previous commit, neither reachable in the pinned bundles, both silent when they do fire. Only block statements pushed an extent, so a `let` in a loop header was recorded against the enclosing body and shadowed a captured name on either side of the loop. `catch (e)` was worse: bound through the generic collector it landed in the function-wide set, and minified handlers reuse `e` freely — one `try/catch` anywhere in a callback would have discounted every read through the parser's own parameter in it. Loop headers, `switch` and `catch` now bound their own declarations. The exception for captured nodes covered reading through one but not handing one to a module-scope helper. `d(e, i)` inside a mapped child, with `i` bound outside it, is the helper-descent case the outer walk exists to resolve — the nested analyser has no binding for `i` — and it was being skipped along with the rest of the callback. generated/ is byte-identical: no parser body in these bundles puts a lexical declaration in a loop header, and none hands a captured node to a helper from inside a callback. The tests carry the evidence instead.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba879bade3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| walk::walk_function(self, func, flags); | ||
| return; | ||
| }; | ||
| self.scopes.push(BoundNames::of(&func.params, body)); |
There was a problem hiding this comment.
When an inline child callback is a named function expression whose name matches a captured child variable, such as var x=e.child("meta"); e.mapChildrenWithTag("row", function x(row){ x.attrString("bad") }), the function name shadows the capture throughout its body. BoundNames::of collects only the parameters and body, not func.id, so the outer analyzer treats this x as the captured node and incorrectly adds bad beneath meta. Add the function expression's own name to the scope pushed here.
Useful? React with 👍 / 👎.
| call.arguments.iter().any(|a| { | ||
| arg_expr(a).and_then(as_identifier).is_some_and(|n| { | ||
| self.child_vars.contains_key(n) && !self.bound_by_inner_scope(n, call.span) | ||
| }) |
There was a problem hiding this comment.
When a recursed callback passes both a shadowed child name and a later captured child to a helper, for example function(row){ let x=row.child("inner"); helper(x, y) } with outer tracked x and y, this predicate succeeds because y is unshadowed but discards which argument qualified. try_helper_descent then selects the first argument present in child_vars without the scope check, chooses the stale outer x, and invents the helper's fields beneath the outer x node. Preserve the qualifying argument/index or apply the same ownership filter during selection.
Useful? React with 👍 / 👎.
Two more places where ownership was decided by the wrong binding.
A named function expression binds its own name inside itself and nowhere
else. `BoundNames::of` collected parameters and body but not `func.id`, so
`function x(row){ x.attrString(…) }` read as the captured `<meta>` bound to
the same name outside. The mirror slip was in the collector: it recorded
every nested function's name into the *enclosing* scope, which is right for a
declaration and wrong for an expression — there it would discount reads
through a capture of that name. Both now follow where the name is visible.
Helper descent picked the first argument found in `child_vars` without asking
whether that name still meant it. Handed `d(x, y)` where a callback re-bound
`x`, it descended on the stale entry and hung the helper's fields under the
node `x` used to name. The gate that decides whether to descend at all
already applied the ownership filter; selection now applies the same one, so
the two cannot disagree.
generated/ is byte-identical — no parser in these bundles names a callback
after a captured node or re-binds a name it later passes to a helper.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d691ccad9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn visit_binding_identifier(&mut self, id: &oxc_ast::ast::BindingIdentifier<'a>) { | ||
| self.names.insert(id.name.as_str().to_string()); | ||
| } |
There was a problem hiding this comment.
Fresh evidence beyond the earlier let/const issue is that class declarations still reach this generic visitor and become function-wide names. In a mapped-child callback such as x.attrString("before"); { class x {} } x.attrString("after"), where x is a captured tracked child, the class shadows x only inside its block, but both valid outer reads are treated as callback-owned; the recursive analyzer cannot resolve the capture, so both fields disappear without a diagnostic. Record class declarations against the current lexical extent rather than inserting them into names.
Useful? React with 👍 / 👎.
| || self.bound_by_inner_scope(n, call.span) | ||
| }); | ||
| if reads_a_node { | ||
| self.unresolved.push(format!("{SHADOWED_READ}@{method}")); |
There was a problem hiding this comment.
When one unrecursed shadowing callback loses multiple fields read by the same accessor, such as e.attrString("a") and e.attrString("b"), both entries become the identical shadowedCallbackRead@attrString string. enum_link::resolve stores site + detail in a BTreeSet, so these distinct losses collapse to one and adding or removing another attrString read does not change dropsByReason; this defeats the coverage ratchet that is meant to expose newly lost fields. Include the wire field/tag or another per-read identity in the diagnostic detail.
Useful? React with 👍 / 👎.
| /// misattributed to the root — flat, unrepeated, and one level too high. | ||
| fn place(fields: &mut Vec<ParsedField>, parent_tag: &str, f: ParsedField) { | ||
| if parent_tag.is_empty() { | ||
| fields.push(f); |
There was a problem hiding this comment.
When conditional branches directly map the same tag with different callbacks, such as cond ? e.mapChildrenWithTag("row", r => r.attrString("jid")) : e.mapChildrenWithTag("row", r => r.attrString("lid")), the structural walk visits both calls and this unconditional push creates two root row fields. collect_response_fields de-duplicates generated fields by name and therefore keeps only the first callback's shape, silently omitting fields accepted by the other runtime branch. Merge shapes sharing the same method/tag, or model them as alternatives, rather than appending duplicate root fields.
Useful? React with 👍 / 👎.
Three rounds of review each found another binding form the collector was hoisting to the whole function: first `let`/`const`, then loop headers and `catch`, now `class`. The list was the bug. Invert it — a binding is confined to the extent it sits in unless it is a parameter or a `var`, the two forms that actually reach the function. Nothing has to be enumerated again. `dropsByReason` folds these losses through a set keyed by site plus detail, and the detail was the accessor alone: two `attrString` fields lost in the same callback collapsed to one entry, so a third going missing moved no number. That is the ratchet this diagnostic exists to feed. Key each by the node and the wire name it read. The counts rise — incoming 2 to 6, iq 4 to 6 — because they were always that many. A tag mapped in both arms of a conditional was appended twice, and the later de-dup by name kept the first and dropped the other arm's reads in silence. Fold same-tag maps into one element carrying both shapes; `w:gp2` had two `participant` and two `sub_group_suggestion` under `children`, and the second of each was a subset, so the merge loses nothing and the linter's flattened identity drops from eleven names to nine.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0713140289
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if self.inside_recursed(decl.span) || self.param_shadowed(decl.span) { | ||
| walk::walk_variable_declaration(self, decl); | ||
| return; |
There was a problem hiding this comment.
When a recursed child callback uses a different parameter but aliases the captured parser node, such as function(row) { var x = e.child("meta"); x.attrString("id") }, this unconditional early return prevents the outer analyzer from recording x, while the recursive analyzer cannot recognize the captured e. Fresh evidence beyond the previously reported direct captured-node read is that meta is created but its id child silently disappears; declarations whose initializer reads an unshadowed outer node still need to update the outer bindings.
Useful? React with 👍 / 👎.
| if !existing | ||
| .iter() | ||
| .any(|e| e.name == c.name && e.method == c.method) | ||
| { | ||
| existing.push(c); |
There was a problem hiding this comment.
Fresh evidence after the prior top-level duplicate-map case is that this merge only unions one level: if two branches both map row, and both map a nested sub where one callback reads a and the other reads b, the second sub matches by name and method and is discarded wholesale. The resulting row.sub exposes only a, so duplicate mapped fields need their child shapes recursively merged rather than skipped.
Useful? React with 👍 / 👎.
| if func.r#type == oxc_ast::ast::FunctionType::FunctionDeclaration | ||
| && let Some(id) = func.id.as_ref() | ||
| { | ||
| self.names.insert(id.name.as_str().to_string()); |
There was a problem hiding this comment.
In strict bundle callbacks, a function declaration inside a nested block binds only within that block, but this inserts every function declaration into the function-wide names set. Fresh evidence beyond the earlier class-binding case is that a callback containing x.attrString("before"); { function x() {} } x.attrString("after") treats both valid reads of a captured child x as shadowed and drops them; record block-level declarations against the current lexical extent.
Useful? React with 👍 / 👎.
Refusing to record any declaration made under suppression was too blunt.
`function (row) { var x = e.child("meta"); x.attrString("id") }` binds `x` in
the callback, but `x` is `<meta>` off the parser's own node — and the
recursion, which starts with no bindings, cannot follow the `e` it came from.
`meta` was created and its `id` dropped without a word. Record such a name,
scoped to the callback that bound it, so the alias cannot outlive it the way
a bare `child_vars` entry would; a declaration that reads a re-bound name is
still refused.
The last commit claimed nothing had to be enumerated again and then left the
one path that still enumerated: function declarations went straight into the
function-wide set. In a strict bundle a `function` declared in a nested block
binds only there. Recording it against the current extent covers both — at
the top of a body that extent is the function.
Merging two branches that map the same tag only unioned one level, so two
arms that both map a nested `<sub>` kept the first whole and published only
what one of them reads. The merge is recursive now.
generated/ is byte-identical.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a02d7f2079
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .is_some_and(|n| !self.bound_by_inner_scope(n, decl.span)) | ||
| || as_identifier(obj).is_some_and(|n| self.aliases_outer(n, decl.span)); | ||
| if (on_param || on_child_var) | ||
| && (!inner_owns_it || reaches_outer) |
There was a problem hiding this comment.
When a callback-local alias reuses an outer child name, such as var x=e.child("meta"); e.mapChildrenWithTag("row", row => { var x=e.child("inner"); x.attrString("a") }); x.attrString("b"), this condition allows the declaration to overwrite the global child_vars[x] entry with inner. The outer_aliases span expires after the callback, but the overwritten map entry does not, so the final runtime read of the outer x is incorrectly attached beneath inner instead of meta; scope or restore child_vars alongside callback aliases.
Useful? React with 👍 / 👎.
| // A node of the callback's own counts too: `var o = r.maybeChild("error")` reads a | ||
| // real element, and it is no more placeable here than a read through the stale | ||
| // outer binding it shadows. | ||
| let reads_a_node = as_identifier(obj).is_some_and(|n| { |
There was a problem hiding this comment.
Fresh evidence beyond the previously fixed direct-accessor deduplication is an unrecursed callback such as t.child("list").mapChildren(function(e){ e.child("x").attrString("id") }): this direct-identifier predicate rejects the outer attrString call because its receiver is a call expression, while the walk records only the inner child("x"). Adding or removing the lost id read therefore does not change dropsByReason, defeating the coverage ratchet; use the receiver chain's base identifier and retain a distinct diagnostic for the outer accessor.
Useful? React with 👍 / 👎.
| } else if let Some(&extent) = self.blocks.last() { | ||
| self.lexical.push((extent, id.name.as_str().to_string())); |
There was a problem hiding this comment.
When a recursed callback captures an outer child x but also contains a named class expression, such as const C = class x {}; x.attrString("id"), the class expression's self-name reaches this generic binding visitor and is recorded against the surrounding callback block. That x is actually bound only inside the class, so the later read targets the captured child; instead, the outer analyzer treats it as callback-owned while the recursive analyzer cannot resolve the capture, silently dropping id. Distinguish class expressions from class declarations when collecting callback bindings.
Useful? React with 👍 / 👎.
| self.blocks.push(stmt.span); | ||
| walk::walk_switch_statement(self, stmt); |
There was a problem hiding this comment.
When a mapped callback reads a captured child in the discriminant and a case declares the same name, such as switch (x.attrString("kind")) { case "a": let x = row.child("inner"); }, JavaScript evaluates the discriminant using the outer x; the case block's lexical binding does not cover that expression. Pushing the entire statement span makes bound_by_inner_scope classify the discriminant read as shadowed, so the outer walk suppresses it and the recursive analyzer cannot recover it. Scope case declarations to the switch case block rather than the whole statement.
Useful? React with 👍 / 👎.
| arg_expr(a).and_then(as_identifier).is_some_and(|n| { | ||
| self.child_vars.contains_key(n) && !self.bound_by_inner_scope(n, call.span) | ||
| }) |
There was a problem hiding this comment.
Fresh evidence beyond helper calls using variables declared outside the callback is a callback-local alias to an outer node, such as e.mapChildrenWithTag("row", row => { var x=e.child("meta"); d(x); }) where module helper d reads fields from its argument. The declaration records x in child_vars and outer_aliases, but this predicate rejects it solely because the callback binds the name; the recursive analyzer cannot resolve the captured e, so neither analyzer descends into d and its fields disappear without a diagnostic. Treat aliases_outer as eligible both here and in try_helper_descent's argument selection.
Useful? React with 👍 / 👎.
The alias the last commit added was half a change. It let a declaration made
inside a callback write into `child_vars`, a map with no scope at all: an
alias reusing an outer child's name overwrote that entry for good, so a read
after the callback attached under the alias' node instead of its own. That is
the stale-binding fault this branch set out to fix, put back by the fix for
something else. Restore the map when a function's scope ends — a name a
callback binds dies with it, alias or not.
Two more places still asked the narrow question. Helper descent rejected an
alias outright, so `d(x)` inside a callback reached neither analyser and the
helper's fields vanished; both the gate and the argument selection now ask
whether the name still means an outer node, which is the same question every
other site asks. And the diagnostic keyed on a bare identifier while the rest
of the walk follows the receiver chain, so `e.child("x").attrString("id")`
recorded the child and not the accessor on it — the ratchet could not see
that read go missing.
Scoping precision, in the same vein as the block forms: a named class
expression binds its name inside the class, not in the block around it, and a
`switch` evaluates its discriminant before the cases' block exists, so a
`let` in a case must not shadow what the discriminant reads.
generated/ is byte-identical.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93f4541ba9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if inner_owns_it && let Some(extent) = self.scopes.last().map(|s| s.extent) | ||
| { | ||
| self.outer_aliases.push((extent, name.as_str().to_string())); |
There was a problem hiding this comment.
Fresh evidence in this revision is that aliases are restored only when the entire callback exits, while this records every callback-local alias with the callback body's full extent. When a block-scoped alias reuses a captured child name, as in { let x = e.child("inner"); x.attrString("a"); } x.attrString("b") with an outer x bound to meta, the child_vars[x] overwrite remains active after the block and the valid final read is attached to inner instead of meta. Scope and restore the tracked binding at the declaration's lexical extent rather than the whole callback.
Useful? React with 👍 / 👎.
… does The previous commit restored `child_vars` when a function's scope closed, which is the right boundary for `var` and too late for everything else. A `let` alias reusing a captured child's name went on meaning its own node for the rest of the callback, so a read after the block attached under it instead of under the node the name was bound to outside. Same misattribution as before, one scope level down. Record what a lexical alias displaced and put it back when its block ends, and give the alias the extent its binding actually has. `var` keeps the function boundary it already had: declared in a block, it does go on meaning its node for the rest of the callback, and restoring it there would be the opposite error. generated/ is byte-identical.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c04e5977e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.outer_aliases | ||
| .iter() | ||
| .any(|(e, n)| n == name && span.start >= e.start && span.end <= e.end) |
There was a problem hiding this comment.
Fresh evidence beyond the earlier alias-restoration cases is a nested callback that reuses an outer alias: in var x=e.child("meta"); row.mapChildrenWithTag("sub", function(sub){ var x=sub.child("inner"); x.attrString("id") }), this any still matches the enclosing meta alias even though the inner function has rebound x. The outer analyzer consequently publishes id beneath meta as well as correctly beneath sub.inner; alias resolution needs to stop at the nearest binding rather than accepting any enclosing alias.
Useful? React with 👍 / 👎.
| let extent = if decl.kind.is_lexical() { | ||
| self.blocks.last().copied() |
There was a problem hiding this comment.
Fresh evidence after the prior loop-header shadowing fix is that aliases declared in those headers still use self.blocks.last(), while this visitor pushes only block statements and never the for/for-in/for-of extent. With an outer x bound to <meta>, for (let x=e.child("inner"); ... ) { ... } x.attrString("after") therefore leaves child_vars[x] pointing at inner, and the post-loop read is attached to inner instead of meta; use the loop's lexical extent and restore the displaced binding when that extent ends.
Useful? React with 👍 / 👎.
| { | ||
| self.lexical.push((extent, id.name.as_str().to_string())); | ||
| } | ||
| self.visit_class_body(&class.body); |
There was a problem hiding this comment.
var declarations in class static blocks
Fresh evidence beyond the earlier block-class cases is that descending through the class body lets visit_variable_declaration treat a static block's var as function-wide. For a captured child x, a callback containing class C { static { var x = 0; } } x.attrString("id") consequently classifies the final read as callback-owned and drops id, even though the static block's x is scoped only to that block; collect static-block bindings in a separate scope or skip them when computing the callback's bindings.
Useful? React with 👍 / 👎.
| let cb_body = &code[body.span.start as usize..body.span.end as usize]; | ||
| let child_result = analyze_with_scope(cb_body, cb_param.as_str(), module); | ||
| sink.recursed.push(cb_span); | ||
| let child_result = analyze_with_scope(cb_body, &cb_param, module); |
There was a problem hiding this comment.
Fresh evidence beyond the captured/aliased-helper cases is a direct callback that delegates its own node, such as e.mapChildrenWithTag("row", function(row){ parse(row) }) where the module-scope parse helper reads row.attrString("id"). This analysis represents row only as the parser parameter, while try_helper_descent recognizes helper arguments only through child_vars, so it never enters parse and emits an empty row shape without an unresolved diagnostic; seed the mapped parameter as its child tag or allow helper descent through the current parameter.
Useful? React with 👍 / 👎.
Following a name a callback bound to one of the parser's nodes needed
`child_vars` to be lexically correct, and three commits running failed to
make it so — each fix attached fields to whichever node the name meant
somewhere else, which is worse than the silent drop it was meant to prevent
and is exactly the fault this branch exists to remove. It never earned its
risk either: generated/ was byte-identical when it went in and is
byte-identical now that it is out, so it only ever covered a shape these
bundles do not contain.
So a name a callback binds stays the callback's, as before. What is kept is
the knowledge that such a binding exists: a declaration refused here is
remembered, and a read through it counted under `readThroughUnknownNode`
rather than passed over. Recorded, never resolved — nothing can be placed
against it, so nothing can be misplaced.
That closes the three findings about alias scoping by removing what they were
about. A fourth, a helper called with a mapped child's own parameter
(`mapChildrenWithTag("row", function (row) { parse(row) })`), is a gap in
extraction rather than in placement, like the chained `t.child("l")
.mapChildren(…)` form already noted; neither belongs in this branch.
1 participant
Retargeted to
mainnow that #37 has landed. Eleven commits: two that fix the extractor, nine that came out of automated review.The defect
Thirteen
WADeprecatedWapParsercall a child method straight on the parser's own node:handle_callhad a door for the chained form (param.child("tag").<childMethod>()) and for a tracked var, but none for this one, so the repeated element was never built.The callback's reads reached the IR by accident: minified code names the callback's parameter after the parser's, and the check was a bare name comparison. They landed at the root — flat, unrepeated, one level too high.
incoming/0publishedtype,mediatypeanddecrypt-failas attributes of<message>when the wire puts them on a repeated<enc>.Order matters
1. The missing door.
incominggainsmessage → enc (repeats) → [type, mediatype, content, count, decrypt-fail, state, session_type];notifgains 54 nested reads; nothing is removed.2. Suppression by ownership. With the reads given somewhere to live, the root copies go: −25
iq, −22notif, −13incoming.Doing step 2 first was tried and rejected: it deletes
incoming's reads outright, because without step 1 there is nothing nested for them to fall back to.What review changed afterwards
The suppression test moved from the parameter's name to who owns the name, in three steps: the span of what the recursion already read, lexical ownership by extent (block, loop header,
switch,catch), and the parser's own parameter. That turned up a fault in the opposite direction nobody had reported —usyncParserre-bindsrinside aforEach, and the walk resolved it against the stale outer binding, publishingusync/refreshand an<error>at the response root. Five fields the wire does not put there.Also: arrow callbacks now build their child;
dropsByReasonno longer collapses distinct losses into one set entry (the ratchet did not move when a new field started going missing); and conditional branches mapping the same tag become a single element, merged recursively —w:gp2carried duplicateparticipantandsub_group_suggestionunderchildren.An attempt to resolve names a callback binds to the parser's nodes was added and then reverted: it needed
child_varsto be lexically correct, failed three times running by attaching fields to the wrong node, andgenerated/was byte-identical with and without it. Only the diagnostic remains — such a read is counted underreadThroughUnknownNode, never placed.Out of scope
Two extraction gaps (as opposed to placement) are left for another PR:
t.child("l").mapChildren(…)chained off a tracked var, andmapChildrenWithTag("row", function(row){ parse(row) })handing the mapped node to a helper. Both are reported as diagnostics today rather than extracted.Verification