Skip to content

Fix empty strings crashing lookup_code#1246

Merged
Arctis-Fireblight merged 3 commits into
Redot-Engine:masterfrom
U-YumeTsuki:editor_bugfix
May 11, 2026
Merged

Fix empty strings crashing lookup_code#1246
Arctis-Fireblight merged 3 commits into
Redot-Engine:masterfrom
U-YumeTsuki:editor_bugfix

Conversation

@U-YumeTsuki
Copy link
Copy Markdown
Contributor

@U-YumeTsuki U-YumeTsuki commented May 4, 2026

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

  • Bug Fixes
    • Fixed editor lookup to properly handle empty symbols: it now returns a clear, consistent "unresolved" result (treated as Variant/Nil) and avoids spurious class/constant resolution or analyzer confusion, improving code-intelligence accuracy and editor stability.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6c0a547e-e63c-4fbd-ae70-ed8cd920ecf7

📥 Commits

Reviewing files that changed from the base of the PR and between b1e8df5 and e6e05c7.

📒 Files selected for processing (1)
  • modules/gdscript/gdscript_editor.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • modules/gdscript/gdscript_editor.cpp

Walkthrough

A 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.

Changes

Early Guard for Empty Symbol Lookup

Layer / File(s) Summary
Edge Case Handling
modules/gdscript/gdscript_editor.cpp
Early return in lookup_code() when p_symbol is empty; sets r_result.type = LOOKUP_RESULT_CLASS, r_result.class_name = "Variant" (comment //Nil), and returns ERR_CANT_RESOLVE.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: fixing a crash in lookup_code when empty strings are encountered.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between c9e566c and 3f916d3.

📒 Files selected for processing (1)
  • modules/gdscript/gdscript_editor.cpp

Comment thread modules/gdscript/gdscript_editor.cpp
Thanks CodeRabbit
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
modules/gdscript/gdscript_editor.cpp (1)

4305-4308: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid writing a resolved class result on an unresolved return path.

At Line 4306 and Line 4307, r_result is populated as a class ("Variant"), but Line 4308 returns ERR_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

📥 Commits

Reviewing files that changed from the base of the PR and between 3f916d3 and b1e8df5.

📒 Files selected for processing (1)
  • modules/gdscript/gdscript_editor.cpp

@Arctis-Fireblight
Copy link
Copy Markdown
Contributor

Thanks for contributing this fix @U-YumeTsuki !
I'll start testing this in a few minutes, but in the meantime it is failing the static checks.
image
Do you mind either running pre-commit run -a or just adding a space before //Nil?

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.

Copy link
Copy Markdown
Contributor

@Arctis-Fireblight Arctis-Fireblight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread modules/gdscript/gdscript_editor.cpp Outdated
Comment thread modules/gdscript/gdscript_editor.cpp
@Arctis-Fireblight Arctis-Fireblight dismissed their stale review May 11, 2026 02:58

Requested changes were made

Copy link
Copy Markdown
Contributor

@Arctis-Fireblight Arctis-Fireblight left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

This is approved for merge once the GH Actions finish running

@Arctis-Fireblight Arctis-Fireblight merged commit 300d075 into Redot-Engine:master May 11, 2026
17 checks passed
@github-project-automation github-project-automation Bot moved this from Open to Done in Engine Overview May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Regression] Empty strings crash the editor when looking up code

2 participants