[WIP] Проброс bsl-процесса при native-компиляции.#1674
Open
[WIP] Проброс bsl-процесса при native-компиляции.#1674
Conversation
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 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 and test helper methods were added. ChangesProcess injection detection and call preparation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
Contributor
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
Contributor
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
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Tests/OneScript.Core.Tests/HelperClasses/TestContextClass.cs (1)
66-101: ⚡ Quick winNew
[ContextMethod]entries are missing English aliases.All pre-existing context methods in this file supply both a Russian and an English name (e.g.
[ContextMethod("УстаревшийМетод", "ObsoleteMethod")],[ContextMethod("ХорошийМетод", "GoodMethod")]). The twelve new methods only supply the Russian name. While this is a test helper and may not affect current tests, it creates an inconsistency and could silently prevent any test that tries to call the method by its English alias from locating the member at runtime.✏️ Example fix (apply the same pattern to all twelve new methods)
- [ContextMethod("Процедура0")] - public void Procedure0() { } + [ContextMethod("Процедура0", "Procedure0")] + public void Procedure0() { } - [ContextMethod("Процедура0СПроцессом")] - public void Procedure0WithProcess(IBslProcess process) { } + [ContextMethod("Процедура0СПроцессом", "Procedure0WithProcess")] + public void Procedure0WithProcess(IBslProcess process) { } - [ContextMethod("Процедура1")] - public void Procedure1(int intValue) { } + [ContextMethod("Процедура1", "Procedure1")] + public void Procedure1(int intValue) { } // … and so on for the remaining nine methods🤖 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/HelperClasses/TestContextClass.cs` around lines 66 - 101, The new ContextMethod attributes only have Russian names; add English aliases to each so tests can find them by English names. Update each attribute on the methods Procedure0, Procedure0WithProcess, Procedure1, Procedure1WithProcess, Procedure1WithDefault, Procedure1WithDefaultWithProcess, Function0, Function0WithProcess, Function1, Function1WithProcess, Function1WithDefault, and Function1WithDefaultWithProcess to include the corresponding English alias (e.g. change ContextMethod("Процедура0") → ContextMethod("Процедура0", "Procedure0"), apply the same pattern for Procedure0СПроцессом→Procedure0WithProcess, Процедура1→Procedure1, Процедура1СПроцессом→Procedure1WithProcess, Процедура1СУмолчанием→Procedure1WithDefault, Процедура1СУмолчаниемСПроцессом→Procedure1WithDefaultWithProcess, Функция0→Function0, Функция0СПроцессом→Function0WithProcess, Функция1→Function1, Функция1СПроцессом→Function1WithProcess, Функция1СУмолчанием→Function1WithDefault, Функция1СУмолчаниемСПроцессом→Function1WithDefaultWithProcess).
🤖 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 `@src/Tests/OneScript.Core.Tests/HelperClasses/TestContextClass.cs`:
- Around line 66-101: The new ContextMethod attributes only have Russian names;
add English aliases to each so tests can find them by English names. Update each
attribute on the methods Procedure0, Procedure0WithProcess, Procedure1,
Procedure1WithProcess, Procedure1WithDefault, Procedure1WithDefaultWithProcess,
Function0, Function0WithProcess, Function1, Function1WithProcess,
Function1WithDefault, and Function1WithDefaultWithProcess to include the
corresponding English alias (e.g. change ContextMethod("Процедура0") →
ContextMethod("Процедура0", "Procedure0"), apply the same pattern for
Procedure0СПроцессом→Procedure0WithProcess, Процедура1→Procedure1,
Процедура1СПроцессом→Procedure1WithProcess,
Процедура1СУмолчанием→Procedure1WithDefault,
Процедура1СУмолчаниемСПроцессом→Procedure1WithDefaultWithProcess,
Функция0→Function0, Функция0СПроцессом→Function0WithProcess, Функция1→Function1,
Функция1СПроцессом→Function1WithProcess,
Функция1СУмолчанием→Function1WithDefault,
Функция1СУмолчаниемСПроцессом→Function1WithDefaultWithProcess).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b7cfaed4-b0ec-4022-8723-4e1bae314570
📒 Files selected for processing (2)
src/Tests/OneScript.Core.Tests/HelperClasses/TestContextClass.cssrc/Tests/OneScript.Core.Tests/NativeCompilerTest.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Tests/OneScript.Core.Tests/NativeCompilerTest.cs
|
0 New Issues
0 Fixed Issues
0 Accepted Issues
No data about coverage (0.00% Estimated after merge)
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 by CodeRabbit