From 983e3404052fbfc77faa4e70c10bfb89eaae352a Mon Sep 17 00:00:00 2001 From: Rahul Krishna Date: Thu, 17 Sep 2026 15:49:38 -0400 Subject: [PATCH] fix(java): a contested name fails its own query, not the whole application 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 --- cldk/analysis/java/backend.py | 34 ++++- .../java/codeanalyzer/codeanalyzer.py | 15 ++- cldk/analysis/java/neo4j/neo4j_backend.py | 18 ++- .../analysis/java/test_java_duplicate_fqn.py | 123 ++++++++++++++++++ 4 files changed, 178 insertions(+), 12 deletions(-) create mode 100644 tests/analysis/java/test_java_duplicate_fqn.py diff --git a/cldk/analysis/java/backend.py b/cldk/analysis/java/backend.py index 2cace7b..89f2b5d 100644 --- a/cldk/analysis/java/backend.py +++ b/cldk/analysis/java/backend.py @@ -512,12 +512,27 @@ class _Addressing(NamedTuple): candidates: List[CallableCandidate] -def duplicate_type_name(qualified_name: str) -> str: - """The defect message for two declarations that spell one qualified name — which would make a +def duplicate_type_name(qualified_name: str, paths: List[str]) -> str: + """The message for two declarations that spell one qualified name — which makes a ``get_call_graph()`` node key and a ``get_class()`` key ambiguous, so it is surfaced rather than letting the second silently shadow the first. Both backends raise this text, identically, and - it names only the qualified name: a ``can://`` id must not appear in a message (E6).""" - return f"type qualified name {qualified_name!r} is declared twice: codeanalyzer-java emitted two declarations that spell one name" + it names the qualified name and the files that declare it: a ``can://`` id must not appear in a + message (E6). + +

It names the files because the reader cannot otherwise act on it. It no longer attributes the + duplication to the analyzer: two compilation units genuinely declaring one name is a property of + the source tree — the realistic case being services that vendor one shared library — and the + analyzer emitted both correctly, with distinct ids. + +

Raised at the ambiguous *query*, never while indexing. Refusing at index time made an entire + application unloadable over a handful of contested names, when every other name in it answers + perfectly well (#420).""" + where = ", ".join(sorted(paths)) + return ( + f"type qualified name {qualified_name!r} is declared in {len(paths)} files ({where}): " + "several declarations spell one name, so a query naming it cannot be answered — " + "analyse the copies separately, or deduplicate them in source" + ) def unhomed_endpoint(node_id: str) -> str: @@ -548,6 +563,17 @@ class JavaAnalysisBackend(AnalysisBackend[JApplication, JCompilationUnit, JType, P: ClassVar[str] = "J" N: ClassVar[str] = "J" + def _refuse_if_contested(self, qualified_class_name: str) -> None: + """Refuse a name-addressed query whose name several declarations spell. + + The index keeps every copy reachable by its own ``can://`` id, so nothing is lost and the + application always loads; what cannot be answered is a question phrased as a *name*, because + the name genuinely denotes more than one declaration. Both backends route their + name-addressed lookups through here, so they fail at the same point on the same input.""" + paths = getattr(self, "_contested", {}).get(qualified_class_name) + if paths: + raise CodeanalyzerExecutionException(duplicate_type_name(qualified_class_name, paths)) + # ===================================================================================== # The addressing surface (leg 3b, Task 1): locate / resolve / source / describe. # diff --git a/cldk/analysis/java/codeanalyzer/codeanalyzer.py b/cldk/analysis/java/codeanalyzer/codeanalyzer.py index c31e78e..0d1a485 100644 --- a/cldk/analysis/java/codeanalyzer/codeanalyzer.py +++ b/cldk/analysis/java/codeanalyzer/codeanalyzer.py @@ -340,6 +340,10 @@ def _index(self) -> None: self._types: Dict[str, JType] = {} self._file_of: Dict[str, str] = {} self._callables: Dict[str, Tuple[JType, JCallable]] = {} + #: qualified name → every file declaring it, for the names more than one file declares. + #: Populated while flattening and consulted by name-addressed queries; see + #: :meth:`JavaAnalysisBackend._refuse_if_contested`. + self._contested: Dict[str, List[str]] = {} for path, unit in self.application.symbol_table.items(): for t in unit.types.values(): self._add_type(t, path) @@ -347,9 +351,13 @@ def _index(self) -> None: def _add_type(self, t: JType, path: str) -> None: name = t.qualified_name if name in self._types: - raise CodeanalyzerExecutionException(duplicate_type_name(name)) - self._types[name] = t - self._file_of[name] = path + # Recorded, not raised: the copies stay reachable by their own ids, and only a query + # phrased as this name is unanswerable. Refusing here would lose the whole application + # over one contested name (#420). + self._contested.setdefault(name, [self._file_of[name]]).append(path) + else: + self._types[name] = t + self._file_of[name] = path for c in t.callables.values(): self._callables[c.id] = (t, c) for lt in c.types.values(): @@ -823,6 +831,7 @@ def get_all_classes(self) -> Dict[str, JType]: return dict(self._types) def get_class(self, qualified_class_name: str) -> JType | None: + self._refuse_if_contested(qualified_class_name) return self._types.get(qualified_class_name) def get_all_methods_in_application(self) -> Dict[str, Dict[str, JCallable]]: diff --git a/cldk/analysis/java/neo4j/neo4j_backend.py b/cldk/analysis/java/neo4j/neo4j_backend.py index 103b2cc..faa0bc7 100644 --- a/cldk/analysis/java/neo4j/neo4j_backend.py +++ b/cldk/analysis/java/neo4j/neo4j_backend.py @@ -645,7 +645,7 @@ def application(self) -> JApplication: return self._application @cached_property - def _idx(self) -> Tuple[Dict[str, JType], Dict[str, str], Dict[str, Tuple[JType, JCallable]]]: + def _idx(self) -> Tuple[Dict[str, JType], Dict[str, str], Dict[str, Tuple[JType, JCallable]], Dict[str, List[str]]]: """The containment tree flattened once: every type (top-level, nested, local/anonymous) by its source-spelled qualified name, its file, and every callable by its ``can://`` id — the join that turns a call-graph endpoint into the ``"."`` node key. @@ -653,13 +653,16 @@ def _idx(self) -> Tuple[Dict[str, JType], Dict[str, str], Dict[str, Tuple[JType, types: Dict[str, JType] = {} file_of: Dict[str, str] = {} callables: Dict[str, Tuple[JType, JCallable]] = {} + contested: Dict[str, List[str]] = {} def add(t: JType, path: str) -> None: name = t.qualified_name if name in types: - raise CodeanalyzerExecutionException(duplicate_type_name(name)) - types[name] = t - file_of[name] = path + # See JCodeanalyzer._add_type: recorded here, refused at the query. + contested.setdefault(name, [file_of[name]]).append(path) + else: + types[name] = t + file_of[name] = path for c in t.callables.values(): callables[c.id] = (t, c) for local in c.types.values(): @@ -670,7 +673,7 @@ def add(t: JType, path: str) -> None: for path, unit in self._application.symbol_table.items(): for t in unit.types.values(): add(t, path) - return types, file_of, callables + return types, file_of, callables, contested @property def _types(self) -> Dict[str, JType]: @@ -684,6 +687,10 @@ def _file_of(self) -> Dict[str, str]: def _callables(self) -> Dict[str, Tuple[JType, JCallable]]: return self._idx[2] + @property + def _contested(self) -> Dict[str, List[str]]: + return self._idx[3] + # -----[ the addressing surface (leg 3b) — the three facts the shared implementation needs ]----- #: The one statement leg 3b's Task 1 adds. Anchored on the **bare** ``:JBodyNode`` label and a #: per-callable id prefix, which is the narrowest predicate on this surface. @@ -1325,6 +1332,7 @@ def get_all_classes(self) -> Dict[str, JType]: return dict(self._types) def get_class(self, qualified_class_name: str) -> JType | None: + self._refuse_if_contested(qualified_class_name) return self._types.get(qualified_class_name) def get_all_methods_in_application(self) -> Dict[str, Dict[str, JCallable]]: diff --git a/tests/analysis/java/test_java_duplicate_fqn.py b/tests/analysis/java/test_java_duplicate_fqn.py new file mode 100644 index 0000000..f4b29d3 --- /dev/null +++ b/tests/analysis/java/test_java_duplicate_fqn.py @@ -0,0 +1,123 @@ +################################################################################ +# Copyright IBM Corporation 2026 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +################################################################################ + +"""Duplicate qualified names must not make the whole application unloadable (#420). + +A monorepo whose services vendor a shared internal library puts several copies of one package +into one analysis, so two compilation units legitimately declare one qualified name. The analyzer +emits both, with distinct ``can://`` ids. The SDK used to raise while *flattening* that tree, which +meant no query at all was available — including the overwhelming majority naming no duplicated +type. These pin the ambiguity to the queries it actually affects. +""" + +import json + +import pytest + +from cldk.analysis.java.codeanalyzer.codeanalyzer import JCodeanalyzer +from cldk.analysis.java.neo4j import JNeo4jBackend +from cldk.models.java.models import JAnalysis +from cldk.utils.exceptions.exceptions import CodeanalyzerExecutionException + +VENDORED = "vendored/" + + +def _duplicate_one_module(analysis_json: str): + """Copy one module to a second path, keeping every name it declares and rewriting only its + ``can://`` ids — exactly what a vendored copy of a library looks like to the analyzer.""" + doc = json.loads(analysis_json) + table = doc["application"]["symbol_table"] + source_key = next(k for k, m in table.items() if m.get("types")) + # Rewriting the key inside the serialized module rewrites the module id and every type and + # callable id beneath it in one pass, so the copy is id-distinct and name-identical. + copy = json.loads(json.dumps(table[source_key]).replace(source_key, VENDORED + source_key)) + table[VENDORED + source_key] = copy + # `types` is keyed by simple name; the qualified name the index keys on is package-qualified. + module = table[source_key] + fqn = f"{module['package']}.{next(iter(module['types']))}" + return json.dumps(doc), source_key, fqn + + +@pytest.fixture +def duplicated(analysis_json): + payload, source_key, fqn = _duplicate_one_module(analysis_json) + return payload, source_key, fqn + + +def _in_memory(payload: str) -> JCodeanalyzer: + """A JCodeanalyzer over a seeded application, bypassing the analyzer subprocess that + ``__init__`` would otherwise drive.""" + backend = JCodeanalyzer.__new__(JCodeanalyzer) + backend.application = JAnalysis.model_validate_json(payload).application + backend._call_graph = None + backend._sdg_cache = None + backend._index() + return backend + + +def _neo4j(payload: str) -> JNeo4jBackend: + backend = JNeo4jBackend.__new__(JNeo4jBackend) + backend.application_name = "daytrader8" + backend.__dict__["_application"] = JAnalysis.model_validate_json(payload).application + return backend + + +def test_an_application_carrying_duplicate_qualified_names_still_loads(duplicated): + payload, _, _ = duplicated + # Flattening must not refuse: the duplication is in the source tree, and every other name in + # the application is perfectly answerable. + _in_memory(payload) + + +def test_a_name_declared_once_answers_as_it_always_did(duplicated): + payload, source_key, duplicated_fqn = duplicated + backend = _in_memory(payload) + table = json.loads(payload)["application"]["symbol_table"] + unambiguous = next( + f"{module['package']}.{name}" + for key, module in table.items() + if not key.startswith(VENDORED) and key != source_key and module.get("package") + for name in (module.get("types") or {}) + ) + assert backend.get_class(unambiguous) is not None + + +def test_the_contested_name_raises_at_the_query_and_names_the_competing_files(duplicated): + payload, source_key, duplicated_fqn = duplicated + backend = _in_memory(payload) + with pytest.raises(CodeanalyzerExecutionException) as excinfo: + backend.get_class(duplicated_fqn) + message = str(excinfo.value) + assert duplicated_fqn in message + # The reader needs to know WHICH copies collided, or they cannot act on it. + assert source_key in message and VENDORED + source_key in message + + +def test_every_copy_stays_addressable_by_its_own_can_id(duplicated): + payload, source_key, duplicated_fqn = duplicated + backend = _in_memory(payload) + # Nothing is dropped: ids are distinct per copy, so both copies' callables remain indexed. + ids = [cid for cid in backend._callables if VENDORED + source_key in cid] + assert ids, "the vendored copy's callables must still be reachable by id" + + +def test_the_neo4j_backend_agrees_with_the_in_memory_one(duplicated): + payload, source_key, duplicated_fqn = duplicated + backend = _neo4j(payload) + # Building the index must not raise here either, and the same query must fail the same way. + backend._idx + with pytest.raises(CodeanalyzerExecutionException): + backend.get_class(duplicated_fqn)