Skip to content

fix(registry): reach a cfg-fenced function from its bare name - #2370

Open
BobbieBarker wants to merge 1 commit into
DeusData:mainfrom
BobbieBarker:fix/registry-cfg-fence-bare-name
Open

BobbieBarker wants to merge 1 commit into
DeusData:mainfrom
BobbieBarker:fix/registry-cfg-fence-bare-name

Conversation

@BobbieBarker

@BobbieBarker BobbieBarker commented Sep 26, 2026 •

Copy link
Copy Markdown

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/main and the commit's parent, so it cherry-picks onto current main with no dependency on the Elixir work.

One correction before you review it. You named pipeline_hcl_block_reference_resolves_to_its_block as the separate defect. That test passes without the commit and is a guard. The reproducer is registry_indexes_by_passed_name_not_qn_tail, and the defect is Rust: rust_cfg_qualified_name mints a #[cfg(test)] twin as proj.lib.add#cfg(test), simple_name() has no # handling, so the function is filed under that whole string and no bare add callee reaches it. Verified both directions on this branch, plain build, darwin arm64:

with the fix:     359 passed, 0 failed        (registry + pipeline suites)
main's registry.c: 69 passed, 1 failed
  registry_indexes_by_passed_name_not_qn_tail  FAIL tests/test_registry.c:338:
    "(null)" != "proj.lib.add#cfg(test)"

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 name only when the derived key carries a #:

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);
}

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 a CBMLanguage parameter on cbm_registry_add and 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_symbol at all three production callers, and it admits neither Module nor Section. 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 label Class, 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/main at 64c23fab. Every metric is identical: Function nodes 21,941, CALLS 50,230, USAGE 31,765, WRITES 11,207, one-line functions 1,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 crosses REG_MAX_CANDIDATES, and the count of lookups that bail on the cap is identical (0, 434, 203). CALLS rise by 31, 57 and 14. 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 knownConditionTrueFalse findings elsewhere, and it reports them on pristine main at 64c23fab as well, so I read those as a difference from the version CI pins. clang-tidy is not installed on this host.

@DeusData DeusData left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Make the coupling explicit. Please add a short comment at the # check in index_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.
  2. 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_CANDIDATES that didn't before.

Once those are in, this is good to go. Thanks again for the careful work across both halves of #2310!

@github-actions

Copy link
Copy Markdown

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. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

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>
@BobbieBarker

BobbieBarker commented Sep 27, 2026 •

Copy link
Copy Markdown
Author

Thank you for checking the mint site independently. Both items are in.

1. The comment

Added at the check itself, since the # test is at cbm_registry_add's call to index_under_name rather than inside it:

    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, Mod.fetch#3, so an assert that # means cfg would be false as soon as that lands. The comment states the coupling without pinning what the fence means. If you want something that fails rather than documents, the enforceable version is a lint check enumerating the mint sites, in the style of the language-count contract; I will add it here if you want it.

2. The Rust numbers

Three corpora, pinned:

corpus pin .rs files
ripgrep 14.1.1 4649aa9700619f94cf9c66876e9549d83420e16c 98
tokio 1.40.0 ea6d652a102dee3f22b490db70545b7f66a23fb7 715
rust-analyzer 2024-09-30 822644d97d7f64e1bdff25b1d636e366a29facc4 1,298

One binary from 64c23fab and one from this branch, each carrying a throwaway patch that dumps the whole by_name index at cbm_registry_free and counts both REG_MAX_CANDIDATES bail sites. Determinism control: tokio indexed twice with the same binary, zero edges differ on any type.

ripgrep tokio rust-analyzer
registered QNs 4,069 8,474 25,490
QNs with a fenced tail 132 176 30
second keys added 131 176 28
by-name keys 1,927 → 2,046 4,124 → 4,269 16,600 → 16,617
largest bucket 106 → 106 265 → 265 625 → 625
buckets over 256 0 → 0 1 → 1 2 → 2
buckets crossing 256 because of this change 0 0 0
REG_MAX_CANDIDATES bails 0 → 0 434 → 434 203 → 203
CALLS 4,337 → 4,368 14,462 → 14,519 59,066 → 59,080
CALLS added 37 102 17
CALLS removed 6 45 3

The largest buckets are update at 106, new at 265 and workspace at 625, and none of those names is cfg-twinned, so none of them moves. Existing buckets that grow at all: 3, 14 and 4, the largest of them reaching 6, 31 and 118. Every other second key opens a new bucket holding one to three entries, because a cfg twin's bare name is usually a name nothing else in the crate defines.

tokio's 434 bails all come from its one bucket over the cap, new at 265, since the bail fires nowhere else. They are there before this change too.

The removed edges

54 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:

  • 40, tokio yield_now. benches/rt_multi_threaded.rs:80 writes tokio::task::yield_now().await, which was landing on src/loom/std/mod.rs:122, a pub(crate) fn yield_now inside mod thread, and now lands on src/task/yield_now.rs:39.
  • 4, tokio thread_rng_n. tests/rt_basic.rs:404 calls tokio::macros::support::thread_rng_n, which was landing in a different crate, tokio-stream/src/stream_map.rs:815, and now lands on src/runtime/context.rs:129.
  • 2, ripgrep file_name. crates/ignore/src/types.rs:94 imports crate::pathutil::file_name and calls it at 273, where it was landing on walk::DirEntry::file_name and now lands on pathutil::file_name.
  • 1, rust-analyzer run. proc-macro-srv-cli/src/main.rs:26 calls run(), defined twice in that same file under opposing cfgs, and had been landing on proc-macro-api's ProcMacroProcessSrv::run in another crate.

Regressed, 6 sites: 3 ripgrep same_file_system, 1 ripgrep error, 1 tokio normal_or_miri, 1 rust-analyzer ProcMacros::expand. The clearest is the first: crates/core/flags/hiargs.rs:885 calls .same_file_system(...) on a WalkBuilder, which is the method at walk.rs:863, and it now resolves to walk.rs:2240, a #[test] #[cfg(target_os = "linux")] fn same_file_system().

Lost outright, 1 site: a JS file in rust-analyzer called run and resolved to editors/code/src/commands.run; the bucket grew from 34 to 36 and it now resolves to nothing.

All 6 regressions have one mechanism. is_test_qn scans the QN for test, mock, stub, spec, fake or Fixture, so REG_TEST_PENALTY applies only when the QN spells one of those. The issue's own example, add#cfg(test), is penalized. same_file_system#cfg(target_os=linux)] is not: its QN is the file path plus the name, and Rust's inline #[cfg(test)] mod tests contributes no path segment. That is pre-existing: a #[test] function carrying no cfg attribute of its own is unfenced and has always competed in the bare-name bucket unpenalized. The fence adds the cfg-decorated subset to that population.

Two ways to close it, if you want it closed in this PR: teach is_test_qn that a #cfg(test) fence is a test QN, which covers the issue's case but not #[test] beside a non-test cfg; or pass the extractor's is_test bit into cbm_registry_add and score on it, which changes the signature. I will do either.

A defect I hit while measuring

rust_cfg_qualified_name looks for the substring cfg( anywhere in a decorator, so #[cfg_attr(docsrs, doc(cfg(feature = "rt")))] at tokio/src/task/yield_now.rs:38 fences a function that is not conditionally compiled at all, as yield_now#cfg(feature=rt)))], unbalanced parens and trailing bracket included. tokio carries 123 such attributes against its 176 fenced QNs, so much of what this change reaches there was never conditionally compiled. ripgrep and rust-analyzer use none. The defect is pre-existing and independent of this change. For those functions the second key is plainly right, since they should not have been keyed under a fence in the first place. I can file it separately.

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 branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A Rust #[cfg(test)] function is unreachable from its bare name in the by-name index

2 participants