Repository navigation
Conversation
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
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 <hubert.tarnacki@gmail.com>
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. |
|
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. |
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)