feat(extract): discover Blazor .razor components and emit Route nodes for @page - #1824
Conversation
|
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. |
|
Thank you for the measured Blazor coverage and for testing both full and incremental route materialization. Mapping a mixed markup file to C# and extracting directives from raw source changes language and route semantics, so we need more time to review the precision boundary carefully. The contribution queue is quite full, but we will return with grounded feedback as soon as possible. |
|
On the scope question you raised: go ahead. Thank you for asking rather than assuming — that is the right instinct, and it is why the answer is easy. The design-discussion requirement exists to stop direction changes arriving unreviewed. This one arrives as the discussion: a stated problem, a measured paired control, an explicit list of what it does not do, and a named alternative you rejected. Sending it back to the issue would ask you to rewrite as prose what you have already written as evidence. #1667 sat at What made this straightforwardThe paired control is the strongest form of this measurement. 4555 → 4566 nodes where +11 is exactly the 11 And you corrected your own numbers rather than shipping them. Rebasing after discovering the base was 129 commits behind, re-measuring, and saying so in the description is the opposite of the usual failure here, which is a benchmark quietly measured against something else. The "what this does not claim" section is the part I would keep in a template. And you defused a real trap. I checked this on Attaching the route to the module def, with the reasoning that a Two thingsPlease rebase — this is One interaction to know about, not to fix here. Indexing On your lint analysis: |
…eusData#963) The gate failed a pull request on the state of the report, not on the change the pull request made. Check 4 compared parse_partial_count against a number checked into scripts/ci/parse-partial-baseline.txt, and checks 1-3 asserted zero findings outright. Any of the four could go red for something main did. That is not theoretical. DeusData#1972 was this exact thing: main gained src/daemon/runtime.c, the count went 58 -> 59 on its own, and the number had to be raised by hand. DeusData#1824 will do it again and larger. Blazor .razor files map to C#, their markup lands in ERROR regions by design, and the count rises by roughly the repo's .razor count. A coverage improvement would read as a gate failure on an unrelated branch, and the person who hit it would have no way to tell that from a real regression. The gate now resolves the base commit, checks it out into a temporary worktree, and indexes both trees with the same binary. All four checks compare the two: 1. a whole-file parse failure fails only when it is new at head 2. a "+N" clipping marker fails only when it is new at head 3. a range over 25% of its file fails only when the file was within the share at the base 4. the flagged-file count fails only when it is above the base's parse-partial-baseline.txt stops being a gate. The script still prints the recorded number so a reader can see the drift, and says plainly that nothing fails on it. Nobody has to raise that number again. What this cannot see: both trees are indexed with the same binary, so a branch that changes the extractor itself moves the base side and the head side together and this gate will not fail on it. Catching that needs the base commit's own binary, which means a second full build -- about twelve minutes against the twenty-six seconds the whole gate step takes. Two things still cover it: the FLOOR asserted in tests/test_index_resilience.c stops the signal being switched off, and the absolute counts for both sides now print on every run, so a jump is visible in the log even when it does not fail. The script header and the scripts/ci/README.md row both say so. tests/test_coverage_gate_contract.sh pins the behaviour. It drives the production script with a fake binary that prints canned JSON, so no seam is added to the script itself and no indexing happens. Fifteen cases: each of the four findings present at both sides (pass) and new at head (fail), the count equal to, below and above the base, the recorded number not gating, both sides printing, a clipped file list still stopping the run outright, the allowlist skipping a path, and an unresolvable base commit stopping the run. Verified by reverting each of the four comparisons one at a time and confirming the matching "present at both sides" case goes red, then restoring. A real run against a built binary passes with both sides reported, and takes 45s for two indexes. pr.yml passes COVERAGE_GATE_BASE_SHA so the gate uses the commit GitHub itself used to build the merge, rather than falling back to the first parent of HEAD. Refs DeusData#963, DeusData#1972 Signed-off-by: Joshua Richter <jrichter5781@gmail.com>
A .razor file had no entry in EXT_TABLE, so discovery skipped it entirely and a Blazor application produced no graph nodes for any of its components. The only way in was an undocumented extra_extensions entry in a per-project .codebase-memory.json. Map .razor to CBM_LANG_CSHARP. This is best-effort by design: the C# grammar recovers the @code block, while the surrounding markup lands in ERROR regions and is reported through parse_partial. That is strictly more than the Module-and-imports the other markup-hosted languages (Vue, Svelte, Astro) extract today, and it needs no new grammar. Covered by tests/test_language.c:lang_ext_razor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: The1nk <23087600+The1nk@users.noreply.github.com>
get_architecture's routes aspect returned nothing for a Blazor application: the entry surface of the app was invisible in the graph even though every @page directive names one. @page lives in markup above the @code block, which tree-sitter's C# grammar never parses, so there is no AST node to read it from. Scan the raw source for it instead. The match is deliberately strict — the directive must be the first token on its line, followed by whitespace and a double-quoted path beginning with '/' — so @pageSize and prose mentions cannot match. The route is attached to the file's module def. A .razor component's class is implicit, so there is no class node to carry it, and the module QN already is the component's identity. insert_def_into_gbuf is label-agnostic and creates Route + HANDLES from route_path. pass_route_nodes.c's ensure_decorator_routes gains "Module" for the same reason. Extraction covers the full-index path on its own; this backstop is what runs on an incremental re-index, so without it a component's Route would appear on a full index and vanish the next time that one file changed. Its loop bound now derives from the labels array rather than borrowing the unrelated RN_STRIP_PASSES, so adding a label cannot silently skip it. Verified no double-creation when both paths can fire. Measured on a real Blazor application (36 .razor files): routes went from 0 to 11, with 11 matching HANDLES edges, and no change to the 363 Class / 1775 Method / 3805 CALLS already extracted from its C#. Covered by two new tests in tests/test_extraction.c. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: The1nk <23087600+The1nk@users.noreply.github.com>
7b9b459 to
fd73c34
Compare
|
Approved on merit, 34/34 green and clean. One consequence I want on the record before it lands, not an objection. What I verifiedNo language-count impact. The Emitting The consequence worth statingMapping That is a deliberate trade, and I think it is the right one — but it changes what I am not asking you to solve that here — the alternative is a dedicated grammar, which is a much larger piece of work and not what this PR set out to do. I am flagging it to our maintainer alongside the merge so the tradeoff is a decision rather than a discovery. If it turns out to matter, the follow-up is probably suppressing the markup ERROR regions for Everything else is ready. I will merge shortly unless the maintainer wants the |
|
Merged as One thing I looked into while reviewing, which turns out to be interesting: your approach is the first of its kind in this codebase. Every other markup-hosted format here has its own language rather than a mapping to a host — That is not a criticism — it is why the Our maintainer has asked me to evaluate whether the same pattern should be offered more broadly. My reading, for what it is worth to you: the Razor/ASP.NET family is the obvious extension — If you have appetite for |
What does this PR do?
Makes Blazor
.razorcomponents visible to the indexer, and turns their@pagedirectives intoRoutenodes.Two changes, in dependency order:
1.
feat(discover): map .razor to C#—.razorhad no entry inEXT_TABLE, so discovery skipped it and a Blazor application produced nograph nodes for any component. The only way in was an undocumented
extra_extensionsentry in a per-project.codebase-memory.json. This maps.razortoCBM_LANG_CSHARP.It is best-effort by design: the C# grammar recovers the
@codeblock, whilethe surrounding markup lands in ERROR regions and is reported through
parse_partial. That is still strictly more than the Module-and-imports theother markup-hosted languages extract today, and it needs no new grammar.
2.
feat(extract): emit Route nodes for Blazor @page directives—get_architecture'sroutesaspect returned nothing for a Blazorapplication: the entry surface of the app was invisible even though every
@pagenames one.@pagelives in markup above the@codeblock, which tree-sitter's C#grammar never parses, so there is no AST node to read it from. This scans the
raw source instead. The match is deliberately strict — the directive must be
the first token on its line, followed by whitespace and a double-quoted path
beginning with
/— so@pageSizeand prose mentions cannot match.The route is attached to the file's module def. A
.razorcomponent'sclass is implicit, so there is no class node to carry it, and the module QN
already is the component's identity.
insert_def_into_gbufis label-agnosticand creates
Route+HANDLESfromroute_path.pass_route_nodes.c'sensure_decorator_routesgains"Module"for thesame reason. Extraction covers the full-index path on its own; this backstop
is what runs on an incremental re-index, so without it a component's
Routewould appear on a full index and vanish the next time that one filechanged. Its loop bound now derives from the labels array rather than
borrowing the unrelated
RN_STRIP_PASSES, so adding a label cannot silentlyskip it. Verified there is no double-creation when both paths can fire (60
files: 60 Routes and 60 HANDLES either way).
Diff vs
main: 5 files, 190 insertions, 12 deletions.Measured
Rebased onto
mainat5fbab7bband re-measured from scratch. Arm A is nowmainitself — the exact merge base — rather than a release, so the onlydifference between the two binaries is this PR and there is no upstream drift
to disclaim.
Paired control on a real Blazor application (36
.razor, 302.cs, 11@page), both binaries run against one checkout, isolated caches, onecontainer each:
main5fbab7b).razorModules+11nodes is exactly the 11Routenodes;+11edges is exactly the 11HANDLES. Nothing else moves.get_architecture(routes)went from the aspectbeing absent to 11 entries. No store schema change.
Repeated independently on a second checkout of the same commit, same
result: 24327 → 24338 edges, 5113 → 5124 nodes,
SEMANTICALLY_RELATED290 →290.
Why "both binaries on one checkout" is stated so insistently
An earlier version of this control gave each binary its own checkout of the
same commit, and reported a
-75swing inSEMANTICALLY_RELATEDthat lookedlike a regression in this branch. It was not.
SEMANTICALLY_RELATEDdependson the absolute path of the checkout:
cbm_project_name_from_path(
src/pipeline/fqn.c:416) maps the whole path to the project name, that nameprefixes every
qualified_name, andpass_semantic_edges.c:469-470tokenizesqualified_nameinto the vector that becomes the LSH signature. Theunmodified
mainbinary reproduces the entire-75on its own, just byreading a second, content-identical checkout at a different path.
So the two rows above are measured with the path held constant, which removes
the effect entirely — hence
365 → 365and290 → 290rather than a deltaneeding explanation. I intend to file this separately once I have a clean
public reproduction; it is not caused by this PR and does not affect it.
End-to-end on a dedicated fixture, with a negative control:
Stable across a re-index.
What this does not claim
@codemethods are not reliably extracted from.razor. Real markup(
class=,role=attributes) defeats recovery and the file comes backparse_partialas a whole. Bare@codefields remain a separate gap.they reproduce on the unmodified binary too: routes report
handler:""although the
HANDLESedge exists, and the language census counts.razoras C# rather than as its own surface.
@pageon a component is taken, becauseCBMDefinitioncarries a single
route_path.Checklist
git commit -s) — required, CI rejectsunsigned commits (DCO, see CONTRIBUTING.md)
make -f Makefile.cbm test)make -f Makefile.cbm lint-ci)Full suite on this branch rebased onto
5fbab7bb: 7626 passed / 0 failed /7 skipped, 141 suites, via
scripts/test.sh. The branch itself adds 3 ofthose tests —
extract_blazor_page_directive_routes_component,extract_blazor_component_without_page_has_no_routeandlang_ext_razor, themiddle one being the negative control.
This PR has now been rebased twice rather than shipped with stale numbers:
first from
49d928beto010569fa(7415/0/8, 139 suites), and now to5fbab7bb. The extra 211 tests and 2 suites between those last two areupstream's, not this branch's.
The second rebase needed two conflict resolutions in
internal/cbm/extract_defs.c(main's newcbm_extract_definitions_without_module, and #519'smod.docstring = extract_config_module_description(ctx)— both sides kept),plus one collision that auto-merged cleanly and then did not compile:
tests/test_extraction.cgrew a secondfind_module_def, since main addedits own for the #518/#519 work while this branch has carried one since August.
Textually disjoint, semantically a redefinition. I kept the earlier
definition — it precedes every call site and has a
label &&null guard theother lacks — and removed the duplicate, leaving the #518/#519 comment block
intact. Main's eight call sites and this branch's two now share one definition.
On lint, stated plainly
make -f Makefile.cbm lint-ci— the gate this checklist names — passes: exit0, run in a clean container against this branch rebased onto
5fbab7bb.scripts/lint.shin full mode does not pass, and I do not believe any ofit is mine. Full mode adds clang-tidy, which fails repo-wide on rules like
readability-magic-numbers,readability-braces-around-statementsandmisc-no-recursion. The same run flags 22 diagnostics on untouchedmaincode, including
internal/cbm/extract_defs.cat lines 281, 588, 601 and1257 — while this branch's hunks in that file are at 7905-7987 and 8009-8019.
Two diagnostics do land on this branch's added lines:
readability-magic- numbersat 7912 (1U) and 7983 (1). Both are idiomatic and match thesurrounding style in that file. Happy to change them if you would rather.
One scope question I would rather raise than assume
CONTRIBUTING.md asks for prior design discussion on "new pipeline passes or
indexing algorithms — anything that changes what gets extracted or how", and
this does change what gets extracted. What I took as the go-ahead was #1667
being open and labelled
bug/language-request/priority/high, plus thestanding exception for focused bug fixes. If you would rather this went
through a design discussion first, say so and I will close it and move the
conversation back to the issue — no hard feelings.
Fixes #1667
Built and measured with Claude Code; all numbers above are from real runs on
a real application, not generated.