Fix empty strings crashing lookup_code#1246
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughA guard clause is added to GDScriptLanguage::lookup_code() that returns early when p_symbol is empty, setting r_result.type = LOOKUP_RESULT_CLASS, r_result.class_name = "Variant" (commented //Nil), and returning ERR_CANT_RESOLVE. ChangesEarly Guard for Empty Symbol Lookup
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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. 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 |
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
Thanks CodeRabbit
There was a problem hiding this comment.
♻️ Duplicate comments (1)
modules/gdscript/gdscript_editor.cpp (1)
4305-4308:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid writing a resolved class result on an unresolved return path.
At Line 4306 and Line 4307,
r_resultis populated as a class ("Variant"), but Line 4308 returnsERR_CANT_RESOLVE. This mixes success-shaped output with an error result.🛠️ Minimal fix
if (p_symbol.is_empty()) { - r_result.type = ScriptLanguage::LOOKUP_RESULT_CLASS; - r_result.class_name = "Variant";//Nil 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 - 4308, The branch handling p_symbol.is_empty() is returning ERR_CANT_RESOLVE while populating r_result as a class (ScriptLanguage::LOOKUP_RESULT_CLASS and class_name = "Variant"), which mixes error and success-shaped output; change this to either leave r_result untouched or set r_result.type to a neutral/error state (e.g., ScriptLanguage::LOOKUP_RESULT_NONE) and remove the class_name = "Variant" assignment so no resolved class data is written when returning ERR_CANT_RESOLVE; update the p_symbol.is_empty() block in the function that uses p_symbol and r_result accordingly.
🤖 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.
Duplicate comments:
In `@modules/gdscript/gdscript_editor.cpp`:
- Around line 4305-4308: The branch handling p_symbol.is_empty() is returning
ERR_CANT_RESOLVE while populating r_result as a class
(ScriptLanguage::LOOKUP_RESULT_CLASS and class_name = "Variant"), which mixes
error and success-shaped output; change this to either leave r_result untouched
or set r_result.type to a neutral/error state (e.g.,
ScriptLanguage::LOOKUP_RESULT_NONE) and remove the class_name = "Variant"
assignment so no resolved class data is written when returning ERR_CANT_RESOLVE;
update the p_symbol.is_empty() block in the function that uses p_symbol and
r_result accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6cc3bc4-bc18-4cdf-840a-90c83944b54a
📒 Files selected for processing (1)
modules/gdscript/gdscript_editor.cpp
|
Thanks for contributing this fix @U-YumeTsuki ! I am not that worried about code rabbit's remaining feedback, so long as I dont run into any problems during testing. Returning an error code should be sufficient here, and I dont think the defaut values you are returning are likely to actually cause problems here, but will need to test to make sure. |
Arctis-Fireblight
left a comment
There was a problem hiding this comment.
I have tested and confirmed the presence of the crash on master, and have created an issue for it #1248
Not sure why/how this crash was introduced but I was not able to reproduce on my official release binaries of 26.1. This was a good catch, and glad you were able to find it before it slipped into the beta!
This will be approved for merge once it is passing the static checks and is passing the GitHub Actions pipeline.
It is what it is
Arctis-Fireblight
left a comment
There was a problem hiding this comment.
Thank you!
This is approved for merge once the GH Actions finish running

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