[WIP] Делегаты в native-режиме#1697
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds explicit argument-count validation in delegate invocation paths, a public Execute context method on DelegateAction, integration tests for stack/native delegate parameter handling, and a small test expectation adjustment for XML doc conversion. ChangesDelegate Argument Validation and Execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/cross-delegate-stack.os (1)
1-100: 💤 Low valueConsider extracting shared test logic to reduce duplication.
This file is nearly identical to
cross-delegate-native.os(only differing in the#stackdirective at line 1). While test isolation may justify the duplication, consider whether the shared logic (lines 10-99) could be extracted to a common module to improve maintainability.If the
#stackdirective must apply to the entire test module for correct behavior, the current duplication is acceptable.🤖 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 `@tests/cross-delegate-stack.os` around lines 1 - 100, The test file duplicates almost all logic from the native variant; extract the shared test functions (ПолучитьСписокТестов, ТестДолжен_ВыполнитьДействиеОбъектаТеста, ТестДолжен_ПроверитьКоличествоПараметров and ТестовыйДелегат) into a single reusable module (e.g., ОбщиеТесты) that both cross-delegate-stack.os and cross-delegate-native.os can include or call, leaving only the module-specific directives and thin wrappers (the exported wrapper procedures like ТестДолжен_ВыполнитьДействиеОбъектаТеста_Стэковый / _Родной and ТестДолжен_ПроверитьКоличествоПараметров_Стэковый / _Родной) in each file; if the `#stack` directive must apply to the whole file, keep that directive in cross-delegate-stack.os and only move function bodies into the shared module while retaining the wrapper exports in the original file so behavior and test isolation remain unchanged.
🤖 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 `@src/OneScript.Native/Runtime/CallableMethod.cs`:
- Around line 46-54: The check in CallableMethod.Invoke (or wherever
CallableMethod.cs validates arguments) incorrectly throws
RuntimeException.TooFewArgumentsPassed when args.Length <
_method.GetBslParameters().Length without considering parameters that have
defaults; update the logic that compares _method.GetBslParameters() to args to
iterate the trailing/missing parameters and allow omitted ones that have
paramDef.HasDefaultValue (same approach as MachineInstance.SetExecutionFrame
uses) instead of immediately throwing, so only throw TooFewArgumentsPassed when
a missing parameter lacks HasDefaultValue.
---
Nitpick comments:
In `@tests/cross-delegate-stack.os`:
- Around line 1-100: The test file duplicates almost all logic from the native
variant; extract the shared test functions (ПолучитьСписокТестов,
ТестДолжен_ВыполнитьДействиеОбъектаТеста,
ТестДолжен_ПроверитьКоличествоПараметров and ТестовыйДелегат) into a single
reusable module (e.g., ОбщиеТесты) that both cross-delegate-stack.os and
cross-delegate-native.os can include or call, leaving only the module-specific
directives and thin wrappers (the exported wrapper procedures like
ТестДолжен_ВыполнитьДействиеОбъектаТеста_Стэковый / _Родной and
ТестДолжен_ПроверитьКоличествоПараметров_Стэковый / _Родной) in each file; if
the `#stack` directive must apply to the whole file, keep that directive in
cross-delegate-stack.os and only move function bodies into the shared module
while retaining the wrapper exports in the original file so behavior and test
isolation remain unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b369ce19-1084-4e00-9945-fe7fb56b030f
📒 Files selected for processing (6)
src/OneScript.Native/Runtime/CallableMethod.cssrc/OneScript.StandardLibrary/DelegateAction.cssrc/ScriptEngine/Machine/MachineInstance.cssrc/Tests/DocumenterTests/XmlDocConversionTest.cstests/cross-delegate-native.ostests/cross-delegate-stack.os
| if (_method.GetBslParameters().Length < args.Length) | ||
| { | ||
| throw RuntimeException.TooManyArgumentsPassed(); | ||
| } | ||
| if (_method.GetBslParameters().Length > args.Length) | ||
| { | ||
| // TODO: значения параметров по-умолчанию? | ||
| throw RuntimeException.TooFewArgumentsPassed(); | ||
| } |
There was a problem hiding this comment.
Missing default-parameter handling causes incorrect TooFewArgumentsPassed exception.
Lines 50-53 throw TooFewArgumentsPassed() whenever args.Length < parameters.Length, but this ignores parameters with default values. A caller passing fewer arguments than the total parameter count is valid when the omitted parameters have defaults.
MachineInstance.SetExecutionFrame (lines 175-184 in the other file) correctly checks paramDef.HasDefaultValue before throwing TooFewArgumentsPassed(). This code must do the same, otherwise delegate invocations of methods with optional parameters will fail.
🔧 Proposed fix
if (_method.GetBslParameters().Length < args.Length)
{
throw RuntimeException.TooManyArgumentsPassed();
}
-if (_method.GetBslParameters().Length > args.Length)
+
+var parameters = _method.GetBslParameters();
+for (int i = args.Length; i < parameters.Length; i++)
{
- // TODO: значения параметров по-умолчанию?
- throw RuntimeException.TooFewArgumentsPassed();
+ if (!parameters[i].HasDefaultValue)
+ {
+ throw RuntimeException.TooFewArgumentsPassed();
+ }
}📝 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.
| if (_method.GetBslParameters().Length < args.Length) | |
| { | |
| throw RuntimeException.TooManyArgumentsPassed(); | |
| } | |
| if (_method.GetBslParameters().Length > args.Length) | |
| { | |
| // TODO: значения параметров по-умолчанию? | |
| throw RuntimeException.TooFewArgumentsPassed(); | |
| } | |
| if (_method.GetBslParameters().Length < args.Length) | |
| { | |
| throw RuntimeException.TooManyArgumentsPassed(); | |
| } | |
| var parameters = _method.GetBslParameters(); | |
| for (int i = args.Length; i < parameters.Length; i++) | |
| { | |
| if (!parameters[i].HasDefaultValue) | |
| { | |
| throw RuntimeException.TooFewArgumentsPassed(); | |
| } | |
| } |
🤖 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 `@src/OneScript.Native/Runtime/CallableMethod.cs` around lines 46 - 54, The
check in CallableMethod.Invoke (or wherever CallableMethod.cs validates
arguments) incorrectly throws RuntimeException.TooFewArgumentsPassed when
args.Length < _method.GetBslParameters().Length without considering parameters
that have defaults; update the logic that compares _method.GetBslParameters() to
args to iterate the trailing/missing parameters and allow omitted ones that have
paramDef.HasDefaultValue (same approach as MachineInstance.SetExecutionFrame
uses) instead of immediately throwing, so only throw TooFewArgumentsPassed when
a missing parameter lacks HasDefaultValue.
1 participant
Summary by CodeRabbit
Bug Fixes
New Features
Tests