[WIP] native: для загрузки библиотек и запуска тестов#1694
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoves NativeClassInstanceWrapper indirection by using IAttachableContext directly for instance targets, centralizes object member-call generation in the compiler, adds passUndef call-argument behavior, and generalizes exception-info APIs to return BslValue instead of BslObjectValue. ChangesWrapper Removal and Context Refactoring
Sequence Diagram(s)sequenceDiagram
participant Caller
participant MethodCompiler as MethodCompiler (runtime path)
participant DynamicOperations as DynamicOperations.GetExceptionInfo
participant Factory as IExceptionInfoFactory.GetExceptionInfo
Caller->>MethodCompiler: runtime exception occurs
MethodCompiler->>DynamicOperations: GetExceptionInfo(factory, exception)
DynamicOperations->>Factory: GetExceptionInfo(Exception)
Factory-->>DynamicOperations: BslValue (exception info)
DynamicOperations-->>MethodCompiler: BslValue
MethodCompiler-->>Caller: exception description / handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRsPoem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
Для запуска надо в oscript.cfg указать |
|
к 10b97bc : компилятор сейчас не может разделить |
Mr-Rm
changed the title
![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 (2)
src/OneScript.Native/Compiler/MethodCompiler.cs (2)
1477-1491:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle trailing omitted arguments when
passUndefis enabled.
passUndefonly covers empty placeholders that already exist inargList. A module-scope call with fewer actual arguments still falls into this tail loop and raises “Недостаточно фактических параметров” instead of filling the remaining parameters withНеопределено, so the new behavior only works for explicit holes, not for omitted trailing args.Suggested fix
if (parametersToProcess > 0) { foreach (var declaredParam in declaredParameters.Skip(parameters.Length + declStart)) { if (declaredParam.HasDefaultValue) { factArguments.Add(Expression.Constant(declaredParam.DefaultValue, declaredParam.ParameterType)); } + else if (passUndef) + { + factArguments.Add(Expression.Constant(BslUndefinedValue.Instance, declaredParam.ParameterType)); + } else { var errText = new BilingualString("Недостаточно фактических параметров", "Not enough actual parameters"); AddError(errText, argList.Location);🤖 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 1477 - 1491, The tail loop currently treats missing trailing actuals as errors (AddError) by checking declaredParam.HasDefaultValue; update the loop in MethodCompiler (the block using parametersToProcess, declaredParameters, parameters, declStart) to respect the passUndef flag: if passUndef is true and a declaredParam does not have a default, append the engine's Undefined value to factArguments instead of calling AddError, otherwise keep the existing behaviour (use default when HasDefaultValue or call AddError). Ensure the Undefined sentinel you add matches the runtime representation used elsewhere (the same value/type used for explicit holes) and preserve argList.Location when producing errors.
1166-1180:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix off-by-one in
CSharpArgumentInfocount for theInvokeMemberdynamic-call fallback.
Expression.Dynamic(..., args)includes the receiver as the first element ofargs, butcsharpArgsis seeded with an extraCSharpArgumentInfoand then adds one more perargselement, so the binder receives one descriptor too many;Binder.InvokeMemberrequires theCSharpArgumentInfo[]count to exactly match the number of argument expressions passed toExpression.Dynamic(including the receiver). This mismatch can cause the fallback path to fail at runtime.Suggested fix
- var csharpArgs = new List<CSharpArgumentInfo> - { - CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, default) - }; - csharpArgs.AddRange(args.Select(x => CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, default))); + var csharpArgs = args + .Select(_ => CSharpArgumentInfo.Create(CSharpArgumentInfoFlags.None, default)) + .ToList();🤖 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 1166 - 1180, The binder argument count is off-by-one: currently csharpArgs is initialized with an extra CSharpArgumentInfo then adds one per element of args, so its length is one greater than args and breaks Binder.InvokeMember; fix MethodCompiler by making csharpArgs match args exactly (since args already includes the receiver) — e.g. build csharpArgs from args.Select(...).ToList() (or seed one and add args.Skip(1) if you prefer), then pass that array to Microsoft.CSharp.RuntimeBinder.Binder.InvokeMember and use Expression.Dynamic(binder, typeof(object), args) as before.
🤖 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 1477-1491: The tail loop currently treats missing trailing actuals
as errors (AddError) by checking declaredParam.HasDefaultValue; update the loop
in MethodCompiler (the block using parametersToProcess, declaredParameters,
parameters, declStart) to respect the passUndef flag: if passUndef is true and a
declaredParam does not have a default, append the engine's Undefined value to
factArguments instead of calling AddError, otherwise keep the existing behaviour
(use default when HasDefaultValue or call AddError). Ensure the Undefined
sentinel you add matches the runtime representation used elsewhere (the same
value/type used for explicit holes) and preserve argList.Location when producing
errors.
- Around line 1166-1180: The binder argument count is off-by-one: currently
csharpArgs is initialized with an extra CSharpArgumentInfo then adds one per
element of args, so its length is one greater than args and breaks
Binder.InvokeMember; fix MethodCompiler by making csharpArgs match args exactly
(since args already includes the receiver) — e.g. build csharpArgs from
args.Select(...).ToList() (or seed one and add args.Skip(1) if you prefer), then
pass that array to Microsoft.CSharp.RuntimeBinder.Binder.InvokeMember and use
Expression.Dynamic(binder, typeof(object), args) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93a47660-f7ba-4cf9-89d0-15d3add376d5
📒 Files selected for processing (1)
src/OneScript.Native/Compiler/MethodCompiler.cs
Недостающие параметры, в отличие от пропущенных, не заполняются Неопределено, а вызывают ошибку. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings used |
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 (2)
src/OneScript.Native/Compiler/MethodCompiler.cs (2)
1134-1145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCast direct object-method
IValuereturns toBslValue.This branch now returns
Expression.Call(...)as-is. If the CLR method is declared to returnIValue, downstream code in this compiler still expectsBslValuein several places;CreateMethodCall(...)already handles that case. Leaving it uncast can break expression-tree generation when the result flows into aBslValue-typed return or assignment path.Proposed fix
- var args = PrepareCallArguments(call.ArgumentList, methodInfo.GetParameters(), InjectedProcessNeeded(methodInfo)); - return Expression.Call(target, methodInfo, args); + var args = PrepareCallArguments(call.ArgumentList, methodInfo.GetParameters(), InjectedProcessNeeded(methodInfo)); + var callExpr = Expression.Call(target, methodInfo, args); + return callExpr.Type == typeof(IValue) + ? Expression.TypeAs(callExpr, typeof(BslValue)) + : callExpr;🤖 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 1134 - 1145, The branch that handles calling CLR methods found via TryFindMethodOfType returns Expression.Call(...) directly but doesn't cast methods whose ReturnType is IValue to BslValue, which downstream compiler code expects; update the branch in MethodCompiler.cs (where TryFindMethodOfType, PrepareCallArguments are used) to convert the call result to BslValue: after building the args, if methodInfo.ReturnType == typeof(IValue) wrap/replace the raw Expression.Call with the same conversion path used by CreateMethodCall (i.e. use CreateMethodCall or otherwise cast the call result to BslValue) so callers get a BslValue-typed expression.
1457-1464:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConvert omitted arguments through the normal parameter pipeline.
Line 1463 materializes
UndefinedasExpression.Constant(BslUndefinedValue.Instance, declaredParam.ParameterType). That only works whendeclaredParam.ParameterTypeis assignable fromBslUndefinedValue; for concrete types likestringit will fail while building the tree, and it also skips the existing conversion semantics for omitted script arguments.Proposed fix
- else if (passUndef) - { - factArguments.Add(Expression.Constant(BslUndefinedValue.Instance, declaredParam.ParameterType)); - } + else if (passUndef) + { + var undef = Expression.Constant(BslUndefinedValue.Instance, typeof(BslValue)); + factArguments.Add(PassSingleParameter(undef, declaredParam.ParameterType, argList.Location)); + }Based on learnings: omitted slots are filled with
Undefined, missing trailing parameters must still error, and concrete script-call parameters such asstringneed the normal conversion path rather than a raw typedUndefinedconstant.🤖 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 1457 - 1464, The code currently emits Expression.Constant(BslUndefinedValue.Instance, declaredParam.ParameterType) when passUndef is true which fails for concrete parameter types and bypasses the normal conversion semantics; instead, create an expression that represents the Undefined value (BslUndefinedValue.Instance) and pass it through the existing parameter conversion pipeline used for actual call arguments (the same conversion helper used elsewhere in MethodCompiler for declaredParam/parameter conversion) before adding to factArguments so concrete types (e.g., string) are produced correctly and existing conversion/error behavior for missing trailing parameters is preserved.
🤖 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 1134-1145: The branch that handles calling CLR methods found via
TryFindMethodOfType returns Expression.Call(...) directly but doesn't cast
methods whose ReturnType is IValue to BslValue, which downstream compiler code
expects; update the branch in MethodCompiler.cs (where TryFindMethodOfType,
PrepareCallArguments are used) to convert the call result to BslValue: after
building the args, if methodInfo.ReturnType == typeof(IValue) wrap/replace the
raw Expression.Call with the same conversion path used by CreateMethodCall (i.e.
use CreateMethodCall or otherwise cast the call result to BslValue) so callers
get a BslValue-typed expression.
- Around line 1457-1464: The code currently emits
Expression.Constant(BslUndefinedValue.Instance, declaredParam.ParameterType)
when passUndef is true which fails for concrete parameter types and bypasses the
normal conversion semantics; instead, create an expression that represents the
Undefined value (BslUndefinedValue.Instance) and pass it through the existing
parameter conversion pipeline used for actual call arguments (the same
conversion helper used elsewhere in MethodCompiler for declaredParam/parameter
conversion) before adding to factArguments so concrete types (e.g., string) are
produced correctly and existing conversion/error behavior for missing trailing
parameters is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4421347-3d2c-4580-b933-46cfab0d0d75
📒 Files selected for processing (1)
src/OneScript.Native/Compiler/MethodCompiler.cs
2 participants
Исправлены ошибки в native, не позволявшие скомпилировать и запустить test/testrunner.os
Summary by CodeRabbit