feat: add watch expression registry core - #1730
Merged
Merged
Conversation
Provide a pure C# registry for ordered per-frame watch evaluation, bounded history, immediate baselines, and re-entry protection. Reuse the dynamic-code compiler and convert only generated evaluator invocation failures into typed history results so Editor updates continue after user expression errors.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces a watch-expression subsystem that compiles expressions, captures evaluation failures, stores bounded evaluation history, and evaluates registered expressions on paused Unity Editor frame changes. NUnit and Unity tests cover compiler, evaluator, and registry behavior. ChangesWatch expressions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WatchExpressionStepMonitor
participant WatchExpressionRegistry
participant UnityWatchEditorStateProvider
participant IWatchExpressionEvaluator
WatchExpressionStepMonitor->>WatchExpressionRegistry: EvaluateIfFrameChanged()
WatchExpressionRegistry->>UnityWatchEditorStateProvider: read paused state and frame count
WatchExpressionRegistry->>IWatchExpressionEvaluator: Evaluate()
IWatchExpressionEvaluator-->>WatchExpressionRegistry: WatchEvaluationResult
WatchExpressionRegistry->>WatchExpressionRegistry: record bounded history
🚥 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: 2
🧹 Nitpick comments (2)
Packages/src/Editor/FirstPartyTools/Watch/CompiledWatchExpressionEvaluator.cs (2)
14-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
Debug.Assertprecondition checks to the constructor.As an internal class, null
instanceorevaluateMethodare programmer errors.Debug.Assertwould surface these early during development rather than as a laterNullReferenceExceptioninsideEvaluate(). Based on learnings, internal Editor code should useDebug.Assertfor preconditions as part of Contract Programming and Fail Fast.🔧 Suggested addition
using System; using System.Reflection; +using UnityEngine; namespace io.github.hatayama.UnityCliLoop.FirstPartyTools { internal sealed class CompiledWatchExpressionEvaluator : IWatchExpressionEvaluator { private readonly object _instance; private readonly MethodInfo _evaluateMethod; public CompiledWatchExpressionEvaluator(object instance, MethodInfo evaluateMethod) { + Debug.Assert(instance != null, "instance must not be null"); + Debug.Assert(evaluateMethod != null, "evaluateMethod must not be null"); _instance = instance; _evaluateMethod = evaluateMethod; }🤖 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/Watch/CompiledWatchExpressionEvaluator.cs` around lines 14 - 18, Add Debug.Assert precondition checks at the start of CompiledWatchExpressionEvaluator’s constructor to validate that instance and evaluateMethod are non-null before assigning them to _instance and _evaluateMethod.Source: Learnings
20-39: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider narrowing the catch to
TargetInvocationException.
catch (Exception)converts infrastructure exceptions fromMethodInfo.Invoke(e.g.,MethodAccessException,TargetException) into user-code failure results, masking genuine bugs. The expected exception isTargetInvocationExceptionwrapping user code. Based on learnings, avoid broad defensive try-catch for unexpected exceptions; only catch expected, domain-specific exceptions and let genuine bugs surface.The practical risk is low since the compiler validates the method before invocation, so marking as optional.
🔧 Suggested narrowing
public WatchEvaluationResult Evaluate() { - // Why: only the generated user-code Invoke may throw during normal watch evaluation; - // converting that exception keeps the Editor update loop alive and preserves the error in history. - try - { - object value = _evaluateMethod.Invoke(_instance, null); - return WatchEvaluationResult.SuccessResult(value); - } - catch (Exception exception) - { - Exception userException = exception is TargetInvocationException - && exception.InnerException != null - ? exception.InnerException - : exception; - return WatchEvaluationResult.FailureResult( - userException.GetType().FullName ?? userException.GetType().Name, - userException.Message); - } + // Why: TargetInvocationException wraps user-code exceptions from the generated Evaluate method; + // converting it keeps the Editor update loop alive and preserves the error in history. + // Infrastructure exceptions from Invoke are not caught — they surface as bugs. + try + { + object value = _evaluateMethod.Invoke(_instance, null); + return WatchEvaluationResult.SuccessResult(value); + } + catch (TargetInvocationException tie) when (tie.InnerException != null) + { + Exception userException = tie.InnerException; + return WatchEvaluationResult.FailureResult( + userException.GetType().FullName ?? userException.GetType().Name, + userException.Message); + } }🤖 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/Watch/CompiledWatchExpressionEvaluator.cs` around lines 20 - 39, In CompiledWatchExpressionEvaluator.Evaluate, narrow the catch from Exception to TargetInvocationException so only user-code invocation failures are converted into WatchEvaluationResult.FailureResult. Preserve unwrapping the InnerException when present, and allow infrastructure exceptions from MethodInfo.Invoke to propagate.Source: Learnings
🤖 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 `@Assets/Tests/Editor/WatchExpressionRegistryTests.cs`:
- Around line 55-69: Correct the test
`EvaluateIfFrameChanged_WhenNotPaused_DoesNotEvaluate` to leave
`stateProvider.IsPaused` false before calling `EvaluateIfFrameChanged`, then
assert that no history entries or evaluator calls occur. Preserve the existing
paused evaluation behavior in a separate test if needed, rather than validating
it under the not-paused test name.
In `@Packages/src/Editor/FirstPartyTools/Watch/WatchExpressionStepMonitor.cs`:
- Around line 13-16: Add an ArgumentNullException guard for the registry
parameter in WatchExpressionStepMonitor’s constructor before assigning it to
_registry, matching WatchExpressionRegistry’s stateProvider validation pattern
and preserving fail-fast behavior.
---
Nitpick comments:
In
`@Packages/src/Editor/FirstPartyTools/Watch/CompiledWatchExpressionEvaluator.cs`:
- Around line 14-18: Add Debug.Assert precondition checks at the start of
CompiledWatchExpressionEvaluator’s constructor to validate that instance and
evaluateMethod are non-null before assigning them to _instance and
_evaluateMethod.
- Around line 20-39: In CompiledWatchExpressionEvaluator.Evaluate, narrow the
catch from Exception to TargetInvocationException so only user-code invocation
failures are converted into WatchEvaluationResult.FailureResult. Preserve
unwrapping the InnerException when present, and allow infrastructure exceptions
from MethodInfo.Invoke to propagate.
🪄 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: 3f260fcf-e1e8-4676-900e-d21af3902520
⛔ Files ignored due to path filters (17)
Assets/Tests/Editor/WatchExpressionCompilerTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/WatchExpressionEvaluationTests.cs.metais excluded by none and included by noneAssets/Tests/Editor/WatchExpressionRegistryTests.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/AssemblyInfo.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/CompiledWatchExpressionEvaluator.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/IWatchEditorStateProvider.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/IWatchExpressionEvaluator.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/UnityWatchEditorStateProvider.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchCompilationResult.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchEvaluationResult.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionCompiler.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionEntry.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionHistoryEntry.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionRegistry.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchExpressionStepMonitor.cs.metais excluded by none and included by nonePackages/src/Editor/FirstPartyTools/Watch/WatchRegistrationResult.cs.metais excluded by none and included by none
📒 Files selected for processing (16)
Assets/Tests/Editor/WatchExpressionCompilerTests.csAssets/Tests/Editor/WatchExpressionEvaluationTests.csAssets/Tests/Editor/WatchExpressionRegistryTests.csPackages/src/Editor/FirstPartyTools/AssemblyInfo.csPackages/src/Editor/FirstPartyTools/Watch/CompiledWatchExpressionEvaluator.csPackages/src/Editor/FirstPartyTools/Watch/IWatchEditorStateProvider.csPackages/src/Editor/FirstPartyTools/Watch/IWatchExpressionEvaluator.csPackages/src/Editor/FirstPartyTools/Watch/UnityWatchEditorStateProvider.csPackages/src/Editor/FirstPartyTools/Watch/WatchCompilationResult.csPackages/src/Editor/FirstPartyTools/Watch/WatchEvaluationResult.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionCompiler.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionEntry.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionHistoryEntry.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionRegistry.csPackages/src/Editor/FirstPartyTools/Watch/WatchExpressionStepMonitor.csPackages/src/Editor/FirstPartyTools/Watch/WatchRegistrationResult.cs
Comment on lines
+13
to
+16
| public WatchExpressionStepMonitor(WatchExpressionRegistry registry) | ||
| { | ||
| _registry = registry; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add null check for registry in the constructor.
If registry is null, OnEditorUpdate throws NullReferenceException on every EditorApplication.update tick. WatchExpressionRegistry validates its stateProvider parameter with ArgumentNullException — this constructor should follow the same pattern for consistency and fail-fast behavior.
🛡️ Proposed fix
public WatchExpressionStepMonitor(WatchExpressionRegistry registry)
{
- _registry = registry;
+ _registry = registry ?? throw new ArgumentNullException(nameof(registry));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public WatchExpressionStepMonitor(WatchExpressionRegistry registry) | |
| { | |
| _registry = registry; | |
| } | |
| public WatchExpressionStepMonitor(WatchExpressionRegistry registry) | |
| { | |
| _registry = registry ?? throw new ArgumentNullException(nameof(registry)); | |
| } |
🤖 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/Watch/WatchExpressionStepMonitor.cs`
around lines 13 - 16, Add an ArgumentNullException guard for the registry
parameter in WatchExpressionStepMonitor’s constructor before assigning it to
_registry, matching WatchExpressionRegistry’s stateProvider validation pattern
and preserving fail-fast behavior.
Keep per-watch frame progress independent so registering a new expression cannot skip existing watches. Bound compile-only test waits, document every test behavior, add direct clear coverage, and cache compiled evaluator delegates to keep reflection out of the per-frame path.
Restore the evaluator field indentation so the test follows the surrounding formatting without changing behavior.
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\n\n- add an in-memory WatchExpressionRegistry with ordered registration, immediate baseline evaluation, bounded history, and clear operations\n- detect paused Play Mode frame changes through an EditorApplication.update monitor with same-frame suppression and re-entry protection\n- reuse the execute-dynamic-code compiler to wrap expressions and convert only generated evaluator invocation failures into typed Result DTOs\n\n## Validation\n\n- Unity compile: 0 errors, 0 warnings\n- targeted EditMode tests: 9/9 passed\n- compile-only watch expression test covers the shared dynamic compiler without executing user code\n- evaluation failure test confirms exception type and message are retained as an explicit failure result