Skip to content

Restore support for parsing Variant::NIL and handling related cases in VariantParser#1276

Open
Arctis-Fireblight wants to merge 1 commit into
Redot-Engine:masterfrom
Arctis-Fireblight:fix-variant-nil
Open

Restore support for parsing Variant::NIL and handling related cases in VariantParser#1276
Arctis-Fireblight wants to merge 1 commit into
Redot-Engine:masterfrom
Arctis-Fireblight:fix-variant-nil

Conversation

@Arctis-Fireblight

@Arctis-Fireblight Arctis-Fireblight commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Fixes #1275

  • Implemented parsing logic for Variant::NIL and error handling for invalid constants.
  • Added tests to validate Variant::NIL, null, and nil parsing, including regression cases.
  • Improved security gating for object parsing with p_allow_objects.
  • Enhanced str_to_var to support Variant::NIL.

Summary by CodeRabbit

  • New Features
    • Added parsing support for Variant::NIL using Variant::NIL syntax.
    • Accepts alternative nil representations (null and nil) as valid inputs.
    • Ensures Variant::NIL parsing still succeeds even when object parsing is disabled.
  • Bug Fixes
    • Improved parse errors for unknown variant constants and incomplete Variant::... expressions.
  • Tests
    • Added coverage for successful Variant::NIL parsing and expected failure scenarios, plus a regression check via string-to-variant conversion.

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

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: d6c7d9af-94ae-4293-8fa7-70316b23e4a2

📥 Commits

Reviewing files that changed from the base of the PR and between d98b24c and aadd319.

📒 Files selected for processing (2)
  • core/variant/variant_parser.cpp
  • tests/core/variant/test_variant.h
🚧 Files skipped from review as they are similar to previous changes (2)
  • core/variant/variant_parser.cpp
  • tests/core/variant/test_variant.h

Walkthrough

VariantParser::parse_value gains a new branch that recognizes the identifier "Variant", then parses a :: token pair and an identifier constant, mapping "NIL" to Variant() and raising a parse error for any other constant. Tests are added covering success paths, error paths, the p_allow_objects security gate, and a str_to_var regression.

Changes

Variant::NIL parsing support

Layer / File(s) Summary
Parser branch and test coverage
core/variant/variant_parser.cpp, tests/core/variant/test_variant.h
Adds the "Variant" identifier branch in parse_value that expects :: followed by an identifier and maps "NIL" to Variant(), raising parse errors otherwise. Tests cover "Variant::NIL", "null", "nil" success paths; "Variant::INVALID" and bare "Variant" error paths; the p_allow_objects flag behaviour; and a VariantUtilityFunctions::str_to_var("Variant::NIL") regression.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 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 PR title accurately summarizes the main change: restoring support for parsing Variant::NIL in VariantParser, which aligns with the core code modifications and objectives.
Linked Issues check ✅ Passed The PR addresses issue #1275 by restoring Variant::NIL parsing in VariantParser and str_to_var(), implementing error handling for invalid constants, and adding comprehensive test coverage to prevent regression.
Out of Scope Changes check ✅ Passed All changes are scoped to restoring Variant::NIL parsing support and related test coverage; no unrelated modifications are present in the changeset.

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

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

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.

Comment on lines +718 to +721
Error err = get_token(p_stream, token, line, r_err_str);
if (err) {
return err;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's moments like this that make me wish for or_return in C and C++

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would be really nice ;-;

@Arctis-Fireblight Arctis-Fireblight changed the title Add support for parsing Variant::NIL and handling related cases in VariantParser Restore support for parsing Variant::NIL and handling related cases in VariantParser Jun 18, 2026
…`VariantParser`

- Implemented parsing logic for `Variant::NIL` and error handling for invalid constants.
- Added tests to validate `Variant::NIL`, `null`, and `nil` parsing, including regression cases.
- Improved security gating for object parsing with `p_allow_objects`.
- Enhanced `str_to_var` to support `Variant::NIL`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Open

Development

Successfully merging this pull request may close these issues.

str_to_var() fails to parse "Variant::NIL"

2 participants