fix: Dynamic code now accepts bare Unity Object calls#1469
Merged
Conversation
Add a UnityEngine.Object alias to wrapped execute-dynamic-code snippets so bare Object.Instantiate and related Unity APIs do not collide with System.Object. Preserve user-provided Object aliases to avoid duplicate using directives.
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 (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds alias tracking to source shaping, passes collected alias names into wrapper generation, and updates tests for default Unity alias injection, alias preservation, and the revised wrapper call signature. ChangesDynamic code alias propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant SourceShaper as SourceShaper.Analyze
participant SourceShapeResult as SourceShapeResult
participant DynamicCodeSourcePreparer as DynamicCodeSourcePreparer.Prepare
participant WrapperTemplate as WrapperTemplate.Build
SourceShaper as SourceShaper.Analyze->>SourceShapeResult: collect UsingDirectives and AliasedNames
DynamicCodeSourcePreparer as DynamicCodeSourcePreparer.Prepare->>WrapperTemplate: pass shape.AliasedNames
WrapperTemplate as WrapperTemplate.Build->>WrapperTemplate: emit default aliases before caller using directives
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
Packages/src/Editor/Compilation/WrapperTemplate.cs (1)
15-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting alias-detection helpers out of
WrapperTemplate.
WrapperTemplatewas previously a pure string-templating class; the newHasObjectAlias/IsObjectAliasDirective/SkipWhitespaceAndComments/IsCommentStarthelpers add a small hand-rolled tokenizer/parser responsibility on top of that. Moving this logic into a dedicated helper (e.g., alongsideSourceShaper, which already owns directive-scanning concerns) would keepWrapperTemplatefocused on template assembly and consolidate using-directive parsing in one place.🤖 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/Compilation/WrapperTemplate.cs` around lines 15 - 147, Move the new alias-detection logic out of WrapperTemplate.Build so the class stays focused on template assembly; extract HasObjectAlias, IsObjectAliasDirective, SkipWhitespaceAndComments, and IsCommentStart into a dedicated helper near SourceShaper, which already handles directive scanning. Keep Build only responsible for writing the wrapper and call the helper from there when deciding whether to emit the Object alias using directive.Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeSourcePreparerTests.cs (1)
42-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for
global using Object = ...;and comment-containing directives.Current tests don't exercise the
globalalias path inIsObjectAliasDirective, nor a directive with an embedded comment (e.g.using /* x */ Object = UnityEngine.Object;), which is the scenario tied to the potential infinite-loop risk flagged inWrapperTemplate.cs. Adding these would close the coverage gap for the new tokenizing logic.🤖 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 `@Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeSourcePreparerTests.cs` around lines 42 - 81, Add coverage in DynamicCodeSourcePreparerTests for the alias-detection paths that are currently untested: create tests around DynamicCodeSourcePreparer.Prepare that verify a global using Object = ... directive is recognized correctly, and that a directive containing an embedded comment such as using /* ... */ Object = ... does not break parsing or cause duplicate insertion. Reuse the existing Prepare_WhenObjectAliasAlreadyExists_ShouldNotAddDuplicateAlias and Prepare_WhenCustomObjectAliasAlreadyExists_ShouldRespectUserAlias patterns to assert the prepared source contains exactly one alias directive and that the user-provided alias is respected.
🤖 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.
Nitpick comments:
In `@Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeSourcePreparerTests.cs`:
- Around line 42-81: Add coverage in DynamicCodeSourcePreparerTests for the
alias-detection paths that are currently untested: create tests around
DynamicCodeSourcePreparer.Prepare that verify a global using Object = ...
directive is recognized correctly, and that a directive containing an embedded
comment such as using /* ... */ Object = ... does not break parsing or cause
duplicate insertion. Reuse the existing
Prepare_WhenObjectAliasAlreadyExists_ShouldNotAddDuplicateAlias and
Prepare_WhenCustomObjectAliasAlreadyExists_ShouldRespectUserAlias patterns to
assert the prepared source contains exactly one alias directive and that the
user-provided alias is respected.
In `@Packages/src/Editor/Compilation/WrapperTemplate.cs`:
- Around line 15-147: Move the new alias-detection logic out of
WrapperTemplate.Build so the class stays focused on template assembly; extract
HasObjectAlias, IsObjectAliasDirective, SkipWhitespaceAndComments, and
IsCommentStart into a dedicated helper near SourceShaper, which already handles
directive scanning. Keep Build only responsible for writing the wrapper and call
the helper from there when deciding whether to emit the Object alias using
directive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 169c71bc-1a13-4d00-9824-b74045b5e305
📒 Files selected for processing (2)
Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeSourcePreparerTests.csPackages/src/Editor/Compilation/WrapperTemplate.cs
Track using aliases during source shaping so wrapped snippets can avoid duplicate aliases, including verbatim identifiers such as @object. Add UnityEngine.Random as a default alias alongside UnityEngine.Object to cover the remaining System/Unity ambiguity.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Packages/src/Editor/Compilation/SourceShaper.cs (1)
685-697: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a
readonly structforAliasNameParseResult.This small immutable pair is allocated on every parsed
usingdirective. Since it's a value-like tuple of(string, int), areadonly struct(or even aValueTuple<string,int>) would avoid the heap allocation without changing behavior.♻️ Proposed refactor
- private sealed class AliasNameParseResult + private readonly struct AliasNameParseResult { public string Name { get; } public int EndPosition { get; } public AliasNameParseResult(string name, int endPosition) { Name = name; EndPosition = endPosition; } }🤖 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/Compilation/SourceShaper.cs` around lines 685 - 697, The AliasNameParseResult type is an immutable value-like pair currently allocated on every parsed using directive, so replace the sealed class with a readonly struct (or equivalent ValueTuple usage) while preserving the Name and EndPosition members and constructor semantics. Update the AliasNameParseResult declaration inside SourceShaper to be stack-allocated/value-based, and make sure the parsing flow that returns or consumes AliasNameParseResult continues to behave identically with the new value type.
🤖 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.
Nitpick comments:
In `@Packages/src/Editor/Compilation/SourceShaper.cs`:
- Around line 685-697: The AliasNameParseResult type is an immutable value-like
pair currently allocated on every parsed using directive, so replace the sealed
class with a readonly struct (or equivalent ValueTuple usage) while preserving
the Name and EndPosition members and constructor semantics. Update the
AliasNameParseResult declaration inside SourceShaper to be
stack-allocated/value-based, and make sure the parsing flow that returns or
consumes AliasNameParseResult continues to behave identically with the new value
type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e4618636-cb28-432d-95d2-4c3c90928c18
⛔ 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/Compilation/DynamicCodeSourcePreparer.csPackages/src/Editor/Compilation/SourceShaper.csPackages/src/Editor/Compilation/WrapperTemplate.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Assets/Tests/Editor/DynamicCodeToolTests/DynamicCodeSourcePreparerTests.cs
There was a problem hiding this comment.
2 issues found across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Prevent the shared substring test helper from hanging on empty targets, and assert that verbatim Object aliases are preserved when wrapping snippets.
Use the parsed alias set directly when adding default wrapper aliases, share comment skipping in SourceShaper, and cover comment-separated using aliases with a focused test.
This was referenced Jul 9, 2026
This was referenced Jul 19, 2026
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
Object.Instantiateand related Unity object calls without qualifyingUnityEngine.Object.Objectaliases are respected, so snippets that define their own alias do not get duplicate using directives.User Impact
Objectreference because the wrapper imported bothSystemandUnityEngine.Objectcompile as expected in script mode.Changes
Object = UnityEngine.Objectalias to wrapped script snippets.Objectalias.Verification
uloop compileuloop run-tests --test-mode EditMode --filter-type regex --filter-value "DynamicCodeSourcePreparerTests"uloop execute-dynamic-code --compile-only true --code "GameObject go = new GameObject(\"compile-only\"); Object.Instantiate(go); return null;"This PR updates dynamic code preparation/wrapping so Unity-style calls like
Object.Instantiatework without requiringUnityEngine.Object, while preventing ambiguousObjectreferences withSystem.Object.What changed
SourceShaper.Analyzenow centralizesusing-directive collection via a helper that also extracts alias declarations fromusing ... = Alias(supports optionalglobal, inline comments, and verbatim identifier aliases likeusing@object= ...).AliasedNames, which is passed throughDynamicCodeSourcePreparerintoWrapperTemplate.Build.WrapperTemplate.Buildnow acceptsaliasedNamesand injects default wrapper aliases only when the corresponding alias is requested/needed (currently:using Object = UnityEngine.Object;using Random = UnityEngine.Random;),while using the parsed alias set to avoid duplicate injection when the user already defines
Object(including verbatim@Object) orRandom.SourceShaperalso refactors comment-aware scanning used during parsing/advancing, improving robustness for alias detection around comments.Tests
DynamicCodeSourcePreparerTests:using Object = UnityEngine.Object;when the snippet uses bareObjectusing Object = UnityEngine.Object;already existsusing Object = System.Object;using@object= System.Object;using Random = UnityEngine.Random;when bareRandomis usedDynamicCodeTestStringUtility.CountSubstringand used it to safely assert substring counts (including returning0for empty targets).SourceShaperTeststo verify alias extraction correctness, includingglobal usingwith comments and verbatim alias syntax.WrapperTemplate.Buildsignature (including added empty alias-collection arguments).