From 8cd12c364082eb2780808e55ce7e525b1eef0e96 Mon Sep 17 00:00:00 2001 From: Aman Goel Date: Sun, 18 Oct 2026 19:25:18 +0530 Subject: [PATCH] fix(move-method): keep static utilities out of data classes and reversed dependencies (#3830) --- .../health/refactoring/move_method.py | 135 ++++++++++++ .../core/ingestion/language_configs.py | 3 + .../core/ingestion/parser/symbol_rules.py | 35 +++ .../repowise/core/ingestion/parser/symbols.py | 6 +- .../core/src/repowise/core/upgrade/version.py | 6 +- .../health/test_refactoring_move_method.py | 204 ++++++++++++++++++ tests/unit/ingestion/parser/golden/java.json | 16 +- tests/unit/ingestion/parser/test_misc.py | 18 +- 8 files changed, 415 insertions(+), 8 deletions(-) diff --git a/packages/core/src/repowise/core/analysis/health/refactoring/move_method.py b/packages/core/src/repowise/core/analysis/health/refactoring/move_method.py index eedb6119f3..7cefa7cfd0 100644 --- a/packages/core/src/repowise/core/analysis/health/refactoring/move_method.py +++ b/packages/core/src/repowise/core/analysis/health/refactoring/move_method.py @@ -55,10 +55,23 @@ class or a C# ``static class`` is never a home for instance behaviour, and neither is a class in another language family (a Swift call the graph resolved onto a Kotlin class of the same name). The other ``partial`` fragments of the method's own class are its own class. + +Two more rules keep static utilities out of the data classes they read: + +- a **static** method (Java ``static``, a Kotlin ``object`` / ``companion + object`` function) has no instance of its own to be envious from, so the + getters it calls on a parameter are its inputs, not envy. It never moves + into a class whose members it uses are all accessors (``getX`` / ``isX`` / + ``setX``, a record component): an entity, a DTO or a value class. +- a move never **reverses a dependency**. On the JVM, when the method calls + something that is not on the target and the target's file cannot see it (no + import, not in its package), the target would have to depend on the + method's home, so that target is not a home for the method. """ from __future__ import annotations +import posixpath import re from typing import Any @@ -119,6 +132,13 @@ class or a C# ``static class`` is never a home for instance behaviour, and ) # Python marks a factory with a decorator. _FACTORY_DECORATORS = ("@staticmethod", "@classmethod") +# ``getX`` / ``isX`` / ``setX``: the accessor shape of a bean, an entity or a +# DTO. ``get`` alone (a map-style lookup) is behaviour, so a suffix is needed. +_ACCESSOR_NAME = re.compile(r"^(?:get|is|set)[A-Z0-9_]") +# Languages whose imports and packages decide whether one file can see another. +_VISIBILITY_LANGUAGES = frozenset({"java", "kotlin"}) +# File-to-file edges that mean "this file reads that one". +_VISIBLE_EDGE_TYPES = frozenset({"imports", "type_use"}) def _node(graph: Any, node_id: str) -> dict | None: @@ -296,6 +316,111 @@ def _binds_method(cls: Any, name: str) -> bool: return any(name in g.methods and (g.fields or g.calls) for g in groups) +def _is_static(data: dict) -> bool: + """Whether the method has no instance: Java ``static``, a Kotlin + ``object`` / ``companion object`` function (the parser records both in + ``modifiers``).""" + return "static" in (data.get("modifiers") or ()) + + +def _split_params(text: str) -> list[str]: + """Split a parameter list on its top-level commas (``Map m`` is one).""" + parts: list[str] = [] + depth = 0 + start = 0 + for i, ch in enumerate(text): + if ch in "<([": + depth += 1 + elif ch in ">)]": + depth -= 1 + elif ch == "," and depth == 0: + parts.append(text[start:i]) + start = i + 1 + parts.append(text[start:]) + return [p.strip() for p in parts if p.strip()] + + +def _constructor_param_names(graph: Any, class_id: str, cache: dict[str, set[str]]) -> set[str]: + """The parameter names of *class_id*'s constructors. A record's components + are its canonical constructor's parameters; each has a same-named accessor.""" + cached = cache.get(class_id) + if cached is not None: + return cached + class_name = _class_name(graph, class_id) + names: set[str] = set() + for member in _class_members(graph, class_id, {}): + node = _node(graph, member) or {} + signature = node.get("signature") or "" + if node.get("name") != class_name or "(" not in signature: + continue + params = signature[signature.index("(") + 1 : signature.rindex(")")] + for param in _split_params(params): + # ``String appId`` / ``final List ids`` / ``val appId: String`` + declared = param.split(":", 1)[0].split("=", 1)[0].split() + if declared: + names.add(declared[-1].lstrip("*&")) + cache[class_id] = names + return names + + +def _is_accessor(graph: Any, member_id: str, component_names: set[str]) -> bool: + """A ``getX`` / ``isX`` / ``setX`` member, or the no-argument accessor of a + record component (``appId()``).""" + node = _node(graph, member_id) or {} + name = node.get("name") or "" + if _ACCESSOR_NAME.match(name): + return True + signature = node.get("signature") or "" + return ( + name in component_names and re.search(rf"\b{re.escape(name)}\(\s*\)", signature) is not None + ) + + +def _only_accessors( + graph: Any, class_id: str, members: set[str], cache: dict[str, set[str]] +) -> bool: + """Whether every member in *members* is an accessor of *class_id*: the class + is a data holder as far as this method is concerned.""" + component_names = _constructor_param_names(graph, class_id, cache) + return bool(members) and all(_is_accessor(graph, m, component_names) for m in members) + + +def _file_of(graph: Any, node_id: str) -> str | None: + return (_node(graph, node_id) or {}).get("file_path") + + +def _can_see(graph: Any, from_file: str, to_file: str) -> bool: + """Whether code in *from_file* can name what *to_file* declares: the same + file, the same package directory, or an import / type reference to it.""" + if from_file == to_file: + return True + if posixpath.dirname(from_file) == posixpath.dirname(to_file): + return True + if from_file not in graph or to_file not in graph: + return False + data = graph.get_edge_data(from_file, to_file) + if not data: + return False + edges = data.values() if graph.is_multigraph() else (data,) + return any(e.get("edge_type") in _VISIBLE_EDGE_TYPES for e in edges) + + +def _reverses_dependency( + graph: Any, language: str, target_file: str, other_callees: set[str] +) -> bool: + """Whether moving the method into *target_file* would make that file depend + on something it cannot see today: a callee that is not on the target and + lives in a file the target does not import and shares no package with. The + move would invert the dependency (the entity depends on the utility).""" + if language not in _VISIBILITY_LANGUAGES or not target_file: + return False + for callee in other_callees: + callee_file = _file_of(graph, callee) + if callee_file is not None and not _can_see(graph, target_file, callee_file): + return True + return False + + def _owning_class_id(graph: Any, callee_id: str) -> str | None: """The class node a callee belongs to: the method's parent class, or the class itself for a constructor call. ``None`` for free functions and @@ -420,10 +545,20 @@ def _envy_for( # Nearest foreign class by Jaccard distance (tie-break on class id). # A constructor call lands the class id in its own member set. own_and_inherited = heritage_ancestors(graph, own_class_id) | own_ids + static = _is_static(data) + components: dict[str, set[str]] = {} foreign = [ (c, m) for c, m in accessed_by_class.items() if _is_target(graph, c, m, own_and_inherited, ctx.file_path) + # A static method reading a data holder's accessors has inputs, not envy. + and not (static and _only_accessors(graph, c, m, components)) + and not _reverses_dependency( + graph, + ctx.language, + _file_of(graph, c) or c.rpartition("::")[0], + accessed - m, + ) ] if not foreign: return None diff --git a/packages/core/src/repowise/core/ingestion/language_configs.py b/packages/core/src/repowise/core/ingestion/language_configs.py index 3e1a66e20c..154f17b8ba 100644 --- a/packages/core/src/repowise/core/ingestion/language_configs.py +++ b/packages/core/src/repowise/core/ingestion/language_configs.py @@ -254,6 +254,9 @@ class LanguageConfig: import_node_types=["import_declaration"], export_node_types=[], visibility_fn=java_visibility, + # ``public static`` etc. land in ``Symbol.modifiers`` (annotations in + # the same node are decorators and are skipped by the reader). + modifier_node_types=frozenset({"modifiers"}), parent_extraction="nesting", parent_class_types=frozenset( { diff --git a/packages/core/src/repowise/core/ingestion/parser/symbol_rules.py b/packages/core/src/repowise/core/ingestion/parser/symbol_rules.py index d55df04692..91e1dbe5ec 100644 --- a/packages/core/src/repowise/core/ingestion/parser/symbol_rules.py +++ b/packages/core/src/repowise/core/ingestion/parser/symbol_rules.py @@ -321,6 +321,41 @@ def _declared_modifiers(def_node: Node, node_types: frozenset[str], src: str) -> return tuple(words) +# Kotlin has no ``static`` keyword: a member of an ``object`` or of a +# ``companion object`` is the language's static member. +_KOTLIN_STATIC_OWNERS = frozenset({"companion_object", "object_declaration"}) +_KOTLIN_CLASS_OWNERS = frozenset({"class_declaration", "object_literal"}) + + +def _declared_or_implied_modifiers( + def_node: Node, node_types: frozenset[str], language: str, src: str +) -> tuple[str, ...]: + """``_declared_modifiers`` plus ``static`` for a Kotlin ``object`` / + ``companion object`` function, which the grammar does not spell out.""" + modifiers = _declared_modifiers(def_node, node_types, src) + if ( + language == "kotlin" + and def_node.type == "function_declaration" + and "static" not in modifiers + and _in_kotlin_static_owner(def_node) + ): + return (*modifiers, "static") + return modifiers + + +def _in_kotlin_static_owner(def_node: Node) -> bool: + """Whether the nearest enclosing type of *def_node* is an ``object`` or a + ``companion object`` (so the function has no instance of its own).""" + node = def_node.parent + while node is not None: + if node.type in _KOTLIN_STATIC_OWNERS: + return True + if node.type in _KOTLIN_CLASS_OWNERS: + return False + node = node.parent + return False + + def _rust_outer_attributes(def_node: Node, src: str) -> list[str]: """Rust: outer attributes (#[...]) are preceding siblings of the item.""" attrs: list[str] = [] diff --git a/packages/core/src/repowise/core/ingestion/parser/symbols.py b/packages/core/src/repowise/core/ingestion/parser/symbols.py index d41caea895..f19014494d 100644 --- a/packages/core/src/repowise/core/ingestion/parser/symbols.py +++ b/packages/core/src/repowise/core/ingestion/parser/symbols.py @@ -52,7 +52,7 @@ _TS_JS_LANGUAGES, _attribute_decorators, _dart_mixin_parent, - _declared_modifiers, + _declared_or_implied_modifiers, _drop_swift_extensions_of_local_types, _is_declaration, _is_fsharp_binding, @@ -387,7 +387,9 @@ def _symbol_from_match(capture_dict: dict, file: _SymbolPass) -> tuple[Symbol, N _csharp_type_parameter_count(def_node) if language == "csharp" else None ), **_kotlin_receiver_fields(def_node, params_nodes, language, name), - modifiers=_declared_modifiers(def_node, config.modifier_node_types, src), + modifiers=_declared_or_implied_modifiers( + def_node, config.modifier_node_types, language, src + ), ) return symbol, def_node diff --git a/packages/core/src/repowise/core/upgrade/version.py b/packages/core/src/repowise/core/upgrade/version.py index 50a7ad0abc..925a1a15c2 100644 --- a/packages/core/src/repowise/core/upgrade/version.py +++ b/packages/core/src/repowise/core/upgrade/version.py @@ -116,7 +116,11 @@ #: public API follows Ruby visibility forms, Ansible playbooks and roles get #: dependency edges, Scala gets dataflow, and Postgres catalog tables are not #: workspace consumers. -PARSER_SCHEMA_VERSION: int = 10 +#: +#: v11: a Java method's symbol carries its keyword modifiers +#: (``public static``), and a Kotlin ``object`` / +#: ``companion object`` function carries ``static``. +PARSER_SCHEMA_VERSION: int = 11 #: state.json key holding the store format version that wrote the store. STORE_FORMAT_VERSION_KEY = "store_format_version" diff --git a/tests/unit/health/test_refactoring_move_method.py b/tests/unit/health/test_refactoring_move_method.py index e8c3362d3a..090d3c1456 100644 --- a/tests/unit/health/test_refactoring_move_method.py +++ b/tests/unit/health/test_refactoring_move_method.py @@ -585,3 +585,207 @@ def test_a_target_in_the_same_language_family_still_moves(): g.nodes[node].update(file_path="T.js", language="javascript") out = _detect_in(g, "C.tsx", "typescript") assert [s.plan["to_file"] for s in out] == ["T.js"] + + +# ---- static utilities and dependency direction ----------------------------- + + +def _java_in(root: str, file_name: str, cls: str, methods: list[str], g: nx.DiGraph) -> str: + path = f"{root}/{file_name}" + _add_class(g, path, cls, methods) + for node in g.nodes: + if node.startswith(f"{path}::") and g.nodes[node].get("kind") in ("class", "method"): + g.nodes[node]["language"] = "java" + return path + + +def _static_graph( + members: list[str], *, static: bool = True, declared: tuple[str, ...] = () +) -> nx.DiGraph: + """``util/K.gen`` calls every member of ``entity/T``; ``gen`` is static. + + *declared* members exist on ``T`` but are not called (a constructor). + """ + g = nx.DiGraph() + util = _java_in("app/util", "K.java", "K", ["gen"], g) + target = _java_in("app/entity", "T.java", "T", [*declared, *members], g) + for m in members: + _call(g, f"{util}::K.gen", f"{target}::T.{m}") + if static: + g.nodes[f"{util}::K.gen"]["modifiers"] = ("public", "static") + return g + + +def _detect_static(g: nx.DiGraph, language: str = "java") -> list: + ctx = RefactoringContext(file_path="app/util/K.java", language=language, nloc=40, graph=g) + return [s for s in detect_refactorings(ctx) if s.refactoring_type == "move_method"] + + +_GETTERS = ["getAppId", "getClusterName", "getNamespaceName", "setAppId"] +_BEHAVIOUR = ["validate", "apply", "merge", "reset"] + + +def test_static_method_reading_getters_never_moves_into_the_data_holder(): + assert _detect_static(_static_graph(_GETTERS)) == [] + assert _detect_static(_static_graph(["isActive", "getName", "getId"])) == [] + + +def test_static_method_calling_behaviour_still_moves(): + out = _detect_static(_static_graph(_BEHAVIOUR)) + assert [(s.target_symbol, s.plan["to_class"]) for s in out] == [("K.gen", "T")] + + +def test_instance_method_reading_getters_is_not_changed_by_the_static_rule(): + assert len(_detect_static(_static_graph(_GETTERS, static=False))) == 1 + + +def test_one_behaviour_call_makes_the_target_more_than_a_data_holder(): + # ``validate`` is not an accessor, so the target is not only a data holder. + assert len(_detect_static(_static_graph(["getAppId", "getName", "validate"]))) == 1 + + +def test_get_alone_and_lowercase_prefixes_are_not_accessors(): + # ``get(k)`` is a lookup and ``settle`` / ``issue`` only start like setters. + assert len(_detect_static(_static_graph(["get", "settle", "issue"]))) == 1 + + +def test_record_components_are_accessors_for_a_static_method(): + g = _static_graph(["appId", "clusterName"], declared=("T",)) + g.nodes["app/entity/T.java::T.T"]["signature"] = "public T(String appId, String clusterName)" + for m in ("appId", "clusterName"): + g.nodes[f"app/entity/T.java::T.{m}"]["signature"] = f"public String {m}()" + assert _detect_static(g) == [] + # The same names are behaviour when they take arguments. + for m in ("appId", "clusterName"): + g.nodes[f"app/entity/T.java::T.{m}"]["signature"] = f"public void {m}(int n)" + assert len(_detect_static(g)) == 1 + + +def test_constructor_parameters_with_generics_and_defaults_are_read(): + g = _static_graph(["ids", "name"], declared=("T",)) + g.nodes["app/entity/T.java::T.T"]["signature"] = ( + "T(Map> ids, String name)" + ) + for m in ("ids", "name"): + g.nodes[f"app/entity/T.java::T.{m}"]["signature"] = f"{m}() -> Object" + assert _detect_static(g) == [] + + +def test_kotlin_companion_function_reading_properties_never_moves(): + g = _static_graph(["getAppId", "getName", "getId"]) + assert _detect_static(g, "kotlin") == [] + + +def test_python_staticmethod_handling_is_unchanged(): + g = _static_graph(_GETTERS, static=False) + g.nodes["app/util/K.java::K.gen"]["decorators"] = ["@staticmethod"] + assert len(_detect_static(g, "python")) == 1 + + +def _reversal_graph(*, target_dir: str = "app/entity") -> nx.DiGraph: + """``util/K.gen`` calls three behaviours of ``T`` and ``util/Base.generate``.""" + g = nx.DiGraph() + util = _java_in("app/util", "K.java", "K", ["gen"], g) + base = _java_in("app/util", "Base.java", "Base", ["generate"], g) + target = _java_in(target_dir, "T.java", "T", ["validate", "apply", "merge"], g) + for m in ("validate", "apply", "merge"): + _call(g, f"{util}::K.gen", f"{target}::T.{m}") + _call(g, f"{util}::K.gen", f"{base}::Base.generate") + return g + + +def test_move_whose_other_callee_the_target_cannot_see_is_refused(): + assert _detect_static(_reversal_graph()) == [] + + +def test_move_is_allowed_when_the_target_imports_or_shares_a_package_with_the_callee(): + g = _reversal_graph() + g.add_edge("app/entity/T.java", "app/util/Base.java", edge_type="imports") + assert len(_detect_static(g)) == 1 + assert len(_detect_static(_reversal_graph(target_dir="app/util"))) == 1 + g = _reversal_graph() + g.add_edge("app/entity/T.java", "app/util/Base.java", edge_type="calls") + assert _detect_static(g) == [] # a call edge is not a visibility edge + + +def test_dependency_direction_is_only_judged_where_imports_decide_visibility(): + assert len(_detect_static(_reversal_graph(), "python")) == 1 + + +def test_a_callee_without_a_file_does_not_block_a_move(): + g = _reversal_graph() + g.add_node("external::lib.fn", node_type="symbol", kind="method", name="fn") + _call(g, "app/util/K.java::K.gen", "external::lib.fn") + g.add_edge("app/entity/T.java", "app/util/Base.java", edge_type="imports") + assert len(_detect_static(g)) == 1 + + +_ISSUE_FILES = { + "src/main/java/app/entity/Namespace.java": """package app.entity; +public class Namespace { + private String appId; private String clusterName; private String namespaceName; + public String getAppId() { return appId; } + public String getClusterName() { return clusterName; } + public String getNamespaceName() { return namespaceName; } + public void setAppId(String appId) { this.appId = appId; } +} +""", + "src/main/java/app/util/KeyGenerator.java": """package app.util; +public class KeyGenerator { + protected static String generate(Object... args) { return null; } +} +""", + "src/main/java/app/util/ReleaseKeyGenerator.java": """package app.util; +import app.entity.Namespace; +public class ReleaseKeyGenerator extends KeyGenerator { + public static String generateReleaseKey(Namespace namespace) { + return generate(namespace.getAppId(), namespace.getClusterName(), namespace.getNamespaceName()); + } +} +""", +} + + +def _plans_from_source(tmp_path, files: dict[str, str], file_path: str) -> list: + from repowise.core.pipeline.incremental.graph import build_repo_graph + + for rel, text in files.items(): + (tmp_path / rel).parent.mkdir(parents=True, exist_ok=True) + (tmp_path / rel).write_text(text, encoding="utf-8") + _, _, builder, _, _ = build_repo_graph( + tmp_path, [], collect_sources=True, log=lambda *a, **k: None, read_only=True + ) + graph = builder.graph() + method = next(n for n in graph.nodes if n.endswith("::generateReleaseKey")) + assert "static" in graph.nodes[method]["modifiers"] + ctx = RefactoringContext(file_path=file_path, language="java", nloc=200, graph=graph) + return [s for s in detect_refactorings(ctx) if s.refactoring_type == "move_method"] + + +def test_issue_3830_key_generator_is_not_moved_into_the_entity(tmp_path): + path = "src/main/java/app/util/ReleaseKeyGenerator.java" + assert _plans_from_source(tmp_path, _ISSUE_FILES, path) == [] + + +def test_static_method_calling_behaviour_from_source_still_moves(tmp_path): + files = dict(_ISSUE_FILES) + files["src/main/java/app/util/ReleaseKeyGenerator.java"] = """package app.util; +import app.entity.Namespace; +public class ReleaseKeyGenerator { + public static String generateReleaseKey(Namespace namespace) { + namespace.validate(); namespace.apply(); return namespace.merge(); + } +} +""" + files["src/main/java/app/entity/Namespace.java"] = """package app.entity; +public class Namespace { + public void validate() {} + public void apply() {} + public String merge() { return null; } +} +""" + path = "src/main/java/app/util/ReleaseKeyGenerator.java" + out = _plans_from_source(tmp_path, files, path) + assert [(s.target_symbol, s.plan["to_class"]) for s in out] == [ + ("ReleaseKeyGenerator.generateReleaseKey", "Namespace") + ] diff --git a/tests/unit/ingestion/parser/golden/java.json b/tests/unit/ingestion/parser/golden/java.json index 0aa6a85f2c..34b8f1fc7a 100644 --- a/tests/unit/ingestion/parser/golden/java.json +++ b/tests/unit/ingestion/parser/golden/java.json @@ -33,7 +33,9 @@ "type_parameter_count": null, "extension_receiver": null, "written_receiver": null, - "modifiers": [], + "modifiers": [ + "public" + ], "alias_of": null }, { @@ -56,7 +58,9 @@ "type_parameter_count": null, "extension_receiver": null, "written_receiver": null, - "modifiers": [], + "modifiers": [ + "public" + ], "alias_of": null }, { @@ -79,7 +83,9 @@ "type_parameter_count": null, "extension_receiver": null, "written_receiver": null, - "modifiers": [], + "modifiers": [ + "public" + ], "alias_of": null }, { @@ -102,7 +108,9 @@ "type_parameter_count": null, "extension_receiver": null, "written_receiver": null, - "modifiers": [], + "modifiers": [ + "public" + ], "alias_of": null } ], diff --git a/tests/unit/ingestion/parser/test_misc.py b/tests/unit/ingestion/parser/test_misc.py index 17ba1bc0cc..8c1db34246 100644 --- a/tests/unit/ingestion/parser/test_misc.py +++ b/tests/unit/ingestion/parser/test_misc.py @@ -158,7 +158,23 @@ def run(): ), ("typescript", "a.ts", b"class A extends B { override f() {} }", {"f": ("override",)}), # Java writes ``@Override`` as an annotation; it stays a decorator. - ("java", "A.java", b"class A { @Override public void f() {} }", {"f": ()}), + ("java", "A.java", b"class A { @Override public void f() {} }", {"f": ("public",)}), + # ``static`` is what tells a utility method from one with an instance. + ( + "java", + "A.java", + b"class A { public static String g(N n) { return null; } void h() {} }", + {"g": ("public", "static"), "h": ()}, + ), + # Kotlin has no ``static`` keyword: an ``object`` / ``companion object`` + # function is the static member, an ordinary member function is not. + ( + "kotlin", + "B.kt", + b"class B {\n companion object {\n fun make() {}\n }\n fun inst() {}\n}\n" + b"object O {\n fun conv() {}\n}\n", + {"make": ("static",), "inst": (), "conv": ("static",)}, + ), # Scala access qualifiers name a scope, not a modifier. ( "scala",