fix(resolve): assigning a variable from a function call erased the type it was declared with - #278
Conversation
…pe it was declared with Rule 2 records a C++ assignment from a call as the variable's type, so `x = Foo()` types `x`. A constructor call and a function call are one grammar node, so the record held the callee's last name whatever it was: `PHI = PHINode::Create( … )` recorded `Create`, `s = GetDBOptionsFromMap( … )` recorded `GetDBOptionsFromMap`, `t = llvm::cast<T>( y )` recorded `cast`. Rule 2's flat per-function table tombstones a variable whose records disagree, so the non-type erased the declared `PHINode *PHI` / `Status s`, and the call fell to the name ladder. The field use-site index (--uses=Owner.field) lost the same pin, and a MEMBER assigned from a call read as a local, so Rule 2b refused its declared type. graph.h's varType note said such a type "simply never produces a type::method hit"; that was true of the narrow and false of the tombstone. The fix: ingest marks an assignment's record (RawBind/Binding::isFromAssignment, ingest_binds.h assignedTypeOf), and buildGraph drops it unless a class of that name exists (resolve.h assignmentNamesNoClass, with the class-name set Rule 2c already built, now built first) — from Rule 2's varType table, from collectFieldUseSites' table and from the local-name set Rules 2b/2c and the external veto read. A declaration's callee-read type is kept: that declaration exists with an unrecorded type, and its conflict with a sibling declaration is what keeps one block's type off the other's calls. Measured, `--pin-census --no-cache`, C rows joined on (caller id, callee, line), main fe28fd4 vs this change: rocksdb 0e2801ac3 1,871 sites change target, bound +1,562 (1,544 newly bound, 327 retargeted, 0 lost; 1,494 are Status::ok); llvm-project 4d5358b1d 4,046 sites, bound +2,984 (2,859 newly bound, 1,187 retargeted, 0 lost). Seeded sample of 60 (seed 20260917; 25 rocksdb, 35 llvm), graded blind by independent readers with A/B randomised per site: 51 better, 6 same, 3 worse (39 NONE->RIGHT, 5 WRONG->RIGHT, 4 PARTIAL->RIGHT, 2 NONE->PARTIAL, 1 WRONG->PARTIAL; 4 WRONG->WRONG, 2 RIGHT->RIGHT). The three worse are resolver floors the tombstone had masked: rocksdb `Iterator*` narrowing onto memtable namesake `Iterator` classes, and two arity picks of the wrong overload of the right class. The local-name-set part alone moves 23 rocksdb and 132 llvm sites; 15 graded 10 better, 2 same, 3 worse — two of those expose Rule 2c reading a member named like a class (`std::unique_ptr<ToolOutputFile> OutputFile`) as that class, a floor this change unmasks and does not cause. Built and rejected: dropping a DECLARATION's callee-read type too. +89 llvm sites (0 rocksdb), 15 graded 11 better, 2 same, 2 worse, but it hands one block's written type to a sibling block's `auto t = ns::cast<Decoy>( y )` — arm 48. Considered for #276's floor (narrowcheck arm 39d, unqualified `Vec<T>()` constructors): a scratch composition of #276 50b97b7 + this change reading that spelling moves 245 llvm sites, 170 lost and 0 gained (779 lost before this change), all declaration conflicts like `auto *LI = cast<LoadInst>( … )` beside another `LI`. The floor stays. Format: the bind record gains one u8 (kCacheVersion 22 -> 23, kMinBindRecordBytes 26 -> 27, cachefuzzcheck's walker reads it); kParserVer 99 -> 104 with both quality.h mirrors (100-103 are declared by lanes queued ahead; the train assigns); test/qschemetrip.hash re-pinned with a RE-PIN LOG entry. graph.h stays line-neutral above rankGraphTeleport. Gate: test/narrowcheck.sh arms 44-51 (fixture generated in-gate). Red on main fe28fd4: 44, 45, 46, 49, 50 (no edge, owner_candidates="2", no census row). Red on variants built from this change: 48 when declarations are dropped too, 46 without the local-name-set filter, 51 when the new byte is not persisted. Green here, with arms 1-25. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sted class-name loop buildFieldNarrowTables gains the class-name set (api-surface 1 -> 2): its local-name set drops an assignment's callee-read type that no class is called, the rule Rule 2's varType table applies. classNameSet (duplication and new-clone-of-reused-helper against layout.h buildAggIndex, 93 tokens) is the Class/Struct/Interface loop buildGraph already carried inline for Rule 2c, hoisted so collectFieldUseSites shares it; layout.h includes graph.h, so resolve.h cannot reuse buildAggIndex, whose value type differs. Written through --quality-ack --ack-only=buildFieldNarrowTables, classNameSet; the ledger diff is exactly three +ack rows. The minor verbosity row (collectFieldUseSites 171 -> 173) is not gating and is left unacked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughChangesThe change preserves assignment-origin metadata for inferred type bindings. Graph analysis now ignores non-class assignment callees while retaining class-based narrowing. Cache formats and extraction identities are updated, with new narrowing, determinism, and cache tests. Assignment type narrowing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The assignment-type narrowing change includes the necessary cache invalidation and validation coverage, with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 45.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 10 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor commits merge as-is. Resolutions: - src/model.h: RecvKind::JavaTypeCandidate is appended after train 2b's Lit* kinds, its note joins the enum's comment block, and kRecvKindCount names it as the last enumerator. - src/graph.h: javaNarrower is built with scopedRecvDecls like the train's Narrower (1b's constructor); the S6-C locality guard keeps !identityClaim and !isJsTsLitRecv and adds the JavaTypeCandidate exclusion. buildFieldNarrowTables merged without conflict and was read by hand against #278's change there: #278 skips only a C++ assignment's Type binding (isFromAssignment), and #235's localShadowSpans records only VarDecl bindings, so neither change reaches the other's rows. - kParserVer and kIngestParserVerMirror: 106 after #278's 105; the PR's two steps (declared 97 and 98 over main's 96) land as one, their notes kept as "PR step". The PR's 96 note moves into descending order. - test/qschemetripcheck.sh: the PR's two RE-PIN LOG entries, renumbered, under train 3's. test/qschemetrip.hash keeps the train's pin; re-derived once on the final tree. - test/callformcheck.sh header: both documented-absent edits (member templates, Java method references) kept. - test/regression.sh: loop union (+javamethodrefcheck = 633); README.md, docs/EVALS.md and present/deck5_ripwire_build.js regenerated by docs/gatecount_build.py. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor commits merge as-is. Resolutions: - src/graph.h / src/resolve.h: #278 hoisted buildGraph's class-name loop into resolve.h classNameSet(), shared by Rule 2c and its assignment guard; #267 widened that same loop to Ruby modules. The train keeps the classNameSet() call and the Ruby-module clause and its note move into classNameSet() (the guard was already a name-only, cross-language test). - kParserVer and kIngestParserVerMirror: 108 after #233's 107 (the PR declared 96 -> 97); the mirror gains a note; the PR's note says kCacheVersion stays #278's 23. - CHANGELOG: the PR's entry moves after the train's other entries. test/regression.sh: loop union (+rubyrecvnarrowcheck = 635); counts regenerated. test/qschemetrip.hash keeps the train's pin until the final re-derive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he composed tree #278 removes the ASSIGNMENT half of the floor's stated reason (`Spec = cast<FunctionDecl>( F )` no longer records `cast`), so the numbers were re-measured on this tree: a variant binary that reads an unqualified template-id callee in ctorNameNode, `--pin-census --no-cache` against the train binary, joined on (caller id, callee, line). llvm-project 4d5358b1d: 463 sites move, 324 edges lost, 137 retargeted, 2 gained; rocksdb 0e2801ac3: none. Sampled against source, every loss is a DECLARATION conflict — a cast helper's name beside the declaration's written type (`const ConstantInt *CI = dyn_cast<ConstantInt>( V )`, `StructType *STyL = cast<StructType>( TyL )`) or beside a second declaration (`CallInst *I = cast<…>` and `auto *I = dyn_cast<Instruction>( V )`), and the four retargets sampled were all worse. The floor stays and arm 39d still passes; its reason is rewritten in ingest_binds.h ctorNameNode, narrowcheck's arm 39 header and #276's CHANGELOG entry, and #278's entry names the base of its own 245/170 measurement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ons move up one Train 1b's fix push bumps kParserVer 102 -> 103 (a signed numeric literal receiver, `(-1).toFixed()`), so every train 3 assignment moves up one in merge order: #276 = 104, small-fixes = 105, #278 = 106, #235 = 107, #233 = 108, #267 = 109. - src/ingest_cache.h, src/quality.h: kParserVer and its mirror 109; every train 3 note renumbered; 1b's 103, which its push recorded only in the RE-PIN LOG, gains a note in both headers; kCacheVersion's note names #278's parser version 106. - test/qschemetripcheck.sh: the TRAIN 3 entry names 1b's fix push and the new numbers, the member entries are renumbered, and 1b's own entry follows them. - test/qschemetrip.hash: re-derived on this merged tree (neither side's pin hashed 109). - CHANGELOG.md: the train 3 entries' parser-version sentences and headings renumbered. Everything else in the fix push (MCP builders, --for terms_total, regex alerts, the impactpartition/nulbyte/timeout gate fixes) merged without conflict. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This PR is part of integration train 3, #281, and will show as merged when that train lands. |
…a local, so Rule 2b refused its declared type Rule 2b's local-shadow veto refused the member whenever buildFieldNarrowTables' localNameSet held (method, name), and that set is built from every binding record. C++ assignments mint records: the assignment arm of bindsVisitNode records the callee's name as a Type binding for `x = Foo()` and `x = std::make_unique<T>( … )`, and the L3 capture records FnAssign for `x = other` and a clobber tombstone for `x = nullptr`. An assignment declares nothing, so a member assigned in a method and then called lost its narrow (llvm-project LVReader.cpp:175, `OutputFile->keep()` in LVSplitContext::open, no edge; now ToolOutputFile::keep). The set's value is now the evidence it holds (resolve.h localNameEvidence): any record sets kLocalNameBound, a VarDecl, ParamType or FnDecl record also sets kLocalNameDeclared, and Rule 2b vetoes on the second bit alone. Rule 2c and the Phase 5 external veto keep reading presence: they ask whether the name is a variable, which an assignment proves. The previous commit made every local-declaring shape emit a declaration record except the vexing-parse local, pinned as a floor. Measured, --pin-census --no-cache, joined on (caller id, callee, line) against the previous commit: rocksdb @ 0e2801ac3 118 retargeted (67 gained / 51 changed / 0 lost), llvm-project @ 4d5358b1d 683 (521 / 162 / 0), all decided by Rule 2b (instrumented scratch build). Vetoing evidence before: FnAssign or clobber 88 / 479, a Type naming a class 0 / 13, only Type records naming no class 30 / 191 (the population redhat-et#278's guard also releases). Seeded blinded sample of 40 graded against source: 35 better, 4 same, 1 worse (a namesake base class across namespaces in the base walk). Rejected variant, measured: Rule 2c reading declarations only moves 0 rocksdb and 7 llvm-project sites, graded in full: 6 worse, 1 same. Composition checked with lane/rule2-irbuilder-local (scratch merge, rerere off): 24 more llvm-project sites retarget, all better — members the old per-method veto refused because a vexing-parse local of the name lives in one nested block; no retargeted site on either corpus sits inside such a block. Gate: test/fieldnarrowcheck.sh arm v. v1 x5 and v3's Rule 2b pickup red on the previous commit; the 12 v2 controls red on a scratch build with the veto removed, 3 of them (reference-returning parameters, attributed declarator) red on this veto without the previous commit; the 3 v3 rows red on a scratch build whose Rule 2c ignores assignments. graph.h stays line-neutral above rankGraphTeleport. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… 113, kCacheVersion 24 PR redhat-et#282 ad7a9d5 (signed off) on main a6868f7. The PR declared kParserVer 103 -> 104 and kCacheVersion 22 -> 23 over its base a5ce95e; main had spent 112 and redhat-et#278's 23, so integration/train-5 assigns 113 and 24 (the ref record grows viaArrow, kMinRefRecordBytes 39 -> 40), with both quality.h mirrors and a RE-PIN LOG entry. The qschemetrip hash is re-derived once on the final train tree. Resolutions (unions; nothing from either side dropped): - src/ingest_relations.h: main's type-alias capture (redhat-et#280) and redhat-et#282's stdSmartPointee sit side by side. - src/graph.h: the member-field loop skips an alias record (redhat-et#280) and a smart pointer's viaArrow pointee (redhat-et#282). - src/ingest_cache.h: redhat-et#282's readRef (viaArrow) with main's writeBind/readBind (isFromAssignment). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nto train 5 — kParserVer 114 lane/rule2b-assignment-veto 626810b (signed off; redhat-et#282 + 8f16d3e + 626810b). The lane declared kParserVer 112 over redhat-et#282's 104; main had spent 112 and redhat-et#282 takes 113, so integration/train-5 assigns 114 (mirror moved; no record layout change, kCacheVersion stays 24). Resolutions: - src/resolve.h: main's type-alias bases, classNameSet and redhat-et#278's assignmentNamesNoClass beside the lane's localNameEvidence; rule2cClassNameRecv keeps main's ClassNameRecvNames signature (redhat-et#280's memberFields) with the lane's wording for condition (2). - src/graph.h: localNameSet takes the lane's evidence bits AND keeps main's VarDecl localShadowSpans (Java shadows). - A composition call, not a textual one: main's localNameSet loop skipped a record assignmentNamesNoClass drops (redhat-et#278: a member assigned from a call read as a local, so Rule 2b refused its declared type). The lane solves that same refusal with the declared bit, and its arm v3 needs Rule 2c to keep the assignment as evidence the token is a variable (`Widget = makePane();`), which the skip would drop. The loop now reads every record; assignmentNamesNoClass still guards the varType and field use-site tables, and its comment says the local-name set joined the two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Defect
Rule 2 records a C++ assignment from a call as the variable's type, so that
x = Foo()typesx. A constructor calland a function call are one grammar node, so the record held the callee's last name whatever it was:
PHI = PHINode::Create( … )recordedCreate,s = GetDBOptionsFromMap( … )recordedGetDBOptionsFromMap, andt = llvm::cast<T>( y )recordedcast. Rule 2's flat per-function table tombstones a variable whose records disagree.So the non-type erased the declared
PHINode *PHI/Status s, andPHI->addIncoming( … )/s.ok()fell to thename ladder. On the fixture and on both corpora that ladder usually declines. The same record:
--uses=Target.countlost its owner pin);cur = ns::cast<Target>( y )) read as a local, so Rule 2b refused the member'sdeclared type.
graph.h's varType note said such a type "simply never produces a type::method hit". That holds for the narrow but not
for the tombstone. (The comment above
ctorNameNodealso claimed a class-name filter in buildGraph that did not exist.)Change
ingest_binds.h): the assignment arm marks its record withassignedTypeOf→RawBind/Binding::isFromAssignment.resolve.h assignmentNamesNoClass): drops such a record unless a class of that name exists. It uses theclass-name set Rule 2c already built, which is now built first (
classNameSet). The record is dropped from:varTypetable;collectFieldUseSites' table;auto t = makeFoo()) is still recorded. Thatdeclaration exists with an unrecorded type, and its conflict with a sibling declaration keeps one block's type off
the other block's calls (arm 48).
kCacheVersion22 → 23;kMinBindRecordBytes26 → 27;cachefuzzcheck's blob walker reads the new byte.kParserVer99 → 104 with bothquality.hmirrors. 100–103 are declared by lanes queued ahead (train 2b: 102,fix(ingest): a receiver typed with template arguments recorded no type, or the wrong class #276: 103); the landing train assigns the number.
test/qschemetrip.hashre-pinned, with a RE-PIN LOG entry.graph.hstays line-neutral aboverankGraphTeleport(showcase seed).Measured
Method:
--pin-census --no-cache; C rows joined on (caller id, callee, line); mainfe28fd49vs this branch.0e2801ac34d5358b1dOn rocksdb, 1,494 of the 1,871 are
Status::ok.Blinded grading. A seeded sample of 60 (seed 20260917; 25 rocksdb, 35 llvm-project) was graded against source by six
independent readers. Each saw the two answers as A and B, with the order randomised per site; the key was kept outside
the grading directory.
Iterator*narrowing onto the memtable's same-namedIteratorclasses (the floor fix(resolve): a member call through a typed parameter was pinned to the caller's own class #248 records);getFirstInsertionPt,StringRef::find).The local-name-set part on its own (this branch vs a build without that filter): 23 rocksdb and 132 llvm-project
sites. 15 graded: 10 better, 2 same, 3 worse. One worse site is the same-named-class floor (
PHINode). Two show a floorthis unmasks but does not cause: Rule 2c reads a member named like a class (
std::unique_ptr<ToolOutputFile> OutputFile; OutputFile->keep()→ vfsOutputFile::keep). That is filed as a follow-up.Built and rejected
better, 2 same, 2 worse (both the same-named-class floor). Rejected on mechanism: it hands one block's written type to a
sibling block's
auto t = ns::cast<Decoy>( y ), a precise edge to the wrong class (arm 48 is red on it).Vec<T>()constructor spelling). Tested on a scratchcomposition of fix(ingest): a receiver typed with template arguments recorded no type, or the wrong class #276
50b97b7b+ this change (never pushed). Reading that spelling moves 245 llvm-project sites: 170lost, 0 gained (779 lost without this change); rocksdb 0. The remaining losses are declaration conflicts, e.g.
auto *LI = cast<LoadInst>( … )beside anotherLI, which this change keeps on purpose. The floor stays.Gates
test/narrowcheck.shgains arms 44–51, with the fixture generated in-gate:t = ns::cast<Target>( y )--uses=Target.countpinsRed first:
fe28fd49: 44, 45, 46 (no edge), 49 (owner_candidates="2"), 50 (NO-CENSUS-ROW); 47a/47b/48/51 green.pick@lib/target.h:1);Results:
narrowcheckALL PASS (46 rows).qschemetripcheck,qextractionkeycheck,cacheidentitycheck,portablecachecheck,cachefuzzcheck: ALL PASS(cachefuzz's ASan sweeps skip locally: no ASan tree, by instruction).
xmllint --nooutclean; the same for this repo's own map.--quality-delta=$(git merge-base origin/main HEAD)..HEAD:gating="0"after 3 acks written through the binary(
--ack-only=buildFieldNarrowTables,classNameSet; the ledger diff is exactly three+ackrows). One minor verbosityrow is left.
python3 test/pargates.py . ./build/ripwire -j 6at 379d9fa:gates=639 pass=636 skip=3 fail=0. The skips areeditchecknotecheck, argvdiffcheck and g1freshcheck (no
asan/tree). The first run, on a binary built before the commit,failed
versioncheckalone (built_from=fe28fd49e+dirty): a true positive, fixed by rebuilding at HEAD, then the wholesuite was re-run.
Pins moved
kCacheVersion22 → 23,kParserVer99 → 104, and bothquality.hmirrors; the train assigns the final number.test/qschemetrip.hash72f930b8→f7ffb332..ripwire_quality_acks+3.Train notes
test/narrowcheck.sh: union arms 26–38 (fix(resolve): a call through an interface pointer landed on unrelated nested classes of the same name #268), 39–43 (fix(ingest): a receiver typed with template arguments recorded no type, or the wrong class #276) and 44–51 (this).src/ingest_binds.h: thectorNameNodecomment conflicts with fix(ingest): a receiver typed with template arguments recorded no type, or the wrong class #276's rewrite; keep fix(ingest): a receiver typed with template arguments recorded no type, or the wrong class #276's text and add this lane'sassignment sentence.
kParserVer/ qschemetrip: re-derive on the merged tree.🤖 Generated with Claude Code
Summary by CodeRabbit