fix: kernel return validation#1411
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughModuleBuilder now raises WarpCodegenTypeError when a kernel indicates a return value either via ChangesKernel Codegen Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Greptile SummaryThis PR tightens kernel validation so that a non-
Confidence Score: 5/5Safe to merge — the change is a targeted one-line guard that closes a loophole in kernel signature validation without touching any code-generation paths. The single changed line in No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[build_kernel called] --> B[kernel.adj.build]
B --> C{return_var is not None?}
C -- Yes --> E[raise WarpCodegenTypeError]
C -- No --> D{return annotation present and non-None?}
D -- Yes --> E
D -- No --> F[kernel built successfully]
Reviews (8): Last reviewed commit: "Adding a fix to Unreleased" | Re-trigger Greptile |
Signed-off-by: falker <arsenija111mot@gmail.com>
Signed-off-by: falker <arsenija111mot@gmail.com>
4be775d to
0d95b60
Compare
|
Thanks for the contribution. This addresses the existing TODO in Could you please make a few cleanup changes before this can move forward?
Also, for future non-trivial codegen behavior changes, please open an issue or discussion first so we can agree on the desired behavior before implementation. @c0d1f1ed @nvlukasz, could you weigh in on whether a non- |
|
Alright, I created a changelog entry and wrote a test regarding the commits, I'm thinking of doing a squash after all the necessary maintainers have shared their feedback, so that all changes are agreed upon if needed also, the single squash commit will be signed as I understand it, intermediate commits don't need signatures since the squash will bring them into one? And next time for similar fixes I will create an issue or a discussion. Thanks. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 201-210: Move the listed bullet items (the duplicate 1.13.0 fixes
starting with "Fix `ValueError: Cell is empty`..." through the
"WarpCodegenTypeError" bullet) out of the released 1.13.0 section and append
them to the end of the Unreleased section, removing the duplicate bullets from
1.13.0; ensure each moved entry uses imperative present tense and retains its GH
reference format (e.g., "([GH-913](...))"), and delete the redundant 1.13.0
copies so Unreleased contains the authoritative entries.
🪄 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.yml
Review profile: CHILL
Plan: Enterprise
Run ID: 0946e968-9275-4e2e-b1fe-a8d75c4b0f52
📒 Files selected for processing (3)
CHANGELOG.mdwarp/_src/context.pywarp/tests/test_codegen.py
|
|
||
| - Initial publish for alpha testing | ||
|
|
||
| [Unreleased]: https://github.com/NVIDIA/warp/compare/v1.13.0...HEAD | ||
| [1.13.0]: https://github.com/NVIDIA/warp/releases/tag/v1.13.0 | ||
| [1.12.1]: https://github.com/NVIDIA/warp/releases/tag/v1.12.1 | ||
| [Unreleased]: https://github.com/NVIDIA/warp/compare/v1.12.0...HEAD |
There was a problem hiding this comment.
Branch appears to pre-date the 1.13.0 release
The diff removes the entire [1.13.0] - 2026-05-04 section and changes the [Unreleased] comparison link from v1.13.0...HEAD to v1.12.0...HEAD. If this PR is targeting main and 1.13.0 has already been cut, merging as-is will silently drop all 1.13.0 release notes. Please rebase onto the current main tip (or at minimum cherry-pick this commit onto the correct base) so the changelog history is preserved.
Description
Kernels are architecturally prohibited from returning values. Previously, the code generation system allowed return type annotations
(e.g., '-> float')for kernels, but the function body did not explicitly return a value. This PR adds a check for rejecting return type hints for kernels to prevent misleading function signatures and avoid user confusion.This also addresses an existing
TODOand includes a corresponding test.Checklist
Test plan
Verified by running the codegen test suite:
test_codegen.pyThe updated
test_error_kernel_return_valueexplicitly checks and passes theWarpCodegenTypeErrorassertion when a return type hint is provided without a return value.Bug fix