[WIP] Проброс bsl-процесса при native-компиляции.#1674
[WIP] Проброс bsl-процесса при native-компиляции.#1674
Conversation
📝 WalkthroughWalkthroughIntroduces a private helper InjectedProcessNeeded(MethodInfo) in MethodCompiler to decide when an IBslProcess should be injected (either by ContextMethodInfo.InjectsProcess or when the method's first parameter is IBslProcess). The helper is used when preparing call arguments for object procedure/function and general method calls; tests added. ChangesProcess injection detection and call preparation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.Native/Compiler/MethodCompiler.cs (1)
1163-1181:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
VisitObjectFunctionCalldoes not applyHasProcessParameter— argument misalignment at runtime
VisitObjectProcedureCall(lines 1112–1113) now injects the process argument wheneverHasProcessParameteris true. However,VisitObjectFunctionCall(line 1180) still uses onlyInjectsProcess, so the same native method called in a function position (i.e., as an expression) will haveinjectsProcess = false, causingPrepareCallArgumentsto map the first script argument into theIBslProcessslot and shift all remaining arguments by one — a runtime type error or silent wrong-argument binding.🐛 Proposed fix — mirror the procedure-call logic
- var args = PrepareCallArguments(call.ArgumentList, methodInfo.GetParameters(), methodInfo is ContextMethodInfo { InjectsProcess: true }); + var injectProcess = methodInfo is ContextMethodInfo { InjectsProcess: true } || HasProcessParameter(methodInfo); + var args = PrepareCallArguments(call.ArgumentList, methodInfo.GetParameters(), injectProcess);🤖 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/Compiler/MethodCompiler.cs` around lines 1163 - 1181, VisitObjectFunctionCall fails to mirror VisitObjectProcedureCall's logic for injecting the process parameter, causing argument misalignment; update VisitObjectFunctionCall (around where methodInfo is obtained) to compute the same injectsProcess flag used in PrepareCallArguments — i.e. treat a method as injecting the process when methodInfo is a ContextMethodInfo with InjectsProcess == true OR when methodInfo.HasProcessParameter() (or the equivalent HasProcessParameter member) — then pass that injectsProcess into PrepareCallArguments so function calls and procedure calls use the same argument mapping.
🧹 Nitpick comments (1)
src/OneScript.Native/Compiler/MethodCompiler.cs (1)
1095-1100: 💤 Low valueMinor: method body can be reduced to a single expression
The
if/return true/return falsepattern is unnecessarily verbose.♻️ Proposed simplification
private static bool HasProcessParameter(MethodInfo mi) { var p = mi.GetParameters(); - if (p.Length > 0 && p[0].ParameterType == typeof(IBslProcess)) return true; - return false; + return p.Length > 0 && p[0].ParameterType == typeof(IBslProcess); }🤖 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/Compiler/MethodCompiler.cs` around lines 1095 - 1100, The method HasProcessParameter currently uses an if/return true/return false pattern; simplify it to a single expression by returning the boolean check directly (inspect the first parameter of MethodInfo mi and compare its ParameterType to typeof(IBslProcess)). Replace the block inside HasProcessParameter with a single return expression (or make the method expression-bodied) using mi.GetParameters() (or the local p variable) and the check p.Length > 0 && p[0].ParameterType == typeof(IBslProcess).
🤖 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.
Outside diff comments:
In `@src/OneScript.Native/Compiler/MethodCompiler.cs`:
- Around line 1163-1181: VisitObjectFunctionCall fails to mirror
VisitObjectProcedureCall's logic for injecting the process parameter, causing
argument misalignment; update VisitObjectFunctionCall (around where methodInfo
is obtained) to compute the same injectsProcess flag used in
PrepareCallArguments — i.e. treat a method as injecting the process when
methodInfo is a ContextMethodInfo with InjectsProcess == true OR when
methodInfo.HasProcessParameter() (or the equivalent HasProcessParameter member)
— then pass that injectsProcess into PrepareCallArguments so function calls and
procedure calls use the same argument mapping.
---
Nitpick comments:
In `@src/OneScript.Native/Compiler/MethodCompiler.cs`:
- Around line 1095-1100: The method HasProcessParameter currently uses an
if/return true/return false pattern; simplify it to a single expression by
returning the boolean check directly (inspect the first parameter of MethodInfo
mi and compare its ParameterType to typeof(IBslProcess)). Replace the block
inside HasProcessParameter with a single return expression (or make the method
expression-bodied) using mi.GetParameters() (or the local p variable) and the
check p.Length > 0 && p[0].ParameterType == typeof(IBslProcess).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ca6af39-d02f-449a-b33c-ce0d03beb3fd
📒 Files selected for processing (1)
src/OneScript.Native/Compiler/MethodCompiler.cs
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
No data about coverage (0.00% Estimated after merge)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Tests/OneScript.Core.Tests/NativeCompilerTest.cs (1)
871-906: ⚡ Quick winAdd observable assertions to these new process-injection tests.
Right now both tests only prove “does not throw”. In particular, Line 899 assigns the function result but never checks it, so the test would still pass if
ВызватьМетодreturned the wrong value. Please assert a real outcome here: e.g. seedМассив, verify the returnedКоличество, and make the procedure test sort a non-trivialValueListImpland check the order afterwards.🤖 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/Tests/OneScript.Core.Tests/NativeCompilerTest.cs` around lines 871 - 906, Update the two tests to include observable assertions: in Can_Call_Member_FunctionsWithBslProcess seed the ArrayImpl with a known number of elements, call the generated delegate (which executes block.CodeBlock calling Рефлектор.ВызватьМетод) and Assert that the returned BslValue represents the expected count; in Can_Call_Member_ProceduresWithBslProcess populate a ValueListImpl with a non-trivial sequence, call the delegate (which runs Список.СортироватьПоЗначению), then Assert the list is sorted as expected by inspecting ValueListImpl contents. Use the existing method delegates, ReflectorContext.CreateNew, ValueListImpl and ArrayImpl instances and convert the returned BslValue to a concrete value for assertions.
🤖 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/Compiler/MethodCompiler.cs`:
- Around line 1095-1107: PrepareCallArguments currently miscomputes parameter
indexes for methods where InjectedProcessNeeded(MethodInfo) is true (explicit
IBslProcess first parameter), causing declaredParameters[decl] to be accessed
incorrectly; modify PrepareCallArguments to detect injected parameters via
InjectedProcessNeeded(methodInfo) and exclude that injected parameter from the
"remaining count" and any indexing math: treat the declared parameter list as
starting at index 1 when injected (or equivalently subtract 1 from declared
parameter count and shift declared index access by +1 when reading
declaredParameters[decl]) so arity checks and error messages count only
script-visible parameters and attempts to read declaredParameters use the
adjusted index (references: PrepareCallArguments, InjectedProcessNeeded,
declaredParameters, decl, IBslProcess).
---
Nitpick comments:
In `@src/Tests/OneScript.Core.Tests/NativeCompilerTest.cs`:
- Around line 871-906: Update the two tests to include observable assertions: in
Can_Call_Member_FunctionsWithBslProcess seed the ArrayImpl with a known number
of elements, call the generated delegate (which executes block.CodeBlock calling
Рефлектор.ВызватьМетод) and Assert that the returned BslValue represents the
expected count; in Can_Call_Member_ProceduresWithBslProcess populate a
ValueListImpl with a non-trivial sequence, call the delegate (which runs
Список.СортироватьПоЗначению), then Assert the list is sorted as expected by
inspecting ValueListImpl contents. Use the existing method delegates,
ReflectorContext.CreateNew, ValueListImpl and ArrayImpl instances and convert
the returned BslValue to a concrete value for assertions.
🪄 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: c3e500f3-1fb7-4742-9d1e-979228b0de00
📒 Files selected for processing (2)
src/OneScript.Native/Compiler/MethodCompiler.cssrc/Tests/OneScript.Core.Tests/NativeCompilerTest.cs
| private static bool InjectedProcessNeeded(MethodInfo methodInfo) | ||
| { | ||
| if (methodInfo is ContextMethodInfo { InjectsProcess: true }) | ||
| { | ||
| return true; | ||
| } | ||
| var p = methodInfo.GetParameters(); | ||
| if (p.Length > 0 && p[0].ParameterType == typeof(IBslProcess)) | ||
| { | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Handle process-only methods in the arity logic.
Once this helper starts treating explicit IBslProcess-first methods as injected, calls to a method whose reflected signature is only (IBslProcess process) can hit declaredParameters[1] inside PrepareCallArguments when the script passes any extra argument. That turns a normal “too many actual parameters” error into an exception/generic compile failure.
Please update PrepareCallArguments so injected parameters are excluded from the remaining-count/indexing math before reading declaredParameters[decl]. A case like Список.СортироватьПоЗначению(1) should be reported as an arity error, not crash the compiler.
🤖 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/Compiler/MethodCompiler.cs` around lines 1095 - 1107,
PrepareCallArguments currently miscomputes parameter indexes for methods where
InjectedProcessNeeded(MethodInfo) is true (explicit IBslProcess first
parameter), causing declaredParameters[decl] to be accessed incorrectly; modify
PrepareCallArguments to detect injected parameters via
InjectedProcessNeeded(methodInfo) and exclude that injected parameter from the
"remaining count" and any indexing math: treat the declared parameter list as
starting at index 1 when injected (or equivalently subtract 1 from declared
parameter count and shift declared index access by +1 when reading
declaredParameters[decl]) so arity checks and error messages count only
script-visible parameters and attempts to read declaredParameters use the
adjusted index (references: PrepareCallArguments, InjectedProcessNeeded,
declaredParameters, decl, IBslProcess).
1 participant
Summary by CodeRabbit