fix: Dynamic code snippets now recognize bare Object and Random calls consistently#1657
Merged
Merged
Conversation
Ports main commit 045f525 (#1469) to v3-beta. The Object alias already existed via WrapperTemplate's ad-hoc string scan (PR #1234), but Random was missing and the detection approach did not generalize. Replace the "Object"-only text scan with a generic mechanism: SourceShaper now collects every user-defined using-alias name (including global/verbatim/commented forms) into SourceShapeResult.AliasedNames, and WrapperTemplate looks up a small DefaultUsingAlias table (Object, Random) against that set instead of re-parsing using-directive strings. Also fixes a latent gap in global-using detection: the keyword scan between "global" and "using" only skipped whitespace, so a comment in between caused the whole global using directive to be misclassified as a top-level statement. Restores unit test coverage for this behavior (deleted in #1310 because the old tests were execution-based and froze the Editor); these are pure preparer/shaper-level tests with no compile-and-run step, so they carry no freeze risk.
Contributor
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWrapperTemplate.Build now accepts aliasedNames and injects default aliases from a table of known mappings. SourceShaper collects alias names from using directives into AliasedNames and passes them through DynamicCodeSourcePreparer. Tests were updated for the new call signature and added coverage for alias parsing, injection, and substring counting. ChangesAlias detection and injection
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
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
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/WrapperTemplate.cs`:
- Around line 2-3: The precondition checks in WrapperTemplate currently resolve
to System.Diagnostics.Debug instead of Unity’s assertion API, so update the
assert calls to use UnityEngine.Debug explicitly or alias Debug to
UnityEngine.Debug. Keep the change scoped to the code that uses these asserts in
the dynamic compilation template so the intended Unity assertion behavior is
preserved.
🪄 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: d012552a-447d-437d-b996-ac0878a0a8a1
⛔ Files ignored due to path filters (1)
Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeTestStringUtility.cs.metais excluded by none and included by none
📒 Files selected for processing (8)
Assets/Tests/Editor/DynamicCodeToolTests/AutoInjectedNamespacesTests.csAssets/Tests/Editor/DynamicCodeToolTests/DynamicCodeSourcePreparerTests.csAssets/Tests/Editor/DynamicCodeToolTests/DynamicCodeTestStringUtility.csAssets/Tests/Editor/DynamicCodeToolTests/PreUsingResolverTests.csAssets/Tests/Editor/DynamicCodeToolTests/SourceShaperTests.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/DynamicCodeSourcePreparer.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceShaper.csPackages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/WrapperTemplate.cs
Comment on lines
+2
to
+3
| using System.Diagnostics; | ||
| using System.Linq; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check using directives in WrapperTemplate.cs and SourceShaper.cs
echo "=== WrapperTemplate.cs (first 10 lines) ==="
head -10 Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/WrapperTemplate.cs
echo ""
echo "=== SourceShaper.cs (first 10 lines) ==="
head -10 Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/SourceShaper.cs
# Check if any file in this directory imports both UnityEngine and System.Diagnostics
echo ""
echo "=== Files importing both UnityEngine and System.Diagnostics ==="
rg -l 'using UnityEngine;' Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/ | xargs rg -l 'using System.Diagnostics;' 2>/dev/null || echo "None found"Repository: hatayama/unity-cli-loop
Length of output: 1312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== WrapperTemplate.cs (all lines with numbers) ==="
cat -n Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/WrapperTemplate.cs
echo ""
echo "=== Search for any explicit Debug qualification in this directory ==="
rg -n '\b(UnityEngine\.)?Debug\.Assert|\busing\s+Debug\s*=' Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/
echo ""
echo "=== Search for UnityEngine imports in WrapperTemplate.cs vicinity ==="
rg -n 'using UnityEngine;|global using UnityEngine;|using Debug = ' Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/WrapperTemplate.csRepository: hatayama/unity-cli-loop
Length of output: 13626
Use UnityEngine.Debug for these asserts
Debug.Assert here resolves to System.Diagnostics.Debug, so these precondition checks can be stripped in non-DEBUG builds instead of using Unity’s assertion path. Qualify them as UnityEngine.Debug.Assert or alias Debug to UnityEngine.Debug if that’s the intended API.
🤖 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
`@Packages/src/Editor/FirstPartyTools/ExecuteDynamicCode/DynamicCompilation/WrapperTemplate.cs`
around lines 2 - 3, The precondition checks in WrapperTemplate currently resolve
to System.Diagnostics.Debug instead of Unity’s assertion API, so update the
assert calls to use UnityEngine.Debug explicitly or alias Debug to
UnityEngine.Debug. Keep the change scoped to the code that uses these asserts in
the dynamic compilation template so the intended Unity assertion behavior is
preserved.
Source: Learnings
…ments
The plain "using" path in TryAnalyzeTopLevelUsing still used
SkipWhitespace instead of SkipWhitespaceAndComments, so a comment
between "using" and "var"/"(" caused a using-statement to be
misclassified as a using-directive and dropped from the user code.
Fixed to match the already-corrected global-using path, and added a
regression test for the comment-before-var case.
Also added the required WHAT comment to each new test method per
this repo's test convention.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Random.Range(...)without an explicitusingnow compile, matching the existing bare-Objectsupport.global usingforms, so a snippet's own custom alias is respected even when written unusually (e.g.using @Object = ...;, or with comments between tokens).User Impact
Objectcalls were auto-resolved; a bareRandom.Range(...)in dynamic code failed to compile unless the user addedusing Random = UnityEngine.Random;manually. Aglobal usingalias with a comment in between tokens was also missed.ObjectandRandomare auto-resolved when the snippet doesn't already alias them, and user-defined aliases (including verbatim/comment-containing forms) are correctly detected and respected.Changes
SourceShapernow collects every using-alias name the snippet defines (AliasedNames), replacing the old ad-hoc string scan that only recognized "Object".WrapperTemplateinjects default aliases (Object,Random) from a small table, skipping any name already inAliasedNames.globalandusingcaused the directive to be misclassified as a plain statement.Verification
dist/darwin-arm64/uloop compile— 0 errors, 0 warningsdist/darwin-arm64/uloop run-tests(EditMode, full suite) — 1725 passed, 0 failed, 7 skipped (pre-existing skips)