feat: add new performance anti-pattern detections#51
feat: add new performance anti-pattern detections#51nnennandukwe wants to merge 4 commits intomainfrom
Conversation
Review Summary by QodoAdd three new performance anti-pattern detections
WalkthroughsDescription• Add 3 new performance anti-pattern detections: exception handling in loops, type conversions in loops, and global variable mutations • Extend ASTAnalyzer with TryExceptInfo and GlobalInfo dataclasses for extracting try/except and global statements • Implement 3 new check methods in PerformanceChecker integrated into check_all() • Add 14 new comprehensive tests covering all new detection patterns Diagramflowchart LR
A["patterns.py<br/>New IssueCategory values<br/>TYPE_CONVERSION_FUNCTIONS set"] --> B["ast_analyzer.py<br/>TryExceptInfo & GlobalInfo<br/>dataclasses"]
B --> C["performance_checker.py<br/>3 new check methods"]
C --> D["test_performance_checker.py<br/>14 new tests"]
File Changes1. src/workshop_mcp/performance_profiler/patterns.py
|
Code Review by Qodo
✅
📎 Requirement gap ✧ Quality |
| class TestExceptionInLoops: | ||
| """Test exception-in-loop detection.""" | ||
|
|
||
| def test_detect_try_except_in_for_loop(self): |
There was a problem hiding this comment.
3. Tests lack type hints 📘 Rule violation ✓ Correctness
• Newly added test functions in tests/test_performance_checker.py do not include parameter and return type annotations. • This violates the requirement that all functions in src/ and tests/ must have type hints and remain mypy-compatible. • Add -> None return annotations (and any needed parameter annotations) to the new test methods.
Agent prompt
## Issue description
Newly added test methods are missing type hints (notably return type annotations like `-> None`), which violates the project requirement that all functions are type hinted and mypy-compatible.
## Issue Context
The new tests introduced for the three new performance patterns define multiple methods without annotations.
## Fix Focus Areas
- tests/test_performance_checker.py[233-444]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if isinstance(node, (astroid.For, astroid.While)): | ||
| current_in_loop = True |
There was a problem hiding this comment.
4. Async for loops missed 🐞 Bug ✓ Correctness
• Try/except-in-loop detection only treats For/While as loops, so try/except inside `async for` won’t be reported. • Type-conversion-in-loop detection has the same limitation because loop context for calls is also tracked only for For/While. • This creates false negatives in async-heavy code (a primary target for performance tooling).
Agent prompt
### Issue description
Async loops (`async for`) are not treated as loops, causing false negatives for the newly added `EXCEPTION_IN_LOOP` and `TYPE_CONVERSION_IN_LOOP` checks.
### Issue Context
The analyzer already recognizes `astroid.AsyncFunctionDef`, so async constructs are within scope.
### Fix Focus Areas
- src/workshop_mcp/performance_profiler/ast_analyzer.py[473-505]
- src/workshop_mcp/performance_profiler/ast_analyzer.py[406-449]
- src/workshop_mcp/performance_profiler/ast_analyzer.py[364-405]
- tests/test_performance_checker.py[233-360]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Added 3 new pattern detections to the performance profiler: 1. Exception handling in loops (EXCEPTION_IN_LOOP) - Detects try/except blocks inside loops - Severity: MEDIUM 2. Type conversions in loops (TYPE_CONVERSION_IN_LOOP) - Detects int(), str(), float(), list(), dict(), etc. in loops - Severity: MEDIUM 3. Global variable mutation (GLOBAL_MUTATION) - Detects functions using 'global' keyword - Severity: MEDIUM Changes: - patterns.py: Added new IssueCategory values and TYPE_CONVERSION_FUNCTIONS set - ast_analyzer.py: Added TryExceptInfo, GlobalInfo dataclasses and extraction methods - performance_checker.py: Added check_exception_in_loops(), check_type_conversions_in_loops(), check_global_mutations() - test_performance_checker.py: Added 14 new tests Closes #29 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: qodo-code-review[bot] <151058649+qodo-code-review[bot]@users.noreply.github.com>
Break long strings in description and suggestion fields to comply with 100-character line length limit. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PathValidator.validate() and validate_exists() return resolved Path objects after symlink resolution, but the returned values were being discarded. The code then used the original user-provided strings for file operations, creating a Time-of-Check-Time-of-Use vulnerability. Changes: - keyword_search: capture validated_paths from validate_multiple() and use validated_path_strings for tool execution - performance_check: capture validated_path from validate_exists() and use validated_file_path for PerformanceChecker and result output Addresses PR #49 review feedback. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
6b99a77 to
bcb605d
Compare
Summary
Adds 3 new performance anti-pattern detections to the performance profiler (closes #29).
New Patterns
EXCEPTION_IN_LOOPTYPE_CONVERSION_IN_LOOPint(),str(),float(), etc. creating objects repeatedlyGLOBAL_MUTATIONglobalkeyword to modify stateChanges
patterns.py: Added 3 newIssueCategoryvalues andTYPE_CONVERSION_FUNCTIONSsetast_analyzer.py: AddedTryExceptInfoandGlobalInfodataclasses with extraction methodsperformance_checker.py: Added 3 new check methods integrated intocheck_all()test_performance_checker.py: Added 14 new tests (29 total now)Test plan
🤖 Generated with Claude Code