Conversation
…ation JCodeanalyzer._add_type raised while flattening the containment tree, so an application carrying duplicate qualified names could not be loaded at all -- and every query became unavailable, including the overwhelming majority naming no duplicated type. The realistic input is a monorepo whose services vendor a shared internal library, which puts several copies of one package into one analysis. Surfacing the ambiguity was right; refusing the whole application for it was not. Both backends now record the competing files while indexing and refuse at the name-addressed query instead, through one shared guard so they fail at the same point on the same input -- previously the in-memory backend raised at construction while the Neo4j backend's cached _idx raised lazily, despite a docstring claiming they mirror each other. Nothing is dropped: callables stay keyed by their own can:// id, which is distinct per copy, so every copy remains addressable even while its name is not. Public signatures are untouched, so no caller changes -- get_class still returns JType | None, and the 113 public-surface tests pass unchanged. That was the reason for preferring this over widening the return types to a collection: those methods live on the shared cross-language ABC, so the plural shape would have touched 44 definitions across 10 files and charged Python and TypeScript for a Java monorepo's vendored copies. The message no longer attributes the duplication to the analyzer, which emitted both declarations correctly with distinct ids, and now names the files that collided so the reader can act on it. Closes #420
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #420. Related: codellm-devkit/codeanalyzer-java#269 (where the duplicates originate).
The defect
JCodeanalyzer._add_typeraised while flattening the containment tree, andself._index()is the last line of__init__— so an application carrying duplicate qualified names could not be loaded, and every query became unavailable, including the overwhelming majority naming no duplicated type. The realistic input is a monorepo whose services vendor a shared internal library, putting several copies of one package into one analysis.Surfacing the ambiguity was right. Refusing the whole application for it was not.
The change
Both backends record the competing files while indexing and refuse at the name-addressed query, through one shared guard on
JavaAnalysisBackend:That also settles an inconsistency: the in-memory backend raised at construction while
JNeo4jBackend._idxis a@cached_propertyand raised lazily, despite a docstring saying it "MirrorsJCodeanalyzer._index". They now fail at the same point on the same input, which the new test asserts directly.Nothing is dropped. Callables stay keyed by their own
can://id, distinct per copy, so every copy remains addressable even while its name is not.The message no longer blames the analyzer — it emitted both declarations correctly, with distinct ids; the duplication is a property of the source tree — and now names the colliding files so the reader can act on it:
Why not "return all candidates"
Considered and rejected; reasoning recorded on #420. The name-addressed methods are on the shared cross-language ABC (
cldk/analysis/commons/backend.py:119-136), so widening their return types touches 44 definitions across 10 files — the ABC plus an in-memory backend, a Neo4j backend and a facade for each of Java, Python and TypeScript — and charges Python and TypeScript for a Java monorepo's vendored copies. A well-formed single-service analysis has exactly one class per FQN. If enumerating copies becomes a real need, the additive move is a parallelget_classes(name) -> List[T], which breaks nothing.Public signatures are untouched —
get_classstill returnsJType | None. The 113 public-surface tests pass unchanged, which is the evidence for that claim rather than an assertion of it.Tests
tests/analysis/java/test_java_duplicate_fqn.py, 5 cases written failing-first. The fixture copies one module of the real daytrader8 analysis to a second path, rewriting only itscan://ids — exactly what a vendored copy looks like to the analyzer.can://idVerification
1795 passed, 4 failed, 404 skipped. All four failures are intest_java_e2e.pyand environmental — the locally installedcodeanalyzer-javais 3.1.1 against a 3.3.3 pin, and two of them state outright that they are "Measured off the 3.3.3 jar". Confirmed pre-existing by re-running on a stashed tree.Sequencing
Worth landing with or behind codellm-devkit/codeanalyzer-java#269. The index-time raise was the only mechanism telling a caller the input was unsafe; bundled analyses now load, and until #269 lands they will load with the wrong-copy call edges the analyzer emits. This PR makes the ambiguity precise, not the resolution correct.
Base branch
Opened against
release/2.0, which is the checked-out line; retarget tomainif that is wrong.