diff --git a/codeflash/languages/java/discovery.py b/codeflash/languages/java/discovery.py index 3d36e7d40..cb610cb18 100644 --- a/codeflash/languages/java/discovery.py +++ b/codeflash/languages/java/discovery.py @@ -129,6 +129,11 @@ def _should_include_method( True if the method should be included. """ + # Skip methods that belong to an inner/nested class — they cannot be reliably + # instrumented or tested in isolation (see discussion in discovery module). + if method.is_class_nested: + return False + # Skip abstract methods (no implementation to optimize) if method.is_abstract: return False diff --git a/codeflash/languages/java/parser.py b/codeflash/languages/java/parser.py index b5715bb09..21c0ad4f0 100644 --- a/codeflash/languages/java/parser.py +++ b/codeflash/languages/java/parser.py @@ -52,6 +52,8 @@ class JavaMethodNode: class_name: str | None source_text: str javadoc_start_line: int | None = None # Line where Javadoc comment starts + formal_parameters_text: str | None = None # Raw formal parameters "(Type name, ...)" for matching + is_class_nested: bool = False # True when the enclosing class is itself nested inside another class @dataclass @@ -182,6 +184,104 @@ def find_methods( return methods + def find_constructors(self, source: str, class_name: str | None = None) -> list[JavaMethodNode]: + """Find all constructor definitions in source code. + + Args: + source: The source code to analyze. + class_name: Optional class name to filter constructors. + + Returns: + List of JavaMethodNode objects describing found constructors. + The ``name`` field of each node is the constructor name (i.e. the class name). + + """ + source_bytes = source.encode("utf8") + tree = self.parse(source_bytes) + constructors: list[JavaMethodNode] = [] + self._walk_tree_for_constructors( + tree.root_node, source_bytes, constructors, current_class=None, target_class=class_name + ) + return constructors + + def _walk_tree_for_constructors( + self, + node: Node, + source_bytes: bytes, + constructors: list[JavaMethodNode], + current_class: str | None, + target_class: str | None, + ) -> None: + """Recursively walk the tree to find constructor declarations.""" + new_class = current_class + type_declarations = ("class_declaration", "interface_declaration", "enum_declaration") + if node.type in type_declarations: + name_node = node.child_by_field_name("name") + if name_node: + new_class = self.get_node_text(name_node, source_bytes) + + if node.type == "constructor_declaration": + constructor_info = self._extract_constructor_info(node, source_bytes, new_class) + if constructor_info: + if target_class is None or constructor_info.class_name == target_class: + constructors.append(constructor_info) + + for child in node.children: + self._walk_tree_for_constructors( + child, + source_bytes, + constructors, + current_class=new_class if node.type in type_declarations else current_class, + target_class=target_class, + ) + + def _extract_constructor_info( + self, node: Node, source_bytes: bytes, current_class: str | None + ) -> JavaMethodNode | None: + """Extract constructor information from a constructor_declaration node.""" + name_node = node.child_by_field_name("name") + if not name_node: + return None + name = self.get_node_text(name_node, source_bytes) + + is_public = False + is_private = False + is_protected = False + for child in node.children: + if child.type == "modifiers": + modifier_text = self.get_node_text(child, source_bytes) + is_public = "public" in modifier_text + is_private = "private" in modifier_text + is_protected = "protected" in modifier_text + break + + # Extract formal parameters text for signature matching + params_node = node.child_by_field_name("parameters") + formal_parameters_text = self.get_node_text(params_node, source_bytes) if params_node else "()" + + source_text = self.get_node_text(node, source_bytes) + javadoc_start_line = self._find_preceding_javadoc(node, source_bytes) + + return JavaMethodNode( + name=name, + node=node, + start_line=node.start_point[0] + 1, + end_line=node.end_point[0] + 1, + start_col=node.start_point[1], + end_col=node.end_point[1], + is_static=False, + is_public=is_public, + is_private=is_private, + is_protected=is_protected, + is_abstract=False, + is_synchronized=False, + return_type=None, + class_name=current_class, + source_text=source_text, + javadoc_start_line=javadoc_start_line, + formal_parameters_text=formal_parameters_text, + ) + def _walk_tree_for_methods( self, node: Node, @@ -190,6 +290,7 @@ def _walk_tree_for_methods( include_private: bool, include_static: bool, current_class: str | None, + class_depth: int = 0, ) -> None: """Recursively walk the tree to find method definitions.""" new_class = current_class @@ -205,6 +306,10 @@ def _walk_tree_for_methods( method_info = self._extract_method_info(node, source_bytes, current_class) if method_info: + # A method is nested when its enclosing class is itself inside another + # class (class_depth >= 2: depth 1 = outermost class, depth 2+ = nested). + method_info.is_class_nested = class_depth >= 2 + # Apply filters should_include = True @@ -217,7 +322,7 @@ def _walk_tree_for_methods( if should_include: methods.append(method_info) - # Recurse into children + # Recurse into children, incrementing depth when entering a type declaration for child in node.children: self._walk_tree_for_methods( child, @@ -226,6 +331,7 @@ def _walk_tree_for_methods( include_private=include_private, include_static=include_static, current_class=new_class if node.type in type_declarations else current_class, + class_depth=class_depth + 1 if node.type in type_declarations else class_depth, ) def _extract_method_info(self, node: Node, source_bytes: bytes, current_class: str | None) -> JavaMethodNode | None: diff --git a/codeflash/languages/java/replacement.py b/codeflash/languages/java/replacement.py index e6628286e..a1bd4fb7c 100644 --- a/codeflash/languages/java/replacement.py +++ b/codeflash/languages/java/replacement.py @@ -35,9 +35,14 @@ class ParsedOptimization: new_fields: list[str] # Source text of new fields to add helpers_before_target: list[str] = field(default_factory=list) # Helpers appearing before target in optimized code helpers_after_target: list[str] = field(default_factory=list) # Helpers appearing after target in optimized code + modified_constructors: list[str] = field(default_factory=list) # Constructor sources that need to replace originals -def _parse_optimization_source(new_source: str, target_method_name: str, analyzer: JavaAnalyzer) -> ParsedOptimization: +def _parse_optimization_source( + new_source: str, + target_method_name: str, + analyzer: JavaAnalyzer, +) -> ParsedOptimization: """Parse optimization source to extract method and additional class members. The new_source may contain: @@ -63,6 +68,7 @@ def _parse_optimization_source(new_source: str, target_method_name: str, analyze helpers_before_target: list[str] = [] helpers_after_target: list[str] = [] + modified_constructors: list[str] = [] if classes: # It's a class - extract components @@ -112,6 +118,22 @@ def _parse_optimization_source(new_source: str, target_method_name: str, analyze else: helpers_after_target.append(helper_source) + # Extract constructors that belong to the same class as the target method. + # When the LLM adds a new field (e.g. a cached value), it also updates the + # constructors to initialize it. We must replace those constructors in the + # original source, otherwise the new final field will be uninitialized + # (Bug 3: uninitialized variable errors). + # Use line-sliced text (same as helper methods) so that the leading whitespace + # is preserved and _dedent_member can normalise indentation correctly. + if target_method: + target_class_name_for_ctors = target_method.class_name + new_constructors = analyzer.find_constructors(new_source, class_name=target_class_name_for_ctors) + ctor_lines = new_source.splitlines(keepends=True) + for c in new_constructors: + ctor_start = (c.javadoc_start_line or c.start_line) - 1 + ctor_end = c.end_line + modified_constructors.append("".join(ctor_lines[ctor_start:ctor_end])) + # Extract fields for f in fields: if f.source_text: @@ -138,6 +160,7 @@ def _parse_optimization_source(new_source: str, target_method_name: str, analyze new_fields=new_fields, helpers_before_target=helpers_before_target, helpers_after_target=helpers_after_target, + modified_constructors=modified_constructors, ) @@ -272,6 +295,89 @@ def format_member(raw: str) -> str: return result +def _replace_constructors( + source: str, + class_name: str, + new_constructor_sources: list[str], + analyzer: JavaAnalyzer, +) -> str: + """Replace constructors in source with updated versions from the optimization. + + Matches constructors by their formal parameter signature. When a matching + constructor is found in the original source it is replaced in-place, + preserving the original indentation. Constructors for which no match + exists in the original are silently skipped (they would need to be inserted + as new members, which is out of scope for this helper). + + Args: + source: The original source code to modify. + class_name: Name of the class whose constructors should be replaced. + new_constructor_sources: Source text of each updated constructor. + analyzer: JavaAnalyzer instance. + + Returns: + Modified source code with constructors replaced. + + """ + if not new_constructor_sources: + return source + + original_constructors = analyzer.find_constructors(source, class_name=class_name) + if not original_constructors: + return source + + result = source + + for new_ctor_src in new_constructor_sources: + # Wrap in a dummy class so the parser can handle a bare constructor + dummy = f"class __Dummy__ {{\n{new_ctor_src}\n}}" + parsed_new = analyzer.find_constructors(dummy) + if not parsed_new: + continue + new_ctor = parsed_new[0] + new_params = (new_ctor.formal_parameters_text or "()").strip() + + # Find the matching constructor in the current (potentially already + # modified) source by parameter signature. + current_constructors = analyzer.find_constructors(result, class_name=class_name) + matching = None + for orig in current_constructors: + if (orig.formal_parameters_text or "()").strip() == new_params: + matching = orig + break + + if not matching: + logger.debug( + "No matching constructor with params %s found in class %s; skipping.", + new_params, + class_name, + ) + continue + + # Determine replacement range (include Javadoc if present) + ctor_start = matching.javadoc_start_line or matching.start_line + ctor_end = matching.end_line + + lines = result.splitlines(keepends=True) + original_first_line = lines[ctor_start - 1] if ctor_start <= len(lines) else "" + indent = _get_indentation(original_first_line) + + # Dedent first to remove any class-level indentation, then re-apply + # the correct indentation (same as _insert_class_members / format_member). + new_ctor_lines = _dedent_member(new_ctor_src).splitlines(keepends=True) + indented_new_ctor = _apply_indentation(new_ctor_lines, indent) + if indented_new_ctor and not indented_new_ctor.endswith("\n"): + indented_new_ctor += "\n" + + before = lines[: ctor_start - 1] + after = lines[ctor_end:] + result = "".join(before) + indented_new_ctor + "".join(after) + + logger.debug("Replaced constructor %s(%s) in class %s", class_name, new_params, class_name) + + return result + + def replace_function( source: str, function: FunctionToOptimize, new_source: str, analyzer: JavaAnalyzer | None = None ) -> str: @@ -305,7 +411,7 @@ def replace_function( func_start_line = function.starting_line func_end_line = function.ending_line - # Parse the optimization to extract components + # Parse the optimization to extract components. parsed = _parse_optimization_source(new_source, func_name, analyzer) # If the parsed optimization has no valid target source (e.g., the LLM generated @@ -467,7 +573,14 @@ def replace_function( before = lines[: start_line - 1] # Lines before the method after = lines[end_line:] # Lines after the method - return "".join(before) + indented_new_source + "".join(after) + result = "".join(before) + indented_new_source + "".join(after) + + # Replace modified constructors if the optimization introduced new field + # initializations (Bug 3: uninitialized variable errors). + if class_name and parsed.modified_constructors: + result = _replace_constructors(result, class_name, parsed.modified_constructors, analyzer) + + return result def _get_indentation(line: str) -> str: diff --git a/tests/test_languages/test_java/test_context.py b/tests/test_languages/test_java/test_context.py index 41c8b7714..27a3eb3a7 100644 --- a/tests/test_languages/test_java/test_context.py +++ b/tests/test_languages/test_java/test_context.py @@ -1486,7 +1486,11 @@ class TestExtractCodeContextWithInnerClasses: """Tests for extract_code_context with inner/nested classes.""" def test_static_nested_class_method(self, tmp_path: Path): - """Test context extraction for static nested class method.""" + """Inner class methods are excluded from discovery and cannot be context-extracted. + + Methods of static nested classes are skipped in discovery because they + cannot be reliably instrumented or tested in isolation. + """ java_file = tmp_path / "Container.java" java_file.write_text("""public class Container { public static class Nested { @@ -1498,26 +1502,15 @@ def test_static_nested_class_method(self, tmp_path: Path): """) functions = discover_functions_from_source(java_file.read_text(), file_path=java_file) compute_func = next((f for f in functions if f.function_name == "compute"), None) - assert compute_func is not None - - context = extract_code_context(compute_func, tmp_path) - - # Inner class wrapped in outer class skeleton - assert ( - context.target_code - == """public class Container { - public static class Nested { - public int compute(int x) { - return x * 2; - } - } -} -""" - ) - assert context.read_only_context == "" + # Inner class method must NOT be discovered + assert compute_func is None def test_inner_class_method(self, tmp_path: Path): - """Test context extraction for inner class method.""" + """Inner class methods are excluded from discovery and cannot be context-extracted. + + Methods of non-static inner classes are skipped in discovery because they + require an outer instance and cannot be instrumented independently. + """ java_file = tmp_path / "Outer.java" java_file.write_text("""public class Outer { private int value = 10; @@ -1531,23 +1524,8 @@ def test_inner_class_method(self, tmp_path: Path): """) functions = discover_functions_from_source(java_file.read_text(), file_path=java_file) get_func = next((f for f in functions if f.function_name == "getValue"), None) - assert get_func is not None - - context = extract_code_context(get_func, tmp_path) - - # Inner class wrapped in outer class skeleton - assert ( - context.target_code - == """public class Outer { - public class Inner { - public int getValue() { - return value; - } - } -} -""" - ) - assert context.read_only_context == "" + # Inner class method must NOT be discovered + assert get_func is None class TestExtractCodeContextWithEnumAndInterface: diff --git a/tests/test_languages/test_java/test_discovery.py b/tests/test_languages/test_java/test_discovery.py index 9411a30c4..e42cfe8c2 100644 --- a/tests/test_languages/test_java/test_discovery.py +++ b/tests/test_languages/test_java/test_discovery.py @@ -333,3 +333,122 @@ def test_discover_tests_from_fixture(self, java_fixture_path: Path): tests = discover_test_methods(test_file) assert len(tests) > 0 + + +class TestInnerClassMethodFilter: + """Tests that methods of nested/inner classes are excluded from discovery. + + Inner class methods cannot be reliably instrumented or tested in isolation: + - Non-static inner classes require an outer instance + - Protected methods are inaccessible from external test code + - The instrumentation layer is not class-aware (wraps by method name only) + + Discovery must skip all methods whose enclosing class is itself nested inside + another class. + """ + + def test_static_inner_class_methods_are_excluded(self): + """Methods in a static nested class must not be discovered.""" + source = """\ +public abstract class Unpacker { + protected abstract T getString(String value); + + public T unpackString() { + return getString(null); + } + + public static final class ObjectUnpacker extends Unpacker { + public ObjectUnpacker() {} + + @Override + protected Object getString(String value) { + return value; + } + + public Object helper() { + return null; + } + } +} +""" + functions = discover_functions_from_source(source) + # Only the outer class method unpackString() should be discovered. + # ObjectUnpacker.getString and ObjectUnpacker.helper are inner-class methods + # and must be excluded. + function_names = {f.function_name for f in functions} + assert "unpackString" in function_names + assert "getString" not in function_names + assert "helper" not in function_names + + def test_non_static_inner_class_methods_are_excluded(self): + """Methods in a non-static inner class must not be discovered.""" + source = """\ +public class Outer { + private int value; + + public int getValue() { + return value; + } + + public class Inner { + public int doubleValue() { + return value * 2; + } + } +} +""" + functions = discover_functions_from_source(source) + function_names = {f.function_name for f in functions} + assert "getValue" in function_names + assert "doubleValue" not in function_names + + def test_outer_class_methods_are_still_discovered(self): + """Outer-class methods must be discovered normally even when inner classes exist.""" + source = """\ +public class Container { + public int size() { + return 0; + } + + public boolean isEmpty() { + return true; + } + + private static class InnerHelper { + public void doWork() {} + } +} +""" + functions = discover_functions_from_source(source) + function_names = {f.function_name for f in functions} + assert "size" in function_names + assert "isEmpty" in function_names + # Inner class method must be excluded + assert "doWork" not in function_names + + def test_deeply_nested_class_methods_are_excluded(self): + """Methods in classes nested more than two levels deep must also be excluded.""" + source = """\ +public class Level1 { + public int method1() { + return 1; + } + + public static class Level2 { + public int method2() { + return 2; + } + + public static class Level3 { + public int method3() { + return 3; + } + } + } +} +""" + functions = discover_functions_from_source(source) + function_names = {f.function_name for f in functions} + assert "method1" in function_names + assert "method2" not in function_names + assert "method3" not in function_names diff --git a/tests/test_languages/test_java/test_replacement.py b/tests/test_languages/test_java/test_replacement.py index f1424361a..7d2461e33 100644 --- a/tests/test_languages/test_java/test_replacement.py +++ b/tests/test_languages/test_java/test_replacement.py @@ -832,8 +832,12 @@ def test_replace_multiple_methods(self, tmp_path: Path): class TestNestedClasses: """Tests for nested class scenarios.""" - def test_replace_method_in_nested_class(self, tmp_path: Path): - """Test replacing a method in a nested class.""" + def test_inner_class_method_is_not_replaced(self, tmp_path: Path): + """Inner-class methods are not supported for optimization and must be skipped. + + Methods of static nested or non-static inner classes are excluded from + discovery and therefore cannot be replaced via the high-level API. + """ java_file = tmp_path / "Outer.java" original_code = """public class Outer { public int outerMethod() { @@ -865,6 +869,8 @@ def test_replace_method_in_nested_class(self, tmp_path: Path): optimized_code = CodeStringsMarkdown.parse_markdown_code(optimized_markdown, expected_language="java") + # Inner class methods are excluded from discovery, so the replacement + # is a no-op and the original file must remain unchanged. result = replace_function_definitions_for_language( function_names=["innerMethod"], optimized_code=optimized_code, @@ -872,21 +878,9 @@ def test_replace_method_in_nested_class(self, tmp_path: Path): project_root_path=tmp_path, ) - assert result is True - new_code = java_file.read_text(encoding="utf-8") - expected = """public class Outer { - public int outerMethod() { - return 1; - } - - public static class Inner { - public int innerMethod() { - return 2 + 0; - } - } -} -""" - assert new_code == expected + assert result is False + # File must be unchanged + assert java_file.read_text(encoding="utf-8") == original_code class TestPreservesStructure: @@ -1927,3 +1921,168 @@ def test_anonymous_iterator_methods_not_hoisted_to_class(self, tmp_path): } """ assert new_code == expected_code + + +class TestConstructorReplacement: + """Tests that constructors in the generated class are propagated to the original. + + When the LLM introduces a new ``final`` field (e.g. a cached hash) it must + also initialise that field inside every constructor. Codeflash must detect + the modified constructors in the generated class and replace the corresponding + constructors in the original source, otherwise the new field is uninitialised + and the compiler rejects the file with "variable X might not have been + initialized". + """ + + def test_constructor_updated_when_new_final_field_added(self, tmp_path): + """Reproduces the Expression.hashCode / LuaMap.valuesIterator pattern. + + The LLM optimises ``hashCode`` by caching the result in a new final + field ``cachedHash``. The generated class includes the updated + constructor that initialises ``cachedHash``. Codeflash must also + replace the original constructor so that the field is properly + initialised. + """ + from codeflash.discovery.functions_to_optimize import FunctionToOptimize, FunctionParent + from codeflash.languages.java.replacement import replace_function + + original_code = """\ +public final class Expression { + private final byte[] bytes; + + Expression(byte[] bytes) { + this.bytes = bytes; + } + + @Override + public int hashCode() { + return java.util.Arrays.hashCode(bytes); + } +} +""" + java_file = tmp_path / "Expression.java" + java_file.write_text(original_code, encoding="utf-8") + + # LLM optimisation: cache the hash in a new final field. The generated + # class includes both the updated constructor and the simplified hashCode. + optimized_source = """\ +public final class Expression { + private final byte[] bytes; + private final int cachedHash; + + Expression(byte[] bytes) { + this.bytes = bytes; + this.cachedHash = java.util.Arrays.hashCode(bytes); + } + + @Override + public int hashCode() { + return cachedHash; + } +} +""" + + func = FunctionToOptimize( + function_name="hashCode", + file_path=java_file, + starting_line=9, + ending_line=11, + parents=[FunctionParent(name="Expression", type="ClassDef")], + is_method=True, + language="java", + ) + + new_code = replace_function(original_code, func, optimized_source) + + # The result should have: + # 1. The new field "cachedHash" added + # 2. The constructor updated to initialise "cachedHash" + # 3. hashCode() returning cachedHash + expected_code = """\ +public final class Expression { + private final byte[] bytes; + private final int cachedHash; + + Expression(byte[] bytes) { + this.bytes = bytes; + this.cachedHash = java.util.Arrays.hashCode(bytes); + } + + @Override + public int hashCode() { + return cachedHash; + } +} +""" + assert new_code == expected_code + + def test_constructor_updated_for_cached_collection_view(self, tmp_path): + """Reproduces the LuaMap.valuesIterator caching pattern. + + The LLM caches ``map.values()`` in a new final field ``valuesView`` + which is initialised in the constructor. The original constructor + must be updated. + """ + from codeflash.discovery.functions_to_optimize import FunctionToOptimize, FunctionParent + from codeflash.languages.java.replacement import replace_function + + original_code = """\ +public class DataStore { + private final java.util.Map data; + + public DataStore(java.util.Map data) { + this.data = data; + } + + public java.util.Collection values() { + return data.values(); + } +} +""" + java_file = tmp_path / "DataStore.java" + java_file.write_text(original_code, encoding="utf-8") + + optimized_source = """\ +public class DataStore { + private final java.util.Map data; + private final java.util.Collection cachedValues; + + public DataStore(java.util.Map data) { + this.data = data; + this.cachedValues = data.values(); + } + + public java.util.Collection values() { + return cachedValues; + } +} +""" + + func = FunctionToOptimize( + function_name="values", + file_path=java_file, + starting_line=8, + ending_line=10, + parents=[FunctionParent(name="DataStore", type="ClassDef")], + is_method=True, + language="java", + ) + + new_code = replace_function(original_code, func, optimized_source) + + expected_code = """\ +public class DataStore { + private final java.util.Map data; + private final java.util.Collection cachedValues; + + public DataStore(java.util.Map data) { + this.data = data; + this.cachedValues = data.values(); + } + + public java.util.Collection values() { + return cachedValues; + } +} +""" + assert new_code == expected_code