ast_canopy: guard Record against dependent/incomplete types#326
ast_canopy: guard Record against dependent/incomplete types#326isVoid wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
The Record constructor called ctx.getTypeSize(type) / ctx.getTypeAlign(type) on class template specialisations reaching the matcher with ANCESTOR_IS_NOT_TEMPLATE. If the specialisation is still dependent or incomplete (e.g. a forward-declared Fwd<int> referenced only by pointer), Clang asserts inside getTypeSize and aborts parsing. Three related changes: 1. Guard the size/align computation with `!type->isDependentType() && !type->isIncompleteType()`. Use INVALID_SIZE_OF / INVALID_ALIGN_OF sentinels when layout cannot be computed, so downstream callers can detect and skip. 2. Wrap each per-child construction (Field, Method, FunctionTemplate, nested ClassTemplate, nested Record) in try/catch (...). A single bad child declaration now skips itself instead of losing the whole parent record. 3. Add a missing `<limits>` include. INVALID_SIZE_OF / INVALID_ALIGN_OF are defined using std::numeric_limits; the header was transitively present in the conda build but missing in the manylinux wheel build container, so the wheel build would fail to compile without it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 0 minutes and 46 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ast_canopy/cpp/src/record.cpp`:
- Around line 58-63: Replace the blanket catch(...) around
fields.emplace_back(Field(FD, access)) with selective catches: rethrow critical
allocation failures by catching std::bad_alloc and then catch std::exception to
handle expected failures; inside the std::exception catch, emit a debug-only log
that includes FD->getNameAsString() (and e.what()) so skipped fields are visible
in debug builds. Apply the same pattern to the other similar catch sites in this
file (the blocks around the other Field/Method/Record construction calls at the
ranges noted) so you don't silently swallow serious errors while still
preventing non-fatal parse failures from aborting processing.
In `@ast_canopy/tests/test_record_incomplete.py`:
- Around line 61-73: Add an assertion to the test to verify incomplete
specializations get the sentinel size: after calling
parse_declarations_from_source (as in test_complete_specialization_has_layout)
locate either the UsesIncomplete struct via decls.structs (match .name ==
"UsesIncomplete") or find the Fwd<int> entry in
decls.class_template_specializations, then assert that that specialization's
sizeof_ equals the sentinel INVALID_SIZE_OF (or that sizeof_ <= 0 if the
sentinel is defined that way) to ensure incomplete templates receive the
sentinel layout value.
- Around line 36-39: The fixture source_path currently returns a file path and
causes each test to re-parse the same file; change this module-scoped fixture to
perform the parse once and return the parsed declarations (e.g., call your
parsing function used elsewhere such as parse_translation_unit / parse_file /
parse_source) instead of a path, and update tests to consume the parsed
declarations from source_path rather than re-parsing the file so parsing is
cached for the whole test module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6687f9d-1036-4673-8e1d-a7bce0600ae3
📒 Files selected for processing (3)
ast_canopy/cpp/src/record.cppast_canopy/tests/data/sample_record_incomplete.cuast_canopy/tests/test_record_incomplete.py
| try { | ||
| fields.emplace_back(Field(FD, access)); | ||
| } catch (...) { | ||
| // Skip fields that cannot be processed (e.g. dependent types in | ||
| // uninstantiated templates). | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging or counting skipped elements in debug builds.
The blanket catch (...) blocks silently swallow all exceptions and skip elements. While this achieves the goal of preventing a single bad child from aborting the entire record, it may make debugging difficult when legitimate issues occur.
Consider at minimum logging skipped elements in NDEBUG builds:
} catch (...) {
`#ifndef` NDEBUG
std::cerr << "Skipped field: " << FD->getNameAsString() << std::endl;
`#endif`
}Additionally, catching all exceptions (including std::bad_alloc) could mask serious issues. If only Clang-specific exceptions are expected, consider catching a more specific base exception type if available.
Also applies to: 72-77, 81-85, 89-93, 97-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/cpp/src/record.cpp` around lines 58 - 63, Replace the blanket
catch(...) around fields.emplace_back(Field(FD, access)) with selective catches:
rethrow critical allocation failures by catching std::bad_alloc and then catch
std::exception to handle expected failures; inside the std::exception catch,
emit a debug-only log that includes FD->getNameAsString() (and e.what()) so
skipped fields are visible in debug builds. Apply the same pattern to the other
similar catch sites in this file (the blocks around the other
Field/Method/Record construction calls at the ranges noted) so you don't
silently swallow serious errors while still preventing non-fatal parse failures
from aborting processing.
| @pytest.fixture(scope="module") | ||
| def source_path(): | ||
| here = os.path.dirname(os.path.abspath(__file__)) | ||
| return os.path.join(here, "data", "sample_record_incomplete.cu") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching parse results to improve test efficiency.
Each test function parses the same source file independently. Consider extending the fixture to return the parsed declarations (with scope="module") to avoid redundant parsing:
♻️ Suggested refactor
`@pytest.fixture`(scope="module")
-def source_path():
+def parsed_decls():
here = os.path.dirname(os.path.abspath(__file__))
- return os.path.join(here, "data", "sample_record_incomplete.cu")
+ source_path = os.path.join(here, "data", "sample_record_incomplete.cu")
+ return parse_declarations_from_source(
+ source_path, [source_path], "sm_80", bypass_parse_error=True,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/tests/test_record_incomplete.py` around lines 36 - 39, The fixture
source_path currently returns a file path and causes each test to re-parse the
same file; change this module-scoped fixture to perform the parse once and
return the parsed declarations (e.g., call your parsing function used elsewhere
such as parse_translation_unit / parse_file / parse_source) instead of a path,
and update tests to consume the parsed declarations from source_path rather than
re-parsing the file so parsing is cached for the whole test module.
| def test_complete_specialization_has_layout(source_path): | ||
| """The fix must not regress layout computation for complete | ||
| specialisations: Complete<float> still gets a valid sizeof_.""" | ||
| decls = parse_declarations_from_source( | ||
| source_path, [source_path], "sm_80", bypass_parse_error=True, | ||
| ) | ||
| complete_specs = [ | ||
| cts | ||
| for cts in decls.class_template_specializations | ||
| if "Complete" in cts.qual_name | ||
| ] | ||
| assert complete_specs, "Complete<float> not in parsed specializations" | ||
| assert complete_specs[0].sizeof_ > 0, complete_specs[0].sizeof_ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add assertion for UsesIncomplete having sentinel size values.
The test verifies that Complete<float> has a valid positive sizeof_, but it doesn't verify that incomplete specializations actually receive the sentinel INVALID_SIZE_OF value. Consider adding an assertion to verify the sentinel behavior:
def test_incomplete_specialization_has_invalid_layout(source_path):
"""Fwd<int> specialization should have INVALID_SIZE_OF sentinel."""
decls = parse_declarations_from_source(
source_path, [source_path], "sm_80", bypass_parse_error=True,
)
uses_incomplete = next(
(s for s in decls.structs if s.name == "UsesIncomplete"), None
)
assert uses_incomplete is not None
# Check that the struct still has a valid layout (it's complete itself)
# but Fwd<int> specialization would have sentinel if exposedAlternatively, if Fwd<int> appears in class_template_specializations, verify its sizeof_ equals the sentinel value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ast_canopy/tests/test_record_incomplete.py` around lines 61 - 73, Add an
assertion to the test to verify incomplete specializations get the sentinel
size: after calling parse_declarations_from_source (as in
test_complete_specialization_has_layout) locate either the UsesIncomplete struct
via decls.structs (match .name == "UsesIncomplete") or find the Fwd<int> entry
in decls.class_template_specializations, then assert that that specialization's
sizeof_ equals the sentinel INVALID_SIZE_OF (or that sizeof_ <= 0 if the
sentinel is defined that way) to ensure incomplete templates receive the
sentinel layout value.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
1 participant
Summary
Three related changes in
ast_canopy/cpp/src/record.cpp:Guard size/align computation.
Record::Recordcalledctx.getTypeSize(type)/ctx.getTypeAlign(type)on class template specializations reaching the matcher withANCESTOR_IS_NOT_TEMPLATE. If the specialization is still dependent or incomplete (e.g. a forward-declaredFwd<int>referenced only by pointer), Clang asserts inside the size query and aborts parsing. Guard with!type->isDependentType() && !type->isIncompleteType()and emitINVALID_SIZE_OF/INVALID_ALIGN_OFsentinels when layout cannot be computed.Try/catch around per-child construction. Wrap each per-child construction (
Field,Method,FunctionTemplate, nestedClassTemplate, nestedRecord) intry { ... } catch (...) { /* skip */ }. A single bad child declaration now skips itself instead of losing the whole parent record.Missing
<limits>include.INVALID_SIZE_OF/INVALID_ALIGN_OFusestd::numeric_limits; the header was transitively present in the conda build but missing in the manylinux wheel build container, so the wheel build would fail to compile without it.Test plan
ast_canopy/tests/data/sample_record_incomplete.cu:Fwd<int>used only by pointer (incomplete specialization).Complete<float>specialization alongside (positive regression check).ast_canopy/tests/test_record_incomplete.py:UsesIncompleteandUsesCompleteare returned in the struct list.Complete<float>still reports a valid positivesizeof_(no regression on layout computation).<limits>include is a build-time concern and cannot be exercised from pytest; the test file docstring documents it for provenance.Summary by CodeRabbit
Bug Fixes
Tests