Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
b7a15f4 to
778e372
Compare
`class Repo { loadAll = function* () {…} }` is a method written as a field.
Two helpers decide that — one classifies the field, one finds the body to walk
— and each lists the node types a field's value may have.
Upstream's generator fix (colbymchenry#1743) is wider than the fork's everywhere except
here: it did not reach those two helpers. The merge took upstream's copy of
`languages/typescript.ts` and `languages/javascript.ts` wholesale, so the
lists lost `generator_function` on the walker side while the kernel kept it —
and the kernel runs by default. The same source then read as a method on one
extraction path and a property on the other:
node:missing-in-kernel:property Repo::loadAll
node:extra-in-kernel:method Repo::loadAll
(with its contains edge and its body's call moving with it)
Which one a project got depended on whether a kernel binary was staged.
`kernel-tsjs-parity` did not catch it: no fixture holds a generator-valued
field.
Restoring the six hunks on the walker side settles it, and settles it the way
the kernel already reads — a generator field is a method, which is what it is.
The fixture now reports 2/2 byte-parity, and a sweep of this repo's own 274
TypeScript sources reports none.
FORK.md's patch list was two short of the tree: this one, and the Dart
extension type carried since aa8d4ae. Both are listed now, the Dart one marked
temporary against upstream PR colbymchenry#1865. CHANGELOG gets an entry and loses a
duplicated `### Fixes` heading left by two separate commits.
`repro-1373/` goes: three scratch files for upstream issue colbymchenry#1373, left at the
repository root by an earlier sync, referenced from nowhere, shipped in every
release and indexed into every graph built here.
|
Option 2 matches the diagnosis in #1784: Option 1 (language-narrow the |
…iant The patch here reached the same result by a different lever than the one sent upstream as colbymchenry#1865: it listed `extension_type_declaration` on `classTypes`, where the pull request lists it on `extraClassNodeTypes` and in the enclosing-type walk. Both make an extension type's members methods under it — on this fixture the two produce the same nodes, spans and kinds — but they are different edits, and FORK.md called them the same change. That costs something precisely when the patch is supposed to be free: colbymchenry#1865 merges, the base moves past it, and whoever drops this patch finds the lines it names are not the lines that arrived. So take the pull request's version verbatim. `dart.ts` and `dart.rs` now match its diff line for line, and the kernel arms were already identical. The issue's author has read colbymchenry#1865 and confirmed it answers his diagnosis; no maintainer review yet. Dart cover passes 21 of 21 — this fork's three cases plus upstream's kernel-dart-parity — and the suite's 18 failures are the same set as before, which is upstream's own set minus the Dart ones.
|
Independently verified on Linux (REDACTED_SECRETS, from-source kernel build). Two findings, one for the fix and one for the branch: 1. The fix is correct and complete. On current 2. The branch needs a rebase, and it is a clean one. As pushed (based at |
…nry#1784) `extension_type_declaration` appears in none of the three places Dart's other type-like declarations do. `extension_declaration` — the older `extension` — is in all three, and the two names are near neighbours. That is why colbymchenry#1780's `isInsideClassLikeNode()` gate dropped these members and no others: the gate asks whether a class-like node is on the stack, and an `extension type` never put one there, while `extension`, `mixin` and `class` all did. The gate surfaced the omission rather than causing it. Before it, the members were still reached — but as top-level `function:km` rather than `method:MetersT::km`, indexed and attributed to nothing. The kernel omits the same node type, so it walks the members through its own fallback and mints them as top-level functions with no `MetersT` to belong to. `kernel-dart-parity.test.ts` fails four cases on `main` because of it; fixing only the wasm side would leave those red. Listing it alongside the others is the whole fix — `extraClassNodeTypes` and `dartEnclosingTypeName` on the wasm side, the two `matches!` arms on the kernel's. Both paths now give: class:MetersT · method:MetersT::km · method:MetersT::report with `report`'s span running to its closing brace rather than stopping at the signature line, and an ordinary class untouched.
778e372 to
8d8c1ec
Compare
|
Rebased onto On current The half you flagged as separate is measured too: reverting just the Full suite here: 4 failures, the same |
|
Option 1 is already in the tree — #1862 narrowed the Worth flagging because it changes what the repro looks like: on What is left is the half the report called strictly better — |
2 participants
Fixes #1784 — implements the report's option 2. Rebased onto
58c07e8.The change
Four places, in two of which the report already pointed:
extension_declaration, the olderextension, is in all four;extension_type_declarationis in none.classTypesandenumTypesare leftalone —
extraClassNodeTypesis whereextensionandmixinalready sit, andthat is enough.
What
maindoes todayOn
58c07e8, both extraction paths agree, and both are wrong. The report'srepro yields two top-level functions and no type for them to belong to:
That is the report's option 1 already in the tree. #1862 narrowed the
isInsideClassLikeNodegate to TS/JS/ArkTS — the same shape the reportproposed — and it does what the report said it would:
kmis back. It landed aspart of a regression audit rather than against #1784, so the issue reads as
though nothing has moved, which is worth knowing before checking the repro: what
you see now is a top-level function, not the missing member the report describes.
What option 1 leaves is the mis-kinding the report also calls out — a top-level
function rather than a member of its type.
kernel-dart-parityis green there — 18/18 — because the two arms agree. Thesuite proves the paths match each other; it cannot prove either is right, so
this defect sits behind a green gate with nothing pinning it.
Result
With this PR, on the report's repro verbatim, both paths:
which is the shape the report calls strictly better — a member of its type
rather than a top-level function. With a fixture that has a second member, that
member's span recovers too: it runs to its closing brace instead of stopping at
the signature line. An ordinary
classis unchanged, and so areextensionandmixin— those two are already covered by the parity suite's Dart torturefixtures, which stay green.
The two sides have to move together: with only
dart.tschanged the walkerreads the type correctly while the kernel keeps minting top-level functions, and
the paths disagree. The numbers for that are under Tests.
Tests
Adds
__tests__/dart-extension-type.test.ts— the extension-type fixture thereport notes is missing. It pins the node kind, the qualified name, the span of
the member that follows, and an ordinary class as a control.
Each half of the change is pinned by a different gate, which is worth stating
exactly. Reverting the
dart.tshunks fails this suite's first case on the wasmpath (1 of 3). Reverting the
.rshunks leaves this suite green — with a kernelstaged it is the kernel that answers — and instead takes
kernel-dart-parityfrom 18/18 to 4 failures. Both measured on this branch.
Full suite on this branch: 4 failures.
58c07e8, built and run on its own:the same 4,
git-hooks.test.ts, which depend on this machine's globalcore.hooksPath. Nothing here is new.kernel-dart-paritystays 18/18, nowagreeing on the right answer.
So this is the report's option 2, on top of option 1 rather than instead of it —
the two are complementary, and #1862 got the first half there ahead of this. What
remains is the difference the report calls strictly better:
method:MetersT::kmrather than
function:km.