Skip to content

feat(scala): parse import selectors and resolve package-aware imports (#2153) - #2360

Open
htarnacki wants to merge 1 commit into
DeusData:mainfrom
htarnacki:fix/scala-package-aware-imports
Open

htarnacki wants to merge 1 commit into
DeusData:mainfrom
htarnacki:fix/scala-package-aware-imports

Conversation

@htarnacki

@htarnacki htarnacki commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

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 the package clause 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 Scala import into one CBMImport per selector (alias = local name, full path = target); record the package clause 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 into cbm_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.
  • Fail-closed at every step: import pkg.Name binds only a top-level Name (module QN + .Name, or the module QN itself for Java's collapsed class Foo in Foo.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.member binds 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.
  • Scala package clauses feed only the Scala resolver: the generic first-declaring-file walk skips Scala files, so Java/Kotlin imports of a shared prefix resolve exactly as on main.
  • pass_calls / pass_definitions / pass_parallel: pass the language and the index through; Alias.method() now resolves with import_map.
  • Incremental parity (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 (package property) and the incremental routes read the clauses of the files that stay back into ctx->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.c 82 → 80 (the namespace-map key now goes through cbm_mem_strdup; the new index allocates through the core).

Every other language is unchanged (verified below).

Before / after on twitter/finagle (ca472de, 1,898 .scala files)

main (5df8b044) vs this branch, same binary flags, fresh CBM_CACHE_DIR each run.

edge type before after delta
IMPORTS 323 6,343 +6,020
CALLS 28,136 30,329 +2,193
USAGE 55,447 56,075 +628
HTTP_CALLS 79 106 +27
TESTS / WRITES / INHERITS / OVERRIDE / THROWS — — +10 / +9 / +1 / +1 / +1

Nodes 28,104 → 28,121. Total edges 126,493 → 135,109.

Edges from non-Scala files are identical to main for every edge type (IMPORTS, CALLS, USAGE, WRITES, …); the only non-Scala churn is 10 SEMANTICALLY_RELATED edges 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_map 2,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):

  • 5,818 identical.
  • 71 removed — all import pkg.Owner.member where member is a val (TimeoutFilter.TimeoutAnnotation, MysqlCharset.Utf8_general_ci, Method.Post, Handshake.CanTinitMsg, …). They used to bind the owner class; now they fail closed (review point 1).
  • 152 added — 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).
  • 270 _ wildcards retargeted File → File to the lexicographically smallest declaring file (e.g. import com.twitter.finagle._ now → finagle-core/…/finagle/package.scala instead of finagle-memcached/…/Memcached.scala).
  • CALLS: 58 removed / 60 added, all suffix_match/import_map heuristics reacting to the changed import sets; the 9 outright drops were wrong targets (MySqlExecutables.getBaseDir → Announcer.get, initDatabase → CookieMapBenchmark.map) that are now callee_suffix hits 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 import line, 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 from import 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_map 13 → 2,659; unique_name 9,283 → 7,894; qualified_suffix 1,897 → 1,169; callee_suffix 130 → 28.

Of the 1,162 removed, 1,054 are retargeted (same caller, same callee name, better target), e.g.

  • Simulation.main .map — was http.CookieMapBenchmark.map, now Stack.map
  • mux.Workload Tping — was Workload.Tping (self), now mux.transport.Message.Tping
  • StatsScopingTest.mkService + — was DtabBase.+, now Stack.Params.+

The remaining 108 are dropped: 101 self-loops (register → register where the body calls BalancerRegistry.get.register, sslSessionInfo → sslSessionInfo, …), 4 were Host bound to http.Fields.Host while the file actually aliases the package zipkin.{host => Host}, 2 same-file. One genuine loss: MutableSpan.toSpan → zipkin.core.Span(...) (was suffix_match @0.28 among 6 candidates). The now-correct import of thrift.thrift.Constants makes the Java sibling thrift.thrift.Span "import-reachable" for resolve_multi_with_imports, and the cross-language suffix_match guard then drops it. The right target is same-package Span, which same_module doesn'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 head
  • edge_imports (72 pass):
    • packages may ignore physical layout; Java import never binds to a Scala prefix declarer
    • Scala imports a Java class from a mixed package (Codec.java + Util.scala in com.acme.core)
    • Java import of a package declared by both Java and Scala files binds the Java file
    • two top-level Codec in a split package → no edge
    • nested Outer.Codec neither matches nor makes top-level Codec ambiguous (red on the previous head: 0 edges)
    • import Config.load → load Method; import Config.missing → no IMPORTS and no CALLS to Config (red on the previous head: the owner was bound)
    • chained package clauses resolve end to end
    • Java nested Request.Builder (directory-based QN, one segment below the module) neither binds import pkg.Builder nor makes the top-level Scala Builder ambiguous
    • incremental reindex keeps the IMPORTS (symbol + wildcard) and the run → build CALLS 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 binds Owner.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

  • Every commit is signed off (git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (full scripts/run-tests-parallel.sh on the rebased head)
  • Lint passes (lint-format with clang-format 21, lint-no-suppress, lint-memory-core; cppcheck 2.17.1 from the pip wheel on the touched files — no warning/style findings; CI runs 2.20)
  • New behavior is covered by a test (reproduce-first for bug fixes)

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

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:

  1. Member-import fallback to the owner (pass_pkgmap.c ~1963-1967). When pkg.Owner.member isn't found under the owner's QN, return owner; produces an IMPORTS edge with local_name=member that points at the class. resolve_import_map returns the import target directly for a bare callee (registry.c ~1004), so member() becomes a CALLS edge to the Owner class. 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.
  2. Package membership cost. scala_symbol_in_namespace walks 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 like apply in 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)?
  3. Top-level only. import pkg.Foo should match only a top-level Foo: the declaring file's module QN plus .Foo. Today a nested class or method named Foo can either become the target or make a real top-level Foo look ambiguous.

Smaller things, fine as follow-ups if you prefer:

  • Join consecutive package a / package b clauses 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.D means a.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 to Owner.
  • 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.

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

@htarnacki
htarnacki force-pushed the fix/scala-package-aware-imports branch from 1ccbb43 to 09f704d Compare September 27, 2026 19:57
@htarnacki

Copy link
Copy Markdown
Contributor Author

Thanks for the review — all three points are in the new head (rebased on main, still one commit), plus the three smaller items except relative imports.

1. Member import fails closed. import pkg.Owner.member now returns the member found under the owner's QN or NULL; the return owner; fallback and the !owner || acceptance are gone. On finagle this drops 71 IMPORTS edges, all import Owner.someVal (TimeoutFilter.TimeoutAnnotation, Method.Post, Handshake.CanTinitMsg, …) that used to point at the class. Test: ei_scala_unresolvable_member_import_yields_no_owner_edge asserts no IMPORTS from the importer and zero CALLS edges targeting any Config node, with import Config.load as the positive control (→ load Method).

2. O(1) membership. scala_symbol_in_namespace / node_file_in_namespace are removed. cbm_pipeline_scala_index_build walks the graph buffer once per pass (pass_definitions and pass_parallel, right after the namespace map) and fills a "<package>.<Name>" → node table from every file that declares a namespace; a second distinct node under the same key is replaced by an ambiguity marker. The resolver does one cbm_ht_get per declared-prefix step. The rels/namespaces arrays already built for the namespace map are reused, so there is no extra per-file work beyond one cbm_pipeline_fqn_module.

3. Top-level only. A node is indexed only when its QN is exactly <module QN>.<Name>, or the module QN itself with name == last segment (Java collapses class Foo in Foo.java onto the module QN — needed for the mixed-package case). Module/File nodes are excluded. On finagle this adds 152 IMPORTS (Path, ServiceFactory, Updater, …) that the old name scan reported as ambiguous because a nested class of the same name exists elsewhere in the package. Tests: ei_scala_import_ignores_nested_same_name (red on the previous head) and ei_scala_split_package_same_name_top_level_is_ambiguous.

Smaller items

  • Chained clauses: capture_namespace_decl joins directly following braceless package_clause siblings (comments skipped; a braced package b { … } is not joined). Extraction test + ei_scala_chained_package_clauses_resolve.
  • import pkg._ picks the lexicographically smallest declaring file — 270 wildcard edges on finagle retarget (e.g. import com.twitter.finagle._ → finagle-core/…/package.scala instead of finagle-memcached/…/Memcached.scala).
  • Relative imports (package a.b; import c.D): left for a follow-up issue as suggested; still the single lost edge (StabilizingAddr) on finagle.

Tests requested: ei_scala_imports_java_class_from_mixed_package, ei_java_import_of_mixed_package_binds_java_file, split-package ambiguity, unresolvable Owner.member, chained clauses — all in tests/test_edge_imports.c (70 pass), plus extract_scala_chained_package_clauses.

Numbers (PR body updated): vs main IMPORTS 323 → 6,310, CALLS 28,136 → 30,317; edges from non-Scala files identical apart from 10 SEMANTICALLY_RELATED. Vs the previous head: 5,818 IMPORTS identical, 71 removed (point 1), 152 added (point 3), 270 wildcard retargets; CALLS 58/60 churn, all heuristic re-targets, the 9 outright drops were wrong suffix_match hits (MySqlExecutables.getBaseDir → Announcer.get) now replaced by the file's own methods.

One housekeeping change: scripts/memory-core-baseline.txt lowers pass_pkgmap.c 82 → 80 — the new index allocates through mem_core, and the namespace-map key moved to cbm_mem_strdup so the file count stays below baseline (the cbm_pipeline_fqn_module result still needs one raw free).

cppcheck 2.17.1 (pip wheel) on the touched files reports no warning/style findings; the full parallel test run is green locally. #2361 and #2366 will be rebased on this head next, with the two cppcheck findings fixed.

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

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.

  1. Incremental reindex parity. Both incremental routes pass only the changed files (pipeline_incremental.c:1254 and :1385). So the namespace map and the new Scala index only know packages declared by changed files (pass_parallel.c:1709-1716). On main, 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 the import_map CALLS 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.
  2. Java nested members in the top-level index. Java class QNs are directory-based, so Outer in Outer.java is <dir>.Outer, which equals the stem module QN compared at pass_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 Java Request.Builder makes a real top-level Builder ambiguous, and the edge is lost. Using the language-aware module QN per file (the cbm_lang_module_is_dir rule, i.e. cbm_pipeline_fqn_module_dir) makes <dir>.<Name> the only top-level shape for Java and lets the qn == module_qn special case go. A test with a nested Java Builder next to a top-level Builder would pin it.
  3. 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 early return NULL when no Scala file has a package clause keeps them exactly as on main.
  4. Wildcard cost. Picking the smallest declaring file was our own request, sorry! It made first_namespace_file scan the whole package once per import 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.

@htarnacki
htarnacki force-pushed the fix/scala-package-aware-imports branch from 09f704d to 5a77358 Compare September 28, 2026 18:59
@htarnacki

Copy link
Copy Markdown
Contributor Author

Thanks for the second round — all four are in the new head 5a773582 (rebased on main 22784983, still one commit).

1. Incremental parity. Went with the File-node shape you suggested. pass_definitions / pass_parallel now write each file's clause as a package property on its File node (cbm_pipeline_file_node_set_package, appended to the existing {"extension":…} object). Both incremental routes read the clauses of the files they do not re-extract back from the store (incr_load_stored_namespaces: json_extract(properties,'$.package') over label='File', gated by LIKE '%"package"%' AND json_valid(...) so files without a clause never hit the JSON parser) and hand them to the passes as ctx->stored_namespaces. cbm_pipeline_import_maps_build builds the namespace map and the Scala index from changed ∪ stored, so both maps cover the project exactly as on a full build. The delta route loads after the purge (every remaining File row is a file the pass won't see); the legacy route loads before its purge and skips the changed and deleted rel paths explicitly. A failed read is treated as a route failure (delta.err / ABORT_PRESERVE_DB) rather than a silent divergence.

Test: ei_scala_incremental_reindex_keeps_imports_into_unchanged_packages — full index, then a body-only edit of Consumer.scala reindexed through the closure-repair route, then another through the legacy-partial route (cbm_pipeline_incremental_test_force_legacy_partial_once); after each, Consumer.scala must have exactly its two IMPORTS (Target + the _ wildcard bound to the package's smallest declaring file) and the run → build CALLS edge. Each step asserts the route through cbm_pipeline_incremental_test_last_route, so a fall-back to a full reindex cannot pass it. With the stored-clause read disabled it fails with IMPORTS: count=0 — which is the bug you described.

2. Java nested members. The index now computes the module QN per file with cbm_pipeline_fqn_module_dir(project, rel, cbm_lang_module_is_dir(lang)), so for Java the only top-level shape is <dir>.<Name>; the qn == module_qn special case is gone. Test: ei_scala_import_ignores_java_nested_type_of_same_name — Request.java with static class Builder next to Builder.scala in the same package; import com.acme.core.Builder binds Builder.scala. Red on the previous head (ambiguous → 0 edges).

On finagle this is worth more than I expected: +33 IMPORTS, all Scala imports of Java classes (ThriftClientRequest, ServerSetImpl, ZooKeeperClient, ZstdDecoder, RequestHeader, …) that the stem-module comparison had been dropping — the Java class QN is one segment below the stem module QN, so it never looked top-level. CALLS +12 net (import_map 2,711 → 2,730; 16 retargets/4 adds/1 drop, all in callers of those classes). All 6,310 previous IMPORTS are byte-identical; DEFINES and node count unchanged. PR body updated.

3. Index only for Scala. cbm_pipeline_import_maps_build builds the index only when a CBM_LANG_SCALA file is among the pass's files (importers, not declarers — a default-package Scala file still imports). Java/Kotlin/C#/PHP passes get the namespace map and nothing else, as on main. Test: pipeline_scala_index_built_only_for_scala_importers (Java-only → scala_index == NULL; one Scala file added → built). Header comment corrected.

4. Wildcard. first_namespace_file is gone. The index keeps, per package, the two lexicographically smallest File-node QNs, updated as files are visited during the single build walk; import pkg._ takes the smallest, or the runner-up when the smallest is the importer. O(1) per import.

The old cbm_pipeline_namespace_map_build{,_names} / cbm_pipeline_scala_index_build pairs are folded into one cbm_pipeline_import_maps_build / _free so the three call sites (sequential, parallel, and now the stored-namespace merge) cannot drift.

Tests/lint: edge_imports 72, pipeline 296, extraction 390, parallel 74, incremental 163, importance 13 — all green on the rebased head; clang-format clean on every touched file; cppcheck reports nothing new in the touched files.

Relative-import issue coming next, as an Issue. #2361 and #2366 will be rebased onto this head right after.

@htarnacki

Copy link
Copy Markdown
Contributor Author

New head bb3f1cdd (still one commit on main 22784983). No behaviour change against 5a773582 — the reason for the push is the lint / lint job, which was red on the memory-core ratchet (pass_pkgmap.c 80 → 88, pipeline_incremental.c 144 → 156 raw allocator sites) while lint-mem was green, so I had not noticed it locally.

What changed:

  • pass_pkgmap.c: the per-package wildcard record now borrows the two File nodes from the graph buffer instead of owning two heap QN copies (the File node is visited in the same index walk as the symbols, so no separate fqn_compute + find_by_qn per declaring file either). The File-node package property is written in the same loop that builds the namespace map, where the file QN already exists, so cbm_pipeline_file_node_set_package is gone from the header and from both pass call sites. Net effect: pass_pkgmap.c is back at exactly its baseline (80), pass_definitions.c and pass_parallel.c are two below theirs.
  • pipeline_incremental.c: incr_load_stored_namespaces allocates through cbm_mem_strdup / cbm_realloc / cbm_free (CBM_MEM_CLASS_OTHER), and the legacy route's ADR-read and stored-namespace failure exits share one cleanup block instead of a second copy of the nine free calls. Two below baseline.
  • scripts/memory-core-baseline.txt: the three improved files tightened as the linter asks (14 → 12, 31 → 29, 144 → 142).

Verified: make lint-memory-core passes (also with --strict), clang-format clean; edge_imports 72, pipeline 296, extraction 390, registry 68, parallel 74, incremental 163, importance 13 green; full scripts/run-tests-parallel.sh 8,236 pass with the only failures being the 15 test_cli install tests that need a non-group-writable install dir on this machine. finagle re-indexed with this head: IMPORTS 6,343 / CALLS 30,329, every (source, local name) → target pair byte-identical to 5a773582.

#2361 and #2366 are rebased onto this head and pushed.

@htarnacki

Copy link
Copy Markdown
Contributor Author

Relative-import follow-up filed as #2428, with a six-file repro covering the three forms (same-package import Owner.member, sub-package import util.Fmt, and import shared.Log relative to the outer clause of a chained package) measured on main and on this head, plus the StabilizingAddr edge as the finagle instance. Nothing else outstanding from my side on this PR.

@htarnacki htarnacki closed this Oct 1, 2026
@htarnacki htarnacki reopened this Oct 1, 2026
@htarnacki
htarnacki force-pushed the fix/scala-package-aware-imports branch from bb3f1cd to ba1946c Compare October 1, 2026 12:57
@htarnacki

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (0f52d30c) — bb3f1cdd → ba1946ce, no functional change.

The only conflict was in src/pipeline/pass_calls.c: main added pc_find_by_computed_qn() directly above build_import_map(), whose signature this PR extends with the CBMLanguage parameter. Resolved by keeping both.

Verified on the rebased head: registry 70, extraction 396, pipeline 306, edge_imports 72, parallel 76, incremental 163, importance 13 — all pass; lint-memory-core clean (no file grew), clang-format clean on touched files.

Context for the earlier red lint / lint on bb3f1cdd: it died in apt-get update (403 from packages.microsoft.com), before any project check ran. The reopen did not retrigger CI because main had meanwhile moved and the PR became unmergeable; this rebase fixes that.

@htarnacki

Copy link
Copy Markdown
Contributor Author

CI status on ba1946ce: 34 pass, 1 fail — the failure is infrastructure, not this change.

  • security / codeql-gate failed in "Wait for CodeQL on current commit (max 150 min)". The codeql.yml queue is backed up repository-wide: every CodeQL run from the last day, including pushes to main, is still queued, so the gate timed out before an analysis existed for this SHA.

Everything project-side is green: lint / lint (cppcheck, clang-format, memory-core ratchet), lint-mem, all test matrix jobs, smoke. I don't have permission to re-run jobs in this repo; a maintainer re-run of security once the CodeQL queue drains should turn ci-ok green with no change to the branch.

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 <hubert.tarnacki@gmail.com>
@htarnacki
htarnacki force-pushed the fix/scala-package-aware-imports branch from ba1946c to 14d4388 Compare October 5, 2026 10:10
@htarnacki

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (268a9d88): ba1946ce → 14d43883, no functional change.

One conflict, in src/pipeline/pass_pkgmap.c: main added the PHP PSR-4 resolver block (#1186) at the same spot where this PR adds the Scala top-level index. Both kept, PSR-4 first; the two don't interact (PSR-4 requires a \-separated name and a composer prefix, the Scala path is gated on language == CBM_LANG_SCALA).

Verified on the rebased head: registry 72, extraction 411, pipeline 323, edge_imports 78, parallel 76, incremental 164, importance 13 — all pass; lint-memory-core reports no file grew; clang-format clean on touched files. Full scripts/run-tests-parallel.sh on the stack head (#2366) is 8471 pass / 20 fail / 9 skip, where the 20 are cli install/uninstall activation-transaction cases that fail identically on clean main 268a9d88 in my environment (334/20 there too).

@htarnacki

Copy link
Copy Markdown
Contributor Author

@DeusData — status check on this series (#2360 → #2361 → #2366), to make sure I spend rebase effort where it's useful.

  • All r2 review items on A and B are addressed (replies inline above and on fix(scala): keep companion objects distinct from their classes (#2154) #2361); C only waited on A/B.
  • On 2026-10-06 the whole stack was fully green on main 268a9d88: 36 pass / 0 fail on every PR, including ci-ok, codeql-gate, the full test matrix and the memory-core ratchet (run for 14d43883).
  • main has since moved ~740 commits and the stack conflicts again in pass_pkgmap.c, pass_definitions.c, pass_parallel.c, pass_calls.c, pipeline_incremental.c, pipeline_internal.h, tests/test_pipeline.c and the baseline — the same files your identity / member-resolution work is touching. This is the fourth rebase in two weeks; each previous one went stale within 1–3 days before a review happened.

Two ways forward, whichever suits you:

  1. If you intend to review this series, say so and I'll rebase A/B/C onto the then-current main within a day and keep it green until it lands.
  2. If Scala package-aware imports / companion identity (Scala: parse import selectors/aliases/wildcards and resolve imports against the declared package instead of the directory layout #2153, Scala: companion object and class share one qualified name, so their members overwrite each other #2154, Scala: receiver.method(...) calls are bound to an arbitrary same-named symbol by short name (weak-member suppression not applied) #2155, Scala: companion $ tolerance stops at the top-level owner, so import pkg.Outer.Inner.member / Outer.Inner.member() fail closed #2426, Scala: relative imports (import Owner.member in the same package, import subpkg.Name) are never resolved; fail closed since #2360 #2428) are going to be handled as part of your identity rework instead, I'm happy to close these PRs and leave the issues (synthetic repros included) as the spec. No hard feelings — I'd just rather not keep resolving the same conflicts without a reviewer on the other end.

Thanks.

@DeusData DeusData added enhancement New feature or request parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency. labels Oct 19, 2026
@DeusData

Copy link
Copy Markdown
Owner

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.

@DeusData

Copy link
Copy Markdown
Owner

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.

@DeusData

DeusData commented Nov 1, 2026

Copy link
Copy Markdown
Owner

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:

  1. main has moved a lot in the same files (pass_pkgmap.c, pass_definitions.c, pass_parallel.c, pass_calls.c, pipeline_incremental.c, pipeline_internal.h, tests/test_pipeline.c), and the memory-core baseline lines will need to be recomputed from the linter on the rebased tree.
  2. main is at index format 4 (src/store/store.h, CBM_INDEX_FORMAT_VERSION; fix(extract): receiver-qualify Go method QNs #1913 made Go method QNs receiver-qualified), in case a Go expectation in the touched tests moves.
  3. Please leave out the reformatting of extract_ts_await_generic_call_issue2210 in tests/test_extraction.c; it is unrelated to this change.

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 package property on its File nodes, so the first incremental run after an upgrade could resolve Scala imports against an incomplete package map. We will bump the index format in a follow-up commit, so every existing index is rebuilt once with the new property instead. The new package property on File nodes (Java, Kotlin, Scala, C#, PHP) is welcome as it is. Please pull before pushing again after that.

#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.

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

enhancement New feature or request parsing/quality Graph extraction bugs, false positives, missing edges priority/normal Standard review queue; useful PR with ordinary maintainer urgency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scala: parse import selectors/aliases/wildcards and resolve imports against the declared package instead of the directory layout

2 participants