fix(registry): reach a cfg-fenced function from its bare name - #2370
BobbieBarker wants to merge 1 commit into
Conversation
79a0c92 to
12de590
Compare
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @BobbieBarker. The split came out clean, and correcting which test is the real reproducer was genuinely helpful.
You're right that this defect is Rust's rather than Elixir's. We checked: rust_cfg_qualified_name is the only code in the tree that mints a # in a qualified name, so the fence gate touches nothing else today. The HCL and TOML dotted names aren't double-indexed either. We're happy to keep the #-fence gate instead of the language gate we first asked for. Two things before merge:
- Make the coupling explicit. Please add a short comment at the
#check inindex_under_name(or a debug assert) saying that#is the Rust cfg-twin fence. Then a future grammar that mints#can't slip into this path unnoticed. - A Rust corpus number, the one you already flagged as unmeasured. Cfg twins now share the bare-name bucket with the real function, so we'd like before/after figures on a sizeable Rust repo at a pinned commit (tokio or ripgrep, for example):
- CALLS count, and how many edges were added or removed;
- the largest bare-name bucket size;
- whether any lookup now hits
REG_MAX_CANDIDATESthat didn't before.
Once those are in, this is good to go. Thanks again for the careful work across both halves of #2310!
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
cbm_registry_add takes the symbol's `name`, discards it, and derives the
bare-name lookup key from the QN's last dot segment. That makes the QN's tail
load-bearing for the by-name index every language shares, and for one grammar
the tail is not a name anybody writes. rust_cfg_qualified_name mints a
`#[cfg(test)]` twin as "proj.lib.add#cfg(test)"; simple_name() has no '#'
handling, so that function is filed under the whole string "add#cfg(test)",
which no bare `add` callee can reach, leaving a test-only twin of a resolved
function absent from the by-name index entirely.
The caller always knows the name, because it is the argument. So the index
takes a second key from it, only where the derived key carries a fence:
const char *derived = simple_name(qualified_name);
index_under_name(r, derived, owned_qn);
if (name && name[0] && strchr(derived, '#') && strcmp(name, derived) != 0) {
index_under_name(r, name, owned_qn);
}
The gate keys on the fence rather than on the file's language, and that is the
part worth reviewing. It makes "unchanged for every other grammar" checkable:
the derived key is written unconditionally exactly as before, and every by-name
lookup keys through simple_name(), which splits on '.' and "::" only. A passed
name that differs from the derived key by anything other than a fence is
therefore a key no lookup can reach, so adding it would only widen the bucket
the scorer walks. A grammar that mints no '#' sees no new key at all.
Two of the rows a language gate would have had to cover turn out not to reach
this function. Registry membership is decided by cbm_label_is_registry_symbol
at all three production callers (pass_definitions.c, pass_parallel.c,
pipeline_incremental.c), and it admits neither "Module" nor "Section". So a
file's Module node never registers, and neither does a Markdown heading. What
does register, under label "Class", is the dotted-name direction:
grammar def->name QN tail
HCL resource.aws_instance.web web
TOML tool.poetry.dependencies dependencies
INI tool.isort isort
These are why the derived key stays unconditional. `instance =
aws_instance.web.id` reaches the block through the tail `web`, so re-keying the
index on the passed name would drop that edge outright. The fence gate leaves
all three byte-identical.
`name` is NULL or empty only for callers that have no symbol name to give;
those have the derived key and nothing else.
tests/test_registry.c: registry_indexes_by_passed_name_not_qn_tail resolves a
bare `add` against a cfg-fenced QN and is the reproducer, failing without this
commit. registry_indexes_a_dotted_name_under_its_tail_too pins the unconditional
derived key for the HCL and Module shapes.
tests/test_pipeline.c: pipeline_hcl_block_reference_resolves_to_its_block indexes
a two-resource Terraform file end to end and asserts the surviving USAGE edge.
The latter two pass without this commit and are guards: what would break them is
re-keying the by-name index, and an HCL block name is dotted where its QN tail is
not, so they pin the shape that regression would take.
A throwaway probe walked tests/grammar_cases.h (one fixture per grammar, all 162)
comparing def->name with simple_name(def->qualified_name), then targeted fixtures
for the dotted shapes above. HCL is the only fixture-level mismatch among
registry-eligible labels; TOML and INI mismatch on targeted fixtures. XML
namespaced elements (`ns:root`) do not mismatch, because ':' is not a QN
separator.
Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
12de590 to
208be7d
Compare
|
Thank you for checking the mint site independently. Both items are in. 1. The commentAdded at the check itself, since the const char *derived = simple_name(qualified_name);
index_under_name(r, derived, owned_qn);
/* '#' is a QN fence, and extract_defs.c's rust_cfg_qualified_name is the
* only thing in the tree that mints one today. A grammar that starts
* minting a '#' opts into this second key by doing so, whatever it means by
* the fence: its symbols become reachable under the passed name as well,
* and they share that name's bucket with everything else filed under it. */
if (name && name[0] && strchr(derived, '#') && strcmp(name, derived) != 0) {I did not add the debug assert. The held Elixir work in this same series mints an arity fence, 2. The Rust numbersThree corpora, pinned:
One binary from
The largest buckets are tokio's 434 bails all come from its one bucket over the cap, The removed edges54 across the three corpora. 53 are the same call site resolving to a fenced twin instead of a same-named function elsewhere, and 1 is lost outright. I read each group against the source: Corrected, 47 sites:
Regressed, 6 sites: 3 ripgrep Lost outright, 1 site: a JS file in rust-analyzer called All 6 regressions have one mechanism. Two ways to close it, if you want it closed in this PR: teach A defect I hit while measuring
The bucket-growth figure I flagged as unmeasured in #2369 and in this PR's description is now measured, so I have replaced that disclosure in both with the result. |
This PR was written by an AI agent working on my behalf. I certify the DCO sign-off on the commit and answer for the change.
Fixes #2369
Split out of #2310 at your request. This is the registry half, and it stands alone: all three files it touches are byte-identical between
origin/mainand the commit's parent, so it cherry-picks onto currentmainwith no dependency on the Elixir work.One correction before you review it. You named
pipeline_hcl_block_reference_resolves_to_its_blockas the separate defect. That test passes without the commit and is a guard. The reproducer isregistry_indexes_by_passed_name_not_qn_tail, and the defect is Rust:rust_cfg_qualified_namemints a#[cfg(test)]twin asproj.lib.add#cfg(test),simple_name()has no#handling, so the function is filed under that whole string and no bareaddcallee reaches it. Verified both directions on this branch, plain build, darwin arm64:The two guards pass in both columns.
On gating this to Elixir
You asked for
cbm_registry_add's dual indexing to be gated on the file's language. I did not do that, because this change is not Elixir's: an Elixir gate deletes the reproducer above and leaves Rust broken. What I did instead should give you the contract you asked for by a shorter route.The derived key is written unconditionally, exactly as before. The second key is taken from the passed
nameonly when the derived key carries a#:That makes "unchanged for every other grammar" something you can check in the code, and it does not depend on my having indexed a corpus in the language. Every by-name lookup keys through
simple_name(), which splits on.and::only. So a passed name differing from the derived key by anything other than a fence is a key no lookup can reach, and a grammar minting no#gets no new key at all. If you would still rather have the language enum, say so and I will thread it, but it will need aCBMLanguageparameter oncbm_registry_addand a carve-out to keep Rust.Two claims from the original commit message I had to withdraw
Registry membership is decided by
cbm_label_is_registry_symbolat all three production callers, and it admits neitherModulenorSection. So the "all 162 grammars" Module-node row I led with, and the Markdown heading row, never reach this function. That removes the breadth claim the original message rested on. What does register, under labelClass, is HCL, TOML and INI. They are why the derived key stays unconditional.Measured: inert on a corpus that does not carry the defect
I indexed a 970-file Elixir tree twice, once with a binary carrying only #2371 and once with #2371 plus this commit, against
origin/mainat64c23fab. Every metric is identical: Function nodes21,941, CALLS50,230, USAGE31,765, WRITES11,207, one-line functions1,260. The second key never fires there, because nothing in that tree mints a#. The same corpus indexes deterministically, so that match is an equality: indexing pristine main twice gives identical counts on every metric.None of it is evidence about Rust, which is the language the defect is in, and where the extra key does fire.
The cost of the second key
The second key puts a cfg twin into the same bare-name bucket as the real function, which costs bucket width and can change which candidate wins. Both are now measured on three pinned Rust corpora, ripgrep 14.1.1, tokio 1.40.0 and rust-analyzer 2024-09-30: the largest bucket is unchanged on all three (
106,265,625), no bucket crossesREG_MAX_CANDIDATES, and the count of lookups that bail on the cap is identical (0,434,203). CALLS rise by31,57and14. 54 call sites resolve somewhere else: 47 onto the function the source actually calls, 6 onto a#[test]function that outscores the real callee, and 1 onto nothing. The full measurement, including why those 6 regress, is in the review thread below.clang-format 20.1.7 clean. cppcheck 2.22 reports nothing in the three files this PR touches. It does report
knownConditionTrueFalsefindings elsewhere, and it reports them on pristinemainat64c23fabas well, so I read those as a difference from the version CI pins. clang-tidy is not installed on this host.