Add support for include_directive in C #57
Conversation
Fixes #46 Add support for processing `include_directive` in C files. * **api/analyzers/c/analyzer.py** - Add `process_include_directive` method to handle `include_directive` nodes and create edges between files. - Modify `first_pass` method to process `include_directive` nodes and create edges between files. * **tests/test_c_analyzer.py** - Add test case to verify the creation of edges between files for `include_directive`. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/FalkorDB/code-graph-backend/issues/46?shareId=XXXX-XXXX-XXXX-XXXX).
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces enhanced functionality for the C analyzer to process include directives in source code. A new method Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_c_analyzer.py (1)
63-69: Consider adding negative or edge case checks.This test validates that the included file is recognized, but adding a scenario for a missing or malformed include file would further strengthen coverage.
api/analyzers/c/analyzer.py (2)
318-347: Avoid star imports and explicitly importFilefor clarity.The static analysis warning highlights that
Fileis defined via a star import. Consider importing it explicitly to make dependencies more transparent and reduce potential naming collisions.-from ...entities import * +from ...entities import File, ...🧰 Tools
🪛 Ruff (0.8.2)
318-318318:
Filemay be undefined, or defined from star imports(F405)
342-342342:
Filemay be undefined, or defined from star imports(F405)
421-429: Consider handling conditional includes or macro-based includes.Currently, the logic processes direct includes, but conditional or macro-based includes might remain unhandled. Consider logging or gracefully skipping such cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/analyzers/c/analyzer.py(2 hunks)tests/test_c_analyzer.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
api/analyzers/c/analyzer.py
318-318318: File may be undefined, or defined from star imports
(F405)
342-342342: File may be undefined, or defined from star imports
(F405)
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
api/analyzers/c/analyzer.py (1)
Tests needed for include directive processing
While there is a basic test case for include directives in
test_c_analyzer.py, it only covers the basic case. The test fixtures don't properly exercise the new include directive functionality. Additional test cases are needed to cover:
- System includes with angle brackets (
<stdio.h>)- Local includes with quotes (
"myheader.h")- Error cases:
- Malformed include directives
- Missing include path field
- Invalid file paths
Example locations to add tests:
tests/test_c_analyzer.py: Add new test methodstests/source_files/c/: Add test fixtures with various include scenariosNote: The current test in
test_c_analyzer.pyasserts the existence of amyheader.hinclude relationship, but the test fixturesrc.cdoesn't actually contain any include directives, making the test unreliable. This inconsistency should be fixed along with adding the new test cases.🔗 Analysis chain
Line range hint
318-429: Consider adding unit tests for the new functionality.The changes look good overall, but to ensure reliability and prevent regressions, consider adding unit tests for:
- Processing of string literal includes
- Processing of system includes with angle brackets
- Error handling scenarios (malformed includes, file creation failures)
- Graph edge creation verification
Let's check if there are any existing tests for this functionality:
Would you like me to help create comprehensive test cases for the new include directive functionality?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for relevant test cases rg -l "test.*include.*directive" tests/Length of output: 41
Script:
#!/bin/bash # Let's try a broader search for test files related to C analyzer fd "test.*\.py" tests/ # Also search for any test files that might test includes or preprocessor directives rg -l "include|preproc|directive" tests/Length of output: 449
Script:
#!/bin/bash # Let's check if there are any test files specifically for the C analyzer fd "test.*analyzer.*\.py" tests/Length of output: 85
Script:
#!/bin/bash # Let's check the actual test content for C analyzer if it exists rg "class.*Test.*Analyzer" tests/ -A 5Length of output: 712
Script:
#!/bin/bash # Let's also check if there are any test fixtures or sample C files used in tests fd -e c -e h tests/Length of output: 351
Script:
#!/bin/bash # Let's check the content of the C analyzer test file cat tests/test_c_analyzer.pyLength of output: 2400
Script:
#!/bin/bash # Let's look for test fixtures in tests directory correctly fd -e c -e h . testsLength of output: 49
Script:
#!/bin/bash # Let's check the content of the test fixture cat tests/source_files/c/src.cLength of output: 213
🧰 Tools
🪛 Ruff (0.8.2)
318-318318:
Filemay be undefined, or defined from star imports(F405)
342-342342:
Filemay be undefined, or defined from star imports(F405)
🧹 Nitpick comments (1)
api/analyzers/c/analyzer.py (1)
318-318: Fix the undefinedFileimport.The static analysis tool indicates that
Filemay be undefined or imported via star import. Consider making the import explicit.-from ..utils import * +from ..utils import find_child_of_type +from ...entities import File, Function, Struct🧰 Tools
🪛 Ruff (0.8.2)
318-318318:
Filemay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/analyzers/c/analyzer.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
api/analyzers/c/analyzer.py
318-318318: File may be undefined, or defined from star imports
(F405)
342-342342: File may be undefined, or defined from star imports
(F405)
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/analyzers/c/analyzer.py (3)
332-332: Enhance type assertion message for better debugging.The assertion should include a more descriptive error message to aid in debugging.
- assert(node.type == 'system_lib_string' or node.type == 'string_literal') + assert node.type in ('system_lib_string', 'string_literal'), \ + f"Expected system_lib_string or string_literal, got {node.type}"
424-432: Add logging and error handling for include directive processing.The include directive processing should log the number of includes found and handle query execution errors.
# Process include directives - query = C_LANGUAGE.query("(preproc_include [(string_literal) (system_lib_string)] @include)") - captures = query.captures(tree.root_node) + try: + query = C_LANGUAGE.query( + "(preproc_include [(string_literal) (system_lib_string)] @include)") + captures = query.captures(tree.root_node) - if 'include' in captures: - includes = captures['include'] - for node in includes: - self.process_include_directive(file, node, path, graph) + if 'include' in captures: + includes = captures['include'] + logger.info("Processing %d include directives in %s", + len(includes), path) + for node in includes: + logger.debug("Processing include at line %d", + node.start_point[0] + 1) + self.process_include_directive(file, node, path, graph) + except Exception as e: + logger.error("Failed to process include directives in %s: %s", + path, e)
Line range hint
318-432: Consider future enhancements for include directive processing.While the current implementation successfully adds support for include directives, consider these future enhancements:
- Add support for include path resolution using compiler-specific search paths
- Implement caching to avoid reprocessing the same included files
- Add support for conditional includes (#ifdef, #ifndef)
- Consider tracking include order for dependency analysis
Would you like me to create GitHub issues to track these potential improvements?
🧰 Tools
🪛 Ruff (0.8.2)
318-318:
Filemay be undefined, or defined from star imports(F405)
345-345:
Filemay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api/analyzers/c/analyzer.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
api/analyzers/c/analyzer.py
318-318: File may be undefined, or defined from star imports
(F405)
345-345: File may be undefined, or defined from star imports
(F405)
🔇 Additional comments (1)
api/analyzers/c/analyzer.py (1)
344-349: 🛠️ Refactor suggestionAdd error handling for graph operations.
The graph operations should be wrapped in try-except blocks to handle potential failures gracefully.
# Create file entity for the included file - included_file = File(os.path.dirname(path), included_file_path, splitted[1]) - graph.add_file(included_file) + try: + included_file = File(os.path.dirname(path), included_file_path, splitted[1]) + graph.add_file(included_file) + except Exception as e: + logger.error("Failed to create/add file entity: %s", e) + return # Connect the parent file to the included file - graph.connect_entities('INCLUDES', parent.id, included_file.id) + try: + graph.connect_entities('INCLUDES', parent.id, included_file.id) + except Exception as e: + logger.error("Failed to create INCLUDES edge: %s", e)Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
345-345:
Filemay be undefined, or defined from star imports(F405)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/analyzers/c/analyzer.py (1)
435-443: Add logging for include directive processing.Consider adding logging to track the number of includes being processed.
# Process include directives query = C_LANGUAGE.query("(preproc_include [(string_literal) (system_lib_string)] @include)") captures = query.captures(tree.root_node) if 'include' in captures: includes = captures['include'] + logger.info("Processing %d include directives in %s", len(includes), path) for node in includes: + logger.debug("Processing include at line %d", node.start_point[0]) self.process_include_directive(file, node, path, graph)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/analyzers/c/analyzer.py(2 hunks)tests/test_c_analyzer.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_c_analyzer.py
🧰 Additional context used
🪛 Ruff (0.8.2)
api/analyzers/c/analyzer.py
318-318: File may be undefined, or defined from star imports
(F405)
356-356: File may be undefined, or defined from star imports
(F405)
🔇 Additional comments (3)
api/analyzers/c/analyzer.py (3)
332-332: LGTM! Comprehensive type assertion.The assertion correctly handles both system includes (
<header.h>) and local includes ("header.h").
359-360: 🛠️ Refactor suggestionAdd error handling for graph connection.
The edge creation could fail and should be handled gracefully.
# Connect the parent file to the included file -graph.connect_entities('INCLUDES', parent.id, included_file.id) +try: + graph.connect_entities('INCLUDES', parent.id, included_file.id) +except Exception as e: + logger.error("Failed to create INCLUDES edge: %s", e)Likely invalid or redundant comment.
352-358: 🛠️ Refactor suggestionAdd error handling for graph operations.
The graph operations could fail and should be handled gracefully.
# Create file entity for the included file path = os.path.dirname(normalized_path) name = os.path.basename(normalized_path) ext = splitted[1] included_file = File(path, name, ext) -graph.add_file(included_file) +try: + graph.add_file(included_file) +except Exception as e: + logger.error("Failed to add file to graph: %s", e) + returnLikely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
356-356:
Filemay be undefined, or defined from star imports(F405)
| try: | ||
| included_file_path = node.text.decode('utf-8').strip('"<>') | ||
| if not included_file_path: | ||
| logger.warning("Empty include path found in %s", path) | ||
| return | ||
|
|
||
| # Normalize and validate path | ||
| normalized_path = os.path.normpath(included_file_path) | ||
| except UnicodeDecodeError as e: | ||
| logger.error("Failed to decode include path in %s: %s", path, e) | ||
| return | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance path validation for security.
While the path normalization is implemented, it should be enhanced to prevent path traversal attacks and validate system includes differently from local includes.
try:
included_file_path = node.text.decode('utf-8').strip('"<>')
if not included_file_path:
logger.warning("Empty include path found in %s", path)
return
- # Normalize and validate path
- normalized_path = os.path.normpath(included_file_path)
+ # Handle system includes and local includes differently
+ is_system_include = node.type == 'system_lib_string'
+ normalized_path = os.path.normpath(included_file_path)
+
+ if is_system_include:
+ # System includes should be searched in system include paths
+ if '/' in normalized_path:
+ logger.warning("System include with path separator in %s: %s",
+ path, included_file_path)
+ return
+ else:
+ # Local includes should not traverse outside the project
+ if normalized_path.startswith('..') or normalized_path.startswith('/'):
+ logger.warning("Path traversal attempt in %s: %s",
+ path, included_file_path)
+ return
except UnicodeDecodeError as e:
logger.error("Failed to decode include path in %s: %s", path, e)
return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| included_file_path = node.text.decode('utf-8').strip('"<>') | |
| if not included_file_path: | |
| logger.warning("Empty include path found in %s", path) | |
| return | |
| # Normalize and validate path | |
| normalized_path = os.path.normpath(included_file_path) | |
| except UnicodeDecodeError as e: | |
| logger.error("Failed to decode include path in %s: %s", path, e) | |
| return | |
| try: | |
| included_file_path = node.text.decode('utf-8').strip('"<>') | |
| if not included_file_path: | |
| logger.warning("Empty include path found in %s", path) | |
| return | |
| # Handle system includes and local includes differently | |
| is_system_include = node.type == 'system_lib_string' | |
| normalized_path = os.path.normpath(included_file_path) | |
| if is_system_include: | |
| # System includes should be searched in system include paths | |
| if '/' in normalized_path: | |
| logger.warning("System include with path separator in %s: %s", | |
| path, included_file_path) | |
| return | |
| else: | |
| # Local includes should not traverse outside the project | |
| if normalized_path.startswith('..') or normalized_path.startswith('/'): | |
| logger.warning("Path traversal attempt in %s: %s", | |
| path, included_file_path) | |
| return | |
| except UnicodeDecodeError as e: | |
| logger.error("Failed to decode include path in %s: %s", path, e) | |
| return |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
api/analyzers/c/analyzer.py:353
- [nitpick] The local variable 'path' shadows the method argument 'path'. Consider renaming the local variable to avoid confusion.
path = os.path.dirname(normalized_path)
Fixes #46
Add support for processing
include_directivein C files.api/analyzers/c/analyzer.py
process_include_directivemethod to handleinclude_directivenodes and create edges between files.first_passmethod to processinclude_directivenodes and create edges between files.tests/test_c_analyzer.py
include_directive.For more details, open the Copilot Workspace session.
Summary by CodeRabbit
New Features
Tests