Skip to content

refactor: consolidate Python function discovery to CST path only#1689

Merged
KRRT7 merged 3 commits intomainfrom
consolidate-python-discovery
Feb 27, 2026
Merged

refactor: consolidate Python function discovery to CST path only#1689
KRRT7 merged 3 commits intomainfrom
consolidate-python-discovery

Conversation

@KRRT7
Copy link
Collaborator

@KRRT7 KRRT7 commented Feb 27, 2026

Summary

  • Remove the AST-based Python discovery path (find_functions_with_return_statement, _find_all_functions_in_python_file, function_has_return_statement, function_is_a_property, and associated constants)
  • Route Python through the unified _find_all_functions_via_language_support like all other languages
  • Fix FunctionVisitor to skip nested functions and exclude @property/@cached_property decorated functions
  • Let parse errors propagate from PythonSupport.discover_functions so invalid files correctly return {}

Test plan

  • uv run pytest tests/test_function_discovery.py -x -q — 23 passed
  • uv run pytest tests/test_async_function_discovery.py -x -q — all passed
  • uv run ruff check / ruff format — clean

Remove the AST-based discovery path for Python, routing all languages
through the unified CST-based `_find_all_functions_via_language_support`.
Delete dead code: `find_functions_with_return_statement`,
`_find_all_functions_in_python_file`, `function_has_return_statement`,
`function_is_a_property`, and associated constants. Fix FunctionVisitor
to skip nested functions and exclude @property/@cached_property, and let
parse errors propagate for correct empty-dict behavior on invalid files.
@claude
Copy link
Contributor

claude bot commented Feb 27, 2026

PR Review Summary

Prek Checks

All checks pass (ruff check, ruff format) — no issues found.

Mypy

All mypy errors on changed files are pre-existing (not introduced by this PR). Errors include missing type exports from codeflash.languages.base, missing type params on list/dict generics in support.py, and missing annotations on test functions — all present on main as well.

Code Review

Overall this is a clean refactor that removes ~100 lines of dead AST-based code and unifies Python discovery through the CST path.

Minor issue (non-blocking):

  • find_all_functions_in_file (line 493-497) has a stale docstring that still references "routing to either the Python-specific implementation (for backward compatibility) or the language support abstraction for other languages". The Python-specific path was removed in this PR — the docstring should reflect that all languages now go through _find_all_functions_via_language_support.

No critical bugs, security issues, or breaking API changes found.

Key observations:

  • Error propagation change is safe: discover_functions no longer catches exceptions, but _find_all_functions_via_language_support still wraps calls in try/except
  • get_functions_within_lines uses FunctionVisitor directly with its own error handling — unaffected by the change
  • Nested function skipping and @property/@cached_property exclusion logic is correct
  • Dict key unification from str to Path is consistent across all modified functions
  • Test updates properly reflect the new behavior (nested functions skipped, errors propagated)

Test Coverage

File PR Main Delta
codeflash/discovery/functions_to_optimize.py 69% (506 stmts) 70% (549 stmts) -1% (43 stmts removed)
codeflash/languages/python/support.py 47% (302 stmts) 49% (311 stmts) -2% (9 stmts removed)
codeflash/lsp/beta.py 0% 0% no change (pre-existing, no tests)
Overall 78% 78% No change
  • Coverage percentages are slightly lower due to dead code removal (fewer stmts in denominator), but absolute covered lines remain similar
  • No coverage regression — overall stays at 78%
  • lsp/beta.py has 0% coverage on both branches (pre-existing)
  • 8 test failures in test_tracer.py are pre-existing on main

Last updated: 2026-02-27

Nested functions are now skipped by FunctionVisitor, and
discover_functions no longer swallows parse/IO errors — callers
handle them. Update test expectations accordingly.
@KRRT7
Copy link
Collaborator Author

KRRT7 commented Feb 27, 2026

@claude Test results: 2463 passed, 13 failed, 57 skipped. The 13 failures are pre-existing (same failures occur on main). what failures are those?

- Change FunctionVisitor.file_path from str to Path
- Unify dict keys to Path across discovery functions (get_all_files_and_functions,
  get_functions_within_lines, get_functions_within_git_diff, etc.)
- Remove redundant isinstance check in discover_functions
- Add assert for found_function narrowing after exit_with_message
- Fix closest_matching_file_function_name return type narrowing
@KRRT7
Copy link
Collaborator Author

KRRT7 commented Feb 27, 2026

@claude is the JUnit XML parsing logic actually being used for JS / TS?

@codeflash-ai
Copy link
Contributor

codeflash-ai bot commented Feb 27, 2026

⚡️ Codeflash found optimizations for this PR

📄 175% (1.75x) speedup for closest_matching_file_function_name in codeflash/discovery/functions_to_optimize.py

⏱️ Runtime : 24.7 milliseconds 8.99 milliseconds (best of 111 runs)

A dependent PR with the suggested changes has been created. Please review:

If you approve, it will be merged into this PR (branch consolidate-python-discovery).

Static Badge

@KRRT7 KRRT7 merged commit cbc66e2 into main Feb 27, 2026
26 of 28 checks passed
@KRRT7 KRRT7 deleted the consolidate-python-discovery branch February 27, 2026 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant