Repository navigation
Conversation
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Hi @htarnacki, thank you very much for this PR, and for the whole Scala series behind it. The write-up is excellent. The before/after table on finagle, the per-edge audit and your honest note about the one lost edge (StabilizingAddr) made this unusually easy to review. Keeping the Scala path fail-closed, and leaving Java and Kotlin byte-identical, is exactly the direction we want. We checked the declaring_file_is_scala skip and the cross-LSP namespace index, and neither leaks into other languages.
We'd like a few changes before merging, all in resolve_scala_namespace_import. Our first rule is that every new edge must be a true edge, and we also keep resolution free of lookup-inside-loop patterns:
- Member-import fallback to the owner (
pass_pkgmap.c~1963-1967). Whenpkg.Owner.memberisn't found under the owner's QN,return owner;produces an IMPORTS edge withlocal_name=memberthat points at the class.resolve_import_mapreturns the import target directly for a bare callee (registry.c~1004), somember()becomes a CALLS edge to theOwnerclass. Could the member path fail closed instead? Please also drop the!owner ||acceptance, so an unknown owner never binds a same-named symbol from elsewhere in the package. - Package membership cost.
scala_symbol_in_namespacewalks the whole package's file list for every same-named node. That is O(same-name hits × files in package) per import, and it gets heavy for names likeapplyin large split packages. Could you build a file→namespace (or (package, top-level name)→node) table once, next to the namespace map, so a membership check is O(1)? - Top-level only.
import pkg.Fooshould match only a top-levelFoo: the declaring file's module QN plus.Foo. Today a nested class or method namedFoocan either become the target or make a real top-levelFoolook ambiguous.
Smaller things, fine as follow-ups if you prefer:
- Join consecutive
package a/package bclauses into one namespace. Only the first is recorded today. - For
import pkg._, pick the lexicographically smallest declaring file rather than the first in discovery order, so the result is the same on every platform. - Scala binds relative names first (
package a.b; import c.Dmeansa.b.c.D). A follow-up issue is perfect for that.
Tests we'd love to see alongside:
- Scala importing a Java class from a mixed package, the most common real case.
- A Java import of a package declared by both Java and Scala files, which should still resolve to the Java file as on
main. - Two top-level same-named symbols in a split package, which should give no edge.
- An unresolvable
Owner.member, which should give no CALLS toOwner. - Chained package clauses.
Once those land, we'll run our own corpus proof: finagle plus a mixed Scala/Java repository, with sampled IMPORTS and CALLS, and full vs incremental index compared. Then it merges after CI. Thanks again. This is a big step up for Scala users, and we really appreciate the care that went into it.
|
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. |
1ccbb43 to
09f704d
Compare
|
Thanks for the review — all three points are in the new head (rebased on 1. Member import fails closed. 2. O(1) membership. 3. Top-level only. A node is indexed only when its QN is exactly Smaller items
Tests requested: Numbers (PR body updated): vs One housekeeping change:
|
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Hi @htarnacki, thank you so much for this rework. You addressed all three points precisely, and the finagle delta keyed by (file, local name) made it easy to trust: 71 owner-fallback edges gone, 152 real top-level imports recovered, and the 270 wildcard retargets explained. We re-read the new head:
- member imports now fail closed, and the new test pins both the IMPORTS and the CALLS side;
- membership is one hash lookup per prefix step;
- nested Scala symbols neither match nor create ambiguity;
- every test we asked for is there, plus chained package clauses and the deterministic wildcard.
While preparing our corpus proof we found four more things. We'd rather raise them all in one round than trickle them out.
- Incremental reindex parity. Both incremental routes pass only the changed files (
pipeline_incremental.c:1254and:1385). So the namespace map and the new Scala index only know packages declared by changed files (pass_parallel.c:1709-1716). Onmain, an import into an unchanged package still binds through the symbol fallback; with the fail-closed resolver it returns NULL. As far as we can read, editing one Scala file and reindexing incrementally would drop its IMPORTS into every unchanged package, and theimport_mapCALLS built on them, until the next full index. Graph content must not depend on whether the last index was full or incremental, so we'd like this closed here. One option is to persist the package clause, for example on the File node, so both maps can be built for all files; we're happy to discuss other shapes. A test that edits one Scala file, reindexes incrementally and compares that file's IMPORTS with a full index would pin it. - Java nested members in the top-level index. Java class QNs are directory-based, so
OuterinOuter.javais<dir>.Outer, which equals the stem module QN compared atpass_pkgmap.c:2488. Its methods and nested types (<dir>.Outer.Builder) are then one segment below it and pass the check at:2427. In a mixed package, a JavaRequest.Buildermakes a real top-levelBuilderambiguous, and the edge is lost. Using the language-aware module QN per file (thecbm_lang_module_is_dirrule, i.e.cbm_pipeline_fqn_module_dir) makes<dir>.<Name>the only top-level shape for Java and lets theqn == module_qnspecial case go. A test with a nested JavaBuildernext to a top-levelBuilderwould pin it. - Build the index only when Scala needs it. The header says the index is NULL when no Scala file declares a package (
pipeline_internal.h:227, 259), but any namespace-declaring file triggers the build. So every Java/Kotlin/C#/PHP repository pays a full node walk plus a key per top-level symbol on each pass. We watch per-pass memory closely on large C# and Java corpora, and an earlyreturn NULLwhen no Scala file has a package clause keeps them exactly as onmain. - Wildcard cost. Picking the smallest declaring file was our own request, sorry! It made
first_namespace_filescan the whole package once perimport pkg._, which is the same O(imports x files) shape as before. Computing the smallest and runner-up file once per package while building the index (the runner-up covers the self-import exclusion) keeps it O(1).
The relative-import follow-up is still welcome as an issue whenever it suits you; a missing edge is a defect, so an issue is the right place for it. Once these land, we'll run the promised corpus proof (finagle plus a mixed Scala/Java repository, full vs incremental), and it merges after CI. Since #2361 and #2366 build on this branch, one rework here plus one rebase of the stack is probably the easiest path. Thank you again. This series is a real step up for Scala users.
09f704d to
5a77358
Compare
|
Thanks for the second round — all four are in the new head 1. Incremental parity. Went with the File-node shape you suggested. Test: 2. Java nested members. The index now computes the module QN per file with On finagle this is worth more than I expected: +33 IMPORTS, all Scala imports of Java classes ( 3. Index only for Scala. 4. Wildcard. The old Tests/lint: Relative-import issue coming next, as an Issue. #2361 and #2366 will be rebased onto this head right after. |
5a77358 to
bb3f1cd
Compare
|
New head What changed:
Verified: |
|
Relative-import follow-up filed as #2428, with a six-file repro covering the three forms (same-package |
bb3f1cd to
ba1946c
Compare
|
Rebased onto current The only conflict was in Verified on the rebased head: Context for the earlier red |
|
CI status on
Everything project-side is green: |
Scala imports were parsed with the generic import fallback, which
ignored selector groups, aliases (`{A => B}`) and wildcards, and the
declared `package` clause was not recorded. IMPORTS edges then had to be
guessed from the physical directory layout, which does not have to
mirror the package in Scala projects.
- parse Scala `import` statements into one CBMImport per selector,
keeping the alias as the local name and the full path as the target
- record the `package` clause as the file namespace; consecutive
braceless clauses (`package a` / `package b`) are joined into `a.b`
- thread the file language through import resolution so Scala imports
are resolved against declared packages; the resolver fails closed at
every step instead of guessing
- build a (package, top-level name) -> node index once per pass, next
to the namespace map, so package membership is an O(1) lookup instead
of a scan of the package's file list per same-named hit; every file
declaring a namespace contributes, so a Scala import of a Java class
in a mixed package resolves. Only the Scala resolver reads it, so it
is built only when a Scala file is among the pass's files
- "top level" is decided against the file's language-aware module QN
(stem vs directory), so a Java nested type (`Request.Builder`, one
segment below the directory module) is not mistaken for a top-level
`Builder` of the package
- `import pkg.Name` binds only a top-level `Name` of a file declaring
`pkg`: nested classes and methods neither match nor make the real
top-level symbol look ambiguous; two top-level declarations of the
same name in a split package yield no edge
- `import pkg.Owner.member` binds the member under the owner's QN and
otherwise yields no edge, so a missing member never turns `member()`
into a CALLS edge to the owner class via resolve_import_map
- `import pkg._` binds the lexicographically smallest declaring file so
the target is identical on every platform; the index records that
file (and the runner-up, for self-import) per package, so the pick is
O(1) instead of a scan of the package's file list
- incremental parity: both incremental routes hand the passes only the
changed files, so the namespace map and the Scala index used to know
only the changed files' packages, and an import into an unchanged
package resolved through the symbol fallback (or, fail-closed, not at
all) until the next full index. The passes now persist each file's
package clause on its File node, and the incremental routes read the
clauses of the files that stay back into the pass context so the
maps cover the whole project exactly as on a full build
- Scala package clauses feed only the Scala resolver: the generic
first-declaring-file walk skips Scala files, so Java/Kotlin imports of
a shared package prefix keep resolving exactly as before
- import-map lookups therefore bind an aliased local name to the exact
imported object, so `Alias.method()` resolves with the `import_map`
strategy
Tests: extraction of selectors/aliases, the package namespace and
chained package clauses; edge_imports guards that packages may ignore
the physical layout, that a Java import never binds to a Scala prefix
declarer, that a Scala import binds a Java class of a mixed package,
that a Java import of a package declared by Java and Scala files still
binds the Java file, that two same-named top-level symbols in a split
package give no edge, that a nested same-named symbol is ignored, that
an unresolvable `Owner.member` gives neither an IMPORTS nor a CALLS edge
to the owner, that chained package clauses resolve, that a Java nested
type of the same name neither binds nor makes the top-level Scala
symbol ambiguous, and that an incremental reindex (closure-repair and
legacy-partial route, each proven by the route probe) keeps the IMPORTS
and CALLS into an unchanged package; pipeline tests for alias
resolution (sequential + parallel), for member imports in a split
package, and that the Scala index is built only when a Scala file is
among the importers.
Closes DeusData#2153
Signed-off-by: Hubert Tarnacki <[email protected]>
ba1946c to
14d4388
Compare
|
Rebased onto current One conflict, in Verified on the rebased head: |
|
@DeusData — status check on this series (#2360 → #2361 → #2366), to make sure I spend rebase effort where it's useful.
Two ways forward, whichever suits you:
Thanks. |
DeusData
added
enhancement
|
Thanks for the sustained work on this series, and sorry for the repeated rebases without a clear landing decision. Your question is fair: the maintainer needs to decide whether to continue reviewing this series or carry the relevant fixes into the identity work. That decision remains open; please hold off on further routine rebases in the meantime. |
|
Thank you again for carrying the Scala series, and sorry that the direction question is still waiting on us. Please continue to hold off on routine rebases until we answer whether to review this series or carry its fixes into the identity work. I’ll follow up soon. |
|
Hi @htarnacki, thank you so much for your patience, and for the care you have put into this whole series. We have made the call: we would like to land all three, in order - #2360 first, then #2361 and #2366. Package-aware Scala imports stand on their own, so #2360 goes first. Taking you up on your kind offer, could you rebase it onto current main one more time? We will review the rebased head right away so it does not go stale again. A few notes for the rebase:
Once the rebased head is up, we will run the corpus proof we promised (twitter/finagle plus a mixed Scala/Java repository, full versus incremental index). Since maintainer edits are enabled, we will push any small follow-ups ourselves on top of your commit, each explained here, so you do not need another round for them. One we already know of: an index written before this change has no #2361 and #2366 follow in order once #2360 is in; a rebase of the stack on top of the merged #2360 is all they should need, and we will review each one as soon as it is up. Thanks again. This is a big step up for Scala users. |
Bring DeusData#2360 (package-aware Scala imports, closes DeusData#2153) up to date with main f5d5746. The contributor's commit 14d4388 is untouched; every conflict is resolved so that the change stays as written and main's changes are kept. Conflict resolutions: - src/pipeline/pass_pkgmap.c: rebuilt as main's file plus the PR's own blocks. Main moved Strategy 2 (namespace map) out of cbm_pipeline_resolve_import_node into resolve_import_strategies, so the Scala dispatch (language == CBM_LANG_SCALA -> resolve_scala_namespace_import) and the declaring_file_is_scala guard on the generic namespace walk now sit there, directly before and inside Strategy 2 as in the PR; resolve_import_strategies takes the PR's language and scala_index. Main's namespace-map builder (fail-closed: NULL on a construction failure, an empty map when no file declares a namespace) stays the single implementation: it becomes the ctx-aware static namespace_map_build that records each clause as the File node's `package` property (the PR's file_node_set_package), and the public cbm_pipeline_namespace_map_build{,_names} and cbm_pipeline_namespace_map_free that main's tests and pipeline cleanup call are kept. The PR's key conversion to cbm_mem_strdup was already on main. cbm_pipeline_import_maps_build builds both maps on top of it. Main's PHP IMPORTS writer (cbm_pipeline_php_create_import_edges) calls the re-signed resolver with CBM_LANG_PHP and no Scala index. - src/pipeline/pass_definitions.c, src/pipeline/pass_parallel.c: main's PHP branch in the IMPORTS writers kept, the PR's call with language and scala_index in the generic branch. Main publishes the namespace map to the pipeline (DeusData#1355) instead of freeing it, so only the Scala index is freed at the end of the pass; main's two new ABORT_PRESERVE_DB exits in the registry loop free the Scala index as well. - src/pipeline/pass_calls.c: the PR's build_import_map(..., language, ...) call next to main's PHP vendor-name lines. - src/pipeline/pipeline_internal.h: the PR's cbm_file_namespace_t, the cbm_scala_index_t declaration, the stored_namespaces context fields and the import-maps API next to main's env-join and Terraform types, stem marks, env-join base and PHP vendor declarations; main's public namespace-map declarations kept. - src/pipeline/pipeline_incremental.c: incr_load_stored_namespaces and incr_free_stored_namespaces next to main's coverage helpers. Closure route: main's doc-link scopes are loaded first, then the stored package clauses, so a failed doc-link read cannot leak them; the context carries the PR's fields and main's. Legacy route: main removed the ADR capture (ADRs live in the sidecar database), so the PR's shared ADR/namespace abort becomes a read of the stored clauses after a successful graph load that shares the failed load's cleanup (a failed load logs and returns exactly as on main; stored_namespaces_failed returns ABORT_PRESERVE_DB as in the PR). - tests/test_pipeline.c: main's file plus the PR's own hunks; the four RUN_TEST lines follow the same anchor line. - tests/test_edge_imports.c: main's two PHP resolver tests (DeusData#1186) adapted to the re-signed cbm_pipeline_resolve_import_node (CBM_LANG_PHP, NULL Scala index), the convention the PR used for its own call sites. - tests/test_extraction.c, tests/test_edge_imports.c: the PR's reformat of extract_ts_await_generic_call_issue2210 and its pipeline_internal.h include were already on main (e8a46c1), so they drop out of the diff. - scripts/memory-core-baseline.txt: recomputed against main's baseline: pass_definitions.c 11 -> 9 and pass_parallel.c 31 -> 29 (the PR drops their raw rels arrays); no line raised. pass_pkgmap.c is one raw site above main's baseline after this merge; the follow-up commit removes it. The index format bump for the new File-node property follows in the next commit. Signed-off-by: Martin Vogel <[email protected]>
Follow-up for DeusData#2360. An index written before this change has no `package` property on its File nodes. Both incremental routes read that property back for the files they do not re-extract, so the first incremental run after an upgrade would build the namespace map and the Scala top-level index from the changed files' clauses only, and the fail-closed Scala resolver would drop every import into an unchanged package until the next full index. - store.h: CBM_INDEX_FORMAT_VERSION 5 -> 6, so every existing index is rebuilt once (format_change_reindex) and records the property. - test_index_format.c: index_format_v5_scala_packages_rebuild takes a format-5 index in the old shape (no File node carries its clause), edits the importer, refreshes, and asserts that the import into the unchanged package survives, that the rebuild ran, that every declaring File node has its clause and that the stored format is current. Red without the bump: the refresh stays incremental and Consumer.scala loses its IMPORTS edge to Target. Signed-off-by: Martin Vogel <[email protected]>
Follow-up for DeusData#2360, memory-core ratchet. After the main merge pass_pkgmap.c has one raw allocator site more than main's baseline (82 vs 81): the free of the cbm_pipeline_fqn_module_dir result in scala_index_free_file_entry. The PR offset it by moving the namespace-map key to cbm_mem_strdup, and main has since made that conversion itself. The per-file module QNs now come from cbm_fqn_module_source_lang in a build-time arena that scala_index_build destroys before it returns. That is the helper the extractor builds definition QNs with, so the top-level check compares against QNs made by the same rule; pkgmap_module_is_dir becomes pkgmap_file_language. pass_pkgmap.c is back at 81. Graph unchanged: twitter/finagle ca472deb and apache/kafka 4.0.0 (985bc995) index to identical nodes and edges with and without this commit. Signed-off-by: Martin Vogel <[email protected]>
Follow-up for DeusData#2360. A Scala wildcard import (`import pkg._`, `import pkg.Owner._`) gets an IMPORTS edge whose local name is "*", and the import-map builders copy that name as a key. Scala operators are method calls, so `h * width` is a call whose callee is "*" as well: it matched the wildcard's key and bound to the wildcard's target, a CALLS edge onto a File or Class that nothing calls (7 edges on twitter/finagle, 20 on apache/kafka, none on main). resolve_import_map now treats a "*" prefix as no import name. The wildcard entry stays in the import map, so the imported package still counts as import-reachable for the other strategies; only the binding by name is gone. Every import-map builder's keys reach this one lookup, so the sequential and the parallel drivers are covered alike. pipeline_scala_wildcard_import_binds_no_operator_call runs a package wildcard, a member wildcard and a named import through both drivers: no CALLS edge with the callee "*" may exist, all three IMPORTS edges stay, and the named import still binds Helper.run through the import map. Red without the check (parallel driver): both operator calls bind to Consts through the import map. Corpora, before vs after this commit: twitter/finagle ca472deb loses exactly its 7 operator edges, apache/kafka 4.0.0 (985bc995) its 20 plus the 3 TESTS edges derived from them; no other CALLS, IMPORTS or USAGE edge moves, node identities are unchanged, and only the derived importance scores and the SEMANTICALLY_RELATED ranking shift. Signed-off-by: Martin Vogel <[email protected]>
|
Hi @htarnacki, thank you again for this whole series, and for your patience while we got to it. As announced on 2026-11-01, we have now brought this branch up to date ourselves (maintainer edits are enabled). Your commit 14d4388 is untouched; we added four commits on top of it:
The corpus proof, main vs this branch, a fresh index for every run: twitter/finagle (ca472de, 1,898 .scala files):
apache/kafka (4.0.0, 436 .scala and 5,177 .java files):
In a seeded sample of the changed edges, every new IMPORTS edge we checked named the imported symbol, Scala imports of Java classes in kafka included, and the new import_map CALLS were right apart from the operator pattern fixed in commit 4. One more thing the proof showed, which is not about this PR's code: now that Scala files have real imports, the existing receiver-blind fuzzy match (a call like Please pull before pushing again. #2361 and #2366 follow the same way once this one is in. Thanks again, this is a big step up for Scala users. |
Bring DeusData#2360 up to date with main b3ec08d (DeusData#2427 registry test verdict, DeusData#2962 MSan CI, DeusData#2952 Windows stdio guard). The merge is clean and needs no follow-up edit: DeusData#2360's code makes no cbm_registry_add* call and never used is_test_qn, and Scala definitions register through the shared seeders in pass_definitions.c, pass_parallel.c and pipeline_incremental.c, which DeusData#2427 already passes the extractor verdict through for every language. The branch's own change against main is byte-identical to 28948f2's against f5d5746. Signed-off-by: Martin Vogel <[email protected]>
|
Hi @htarnacki, a quick heads-up: we merged current main (b3ec08d) into this branch once more, so the PR stays mergeable. The CI run on the previous head was caught by a GitHub artifact outage, and in the meantime #2427 landed, which reworks how the registry ranks test code and touches several of the same files (pass_definitions.c, pass_parallel.c, pipeline_incremental.c, registry.c). The merge was clean and needed no edit: your code makes no registry-add call, and Scala definitions go through the shared registration path that #2427 already updated for every language. Your change against main is byte-identical to the previous head's, and nothing else was added. We re-ran the narrow test suites (all green, including the Scala, wildcard and index-format tests) and re-indexed twitter/finagle: IMPORTS (6,336) and the import_map CALLS (2,721) are unchanged. The only movement comes from #2427's new test-candidate ranking, which shifts some low-confidence fuzzy matches toward test code. That comes from main, not from your change, and we are handling it there and in the Scala-only follow-up we mentioned. Thank you again, and as before, please pull before pushing again. |
Signed-off-by: Martin Vogel <[email protected]>
DeusData#2427 (b3ec08d) ranks registry candidates by the extractor's stored test verdict instead of the old QN substring scan. The extractor gives a definition its file's verdict only for functions, methods and the module (extract_defs.c), so a variable, field, class, struct or interface in a test file keeps is_test=false. The QN scan had masked that: Java and Scala QNs spell src.test and test class names. Since DeusData#2427 such a symbol ranks as production code, and a name-only match (suffix_match) from production code picks it whenever it shares more of the caller's path than the real candidate. finagle's `Future.exception` bound to a Variable in TimeoutFactoryTest; Go structs in _test.go files won the same way. The three registry seeders (pass_definitions.c, pass_parallel.c, pipeline_incremental.c) now also apply cbm_is_test_file, the rule the extractor computes is_test_file with, on the same path and language, so every definition in a test file ranks as test, in full and incremental indexes alike. Ranking only: the stored is_test property, TESTS edges and importance keep their own rules. Proof, production-source edges into test files (cbm_is_test_file on both ends), pre-DeusData#2427 cf337a6 / main b3ec08d / this change: kafka 4.0.0 985bc995 CALLS 849 / 1023 / 750 (onto Variables 6/362/6) USAGE 5936 / 8254 / 5832, WRITES 163 / 218 / 121 gardener 58d1ab91 CALLS 125 / 123 / 52 (onto Structs 75/75/4) USAGE 837 / 730 / 572, WRITES 323 / 254 / 160 finagle ca472deb CALLS 281 / 505 / 273, USAGE 1568 / 2436 / 1541 finagle with DeusData#2360 CALLS 566 -> 300 (Variables 391 -> 107) Every edge DeusData#2427 moved from a test file to production stays there (gardener USAGE 125, WRITES 51, CALLS 1; kafka CALLS 59, USAGE 45, WRITES 13; finagle USAGE 17, CALLS 7). Only suffix_match CALLS move. Two runs are identical, and an incremental re-index of 25 seeded files matches the full index for every caller outside the edited files. On kafka the change also restores about 6,800 suffix_match CALLS from test files, with their TESTS edges, that main had lost because an unmarked test-file field won the ranking and the CALLS guard refused it; they are the edges the index had before DeusData#2427. Signed-off-by: Martin Vogel <[email protected]>
|
One more merge of current main into this branch (ea98999), with no change to your code. The fix we mentioned in our last note, for fuzzy matches drifting toward test code after #2427, has now landed as #2968, and it touches some of the same files. Your change against main is unchanged (same patch), and once CI is green we will merge. Thanks again, and as always, please pull before pushing again. |
Brings in main up to 09bd6bb (DeusData#2967 FreeBSD peer pid, DeusData#2974 graph variation and content loss fixes). Resolutions: - scripts/memory-core-baseline.txt (textual conflict): both sides lowered counts in the same hunk. pass_parallel.c keeps this branch's 29 (arena QNs); pass_pkgmap.c takes main's 80 (DeusData#2974 dropped one raw free in cbm_pkgmap_build; this branch's pass_pkgmap.c additions add no raw site). Both values re-derived by scanning the merged tree with scripts/lint-memory-core.py; the lint passes. - src/pipeline/pass_pkgmap.c (auto-merged): main's deterministic manifest order in cbm_pkgmap_build and this branch's Scala package index sit in separate functions; the merged file is main plus this branch's change unchanged (same per-file patch-id). - tests/test_edge_imports.c, tests/test_pipeline.c (auto-merged): both sides' new tests are kept and registered; main's new tests call no function this branch re-signed. Same per-file patch-id as this branch's change. Every other file equals main. Signed-off-by: Martin Vogel <[email protected]>
|
Hi @htarnacki, one more short heads-up: we merged current main into this branch again, because GitHub had started to report a conflict. Main picked up #2974 (a set of fixes for run-to-run graph variation and content loss) and the FreeBSD peer check from #2967. #2974 touched pass_pkgmap.c, the two test files you extended, and the memory-core baseline, so the branch no longer merged cleanly. The only real conflict was two neighbouring lines in scripts/memory-core-baseline.txt, where both sides had lowered a count. We re-derived both values by running the memory-core lint script on the merged tree, so they match the code exactly. Everything else merged on its own: main's change to pass_pkgmap.c lives in the manifest-merge function, well away from your Scala package index. Your change against main is identical to the previous head's, and nothing else was added. We re-ran the narrow test suites (all green, including your Scala import tests and the index-format test) and re-indexed twitter/finagle with the previous head and the new one. The graphs are identical: IMPORTS are still 6,336 and the import_map CALLS are still 2,721. Thank you again for your patience while main keeps moving under this PR. As before, please pull before pushing again. |
…iver DeusData#1897's receiver-chain gate judged only chains rooted in a type and let a lower-case root through ("a value whose type the chain does not show"), and the two suffix_match exits of resolve_multi_with_imports were never gated. So a member call no LSP had typed bound whatever project symbol shared its final name: kafka's `tp.topic()` on a TopicPartition bound ConsumerRecord.topic, gardener's `strings.Join` bound pkg/utils/net.Join, finagle's `Future.exception` a thrift service method. At the same name-only exits (unique_name, suffix_match, and now the import fallback) a CALLS resolve of a member call whose receiver is a value or a `new X(...)` constructor binds only on evidence that it means the chosen candidate: a chain segment names its ancestry word by word (`repo` / Repo, `cleanerManager` / LogCleanerManager, `otel_guard` / OtelGuard, the receiver of `verify(x)` / `when(x)`), it lives in the caller's own module, it is import-reachable, or, for a C/C++ call onto a struct member, the C member resolver decides it from the object's type. When another candidate is the field-type hint's own pick the call is left for that re-pick. Otherwise the guess is refused: a veto, never a re-pick. Self receivers (this, self, cls, super, base, this_, _this) and `::` paths are exempt; import_map, same_module, qualified_suffix and lsp_* are untouched. A type root is checked at the import fallback too. Scope by measured language (share of refused edges that were wrong, hand- read seeded samples): whole for Go (50/50), C++ (25/26), C#, Lua, Tcl, Groovy and C; suffix_match picks only for Scala (kafka 29/30, finagle on DeusData#2360 28/30). Left out: Java (suffix_match-only still 47/60), Kotlin (unmeasured), Ruby (3/15), and Python, the JS/TS family, Perl and PHP, whose own guards run after resolution and keep their HTTP classification. kafka 4.0.0 loses 5,189 CALLS (Scala 5,183) and 620 fake async routes, finagle on DeusData#2360 6,302 Scala CALLS and 13 false HTTP_CALLS, gardener 339 Go CALLS, redis 15, otel 168. Deterministic; incremental matches full. The DeusData#2121 controls for value roots now give their candidate a value receiver's evidence (the caller's own module), which a type root would not admit. Signed-off-by: Martin Vogel <[email protected]>
What does this PR do?
Closes #2153. First PR of the Scala series; #2154 (companion objects) and #2155 (weak-member suppression) will follow on top.
Scala imports went through the generic import fallback, which ignored selector groups, aliases (
{A => B}) and wildcards, and thepackageclause was not recorded — IMPORTS edges had to be guessed from the directory layout, which sbt multi-module projects do not mirror.internal/cbm/extract_imports.c: parse Scalaimportinto oneCBMImportper selector (alias = local name, full path = target); record thepackageclause as the file namespace, joining consecutive braceless clauses (package a/package b→a.b).src/pipeline/pass_pkgmap.c: thread the importing file's language intocbm_pipeline_resolve_import_node. For Scala: resolve the longest declared package prefix, then look the top-level symbol up in a (package, top-level name) → node index built once per pass next to the namespace map (cbm_pipeline_import_maps_build) and only when a Scala file is among the pass's files, so package membership and the_wildcard pick are O(1). Every file declaring a namespace contributes, so a Scala import of a Java class in a mixed package resolves; "top level" is decided against the file's language-aware module QN (stem vs directory), so a Java nested type is not mistaken for a top-level symbol.import pkg.Namebinds only a top-levelName(module QN +.Name, or the module QN itself for Java's collapsedclass FooinFoo.java); nested same-named symbols neither match nor create ambiguity; two top-level declarations of the same name in a split package yield no edge;import pkg.Owner.memberbinds the member under the owner's QN or nothing at all — never the owner.import pkg._binds the lexicographically smallest declaring file, so the target is the same on every platform.main.pass_calls/pass_definitions/pass_parallel: pass the language and the index through;Alias.method()now resolves withimport_map.pipeline_incremental.c): both incremental routes hand the passes only the changed files, so the namespace map and the Scala index knew only those files' packages. The passes now persist each file's package clause on its File node (packageproperty) and the incremental routes read the clauses of the files that stay back intoctx->stored_namespaces, so the maps cover the whole project exactly as on a full build. A failed read aborts the incremental route (ABORT_PRESERVE_DB) rather than let the maps silently diverge.scripts/memory-core-baseline.txt:pass_pkgmap.c82 → 80 (the namespace-map key now goes throughcbm_mem_strdup; the new index allocates through the core).Every other language is unchanged (verified below).
Before / after on
twitter/finagle(ca472de, 1,898.scalafiles)main(5df8b044) vs this branch, same binary flags, freshCBM_CACHE_DIReach run.Nodes 28,104 → 28,121. Total edges 126,493 → 135,109.
Edges from non-Scala files are identical to
mainfor every edge type (IMPORTS, CALLS, USAGE, WRITES, …); the only non-Scala churn is 10SEMANTICALLY_RELATEDedges between thrift-generated Java files, which is that pass's ranking and not import-related.Effect of the second-round review changes (
09f704d2→ this head)IMPORTS 6,310 → 6,343: all 6,310 previous edges identical, 33 added, every one a Scala import of a Java class (
ThriftClientRequest,ServerSetImpl,ZooKeeperClient,ZstdDecoder,RequestHeader,JsonCodec, …). These were dropped before: a Java class QN is directory-based (…zookeeper.ServerSetImpl, one segment below the stem module QN the index compared against), so the language-aware module QN is what makes them top-level. CALLS 30,317 → 30,329:import_map2,711 → 2,730; 16 retargets, 4 added, 1 dropped, all in callers of the newly imported Java classes (ZkInstance.start → ZooKeeperClient,ZstdDecoderTest.createChannel → ZstdDecoder, …). DEFINES, node count and every non-Scala edge are identical.Effect of the first-round review changes (
1ccbb432→09f704d2)IMPORTS 6,229 → 6,310; CALLS 30,315 → 30,317. Keyed by (source file, local name):
import pkg.Owner.memberwherememberis aval(TimeoutFilter.TimeoutAnnotation,MysqlCharset.Utf8_general_ci,Method.Post,Handshake.CanTinitMsg, …). They used to bind the owner class; now they fail closed (review point 1).Path,ServiceFactory,Updater, … imports of real top-level symbols that the old name scan reported as ambiguous because a nested class of the same name exists elsewhere in the package (review point 3)._wildcards retargeted File → File to the lexicographically smallest declaring file (e.g.import com.twitter.finagle._now →finagle-core/…/finagle/package.scalainstead offinagle-memcached/…/Memcached.scala).suffix_match/import_mapheuristics reacting to the changed import sets; the 9 outright drops were wrong targets (MySqlExecutables.getBaseDir → Announcer.get,initDatabase → CookieMapBenchmark.map) that are nowcallee_suffixhits on the file's own methods.IMPORTS (+5,988 added, 1 removed vs
main)Target labels after: Class 5,186 · File 549 · Interface 475 · Method 55 · Type 30 · Module 12 (before: File 193, Class 79, Method 30, Module 12). On the previous head I checked every added edge against the source: 5,385 named the imported symbol on a single
importline, 356 were_wildcards to the package's declaring file, 166 were multi-line grouped selectors (Bufs.{\n RichBuf,\n …}) that my one-line regex missed but are correct on inspection. Random sample:finagle-thriftmux/…/test/EndToEndTest.scala→FailureAccrualFactory(Class,finagle-core/…/liveness/FailureAccrualFactory.scala)finagle-http/…/exp/routing/HttpRouter.scala→PathField(Class,…/exp/routing/Fields.scala)finagle-mysql/…/HandshakeSettings.scala→PathToServerRsaPublicKey(Class,finagle-mysql/…/param/params.scala)finagle-http2/…/client/ClientServiceImpl.scala→Service(Class,finagle-core/…/Service.scala)finagle-postgresql/…/PgSqlTransporter.scala→Transporter(Class,finagle-core/…/client/Transporter.scala)The one removed IMPORTS edge (
StabilizingAddrTest→StabilizingAddr) comes fromimport StabilizingAddr.State._— a relative import (no package prefix, resolved against the file's own package). The new resolver doesn't handle relative imports yet and fails closed; the old symbol-name fallback happened to hit the class. That's the only occurrence in finagle; I'd add relative-import support in a follow-up rather than widen this PR, unless you prefer it here.CALLS (net +2,181 vs
main; audit below is from the previous head, +3,341 added / 1,162 removed, and the review changes moved 58 → 60 edges as listed above)Resolution mix:
import_map13 → 2,659;unique_name9,283 → 7,894;qualified_suffix1,897 → 1,169;callee_suffix130 → 28.Of the 1,162 removed, 1,054 are retargeted (same caller, same callee name, better target), e.g.
Simulation.main.map— washttp.CookieMapBenchmark.map, nowStack.mapmux.WorkloadTping— wasWorkload.Tping(self), nowmux.transport.Message.TpingStatsScopingTest.mkService+— wasDtabBase.+, nowStack.Params.+The remaining 108 are dropped: 101 self-loops (
register → registerwhere the body callsBalancerRegistry.get.register,sslSessionInfo → sslSessionInfo, …), 4 wereHostbound tohttp.Fields.Hostwhile the file actually aliases the packagezipkin.{host => Host}, 2 same-file. One genuine loss:MutableSpan.toSpan→zipkin.core.Span(...)(wassuffix_match@0.28 among 6 candidates). The now-correct import ofthrift.thrift.Constantsmakes the Java siblingthrift.thrift.Span"import-reachable" forresolve_multi_with_imports, and the cross-languagesuffix_matchguard then drops it. The right target is same-packageSpan, whichsame_moduledoesn't find because Scala QNs nest under the file stem (Span.Span) — that's the companion/QN topic of #2154, not this PR.Tests
extraction: selectors, aliases, wildcards, package namespace, chained package clauses (package a+package b→a.b; a braced clause is not joined) — 381 pass; the chained test is red on the previous headedge_imports(72 pass):Codec.java+Util.scalaincom.acme.core)Codecin a split package → no edgeOuter.Codecneither matches nor makes top-levelCodecambiguous (red on the previous head: 0 edges)import Config.load→loadMethod;import Config.missing→ no IMPORTS and no CALLS toConfig(red on the previous head: the owner was bound)Request.Builder(directory-based QN, one segment below the module) neither bindsimport pkg.Buildernor makes the top-level ScalaBuilderambiguousrun → buildCALLS into an unchanged package on both the closure-repair and the legacy-partial route — each route asserted through the test probe, so a silent fall-back to a full reindex cannot pass; red with the stored-clause read disabled (0 IMPORTS)pipeline: alias →import_map(sequential + 4-worker parallel); member import in a split package bindsOwner.member; the Scala index is built only when a Scala file is among the pass's files (Java-only → namespace map only, index NULL)Checklist
git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)scripts/run-tests-parallel.shon the rebased head)lint-formatwith clang-format 21,lint-no-suppress,lint-memory-core;cppcheck2.17.1 from the pip wheel on the touched files — no warning/style findings; CI runs 2.20)