Repository navigation
Conversation
… and the global spelling search is deleted The native route refused the fixed workload at prepare on ONE reference -- `SubstrateInputsOnly`, the bare variant in the `data live_tree_disposition` row -- with resolve_ambiguous_on_global_bare, naming no binder. Reading the slice from the top: v2's resolver ALREADY walks the ancestor chain and already refuses when two positions bind (docs/plans/namespace-resolution-design.md section 13's unique-on-chain, stricter than [basic.lookup.unqual]'s nearest-wins as PR #11924's departure note says). The chain was never the defect. What was missing is the mechanism section 13 says must land WITH OR BEFORE the strict flip: import->alias transmutation. Under NamespaceOnlyY imports were admitted nowhere, so every cross-module reference was off its own chain and fell through to symbol_index_global_unique_lookup -- "imports-deleted-first", which that section names as the thing never to do, with the global-bare tier standing in for it. Deleting the tier alone would have turned the workload's references unbound, not resolved. So the three land together, as one authority transition: 1. An import is transmuted into AliasBindingRow rows -- one per NAMED ITEM, never the whole module, because wholesale admission is the using-directive the cited model carries as an excluded form. The rows are captured at NORMALIZE, beside test_marker_capture and for the same reason: the module graft dissolves import decls (UnitDissolved), so that is the last moment they exist. They ride on NormalizedTree, the carrier that already carries one graft-erased fact. 2. The fill is two passes over the roots -- every root's declarations, then every root's binding rows. One pass made a binding row that named a not-yet-walked root bind nothing, so whether a cross-module reference resolved depended on FILE ORDER. That is removed by construction, not by sorting. 3. The global bare search is deleted as a RESOLUTION mechanism, with AmbiguousOnGlobalBare -- the one ambiguity class that could name no binder. The index's global_bare map survives as exactly what section 13 leaves it: the migration oracle. Identity at a binding position is the DECLARING PATH, not the node. The cheaper test -- treat a second writer as the same binding when its node is equal -- was measured merging two genuinely distinct binders, because `type NcrTwin = Bool` in two modules builds the same node: the ambiguity control passed as RESOLVED until this was fixed. Evidence, five arms on the real route (supplied source bytes, then production tokenize, parse, normalize and resolve via native_test_context_from_ingest): - the workload's own refusal at fixture scale -- a bare variant whose spelling a second module also uses -- resolves through its import; - a same-named pair in sibling scopes resolves by containment; - a genuine ambiguity (one name bound at one position by two imports) refuses naming BOTH binders, count asserted; - a name bound on no chain that the corpus spells twice is UNBOUND, not ambiguous; - permuting the ingest changes no verdict, compared at cause and binder-set grain. Mutation control RUN, not described: reinstating the global bare search reds an_unbound_name_the_corpus_spells_twice_stays_unbound and the re-homed arm in native_refusal_detail, and nothing else. Disposition of what is replaced: native_refusal_detail's global_bare_collision_row_names_its_lookup_class_without_fabricating_candidates asserted the old answer for a fixture whose question has changed. It is not retired -- it becomes a_bare_name_bound_on_no_chain_is_unbound_not_ambiguous and stays enrolled as the discriminator for the deletion (DESIGN section 4b(4)). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l runs on the transmuted-import route Review 69708 on #12009 found the declare-and-import collision still silent, and it was right. `bound_declarings` was written only by symbol_index_bind_at; the pass that writes a module's OWN declarations (symbol_index_insert) records no claimant, because a containment path names one place in the tree and cannot be claimed twice. That left a position a DECLARATION holds indistinguishable from a position nothing holds -- `prior` empty either way -- so for a module that declares `N` and also imports `N`, pass B saw an empty position and overwrote the local declaration with the import. Silently. Which is the pick this machinery exists to refuse. symbol_index_claimants_at reads the incumbent instead of registering every declaration as it is written: an occupied position with no recorded claimant was written by pass A, where the binding path IS the declaring path, so that path is the incumbent's name. Registering all of them up front would double the index for the one position in a thousand that a second claimant ever reaches. New arm, and its mutation control run: declaring_and_importing_one_name_refuses_naming_both refuses naming `v2.ncr_shadow.NcrTwin` and `v2.ncr_home_a.NcrTwin`, count asserted, and dropping the incumbent seed reds it and nothing else. Also from the same review: ~20 admit_normalized_tree call sites. Fixed -- and the absence now has a name, `no_import_bindings()`, beside test_marker_channel_empty() and saying the same kind of thing: this input carries no import decls, rather than `Empty` leaving a reader to guess whether the caller dropped something. Two reds this surfaced in v2.test.claim.name_resolve.test_code_reference_wall, both green on main and both mine: the serving module reached its PEER's declarations with no binding at all, because the corpus-wide bare-name search found the spelling. Those claims were asserting the test-code wall on a route that no longer exists. The subject now carries the two binding rows `import wall.peer { peer_leaf, shared_fixture }` becomes, which STRENGTHENS the wall rather than accommodating it: a transmuted import is the one route that could let a test-marked declaration past by arriving under a local name, and the RED now runs on exactly that route. 16/16 green. Also moved the LexicalUnbound annotation above its declaration -- DESIGN section 4c admits module-item grain only, and the annotation parser refused it inside the body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…w whose target is another row Review 69735 on #12009 found the order-independence claim larger than its evidence, and it was right. Splitting declarations from bindings orders a row whose target is a DECLARATION -- pass A writes every one in the corpus before any row is tried. It does NOT order a row whose target is another row's BINDING. Collecting rows per root and binding them as the fold walked still gave each row exactly one attempt, in root order, so a re-export chain resolved in one root order and silently not in the other, and the loser bound nothing: the same quiet absence the two-pass split was supposed to remove, surviving one level up. Not hypothetical in this corpus, and live in this PR's own diff: v2.std.collection imports `Absent`/`Present` from v2.std.optional and declares neither, and v2.compiler.normalize -- a file this PR edits -- imports `Absent`/`Present` from v2.std.collection. That row's target exists only after collection's own row has bound. So rows are now gathered from EVERY root as values before any is applied, and retried until no further row can bind. A row whose target is not yet present is DEFERRED rather than dropped. It terminates because the recursion is entered only when the deferred list got strictly shorter, so the measure decreases; rows still deferred at the end name targets nothing declares or binds, they bind nothing, and the reference that wanted them refuses at the REFERENCE site, which is where section 13 puts that refusal. Evidence, because the finding was precisely that the comment claimed a rung the executed evidence did not establish (DESIGN section 4b(1)). The permutation control's fixtures were all single-level -- ncr_home_a DECLARES NcrTwin -- so nothing discriminated the chained case. Added v2.ncr_relay (imports NcrTwin, declares nothing) and v2.ncr_chain (imports it from the relay), placed so the permutation SWAPS them: relay before chain in one ingest, after it in the other. Two arms, because neither alone is enough. a_re_export_chain_resolves_through_its_relay says the chain resolves at all -- "unchanged under permutation" would be satisfied by two matching UNBOUND refusals. reordering_the_files_changes_no_verdict says the two orders agree. Mutation control run: collapsing the fixed point to a single round reds reordering_the_files_changes_no_verdict and nothing else. The chain arm stays green under that mutation, because it reads the ingest whose order happens to work -- which is the whole reason both arms exist. Sweep: 18 resolve-adjacent suites, zero failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…gling declarations go Review 69760 on #12009, both findings verified against the code and fixed. ONE RULE MAY NOT HAVE TWO ANSWERS DECIDED BY HOW THE REFERENCE WAS SPELLED. symbol_index_bind_at records every claimant of a binding position, but `entries` still held whichever claimant was applied first, and only the LEXICAL collector consulted the claimant list. So try_resolve_qualified_name_node read that slot directly: a module that declares `N` and imports `N` refused as ambiguous when `N` was spelled bare, and resolved to a fill-order pick with NO diagnostic when the same position was spelled by its absolute path. The contest is now checked at the single read every consumer already goes through rather than at each door. A contested path reads as ABSENT, so no consumer can be handed a pick; the door that must say WHY asks symbol_index_absolute_candidates first and refuses naming both binders, and every other reader gets an absence and refuses at its own boundary. That split needed one more distinction than the first attempt had: the candidate collector is asking about STORAGE, not meaning, and reading it through the contest-aware answer handed it an absence for exactly the claimants it was collecting -- silently collapsing a two-binder refusal back to one. symbol_index_entry_at is the raw slot; symbol_index_lookup is what a reference is entitled to be told. The existing arm caught that regression, which is the second time these controls have caught a fail-open in their own fix. New arm a_qualified_reference_to_a_contested_position_refuses_naming_both, and its mutation control run: reinstating the raw slot read on the qualified route reds it and nothing else. It reaches the SAME two claimants as the bare-name arm by the other spelling the language admits, and the point is that the two now agree. DANGLING DECLARATIONS DELETED (DESIGN section 3c). symbol_index_fill_import_binding had no call site at all; symbol_index_fill_alias_binding and symbol_index_try_lexical_at were orphaned BY THIS DIFF when symbol_index_alias_rows_of and symbol_index_candidates_at replaced their callers. symbol_index_fill_binding_row became orphaned by the same deletion and goes with them, along with two imports that no longer resolve to a use. 8/8 in v2.test.claim.resolve.namespace_candidate_rule; four mutation controls now, each reddening only the arm that owns its claim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s not execute, and a stale import that is no longer inert Review 69789 on #12009, both findings verified by call-site count and fixed. symbol_index_fill_module_tree had no caller anywhere in the tree, and symbol_index_fill_module_bindings was called only from it -- new residue of exactly the class an earlier commit here is titled after, left behind when the fixed-point refactor moved the corpus-wide fill onto symbol_index_pending_of_source. The comment beside them was worse than the code: it named symbol_index_fill_module_tree as the door "a caller holding a NormalizedTree goes through", and no caller does. It now names the door that executes and says why single-root and whole-corpus are deliberately not symmetric -- a binding row's target may be another root's declaration, so binding ONE root in isolation can only ever see what that root itself declares. The stale `normalized_tree_roots_to_nodes` import in 03_name_resolve is dropped. The FUNCTION stays: it has consumers in six other modules, so only the import was stale. THE REVIEWER'S SECOND POINT IS RECORDED ON THE CARRIER, because it is a consequence of this change rather than a tidiness matter. Before the transmutation, an import block naming something the module never used was bookkeeping. Now it MINTS A BINDING at the importing position and is a claimant like any other, so a stale name colliding with something else bound there is an ambiguity the resolver refuses. That is the ruled model working -- section 13 makes an alias an ordinary binding node and calls an unused one lintable dead code -- but it moves stale imports from harmless to load-bearing, and an author deleting a use must now delete its import row with it. The note sits beside import_binding_rows_from_decl_node, which causes it. 8/8 controls unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
quick-bat-813 asked whether #12009 changes what symbol_index_global_unique_lookup returns for a bare variant name, because gunbc#12033's pattern classifier reads it. It does, it was my defect, and it is fixed here. symbol_index_bind_at wrote through symbol_index_insert, which calls symbol_index_track_global_bare. That census answers ONE question -- how many DECLARATIONS spell this leaf -- and a transmuted import is not a declaration; it is a second PATH to one that already exists. track_global_bare cannot tell the difference on its own, because its uniqueness test demands `existing_path == qualified_path && existing == resolved`, so an import carrying the IDENTICAL declaration node under a different path flips the leaf to Ambiguous. MEASURED on the emitted route before the fix, not reasoned: with v2.acp_home declaring `AcDisposition` and v2.acp_user importing it, global_bare[AcDisposition] read AMBIGUOUS although exactly one module declares it. The oracle answering "two declarations" about one. WHY IT IS NOT HOUSEKEEPING. The oracle has a live reader. #12033's resolve_pattern_atom_names_constructor asks global_unique_lookup whether a bare atom in a match arm names a constructor and treats Ambiguous as YES, so every leaf this polluted would have pushed a fresh arm BINDER toward being read as a constructor and refused -- and my change widens that population to every imported name in the corpus. Writing a spelling census from a binding is the global-spelling-search defect wearing a different hat, which is the one thing this package exists to remove. New arm a_transmuted_import_does_not_make_its_leaf_globally_ambiguous, and it discriminates in BOTH directions, which is why it names two symbols. `NcrDisp` is declared once and imported once, so UNIQUE can only survive if the binding stayed out of the census. `NcrTwin` is genuinely declared by two modules, so it must stay AMBIGUOUS -- a "fix" that simply stopped writing the census would show up here as a false UNIQUE. Mutation control run: restoring the insert reds the new arm and nothing else. 9/9. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review 69926, second finding. When symbol_index_fill_module_bindings and symbol_index_fill_module_tree were deleted as dangling, the deletion cut their "PASS B" annotation mid-sentence -- "...an `alias` decl is grafted into the tree as an ordinary" -- and spliced the remainder directly onto the next, unrelated "THE NODE-ONLY DOOR" block. So an annotation describing a function that no longer exists was sitting above a function it half-described. The fragment is removed; the NODE-ONLY DOOR annotation was already complete and correct on its own. Nothing about the surviving text needed changing: the two-pass story now lives on symbol_index_fill_module_roots, where the fixed point is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rop retires by its own trigger
Review 69926 found that this PR deletes the class native_route_live_pair_standing was
pinned to, orphaning an enrolled 4b(4) probe. The manager ruled the flip is gated on the
OBSERVATION, not on resolve -- flipping on resolve alone would be the rung inflation
4b(1) names -- so the observation was executed before this edit, not predicted by it.
EXECUTED, in this order, on the emitted route:
1. v2.native_lane_fixture.control RESOLVES. Real source bytes (control.dag,
live_tree.dag, logic.dag), not an isomorphic fixture.
2. Through the emitted binary's own native_test_eval_one:
native_lane_false_control = ReturnedFalse
native_lane_true_control = Passed
That is the first observation of the pair on ANY head. It could not be observed on
main 79e745b or on gunbc#11952, because the module both controls live in refused at
prepare with resolve_ambiguous_on_global_bare on the bare SubstrateInputsOnly.
3. Standing set to LivePairRequired here, in the same change, as the trigger requires.
Receipt: dag head 5aee537 plus the two emitter files from #12026
(origin/session/sunny-ibex-112 @ 0f0ee6f, cherry-picked -- merging that branch whole
drags in main's extdeps_numeric_base16 UInt8 gap and the emitted crate will not compile);
emitting gunbc sha256 682bfa1247cd3a04; emitted closure 190 files sha256 038f40828900249f;
probe sha256 1d5a6a98ebc5dac6. Command in the PR body.
IT FLIPS, IT DOES NOT RETIRE (DESIGN 4b(4)). The pair is a permanent regression control
from here: a route that stops discriminating false from true reds this clause. What the
enrolled-red form bought was tolerance of a refusal no lane change could move, and that
refusal is gone. gunbc.rung_drop.native_lane_live_pair_expected_red is Retired by its own
trigger and by nothing else, with the three conjuncts recorded on the row;
docs/design-rung-drops.md regenerated rather than hand-edited.
The leading annotation no longer describes the enrolled arm as current. It keeps WHY that
arm pins three axes, because that is how a future stall would be declared: an arm pinned
to stage, fatal reason AND lookup class cannot be satisfied by the subject getting worse
in a new way, and cannot outlive the condition it describes -- which is exactly how this
one failed when the class was deleted underneath it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ow the floor asked for TWO FIXES, and the first is the one worth reading. 1. REVIEW 69963: src/v2/compiler/03_resolve.dag called symbol_index_absolute_candidates at two sites with no import row for it. Under the seed that resolves; under THE RULE THIS PR LANDS it does not -- a bare cross-module name with no chain binding and no transmuted import row is LexicalUnbound, then SymbolIndexAtomUnbound. So the resolver module was, by its own new rule, unresolvable when v2 ingests v2: exactly the class this package exists to remove, reintroduced on a line the package added. The cause is worth recording because it is mechanical: the edit that should have added the row was a string replace with no assertion, and it matched nothing. A silent no-op in the one file where the consequence is a latent break in the corpus's own compiler. I then scanned every line this diff ADDS in production modules for the same class -- called identifiers absent from both the local declarations and the import block -- and this was the only real instance; the two remaining hits were false positives with no such call on any added line. Worth stating what the scan also found, because it is not mine to fix: a number of corpus modules carry NO import block at all and resolve today only because the seed searches globally. v2.test.claim.name_resolve.test_code_reference_wall is one. That is the population the namespace cut has to migrate, and it is what a corpus-wide census measures; it is not a regression this PR introduces. 2. THE FLOOR REFUSED THIS PR'S OWN WITNESS: nine identities at 941,902 eval steps against a 72,300 new-witness budget, cause=completed_over_cost_requirement. The harness I modelled this file on carries the answer in its own annotation -- STOP RECOMPUTING, DO NOT RAISE THE LINE -- and I followed the shape without completing it: ncr_outcomes_warm was a pure nullary producer but was never enrolled, so every claim rebuilt it, and a third front-end run (ncr_context) sat outside it entirely. The oracle readings now come from the context the warm producer already built, so the third run is gone, and the row is enrolled in v2.workflow.floor_pure_producer_share floor_cross_claim_pure_producers_warm with the arithmetic stated: the fill is two front ends over twelve modules -- two because the file-order claim's whole content is that the two orders agree, and a second front end is the only way to have two orders -- and the alternative is paying it nine times. 9/9, and the oracle arm's mutation control re-run after the restructure: restoring the census write still reds it and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The manager asked, on the strength of an independent specimen, whether an unimported
name refuses with a LOCATED cause naming the missing import or a generic not-found. I
measured: located at the reference occurrence, but GENERIC -- reason
resolve_reason_unbound_symbol, correction Unavailable { UserInputBoundary }. It named the
symbol and the site and said nothing about a missing binding.
That is a gap against this package's own brief, which asks that MISSING and competing
bindings both refuse with evidence. Competing had it -- lookup class plus both binders at
a DeclarationLocus -- and missing did not.
The specimen is why it matters. calm-koi-296, on gunbc#12035 and with no knowledge of
this work, found a production reference resolving with NOTHING importing it, confirmed by
a mutation that reds. Every module quietly relying on the corpus-wide search starts
refusing when the search is deleted, so that population is real and it gets whatever this
refusal says.
THE ORACLE IS THE RIGHT SOURCE FOR THIS AND THE WRONG SOURCE FOR RESOLUTION, which is the
distinction section 13 already draws when it keeps global_bare as a migration oracle after
deleting it as a mechanism. Asking the corpus "does this spelling exist anywhere" may not
CHOOSE a meaning -- that is the defect this package removes -- but it is exactly the right
question for explaining an absence. So an unbound reference now carries an advisory in
front of the fatal: resolve_unbound_name_is_declared_elsewhere at the declaring path when
the corpus declares the name exactly once, resolve_unbound_name_is_declared_in_several_modules
when more than one does, and NOTHING when no module declares it -- because there the honest
reading is a typo and pointing anywhere would be fabrication.
Two arms, and the second is what stops the first being satisfied wrongly:
- a_missing_binding_names_where_the_name_is_declared: v2.ncr_missing_import names
NcrDisp, declared exactly once, and the chain carries the declaring path.
- a_missing_binding_the_corpus_declares_twice_names_no_single_path: v2.ncr_unbound names
NcrTwin, which two modules declare, so the advisory says several and carries NO path.
Without it, an implementation that always pointed at whichever declaration it found
first would pass -- the spelling search readmitted as a diagnostic.
The fatal is unchanged in both: the advisory explains the absence, it does not change what
the refusal IS. 11/11.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…arm producer is a fn so the serve reaches it TWO FIXES. 1. REVIEW 69997: the census and the repair-input roster built their index through the node-only door -- whose own comment says it fills "no import bindings" -- while production fills from normalized_tree_roots_to_binding_sources. That was harmless while the corpus-wide spelling search was a resolution tier. THIS PR DELETES THE TIER and replaces it with exactly those bindings, so every cross-module reference production BINDS would have been reported unbound by the instrument. It is not an idle instrument. The roster's digest is what gunbc.namespace_cut_stage consumes as DenominatorRosterDigest, so the effect would have been to inflate the very denominator the namespace cut is defined by, with nothing red to say so. Fixed at the DOOR, not the call sites: both producers now take ModuleBindingSource, so a caller cannot hold roots and bindings separately and pair them by position. Of the three call sites, two already held NormalizedTrees and threw the bindings away one line before the call; the third builds Nodes by hand and now says so through module_binding_sources_without_imports -- named rather than a bare Empty, for the same reason no_import_bindings() is. This is the third defect in this package of one shape: a previously harmless approximation became load-bearing when the fallback was deleted. Node equality, the global-bare census write, and now the instrument's index. 2. THE FLOOR REFUSED AGAIN AND THE ENROLMENT WAS THE WRONG SHAPE. The warm row DID store -- [floor-phase] pure-producer-share-warm disposition=Stored cpu_ms=2370 provenance=built-by-preparation -- and the eleven claims still paid ~1,005,000 evaluator steps each. The store happened and nothing was served from it. The cause is the shape of the enrolled declaration. Both admitted precedents (census_probe_outcomes, ndp_resolutions) are nullary FNs that claims CALL; I enrolled a `data` row wrapping the producer and had the claims read the row. Enrolling `ncr_outcomes` and calling it is what makes the serve reach the consumer. Recorded on the roster entry with the measurement, because "it stored but was not served" is not a distinction the roster's prose made anywhere and the next author will hit it. 11/11 controls; the three instruments touched by (1) re-run at 7, 11 and 16 -- correcting the index changed none of their answers, which is the result that says the instruments were measuring reachable-but-equal populations rather than silently disagreeing already. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…xposed REVIEW 70024: ncr_verdicts_for had no call site -- a leftover of the pre-warm-producer shape that re-entered native_test_context_from_ingest per call, which is precisely what the warm row exists to avoid. Leaving it would have left a second route to the same fact for a later author to pick up. Deleted. THE FLOOR'S LAST BLOCKER, and it is not a regression. The warm-producer repair worked: the nine namespace_candidate_rule identities went from ~1,005,000 eval steps each to no blocking lines at all, with [floor-phase] pure-producer-share-warm producer=...namespace_candidate_rule.ncr_outcomes disposition=Stored cpu_ms=2044. Nine blockers to one. The one left is v2.test.manual.body_lowering_normalize_add.body_lowering_normalized_arrow_root_resolves at 115,051 steps against the 72,300 new-witness budget. It is not mine and its cost did not change. What changed is that it became VISIBLE: this PR alters the arity of admit_normalized_tree, that file calls it, so its identities read as changed and a withheld cost debt stopped being withheld. I CHECKED THE RELAYED EXPLANATION INSTEAD OF ACCEPTING IT. The standing reads withhold-overridden-for-changed-verdict and it was reported to me as the verdict having moved in my run. It did not: body_lowering_normalized_arrow_root_resolves returns true on origin/main and on this head, same interpreter, sources the only variable. So nothing about that claim's behaviour changed and there is nothing to revert. body_lowering_normalized_module() is a nullary pure producer called from NINETEEN sites in that file, never enrolled, each call paying one front end over a source literal. Its value is an Optional<NormalizedTree> -- root Node, marker channel, binding rows, no resolution context and no closure -- portable for the same reason ndp_resolutions is. Enrolled warm, with the measurement and the it-was-always-this-expensive reasoning on the entry. That is the repair this roster exists for. Re-hiding the debt would have been the alternative, and a cost that is only invisible while nobody touches the file is not a cost that has been dealt with. 11/11 controls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
briansrls
marked this pull request as ready for review
September 22, 2026 11:20
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 6bda570e74
ℹ️ 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".
Comment on lines
+260
to
+268
| match symbol_index_lookup(index: acc.index, qualified_path: p.row.target) { | ||
| Present { value: resolved } => | ||
| symbol_index_insert( | ||
| index: index, | ||
| qualified_path: qualified_name_snoc(qn: module_qn, segment: row.binding), | ||
| resolved: resolved | ||
| ) | ||
| Absent => index | ||
| BindRound { | ||
| index: symbol_index_bind_at( | ||
| index: acc.index, | ||
| binding_path: qualified_name_snoc(qn: p.module_qn, segment: p.row.binding), | ||
| declaring_path: p.row.target, | ||
| resolved: resolved | ||
| ), |
There was a problem hiding this comment.
When one module declares X and also imports a different X, pass A makes module.X temporarily readable before its import row marks that position contested. If a downstream module's import row is visited first, this lookup binds it to the temporary entry; the later contest never invalidates that dependent binding, and the downstream reference continues to resolve through the raw slot. Reversing the root order instead leaves the downstream row deferred, so resolution becomes file-order-dependent and can silently accept an ambiguous target. Bindings need to be recomputed or invalidated when a target becomes contested.
Useful? React with 👍 / 👎.
Comment on lines
+263
to
+267
| index: symbol_index_bind_at( | ||
| index: acc.index, | ||
| binding_path: qualified_name_snoc(qn: p.module_qn, segment: p.row.binding), | ||
| declaring_path: p.row.target, | ||
| resolved: resolved |
There was a problem hiding this comment.
For a re-export chain where home declares a test-marked T, relay imports it, and consumer imports relay.T, resolved is still the node declared at home.T, but this records relay.T as its declaring path. The resulting lexical hit passes relay.T to test_code_reach, whose index is keyed by the actual marked path home.T, so serving code can reference the test declaration without triggering resolve_reason_test_code_referenced. The binding must retain and propagate the ultimate declaration path rather than replacing it with each immediate alias target.
Useful? React with 👍 / 👎.
Contributor
|
Closing UNMERGED, same disposition as #12009. Do not resolve the conflict and push. Resolving it would regress main. This is the same branch at the same head ( Why the conflict must not be "resolved"
The specific instance that makes it a regression rather than a duplicate: this branch carries four occurrences of The work is on main and was verified there
Note for whatever opens these
— sent from neat-boar-16 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Auto-opened by session-dashboard for session
witty-cat-84.Pushing to
session/witty-cat-84advances this PR.Worker attestation
Before flipping this PR to ready for review, confirm each item:
npm test,cargo test) and the result.Closes #Ndirective.Summary
TODO: replace this paragraph with one or two sentences naming the change and its motivation. Reviewers read this first.
Test plan