feat(pkl): extract calls, branches, throws and decorators - #1426
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. |
54a64c8 to
4e8d01d
Compare
DeusData
added
enhancement
|
Thank you for expanding the Pkl extraction coverage across calls, branches, throws, and decorators. I have routed this language-support contribution to |
|
Thank you for the Pkl language work and the coverage across calls, branches, throws, and decorators. The Pkl changes form one coherent language contribution and do not need to be split further. One commit is outside that scope: |
12bf64c to
597985d
Compare
|
Same 14 failures either way ( |
|
Split-out CI change is now #1823. |
|
@tthayer — your 24 August restack did everything the 18 August ruling asked, and cleanly; the split-out #1823 is exactly right. One more rebase is needed through no fault of yours: ArkTS and PL/SQL landed on main in the four days after your restack, and both moved the same ledger constants your PR moves. So you don't have to re-derive anything, here is the complete map — including the part that will bite silently. The trap first:
The other two ledger files conflict visibly, so git will make you look; correct values there: Two smaller items for the same pass:
The review verified the parts that matter: every extraction edit is behind a |
Pkl was wired end-to-end but shallow: on a real-world 172-file Pkl corpus
the graph had 386 IMPORTS edges and 0 CALLS edges, because pkl_call_types
was empty_types.
Pkl has no dedicated call node. `unqualifiedAccessExpr` and
`qualifiedAccessExpr` are the same node whether they are a call
(`helper(a)`) or a bare property read (`host`) — the only discriminator is
an `argumentList` child. extract_pkl_callee gates on that child, and is
dispatched from extract_callee_name with an unconditional return: falling
through to field-based or generic first-identifier resolution would mint a
CALLS edge for every property read in every Pkl file, since a bare access
expr's first child is an identifier.
Callee resolution:
helper(a) -> "helper"
utils.fallback(a) -> "utils.fallback" (module-qualified; cbm.c
shortens to the last dotted segment)
s.trim().toLowerCase() -> "toLowerCase" (a receiver that is itself a
call has parens in its text and is not prefixed)
new Server { ... } -> "Server" (links to the class def)
Also fills the other empty slots in the Pkl spec, all confirmed against
real parse trees rather than guessed: typeAlias (class), importGlobClause /
importExpr (import), ifExpr / whenGenerator / forGenerator (branch),
throwExpr (throw), annotation (decorator). forGenerator is additionally
registered in cbm_is_loop_node_type so `for (x in xs)` counts toward
loop-nesting depth; the name is Pkl-unique so no other grammar collides.
The Pkl repro test moves from the structural battery (dims 1-5) to the
full callable battery (dims 1-8) and adds an inline negative assertion
that bare property reads do NOT become CALLS edges — the regression the
argumentList gate exists to prevent.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Tony Thayer-Osborne <tony.thayerosborne@conductorone.com>
Merging main brought in three contracts this branch violated. repro_call_node_manifest requires a table row for every live call_node_types entry, so Pkl's unqualifiedAccessExpr/qualifiedAccessExpr (direct) and newExpr (constructor) each get one, with the partition totals moved to match. The historical-snapshot column is the audited ledger, not an archaeology record -- the suite hard-fails on a row that claims it was absent -- so a newly registered kind joins the snapshot with the change that registers it. repro_language_registry then requires every call-capable language to own exactly one call-argument matrix row, which turned out to be a real extractor gap rather than a bookkeeping entry. Pkl's grammar labels no production with a `name` field, so the generic declared-container rule never recognised a Pkl binding: method names, parameter names, and property names were each re-emitted as an ordinary read of themselves, which Go and Nickel do not do. Add a Pkl occurrence policy that binds the declared identifier of methodHeader, typedIdentifier, classProperty, objectProperty, clazz, and typeAlias, resolved against the nearest such container so annotations, defaults, and bodies stay reads. Recording the binding also supplies the lexical-shadow proof the matrix row asserts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Tony Thayer-Osborne <tony.thayerosborne@conductorone.com>
597985d to
7b72652
Compare
|
Rebased onto main (
|
2 participants
Problem
Pkl was already wired end-to-end — grammar vendored,
CBM_LANG_PKL, extension mapping, import edges — but the spec was shallow.pkl_call_typeswasempty_types, so on a real-world 172-file Pkl corpus the graph had 0 CALLS edges while imports worked fine.branching_node_types,throw_node_typesanddecorator_node_typeswere empty too, so Pkl contributed nothing to complexity metrics.Why this needs a custom extractor
Pkl has no dedicated call node.
unqualifiedAccessExprandqualifiedAccessExprare the same node whether they are a call or a bare property read — the only discriminator is anargumentListchild:extract_pkl_calleegates on that child. It is dispatched fromextract_callee_namewith an unconditional return, not viaextract_callee_lang_specific: returning NULL from the latter falls through to the generic "first identifier child" fallback, which would mint a CALLS edge for every property read in every Pkl file.Callee resolution:
helper(a)helperutils.fallback(a)utils.fallback(module-qualified;cbm.cshortens to the last dotted segment)s.trim().toLowerCase()toLowerCase(a receiver that is itself a call has parens in its text, so it is not prefixed)new Server { ... }Server(links to the class def)Other spec slots filled
All node names confirmed against real parse trees from a probe linking only the Pkl grammar, not guessed:
+ typeAlias+ importGlobClause,importExprifExpr,whenGenerator,forGeneratorthrowExprannotationforGeneratoris also registered incbm_is_loop_node_typesofor (x in xs)counts toward loop-nesting depth. The name is Pkl-unique, so no other grammar collides.Validation
Repro battery — the Pkl test moves from the structural battery (dims 1-5) to the full callable battery (dims 1-8), plus an inline negative assertion that bare property reads do not become CALLS edges.
repro_grammar_config_pklPASS.Full suite: 318 passed, 6 failed, 2 skipped — byte-identical to the pre-change baseline (verified by stashing and re-running). The 6 failures are pre-existing and unrelated (
repro_issue480/514/546/581,repro_issue409,repro_ts_inherited_method).Real corpus — 172 Pkl files of production infrastructure config, indexed clean (0 skipped, 0 parse failures):
Split: 270 to
Function, 97 toClass(constructors). ~2.1 calls/file — proportionate; a broken gate would have produced tens of thousands.Quality was spot-checked rather than inferred from the count: sampled
Functioncallees are genuinefunctiondefinitions invoked with arguments, and a bare property read that shares its name with one of those functions correctly produced no edge — the exact case theargumentListgate exists to catch.Note for reviewers
The repro fixture deliberately keeps every call site inside a method body, because dim 7 treats Module-sourced in-body calls as the enclosing-func gap. Real-world Pkl does call at module level (many of the 97 constructor edges are such sites), so this is a fixture constraint, not a claim about the language.
🤖 Generated with Claude Code