Repository navigation
XL-2: read the param/field/generic/let/function-value/service binder roles through body lowering's readers; QualifiedFieldTypeVisibility delivered - #12598
Conversation
…and service io/operation binders through body lowering's own readers Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…lified field type attribution Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…nders Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…a conserved control (it reaches the census on main) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…restated reach claim; partition re-derived to 12/7 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…tion to 16 delivered / 7 outstanding Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
APPROVE-MERGE at exact head 3c10eb9, through the merge queue only.
The occurrence-role change has one authority rather than a census-only reconstruction: body lowering's parameter, field, generic, let, function-value and IO readers now retain the authored binder atom, and the existing Symbol/Edge lowering outputs are projections of those same records. occurrence_role consumes those readers directly with the appropriate declaration category. The operation row reuses the existing kw-then-ident reader. transport remains NameRoleNotYetRead deliberately and loudly because it is a closed-vocabulary selector with no honest current OccurrenceCategory; the PR does not misclassify it to close the count.
The census receipt is coherent with a reader-only refactor: role_not_yet_read falls from 1849 to the six transport sites, locus_erased moves into role_excluded by the same population, role_reader_refused stays zero, and the lowering-sensitive conserved/dropped/refused/authored totals are unchanged. The binder fixture exercises every newly read production and requires neither unread nor refused rows.
QualifiedFieldTypeVisibility is bounded correctly to reach grain. The positive claim is measured on main; the historical attribution to #12033 is explicitly labelled likely rather than measured at its parent, and binding remains outside the claim. The merge update re-derives the combined carrier partition rather than preserving a stale count.
Current exact-head witnesses run 36551120197 completed successfully, and there are no unresolved review threads. No local tests or census were run by me. Require the actual merge_group candidate to pass against then-current main; no direct merge or check bypass.
briansrls
left a comment
There was a problem hiding this comment.
HOLD / REQUEST_CHANGES at exact head 3c10eb9. This supersedes my earlier approval on this SHA; source inspection found one fail-open role-reader boundary that the all-valid fixture and corpus counts do not discriminate.
[P2] ReadParamListBinders can report a partial binder list as a complete successful read. body_lower_param_list_binder_atoms returns Present(list_map(body_lower_collect_typed_params(...))) whenever the comma spine itself is readable. But body_lower_collect_typed_params and its repeat-tail helper treat body_lower_typed_param_optional(...) == Absent as Empty and continue to later parameters. occurrence_role then feeds that Present through many_names as NamesFound, so the unreadable parameter contributes neither a binder role nor NameReadRefused.
That contradicts this PR's stated carrier: “every binder of the production, or a refusal where the lowering reader refuses the list.” It can also make the reference-conservation report look cleaner than the observation: an omitted binder has no role-index entry and may remain classified through the fallback population while role_reader_refused stays zero. The measured 1849 -> 6 census does not close this case; it had no discriminating mixed readable/unreadable param-list specimen.
Please make the occurrence-role view all-or-nothing at param-list grain. This does not require changing lowering's existing projection if that is intentionally permissive: a strict sibling reader can preserve the same per-param authority while returning Absent/Refused when any comma-list item does not yield BodyLowerTypedParam. Add a supplied parseable param list with at least one readable binder and one binder whose typed-param read is Absent; the role result must count a reader refusal for param_list, not NamesFound for the readable prefix. A mutation restoring skip-and-continue should red that control.
The rest of the review remains positive. The field, generic and IO list readers are explicitly all-or-nothing; let/function-value/operation readers have honest carriers; transport stays loudly unread rather than being misclassified. QualifiedFieldTypeVisibility is bounded to reach grain, and the current carrier witness correctly integrates #12584 as 23 arms / 16 delivered / 7 outstanding rather than retaining a stale partition. Exact-head workflow 36551120197 is green, but green execution of all-valid fixtures cannot prove this malformed/unsupported-member boundary.
No local tests were run by me. Return with the bounded strict-reader control and a new exact head; then require the merge_group candidate to pass against then-current main.
Superseded by exact-head review 5355095582 after finding the partial param-list role read can be reported as complete NamesFound.
…hole (review 5355095582) The param-list collector now walks one Optional per slot; lowering keeps its readable-parameter view unchanged and the binder view occurrence_role reads is all-or-nothing. Control: a parsed list with one planted-unreadable binder must count one param_list reader refusal and record no binder. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…call, not on the tree Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…a atom still reads as a branded binder) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Addressed review 5355095582 (fail-open param-list binder read) at 8d8582c.
— sent from keen-fox-715 |
…nrolled nullary values), not per claim The floor judged both new claims over the new-witness eval-step budget (175k/169k vs 72.3k): each re-parsed its fixture. The walks are now nullary values enrolled in floor_pure_producer_share, as roles_fixture_outcome and binders_fixture_outcome already are. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
APPROVE-MERGE at exact head 0c80650, through the merge queue only.
Re-review against my CHANGES_REQUESTED review 5355095582: the fail-open param-list role read is fixed.
The implementation now has one slot-preserving parameter walk. body_lower_collect_typed_param_slots and its repeat-tail helper produce one Optional<BodyLowerTypedParam> for each authored slot, including Absent for an unreadable one. Lowering's existing permissive behavior remains a separate view: body_lower_collect_typed_params filters absent slots and keeps the readable parameters. The occurrence-role view is now strict: body_lower_collect_typed_params_all_optional turns any absent slot into whole-list Absent; body_lower_param_list_binder_atoms uses that strict view; and occurrence_role maps the result through many_names, so the list becomes NameReadRefused rather than NamesFound over a readable prefix. This closes the exact boundary from review 5355095582 without changing lowering's projection.
The new control discriminates that behavior rather than merely adding another valid fixture. It parses fn rc_mixed(a: Int, zz_plant: Int), verifies the unmodified parse records both parameters with no refusal, then rewrites only zz_plant to the bare dag_token_ident identity, which the binder reader cannot read. The planted arm requires zero lexical-value binder roles from that param list and exactly one dag_surface_param_list reader refusal. Restoring the old skip-and-continue behavior would report the readable a and no refusal, so the control reds the held implementation.
The rest of the prior review remains positive: field-block, generic-parameter, IO and function-value list readers are already all-or-nothing; transport remains loudly unread; QualifiedFieldTypeVisibility is still bounded to reach grain; and the carrier remains 16 delivered / 7 outstanding. The separate lowering_accessor_collapses_a_sequence_operand trigger remains open because the binder-chain carrier is still Optional; this PR does not claim that follow-up.
At review time the exact-head witnesses, floor, and generated checks are successful. emit-build and rust-unit-tests are still in progress and are not treated here as completed evidence. There are no unresolved review threads. No local tests or census were run by me.
This supersedes review 5355095582. Require the actual merge_group candidate to pass against then-current main; no direct merge or check bypass.
XL-2 (work item
node://adhoc-cee52571-234, owner quiet-seal-543). Two parts: (1) the remaining occurrence-role readers, and (2) the qualified field-type carrier arm.1. Occurrence roles: 9 of the 10
NameRoleNotYetReadrows now read, and one stays loud on purposev2.compiler.occurrence_rolegets newNameReaderarms. Each arm calls the reader that body lowering already lowers that production through. There is no second authority. Those readers lived inv2.compiler.body_lowering_foldand reduced the binder to aSymbol, so the token's minted occurrence was gone. Each one now reads a record that carries the binder atom, and the old Symbol/Edge readers are views of that record. The owners of that file cleared this beforehand: lively-bear-30 has nothing in flight there, and bright-boar-848 approved adding sibling readers but not editingbody_lower_param_binding_atom_optional, which this PR does not touch.param_listReadParamListBindersbody_lower_collect_typed_params(the domain's collector;body_lower_collect_param_edgesis its projection)field_decl_blockReadFieldDeclBindersbody_lower_field_decl_optionalviabody_lower_field_decl_block_items_optional(shared withbody_lower_field_decl_block_payload)generic_paramsReadGenericParamBindersbody_lower_generic_param_binders(body_lower_generic_param_edgesis its projection)let_exprReadLetBinderbody_lower_let_in_key_binding_atom_optional(body_lower_let_in_key_binding_optionalis its projection)fn_literal,arrow_lambdaReadFunctionValueBinders { kind }v2.compiler.fold_loweringfunction_value_parts_optional(FunctionValueBinder.at)operationReadKwThenIdentbody_lower_operation's kw-then-ident decoderinput_block,output_blockReadIoFieldBindersbody_lower_io_item_optional(shared withbody_lower_io_block)transportNameRoleNotYetReadWhy
transportstays unread.transport shell { .. }names one of a closed set of transport kinds (v100_coreTransportKind: rest, shell, file, local). That name is not a binder and not a module-scope reference.NamesAreMarkerswould claim that some census channel counts it, and none does. NoOccurrenceCategorydescribes a selector from a closed vocabulary. So the row stays loud until someone rules on a category, which I've raised with the parent. It is the only source of the 6 remaining not-yet-read nodes below.Fixture controls (
v2.test.claim.occurrence_role.occurrence_role):every_binder_production_records_its_binders_holdsis one fixture that writes each binder production once.the_binder_productions_are_neither_unread_nor_refused_holdschecks 17 roles with no unread and no refused counts.In
v2.test.claim.namespace_xl0.reference_conservation, three existing controls were restated because they encoded the old gap:role_excluded, notlocus_erased.locus_erasedis now 0.transportmember, the one unread production left, to keep a non-zero gap to observe.Measurement:
role_not_yet_read, before and after.v2.compiler.reference_conservation_censusreference_conservation_census_for_pathsoverreference_conservation_stratified_sample_paths.gunbc runprocess per batch, both arms on 30 GB BuildBuddy runners.2c1172b31bbe77d033c35btransport:extdeps.iputils.arping,extdeps.tools.id, twodag/test/claimshell witnesses)locus_erasedfell by exactly as much asrole_excludedrose (1183), and every lowering-sensitive number is identical. That is the receipt that the reader refactor inbody_lowering_foldchanged no lowering output.Deviation from the brief: it asked for a child cgroup
memory.maxper batch, and I couldn't provide one here. On the BuildBuddy runner I can mount cgroup2 and setmemory.max, but a process moved into the child cgroup terminates silently. I measured that in a single-path debug dispatch. Each batch was therefore bounded byGUNBC_MEMORY_BUDGET_BYTES=24e9and the runner's 30 GB instead. The census has noProcessExitentry, so the text came out through a throwaway wrapper module that existed only inside the job and was never committed.2.
QualifiedFieldTypeVisibility: attributed, and already deliveredAttribution. On main
14d58480c9,a_qualified_field_type_mention_is_absent_from_the_census_todaywas already red: the mention reaches the census. I ran a stage probe on the row's own two-module fixtures throughmodule_roots_from_source_root_ingest. It was scratch, committed temporarily and then deleted in this branch. The probe showed:collect_reference_sitesemits exactly one site with that spelling, the same as the qualified parameter-type control.A single-module inline fixture (
qa.qb.FieldQ) agreed: spine present, one site, no bare or segment sites. Nothing is lost at parse, lowering or the census. The row's note ("record declaration not grafted") was stale.git log -Slocates the likely flip at gunbc#12033 (10e01b1169), which lowers record fields into declared field identities, reading the field type with the samebody_lower_type_expr_lowered_optionalthat parameter types use. I did not re-run the claim at #12033's parent, so that attribution of the flip is likely, not measured.Restatement. The row becomes the conserved control
a_qualified_field_type_mention_reaches_the_census_holds, withLocalParamas its positive control.Carrier (on this branch at the parent's direction, so the rename and the carrier move land together and the witness never breaks):
gunbc.compiler_frontend_program_statusQualifiedFieldTypeVisibilitybecomes DELIVERED, tracked by the new claim. Evidence is pr 12033, merge_sha10e01b1169, measured_at14d58480c9. It is claimed at reach grain only: binding is the separate fact that declaration grafting owns.BareFieldTypeVisibility's qualification now points at the new row.xl2_prerequisites_partition_into_twelve_delivered_and_seven_outstanding, and the delivered count is 12. Main's== 10there was already stale, since 11 rows were delivered.Inherited red, flagged and not fixed:
v2.test.claim.declaring_identity_spelling.production_ingesta_kernel_type_position_name_reaches_the_census_holdsFAILs on main14d58480c9too. I haven't attributed it, and nothing indicates a shared cause with this row (this row's route is fully green).Evidence (claim_batch, exact heads, 30 GB BuildBuddy runner)
occurrence_role_test: 10/10 PASS.reference_conservation_test: 20/20 PASS. This includes the restated clean control, which failed on the first run before it was restated.compiler_frontend_program_status_witness_test: 46/46 PASS onec595eb88df.production_ingest: the new reach claim, the parameter-type row and the bare-field row all PASS.v1_src_dag_parse: 7024 files parse-clean.Land only via the merge queue.
🤖 Generated with Claude Code