Conversation
DeusData
left a comment
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @htarnacki. This is a really careful piece of work, and the finagle numbers together with the honest residual-cost section (the 223 same-file suffix_match edges) make it easy to review.
The new commit itself (06f97613) looks right to us. The weak-member guard is Scala-gated in both resolvers (pass_calls.c and its pass_parallel.c twin). The same-file exemption is Scala-only in registry.c, the extractor flag is set only under CBM_LANG_SCALA, and there's one cbm_gbuf_find_by_qn per call, so there's no quadratic pattern. That is exactly the per-language shape we want for a suppressor. The tests would fail without the change, too.
The only thing holding it up is procedural. This branch carries #2360 and #2361 as its first two commits, so the changes we requested on #2360 are inside this diff as well: the owner-fallback edges, the per-hit membership scan, and the top-level-only handling. Once those land, a rebase down to 06f97613 should be quick to approve. A follow-up issue for receiver-type inference would be very welcome. Thank you again!
|
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. |
06f9761 to
06ef2f6
Compare
|
Thanks. Rebased onto the reworked #2360 (
Re-measured on finagle against the new stack (PR body updated): CALLS 30,311 → 21,161 (−9,150, 0 added), Follow-up issue for receiver-type inference: #2384. Once #2360 and #2361 merge I will rebase so the diff collapses to this one commit. |
|
Thank you, @htarnacki. We diffed Our view is unchanged: this commit is ready. The landing order is #2360 -> #2361 -> this one, and we'll approve it once the first two land and the branch collapses to this commit. Thank you also for filing #2384 so quickly. We keep Issues for actionable defects, so the receiver-type-inference idea was moved to Discussions. Sorry for the mixed signal after we asked you for an issue; the Discussion, linked to this PR, is the right home for it. |
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>
A Scala `class Foo` (or trait/enum) and its companion `object Foo` share a source name, so both containers and their methods were assigned the same qualified name and overwrote each other in the graph. Use scalac's `Foo$` suffix for the companion object in every place a class QN is computed. A standalone `object Foo` with no companion keeps its plain QN, so existing graphs only change where two nodes used to collide. Resolution treats the pair as one importable/callable name: - pass_pkgmap: the companion enters the top-level index under `Foo$`, so `import pkg.Foo` binds the class (which stands for the pair) and never trips the split-package ambiguity marker; `import pkg.Foo.member` looks the member up under `Foo$` first, since only object members are importable. - registry: `Foo.make()` reaches `Foo$.make` through import_map, same_module, the qualified-tail disambiguator and the receiver-chain guard. A `$`-terminated segment is emitted only by this change, so the comparison is exact equality for every other language. Class-body `val`/`var` definitions keep their module-level QN like every other language (parent_class already records the declaring owner). Closes DeusData#2154 Signed-off-by: Hubert Tarnacki <hubert.tarnacki@gmail.com>
Scala has no LSP resolver, so every receiver call reaches the registry. `values.get(k)`, `xs.foreach(_.register())` or `xs contains n` then bind an arbitrary project-wide `get` / `register` / `contains` by short name (suffix_match, unique_name, field_type_hint) and produce CALLS edges to unrelated classes. Python and TS/JS already avoid this through the receiver-aware weak-member guard (DeusData#592/DeusData#606/DeusData#1276); extend it to Scala. extract_calls.c sets `is_method` for a call that names a lower-case method on a VALUE receiver: `recv.m(...)`, curried `recv.m(a)(b)`, named infix `recv m arg`, and placeholder `_.m()`. Calls on `this`/`super`, on chains rooted at an upper-case name (`Utils.helper`, `Bijections.finagle.m`) and upper-case applies through a package path (`http.param.Streaming(x)`) stay unflagged — the registry's receiver-chain check already judges those. Bare calls are untouched. pass_calls.c / pass_parallel.c enable the guard for CBM_LANG_SCALA with one exemption (cbm_weak_member_same_file_exempt): a unique_name / field_type_hint match whose target sits in the caller's own file keeps its edge, so `d.describe()` still binds the inherited method the same file declares. suffix_match — several same-named candidates picked by distance — stays suppressed even when it lands nearby. Measured on twitter/finagle (~1,900 Scala files, on top of DeusData#2153/DeusData#2154): CALLS 30,309 -> 21,159 (-9,149, 0 added; IMPORTS unchanged). 72% of the removed edges target a class that is never named anywhere in the caller file; 82% had more than one candidate. Residual recall cost: 223 same-file suffix_match edges (delegating wrappers such as `Client.set -> BaseClient.set`) go with them. Closes DeusData#2155 Signed-off-by: Hubert Tarnacki <hubert.tarnacki@gmail.com>
06ef2f6 to
4a0f966
Compare
|
Rebased onto the new #2360 ( Re-measured on finagle against the new stack (PR body updated): CALLS 30,323 → 21,177 (−9,146, 0 added), Tests: |
This branch has not been deployed
2 participants
What does this PR do?
Closes #2155. Stacked on #2361 (which is stacked on #2360): the first two commits here are those PRs, only the last commit (
4a0f9660) is this one. Once they merge I'll rebase and the diff collapses to that commit.Scala has no LSP resolver, so every receiver call reaches the registry.
values.get(k),xs.foreach(_.register())orxs contains nthen bind an arbitrary project-wideget/register/containsby short name (suffix_match,unique_name,field_type_hint) and produce CALLS edges to unrelated classes. Python and TS/JS already avoid this through the receiver-aware weak-member guard (#592/#606/#1276); this PR extends that guard to Scala rather than adding a new mechanism.extract_calls.c—scala_call_is_value_membersetsis_methodfor a call that names a lower-case method on a value receiver:recv.m(...), curriedrecv.m(a)(b), named infixrecv m arg, placeholder_.m(). Left unflagged, because the registry'sreceiver_chain_admitsalready judges them:this.m()/super.m(), chains rooted at an upper-case name (Utils.helper(),Bijections.finagle.toStack(x)) and upper-case applies through a package path (http.param.Streaming(x)). Bare calls are untouched. Symbolic operators (a + b) are not methods for this purpose.pass_calls.c/pass_parallel.c— addCBM_LANG_SCALAtosuppress_weak_memberin both resolvers (the two gates must stay identical, per the existing comment). One exemption,cbm_weak_member_same_file_exemptinregistry.c: aunique_name/field_type_hintmatch whose target sits in the caller's own file keeps its edge, sod.describe()still binds the inherited method the same file declares (themkc_c7_scala_inherited_methodshape).suffix_match— several same-named candidates picked by distance — stays suppressed even when it lands nearby; see the residual below for what that costs.Before / after on
twitter/finagle(ca472de, 1,898.scalafiles)#2361 head vs this branch, fresh
CBM_CACHE_DIReach run, edge identity normalized as in #2361.suffix_matchunique_namefield_type_hintimport_map/same_module/qualified_suffix/lsp_*Precision proxy for the 9,145 removed edges (normalized; one more is a
$-only rename): for each one I checked whether the target's owner class is named anywhere in the caller's source file (import, type annotation, constructor). 72% (6,596) target a class that the caller file never mentions —Bufs.split → CookieMapBenchmark.map,Netty4FormPostEncoder.makeNetty4Request → Stack.foreach,TracingFilterTest → postgresql.PgBuf.Reader.collect. 82% had more than one candidate recorded. Retargeting (a weaker edge replaced by a stronger one) accounts for 85 of them.Residual recall cost, stated plainly. 219 of the removed edges were same-file
suffix_matchand a good share of those were right: delegating wrappers (memcached.Client.set → BaseClient.set,Transport.read → QueueTransport.read) and builder chains (MutableSpan.copyForImmediateLogging → MutableSpan.setName). The same shape also produced wrong ones —underlying.close()insideRefPushSessionbound toRefPushSession.closeout of 180 candidates — which is why I did not extend the exemption tosuffix_match. Among the 28% of removals whose owner is mentioned in the caller file there are correct edges too (Memcached.serve → Server.serve,CachingPool.checkout → Service.close); recovering those needs receiver-type inference (field/param type → owner), which is a different change from this guard. Happy to open a follow-up issue for that if wanted.The TESTS drop is the same mix:
TypeTest.testRow → Row.intOrZero(correct, lost) alongsideRedisTest.testDecodingInChunks → CookieMapBenchmark.map(noise, gone).Tests
extraction(394):extract_scala_member_call_flags_is_methodpins the flag onxs.foreach,_.register,values.get, curriedvalues.getOrElse(..)(..), infixxs contains n; and pins it off forthis.helper(),super.finish(),Utils.helper(),Bijections.finagle.toStack(n),http.param.Streaming(n), barehelper(),n + 1.pipeline(300):pipeline_scala_receiver_suppresses_weak_method_edges+ the parallel-resolver twin —registerAll → register,lookup → get,has → containsproduce no edge across files with same-named methods; controlscallsLocal → localHelper(bare,unique_name, cross-file) andshow → describe(d.describe()on an inherited method declared in the same file) keep theirs. Red without the guard (three noise edges) and red without the exemption (describelost).registry(72):cbm_weak_member_same_file_exempt—unique_name/field_type_hint+ same file → exempt;suffix_match, cross-file, non-Scala, NULL inputs → not.edge_imports(72),matrix_known_classes(43) unchanged.Checklist
git commit -s) — required, CI rejects unsigned commits (DCO, see CONTRIBUTING.md)extraction,pipeline,registry,edge_imports,matrix_known_classessuites; the 15 failures in the full runner are alltest_cli.cinstall/uninstall paths — environment-dependent, none touch the pipeline; fullmake -f Makefile.cbm testleft to CI)lint-formatwith clang-format 21,lint-no-suppress;cppcheck2.17.1 with thelint-cppcheckflag set clean on the touched files)