Fix empty strings crashing lookup_code#1246
Fix empty strings crashing lookup_code#1246U-YumeTsuki wants to merge 1 commit intoRedot-Engine:masterfrom
Conversation
WalkthroughA guard clause is added to ChangesEarly Guard for Empty Symbol Lookup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 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 `@modules/gdscript/gdscript_editor.cpp`:
- Around line 4305-4309: When p_symbol.is_empty(), do not emit a fake sentinel
result; instead return ERR_CANT_RESOLVE and remove the assignment r_result.type
= ScriptLanguage::LOOKUP_RESULT_CLASS and r_result.class_name = "Nil". Use the
established error code ERR_CANT_RESOLVE for unresolvable symbols (consistent
with other branches that return ERR_CANT_RESOLVE) so callers don't receive an
invalid "Nil" ClassDB name and can handle the unresolved case uniformly.
🪄 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: CHILL
Plan: Pro
Run ID: cdadfa9c-3f60-4083-bd89-4a9a0234679c
📒 Files selected for processing (1)
modules/gdscript/gdscript_editor.cpp
| if (p_symbol.is_empty()) { | ||
| r_result.type = ScriptLanguage::LOOKUP_RESULT_CLASS; | ||
| r_result.class_name = "Nil"; | ||
| return OK; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the LookupResult struct definition to check default value of `location`.
rg -n "LookupResult" --type cpp -A 20 | grep -A 20 "struct LookupResult"Repository: Redot-Engine/redot-engine
Length of output: 1167
🏁 Script executed:
cat -n core/object/script_language.h | sed -n '371,430p'Repository: Redot-Engine/redot-engine
Length of output: 2055
Return ERR_CANT_RESOLVE for an empty symbol instead of OK with a fake "Nil" class
Two concrete problems with the current guard:
-
OKmisrepresents the outcome. There is no resolved symbol here.ERR_CANT_RESOLVEis the established return code for all other unresolvable symbols in this function (line 4647), and all callers already handle it correctly. -
"Nil"is not a ClassDB class. While the first caller ingdscript.cppis shielded by its ownClassDB::class_exists(result.class_name)guard, this relies on every present and future caller independently filtering out a sentinel value that should never have been emitted in the first place.
The early exit correctly prevents any crashes downstream. The improvement is about semantic correctness and avoiding the creation of invalid sentinel results that callers must defensively filter.
🛠️ Proposed fix
if (p_symbol.is_empty()) {
- r_result.type = ScriptLanguage::LOOKUP_RESULT_CLASS;
- r_result.class_name = "Nil";
- return OK;
+ return ERR_CANT_RESOLVE;
}📝 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.
| if (p_symbol.is_empty()) { | |
| r_result.type = ScriptLanguage::LOOKUP_RESULT_CLASS; | |
| r_result.class_name = "Nil"; | |
| return OK; | |
| } | |
| if (p_symbol.is_empty()) { | |
| return ERR_CANT_RESOLVE; | |
| } |
🤖 Prompt for 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.
In `@modules/gdscript/gdscript_editor.cpp` around lines 4305 - 4309, When
p_symbol.is_empty(), do not emit a fake sentinel result; instead return
ERR_CANT_RESOLVE and remove the assignment r_result.type =
ScriptLanguage::LOOKUP_RESULT_CLASS and r_result.class_name = "Nil". Use the
established error code ERR_CANT_RESOLVE for unresolvable symbols (consistent
with other branches that return ERR_CANT_RESOLVE) so callers don't receive an
invalid "Nil" ClassDB name and can handle the unresolved case uniformly.
1 participant
The fixed bug in question could be replicated as simply as holding ctrl and dragging the mouse around in the code editor
Summary by CodeRabbit