fix(rust): fence a QN on the function's own cfg predicate, balanced - #2391
Open
BobbieBarker wants to merge 1 commit into
Open
BobbieBarker wants to merge 1 commit into
BobbieBarker wants to merge 1 commit into
Conversation
rust_cfg_qualified_name folds a function's `#[cfg(...)]` predicate into its qualified name so two mutually-exclusive twins get distinct QNs instead of one overwriting the other (DeusData#495). It read the attribute three ways that the QN it minted shows are wrong. The suffix was copied from the `cfg(` it found to the END of the decorator text. extract_decorators stores the bracketed form, so that appended the attribute's own `]` to every QN: `#[cfg(test)]` minted `add#cfg(test)]`. Across ripgrep 14.1.1, tokio 1.40.0 and rust-analyzer 2024-09-30 that is 332 of 335 fenced QNs. On a multi-line attribute the copied text also carried the newline, and cbm_defs_push cuts a qualified name at a line break by design, pinned by defs_push_cuts_multiline_names_and_rejects_js_literal_names. What survived was everything before the newline, so the suffix stopped mid-predicate: `thread_rng_n#cfg(any(`. Two twins whose predicates differ only past that newline then computed ONE qualified name and the second upsert overwrote the first, which is the loss this function exists to prevent. 10 tokio QNs are truncated this way. None of them happens to collide, because the visible prefixes stay distinct there; the collision is in the test below, which drops from two nodes to one without this commit. `strstr(decorators[i], "cfg(")` was also unanchored, and `cfg_attr` carries its own nested `cfg(`. In `#[cfg_attr(docsrs, doc(cfg(feature = "rt")))]` that inner predicate gates DOCUMENTATION, not compilation: the function is compiled unconditionally and has no twin to be told apart from, and it was fenced anyway as `yield_now#cfg(feature=rt)))]`. 7 tokio functions are fenced solely by a cfg_attr. rust_def_is_test, twenty lines above, already anchors on the bracketed path and says why; this was the one Rust decorator inspector that did not. The fix anchors on `#[cfg(` and copies the balanced span, stopping at the paren that closes it and dropping newlines with the other whitespace. A predicate longer than the 256-byte buffer still truncates, as before, and stays distinct over its first 255 characters. Measured by indexing the three corpora above with a binary built from 5df8b04 and with this one: corpus QNs changed stop being fenced nodes added or lost ripgrep 131 0 0 tokio 176 7 0 rust-analyzer 28 0 0 The 7 are the cfg_attr-only functions, which now carry their plain QN. Node counts are identical in all three, and tokio's multi-line predicates are reconstructed in full: `main#cfg(all(tokio_unstable,tokio_taskdump,target_os=linux,any(target_arch=aarch64,target_arch=x86,target_arch=x86_64)))`. This changes node identity for cfg-fenced Rust functions without moving CBM_INDEX_FORMAT_VERSION. A store written by an older build keeps the old QN for any Rust file that has not changed since, and picks up the new one when that file is next extracted, so such a store is mixed until then. I left the version alone deliberately: bumping it deletes and reindexes every project in every language for a Rust-only change affecting a few hundred nodes. Say the word if you would rather have the bump. tests/test_extraction.c: three tests, one per misreading, and the only coverage this function has in the gating suite. tests/repro/repro_issue495.c exercises it today but Makefile.cbm keeps that file out of `make test`, and it asserts only that two QNs differ and that one contains `not(`, never the string minted. Each of the three fails without this commit. Signed-off-by: Chad <4307099+BobbieBarker@users.noreply.github.com>
|
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. |
This branch has not been deployed
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.
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 #2390
Filed at your request in #2370. Looking at the
cfg_attrfencing turned up two more misreadings in the same function, and one of them re-opens #495, so all three are here as one claim: the fence should be the function's own cfg predicate, balanced, and nothing else. #2390 carries the reproducer and the evidence for each.The search now anchors on
#[cfg(rather than a barecfg(, so acfg_attrcarrying a nesteddoc(cfg(...))stops fencing a function that is compiled unconditionally. The copy then takes the balanced span, stopping at the paren that closescfg(and dropping newlines along with the other whitespace, which keeps the attribute's own]out of the QN and stops a multi-line predicate being cut at its first line break. A predicate longer than the 256-byte buffer still truncates, as before, and stays distinct over its first 255 characters.Measured
ripgrep 14.1.1
4649aa97, tokio 1.40.0ea6d652a, rust-analyzer 2024-09-30822644d9, indexed with a binary from5df8b044and with this branch:The 7 are the
cfg_attr-only functions, which now carry their plain QN. tokio's multi-line predicates come back whole:main#cfg(all(tokio_unstable,tokio_taskdump,target_os=linux,any(target_arch=aarch64,target_arch=x86,target_arch=x86_64))).Index format version
This changes node identity for cfg-fenced Rust functions and I did not move
CBM_INDEX_FORMAT_VERSION. A store written by an older build keeps the old QN for any Rust file that has not changed since, and takes the new one when that file is next extracted, so the store is mixed until then.I left the version alone because bumping it deletes and reindexes every project in every language, for a Rust-only change touching a few hundred nodes that heals itself per file. The Elixir arity work is the other case, where every Function QN moves at once and a bump is the only honest option. Happy to add it here if you would rather not carry a mixed store.
Tests
Three in
tests/test_extraction.c, one per misreading, and the first coverage this function has in the gating suite.tests/repro/repro_issue495.cexercises it today, butMakefile.cbmdeliberately keeps that file out ofmake testand it asserts only that two QNs differ and that one containsnot(, never the string minted. That is why a trailing]on every fenced QN went unnoticed.Without the commit:
381 passed, 3 failed, one failure per test. With it:384 passed.Full suite on this branch,
5df8b044plus this commit: 144 of 144 suites, 8,208 passed, 0 failed, 10 skipped. clang-format clean; the four violations my local clang-format-20 reports intests/test_extraction.care present on pristine5df8b044too and are not in the lines this PR adds.