Конвертеры CLR-BSL #1566
Конвертеры CLR-BSL #1566
Conversation
…ер и возможность установки конвертера clr-bsl значений для свойств и возвращаемых значений методов
WalkthroughAdds a pluggable CLR↔script conversion mechanism: new generic converter interface, attribute-level Converter properties with validation, metadata exposure and instantiation helpers, mapper/marshaller updates to use converters, a NotBslValueWrapper to expose CLR objects to script, and small runtime/debug tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as ScriptEngine/Wrapper
participant Meta as ContextMethodInfo/ContextPropertyInfo
participant Conv as IContextValueConverter
participant Marshaller as ContextValuesMarshaller
participant CLR as CLR value
participant IVal as IValue
Caller->>Meta: Read ConverterType
alt ConverterType exists
Caller->>Meta: TryGetConverter(out conv)
Meta-->>Caller: conv instance
Caller->>Conv: ToIValue(CLR)
Conv-->>Caller: IVal
else no converter
Caller->>Marshaller: ConvertReturnValue(CLR)
Marshaller-->>Caller: IVal
end
sequenceDiagram
participant Script
participant Wrapper as NotBslValueWrapper
participant Under as Underlying CLR object
participant Meta as ContextMethodInfo
participant Conv as IContextValueConverter
participant Marshaller as ContextValuesMarshaller
participant IVal as IValue
Script->>Wrapper: CallAsFunction(methodNum, args)
Wrapper->>Meta: resolve MethodInfo, ConverterType
alt ConverterType exists
Wrapper->>Meta: TryGetConverter(out conv)
Wrapper->>Conv: conv.ToClr(argIValue) for each arg
else
Wrapper->>Marshaller: ConvertParams(args) -> CLR args
end
Wrapper->>Under: invoke method with CLR args
Under-->>Wrapper: CLR result
alt ConverterType exists
Wrapper->>Conv: conv.ToIValue(result)
Conv-->>Wrapper: IVal
else
Wrapper->>Marshaller: ConvertReturnValue(result)
Marshaller-->>Wrapper: IVal
end
Wrapper-->>Script: ret IVal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
src/OneScript.Core/Contexts/ContextValueConverter.cs (1)
6-11: Define and document nullability and conversion invariantsPlease clarify whether null inputs/outputs are allowed for both directions and enforce a consistent contract. Consider enabling nullable annotations here to make the expectations explicit.
Example update:
+// Consider enabling nullable annotations to make the contract explicit +#nullable enable using System; using ScriptEngine.Machine; namespace OneScript.Contexts { public abstract class ContextValueConverter<TClr> { - public abstract IValue ToIValue(TClr obj); + // Decide and document: Should null TClr produce BSL Null/Undefined? + public abstract IValue ToIValue(TClr obj); - public abstract TClr ToClr(IValue obj); + // Decide and document: Should BSL Null/Undefined produce null/default TClr or throw? + public abstract TClr ToClr(IValue obj); } }src/OneScript.Core/Contexts/ContextPropertyAttribute.cs (1)
44-47: Make Converter nullable and trimming-friendly (symmetry with ContextMethodAttribute)Align with
ContextMethodAttributeby making theConverternullable and add a trimming hint for activator-based construction.@@ using System; using OneScript.Commons; +using System.Diagnostics.CodeAnalysis; @@ - /// Конвертер значения свойства + /// Конвертер значения свойства /// </summary> - public Type Converter { get; set; } + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] + public Type? Converter { get; set; }src/OneScript.Core/Contexts/ContextMethodAttribute.cs (2)
33-36: Fix nullable annotations in constructors under #nullable enableParameters that are allowed to be null should be annotated accordingly to avoid NRT warnings and better convey intent.
- public ContextMethodAttribute(string name, string _ = null, - [CallerMemberName] string nativeMethodName = null) + public ContextMethodAttribute(string name, string? _ = null, + [CallerMemberName] string? nativeMethodName = null) : this(name, nativeMethodName) { }Optionally also update the other ctor to accept nullable alias:
- public ContextMethodAttribute(string name, string alias) + public ContextMethodAttribute(string name, string? alias)Note: changing reference-type nullability does not alter the runtime signature.
51-54: Add trimming hint to Converter for reliable activation in trimmed appsIf converters are instantiated via reflection, help the linker keep the default constructor.
+using System.Diagnostics.CodeAnalysis; @@ - public Type? Converter { get; set; } + [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] + public Type? Converter { get; set; }src/OneScript.Core/Contexts/ContextMethodInfo.cs (1)
54-58: Validate converter type early to fail fast on misconfigurationOptionally validate that the converter derives from
ContextValueConverter<T>whereTmatches the method return type and that it has a public parameterless constructor.Sketch:
private void InitConverter() { if (_scriptMark.Converter == null) return; var convType = _scriptMark.Converter; if (convType.IsAbstract || convType.IsInterface) throw new ArgumentException($"Converter '{convType}' must be a concrete type."); // Check inheritance chain for ContextValueConverter<> var baseType = convType; var ok = false; while (baseType != null && baseType != typeof(object)) { if (baseType.IsGenericType && baseType.GetGenericTypeDefinition() == typeof(ContextValueConverter<>)) { var tArg = baseType.GetGenericArguments()[0]; if (tArg == _realMethod.ReturnType) ok = true; break; } baseType = baseType.BaseType; } if (!ok) throw new ArgumentException($"Converter '{convType}' must inherit ContextValueConverter<{_realMethod.ReturnType}>."); ConverterType = convType; }src/ScriptEngine/Machine/Contexts/ContextMethodMapper.cs (1)
173-182: Return value conversion uses converter when specified — consider avoiding per-call allocationsThe logic is correct. Minor perf nit: Expression.New(target.ConverterType) allocates a converter instance per invocation. If converters are stateless, consider caching a single instance (e.g., on ContextMethodInfo) and using Expression.Constant(cachedInstance) to avoid allocations.
src/OneScript.Core/Contexts/ContextPropertyInfo.cs (1)
13-13: Remove unused using to avoid unintended layering dependencyThe file doesn’t use types from ScriptEngine.Machine. Dropping this using prevents accidental cross-layer coupling.
-using ScriptEngine.Machine;src/ScriptEngine/Machine/Contexts/ContextValuesMarshaller.cs (1)
298-311: Return conversion: support for ContextClassAttribute via NotBslValueWrapper — good, minor nits
- New wrapper path for non-IRuntimeContextInstance types marked with ContextClassAttribute looks correct.
- Minor nit: typeof(IValue).IsAssignableFrom(type) is redundant with the earlier switch case IValue v, but harmless.
src/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs (1)
78-89: Unnecessary return in constructor; local functions after return are fine but confusingThe return; makes subsequent code unreachable at runtime (local function declarations are still fine), but it’s unconventional and confusing. Remove return; and keep local functions; or move them above for clarity.
- return; - void CantWriteAction(TInstance inst, IValue val) { throw PropertyAccessException.PropIsNotWritableException(Name); } IValue CantReadAction(TInstance inst) { throw PropertyAccessException.PropIsNotReadableException(Name); }src/ScriptEngine/Machine/NotBslValueWrapper.cs (2)
35-48: Property/method discovery is fine; consider stable orderingUsing reflection order works but isn’t guaranteed stable across runtimes. If stability matters (e.g., persistent indices), consider ordering by Name/Alias.
1-13: Prune unused usingsTinyIoC, ScriptEngine.Types, and OneScript.Values aren’t used here.
-using OneScript.Values; -using ScriptEngine.Types; -using TinyIoC;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/OneScript.Core/Contexts/ContextMethodAttribute.cs(2 hunks)src/OneScript.Core/Contexts/ContextMethodInfo.cs(4 hunks)src/OneScript.Core/Contexts/ContextPropertyAttribute.cs(1 hunks)src/OneScript.Core/Contexts/ContextPropertyInfo.cs(3 hunks)src/OneScript.Core/Contexts/ContextValueConverter.cs(1 hunks)src/ScriptEngine/Machine/Contexts/ContextMethodMapper.cs(2 hunks)src/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs(3 hunks)src/ScriptEngine/Machine/Contexts/ContextValuesMarshaller.cs(5 hunks)src/ScriptEngine/Machine/NotBslValueWrapper.cs(1 hunks)src/ScriptEngine/Machine/TypeFactory.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-25T09:44:11.960Z
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1426
File: src/OneScript.Web.Server/HeaderDictionaryWrapper.cs:43-50
Timestamp: 2024-07-25T09:44:11.960Z
Learning: The `CanWrite` attribute in the `ContextProperty` attribute is used for different purposes and is not related to the presence of a setter in the property.
Applied to files:
src/OneScript.Core/Contexts/ContextPropertyAttribute.cssrc/OneScript.Core/Contexts/ContextMethodInfo.cssrc/OneScript.Core/Contexts/ContextMethodAttribute.cssrc/OneScript.Core/Contexts/ContextPropertyInfo.cssrc/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs
🔇 Additional comments (10)
src/OneScript.Core/Contexts/ContextValueConverter.cs (1)
2-2: Namespace Verified:IValueis inScriptEngine.Machine
Theusing ScriptEngine.Machine;directive is correct—IValueis declared in that namespace. No import changes are needed.src/ScriptEngine/Machine/Contexts/ContextMethodMapper.cs (3)
155-164: Metadata initializer refactor is correctInitializer-based construction is clear and equivalent to previous assignments. No functional change.
239-240: Args list initialization simplification LGTMInitializing with instParam directly improves readability without behavior change.
256-261: Parameter conversion call fix (process included) is correctPassing processParam to ConvertParam(..., process) fixes the missing argument and ensures conversions that may depend on process behave properly.
src/OneScript.Core/Contexts/ContextPropertyInfo.cs (1)
29-30: Converter type capture is correctStoring the converter type from attribute at construction is fine.
src/ScriptEngine/Machine/Contexts/ContextValuesMarshaller.cs (3)
37-39: Expose generic return converter MethodInfo — LGTMThis enables expression trees to call ConvertReturnValue. Looks good.
91-91: Correct generic overload dispatch for ConvertParamRouting through the (value, defaultValue, ForbiddenBslProcess.Instance) overload is correct for “no process” contexts.
104-106: Generic ConvertParam via Type-based converter is correctUsing ConvertParam(value, typeof(T), process) centralizes conversion logic and exception mapping.
src/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs (2)
19-20: Switching to ContextPropertyInfo makes senseAligns the mapper with converter-aware property metadata.
113-114: Confirm strict conversion in property setters
In ContextPropertyMapper.cs, CreateSetter now calls ConvertValue, which by default invokes ContextValuesMarshaller.ConvertValueStrict(value) instead of the previous ConvertParam logic. This change disallows implicit coercions (e.g. AsString conversions) when no custom converter is specified.– File: src/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs
– Method: CreateSetter → ConvertValue default branch (lines 113–119)Please verify that switching from the more permissive ConvertParam behavior to strict conversion was intentional.
| _scriptMark = binding; | ||
| InitConverter(); | ||
|
|
||
| InjectsProcess = _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess); | ||
| HasReturnConverter = binding.Converter != null; | ||
| } |
There was a problem hiding this comment.
Initialize consistently across constructors; compute HasReturnConverter from ConverterType
HasReturnConverteris assigned in only one constructor, leaving it false in the other. Prefer computing it fromConverterTypeto keep logic in one place.InjectsProcessis set only in the 2nd ctor; set it in the 1st as well for consistency.
@@ public ContextMethodInfo(MethodInfo realMethod)
- _scriptMark =
- (ContextMethodAttribute) GetCustomAttributes(typeof(ContextMethodAttribute), false).First();
-
- InitConverter();
+ _scriptMark =
+ (ContextMethodAttribute) GetCustomAttributes(typeof(ContextMethodAttribute), false).First();
+ InitConverter();
+ InjectsProcess = _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess);
@@ public ContextMethodInfo(MethodInfo realMethod, ContextMethodAttribute binding)
- InjectsProcess = _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess);
- HasReturnConverter = binding.Converter != null;
+ InjectsProcess = _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess);
@@
- public bool HasReturnConverter { get; }
+ public bool HasReturnConverter => ConverterType != null;Also applies to: 54-58, 120-123
🤖 Prompt for AI Agents
In src/OneScript.Core/Contexts/ContextMethodInfo.cs around lines 47 to 52,
ensure that HasReturnConverter is consistently initialized in all constructors
by computing it from ConverterType instead of directly from binding.Converter.
Also, set InjectsProcess in the first constructor as it is in the second to
maintain consistent initialization. Apply the same changes to lines 54-58 and
120-123 to unify the logic across constructors.
There was a problem hiding this comment.
@akpaevj посмотри, кажется, кролик прав
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| public Type ConverterType { get; private set; } | ||
|
|
||
| public bool TryGetConverter(out object converter) | ||
| { | ||
| if (ConverterType == null) | ||
| { | ||
| converter = null; | ||
| return false; | ||
| } | ||
|
|
||
| converter = Activator.CreateInstance(ConverterType); | ||
| return true; | ||
| } | ||
|
|
||
| public bool TryGetStrictConverter<T>(out ContextValueConverter<T> converter) | ||
| { | ||
| var result = TryGetConverter(out var c); | ||
|
|
||
| if (result) | ||
| converter = c as ContextValueConverter<T>; | ||
| else | ||
| converter = null; | ||
|
|
||
| return result; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix TryGetStrictConverter to return false when cast fails
Currently it returns true even if the created instance isn’t a ContextValueConverter, potentially leading to null deref downstream. Return false when the cast fails.
- public bool TryGetStrictConverter<T>(out ContextValueConverter<T> converter)
- {
- var result = TryGetConverter(out var c);
-
- if (result)
- converter = c as ContextValueConverter<T>;
- else
- converter = null;
-
- return result;
- }
+ public bool TryGetStrictConverter<T>(out ContextValueConverter<T> converter)
+ {
+ converter = null;
+ if (ConverterType == null)
+ return false;
+
+ var instance = Activator.CreateInstance(ConverterType);
+ if (instance is ContextValueConverter<T> typed)
+ {
+ converter = typed;
+ return true;
+ }
+
+ return false;
+ }📝 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.
| public Type ConverterType { get; private set; } | |
| public bool TryGetConverter(out object converter) | |
| { | |
| if (ConverterType == null) | |
| { | |
| converter = null; | |
| return false; | |
| } | |
| converter = Activator.CreateInstance(ConverterType); | |
| return true; | |
| } | |
| public bool TryGetStrictConverter<T>(out ContextValueConverter<T> converter) | |
| { | |
| var result = TryGetConverter(out var c); | |
| if (result) | |
| converter = c as ContextValueConverter<T>; | |
| else | |
| converter = null; | |
| return result; | |
| } | |
| public bool TryGetStrictConverter<T>(out ContextValueConverter<T> converter) | |
| { | |
| converter = null; | |
| if (ConverterType == null) | |
| return false; | |
| var instance = Activator.CreateInstance(ConverterType); | |
| if (instance is ContextValueConverter<T> typed) | |
| { | |
| converter = typed; | |
| return true; | |
| } | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.Core/Contexts/ContextPropertyInfo.cs between lines 59 and 84,
the method TryGetStrictConverter<T> returns true even if the cast to
ContextValueConverter<T> fails, which can cause null reference issues later.
Modify the method so that after attempting the cast, it returns false if the
cast result is null, ensuring it only returns true when the cast succeeds and
the converter is valid.
| public override void CallAsFunction(int methodNumber, IValue[] arguments, out IValue retValue, IBslProcess process) | ||
| { | ||
| var method = _methods[methodNumber]; | ||
|
|
||
| var args = arguments.Select(c => ContextValuesMarshaller.ConvertParam(c, method.ReturnType, process)).ToArray(); | ||
| var result = method.Invoke(UnderlyingObject, args); | ||
|
|
||
| retValue = ContextValuesMarshaller.ConvertDynamicValue(result); | ||
| } |
There was a problem hiding this comment.
Critical: argument conversion uses method.ReturnType; also ignore method return converters
- Arguments are currently converted to the method’s return type — this is incorrect.
- Return value should honor the method’s ConverterType when present.
public override void CallAsFunction(int methodNumber, IValue[] arguments, out IValue retValue, IBslProcess process)
{
var method = _methods[methodNumber];
- var args = arguments.Select(c => ContextValuesMarshaller.ConvertParam(c, method.ReturnType, process)).ToArray();
+ // Convert args generically (ContextMethodInfo.Invoke should handle defaults/process injection)
+ var args = arguments.Select(ContextValuesMarshaller.ConvertToClrObject).ToArray();
var result = method.Invoke(UnderlyingObject, args);
- retValue = ContextValuesMarshaller.ConvertDynamicValue(result);
+ if (method.ConverterType != null)
+ {
+ var converter = Activator.CreateInstance(method.ConverterType);
+ var toIValue = method.ConverterType.GetMethod("ToIValue");
+ retValue = (IValue)toIValue!.Invoke(converter, new[] { result });
+ }
+ else
+ {
+ retValue = ContextValuesMarshaller.ConvertDynamicValue(result);
+ }
}📝 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.
| public override void CallAsFunction(int methodNumber, IValue[] arguments, out IValue retValue, IBslProcess process) | |
| { | |
| var method = _methods[methodNumber]; | |
| var args = arguments.Select(c => ContextValuesMarshaller.ConvertParam(c, method.ReturnType, process)).ToArray(); | |
| var result = method.Invoke(UnderlyingObject, args); | |
| retValue = ContextValuesMarshaller.ConvertDynamicValue(result); | |
| } | |
| public override void CallAsFunction(int methodNumber, IValue[] arguments, out IValue retValue, IBslProcess process) | |
| { | |
| var method = _methods[methodNumber]; | |
| // Convert args generically (ContextMethodInfo.Invoke should handle defaults/process injection) | |
| var args = arguments.Select(ContextValuesMarshaller.ConvertToClrObject).ToArray(); | |
| var result = method.Invoke(UnderlyingObject, args); | |
| if (method.ConverterType != null) | |
| { | |
| var converter = Activator.CreateInstance(method.ConverterType); | |
| var toIValue = method.ConverterType.GetMethod("ToIValue"); | |
| retValue = (IValue)toIValue!.Invoke(converter, new[] { result }); | |
| } | |
| else | |
| { | |
| retValue = ContextValuesMarshaller.ConvertDynamicValue(result); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/NotBslValueWrapper.cs lines 119 to 127, the argument
conversion incorrectly uses method.ReturnType instead of the parameter types,
and the return value conversion does not consider the method's ConverterType.
Fix this by converting each argument to the corresponding parameter type of the
method, not the return type, and when converting the return value, use the
method's ConverterType if it is specified.
| var value = methodInfo.Invoke(null, methArgs); | ||
|
|
||
| if (value is IValue iValue) | ||
| return iValue; | ||
|
|
||
| if (value is IRuntimeContextInstance rValue) | ||
| return (IValue)rValue; | ||
|
|
||
| return ContextValuesMarshaller.ConvertDynamicValue(value); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Defensive handling for null returns and non-IValue runtime instances
Two small hardening tweaks:
- If the constructor returns null, ensure it’s mapped to a valid BSL value (e.g., Null/Undefined) instead of flowing null.
- Avoid assuming
IRuntimeContextInstanceis alwaysIValue; cast safely and fall back to conversion.
- var value = methodInfo.Invoke(null, methArgs);
+ var value = methodInfo.Invoke(null, methArgs);
- if (value is IValue iValue)
+ if (value is IValue iValue)
return iValue;
- if (value is IRuntimeContextInstance rValue)
- return (IValue)rValue;
+ if (value is IRuntimeContextInstance rValue)
+ return rValue as IValue ?? ContextValuesMarshaller.ConvertDynamicValue(rValue);
- return ContextValuesMarshaller.ConvertDynamicValue(value);
+ if (value is null)
+ return ContextValuesMarshaller.ConvertDynamicValue(null);
+
+ return ContextValuesMarshaller.ConvertDynamicValue(value);Please verify ContextValuesMarshaller.ConvertDynamicValue(null) maps to the expected BSL null/undefined value:
🏁 Script executed:
#!/bin/bash
rg -nA5 "ConvertDynamicValue\\s*\\(" | sed -n '1,200p'Length of output: 4320
🏁 Script executed:
#!/bin/bash
# Inspect ValueFactory for a null/undefined BSL value mapping
rg -n "public static .*Null" src/ScriptEngine/Machine/ValueFactory.cs || true
rg -n "Undefined" src/ScriptEngine/Machine/ValueFactory.cs || true
rg -n "Null" src/ScriptEngine/Machine/ValueFactory.cs | sed -n '1,200p'Length of output: 606
Ensure null returns map to a BSL null value and safely cast runtime instances
ContextValuesMarshaller.ConvertDynamicValue(null) throws an exception, so null must be handled explicitly via ValueFactory.CreateNullValue(). Also, when receiving an IRuntimeContextInstance, cast it to IValue safely and fall back to the regular conversion for any other CLR objects.
• File src/ScriptEngine/Machine/TypeFactory.cs, around lines 182–191
- var value = methodInfo.Invoke(null, methArgs);
+ var value = methodInfo.Invoke(null, methArgs);
if (value is IValue iValue)
return iValue;
- if (value is IRuntimeContextInstance rValue)
- return (IValue)rValue;
+ if (value is IRuntimeContextInstance rValue)
+ return rValue as IValue ?? ContextValuesMarshaller.ConvertDynamicValue(rValue);
- return ContextValuesMarshaller.ConvertDynamicValue(value);
+ if (value is null)
+ return ValueFactory.CreateNullValue();
+
+ return ContextValuesMarshaller.ConvertDynamicValue(value);• Confirmed in ContextValuesMarshaller.ConvertDynamicValue(object) that passing null throws instead of returning a BSL null.
• Use ValueFactory.CreateNullValue() to return BslNullValue.Instance.
🤖 Prompt for AI Agents
In src/ScriptEngine/Machine/TypeFactory.cs around lines 182 to 191, handle null
return values explicitly by checking if value is null and returning
ValueFactory.CreateNullValue() to avoid exceptions from
ContextValuesMarshaller.ConvertDynamicValue(null). Also, when value is an
IRuntimeContextInstance, cast it safely to IValue; for all other cases, use
ContextValuesMarshaller.ConvertDynamicValue(value) as a fallback.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/OneScript.Core/Contexts/ContextMethodInfo.cs (3)
27-41: InjectsProcess not initialized in this constructor (inconsistent behavior)Methods discovered via this ctor won’t have InjectsProcess set, unlike the other ctor. Either compute the property or set it here as well. Preferred: compute from signature (see separate comment).
Minimal fix inside this ctor:
InitConverter(); + InjectsProcess = _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess);
48-52: HasReturnConverter assigned only here; compute it from ConverterType to avoid ctor driftSetting HasReturnConverter in only one ctor leaves it false when the other ctor is used. Compute from ConverterType instead and remove this assignment. This removes duplication and keeps a single source of truth.
InjectsProcess = _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess); - HasReturnConverter = binding.Converter != null;Then implement HasReturnConverter as an expression-bodied property (see property section below).
132-135: Compute flags from the true sources to remove ctor couplingDerive both InjectsProcess and HasReturnConverter from _realMethod and ConverterType. This eliminates duplication and fixes the inconsistency across constructors.
- public bool InjectsProcess { get; } - - public bool HasReturnConverter { get; } + public bool InjectsProcess => + _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess); + + public bool HasReturnConverter => ConverterType != null;
🧹 Nitpick comments (2)
src/OneScript.Core/Contexts/ContextMethodInfo.cs (2)
25-25: Mark ConverterType as nullable to match usageYou already treat ConverterType as potentially null; make it explicit to improve clarity and nullable analysis.
- public Type ConverterType { get; private set; } + public Type? ConverterType { get; private set; }
54-58: Simplify InitConverter: direct assignment is sufficientBranch is unnecessary; direct assignment is clearer and equivalent given Converter is already nullable.
- private void InitConverter() - { - if (_scriptMark.Converter != null) - ConverterType = _scriptMark.Converter; - } + private void InitConverter() + { + ConverterType = _scriptMark.Converter; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/OneScript.Core/Contexts/ContextMethodInfo.cs(5 hunks)src/ScriptEngine/Machine/NotBslValueWrapper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/ScriptEngine/Machine/NotBslValueWrapper.cs
| public bool TryGetConverter(out object converter) | ||
| { | ||
| if (ConverterType == null) | ||
| { | ||
| converter = null; | ||
| return false; | ||
| } | ||
|
|
||
| converter = Activator.CreateInstance(ConverterType); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
TryGetConverter can throw; enforce “Try” semantics and validate type viability*
Activator.CreateInstance will throw for abstract types, open generics, or when no public parameterless ctor exists. A Try* method should not throw. Guard and catch, returning false.
public bool TryGetConverter(out object converter)
{
if (ConverterType == null)
{
converter = null;
return false;
}
-
- converter = Activator.CreateInstance(ConverterType);
- return true;
+
+ try
+ {
+ if (ConverterType.IsAbstract || ConverterType.IsInterface || ConverterType.ContainsGenericParameters)
+ {
+ converter = null;
+ return false;
+ }
+
+ if (ConverterType.GetConstructor(Type.EmptyTypes) == null)
+ {
+ converter = null;
+ return false;
+ }
+
+ converter = Activator.CreateInstance(ConverterType);
+ return converter != null;
+ }
+ catch (Exception)
+ {
+ converter = null;
+ return false;
+ }
}Optional: if converters are stateless, consider caching a single instance per ContextMethodInfo to reduce allocations. Happy to propose a thread-safe cache if desired.
📝 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.
| public bool TryGetConverter(out object converter) | |
| { | |
| if (ConverterType == null) | |
| { | |
| converter = null; | |
| return false; | |
| } | |
| converter = Activator.CreateInstance(ConverterType); | |
| return true; | |
| } | |
| public bool TryGetConverter(out object converter) | |
| { | |
| if (ConverterType == null) | |
| { | |
| converter = null; | |
| return false; | |
| } | |
| try | |
| { | |
| if (ConverterType.IsAbstract || ConverterType.IsInterface || ConverterType.ContainsGenericParameters) | |
| { | |
| converter = null; | |
| return false; | |
| } | |
| if (ConverterType.GetConstructor(Type.EmptyTypes) == null) | |
| { | |
| converter = null; | |
| return false; | |
| } | |
| converter = Activator.CreateInstance(ConverterType); | |
| return converter != null; | |
| } | |
| catch (Exception) | |
| { | |
| converter = null; | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.Core/Contexts/ContextMethodInfo.cs around lines 103 to 113, the
TryGetConverter method can throw exceptions when Activator.CreateInstance is
called on abstract types, open generics, or types without a public parameterless
constructor, violating the expected "Try*" method semantics. To fix this, add
checks to ensure ConverterType is not abstract, not an open generic, and has a
public parameterless constructor before calling CreateInstance. Wrap the
instantiation in a try-catch block to catch any exceptions and return false if
instantiation fails. Optionally, consider caching a single instance of the
converter if it is stateless to reduce allocations.
There was a problem hiding this comment.
@akpaevj может есть какой-то способ запретить указывать абстрактный тип в атрибуте? Ну или можно забить...
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| _scriptMark = binding; | ||
| InitConverter(); | ||
|
|
||
| InjectsProcess = _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess); | ||
| HasReturnConverter = binding.Converter != null; | ||
| } |
There was a problem hiding this comment.
@akpaevj посмотри, кажется, кролик прав
| public bool TryGetConverter(out object converter) | ||
| { | ||
| if (ConverterType == null) | ||
| { | ||
| converter = null; | ||
| return false; | ||
| } | ||
|
|
||
| converter = Activator.CreateInstance(ConverterType); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
@akpaevj может есть какой-то способ запретить указывать абстрактный тип в атрибуте? Ну или можно забить...
| return false; | ||
| } | ||
|
|
||
| converter = Activator.CreateInstance(ConverterType); |
There was a problem hiding this comment.
Не совсем понял, какой смысл в этом методе, что если я укажу в атрибуте в качестве конвертера typeof(Int32) и получу тут число? Может он должен быть private?
|
|
||
| namespace OneScript.Contexts | ||
| { | ||
| public abstract class ContextValueConverter<TClr> |
There was a problem hiding this comment.
Не лучше ли интерфейс, раз это полностью абстрактный класс?
|
|
||
| return; | ||
|
|
||
| void CantWriteAction(TInstance inst, IValue val) |
There was a problem hiding this comment.
а чем помешали в качестве локальных методов?
| BslNullValue _ => null, | ||
| BslTypeValue type => type.SystemType.ImplementingClass, | ||
| IObjectWrapper wrapper => wrapper.UnderlyingObject, | ||
| NotBslValueWrapper notBsl => notBsl.UnderlyingObject, |
There was a problem hiding this comment.
Почему NotBslValueWrapper не может быть IObjectWrapper? Тогда бы и отдельную ветку логики тут не пришлось бы делать и в других местах тоже. Не?
| } | ||
|
|
||
| private static int GetMemberNumberByName<T>(Dictionary<int, T> items, string name) where T : INameAndAliasProvider | ||
| => items.First(c => |
There was a problem hiding this comment.
Предлагаю закешировать имена в NameIndexCollection (или как-то так походим образом называется) и не создавать итераторы для First на каждое обращение
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/ScriptEngine/Machine/NotBslValueWrapper.cs (1)
148-165: Критично: конвертация аргументов по ReturnType; и жёсткий поискToIValueпо типу результатаАргументы конвертируются в тип возвращаемого значения метода — это неверно. Кроме того, поиск
ToIValueс конкретной сигнатурой может не сработать, еслиTClrконвертера не совпадает бит-в-бит сReturnType.Предлагаемый фикс:
- Конвертировать аргументы без привязки к
ReturnType(или по их фактическимParameterInfo, если это требуется).- Для результата получать
ToIValueпо имени без сигнатуры.- var args = arguments.Select(c => ContextValuesMarshaller.ConvertParam(c, method.ReturnType, process)).ToArray(); + // Универсальная конвертация аргументов — без привязки к ReturnType. + // Если требуется специфическая обработка по параметрам/инъекция process, + // это должно выполняться внутри ContextMethodInfo.Invoke. + var args = arguments.Select(ContextValuesMarshaller.ConvertToClrObject).ToArray(); var result = method.Invoke(UnderlyingObject, args); - if (method.TryGetConverter(out var converter)) + if (method.TryGetConverter(out var converter)) { - var type = converter.GetType(); - var convertMethod = type - .GetMethod("ToIValue", new [] { method.ReturnType }); - - retValue = (IValue)convertMethod!.Invoke(converter, new[] { result }); + var toIValue = converter.GetType().GetMethod("ToIValue"); + retValue = (IValue)toIValue!.Invoke(converter, new[] { result }); } else retValue = ContextValuesMarshaller.ConvertDynamicValue(result);Если
InvokeНЕ занимается подготовкой параметров (умолчания, IBslProcess-инъекции и т.п.), дайте знать — предложу вариант конвертации с использованиемParameterInfo[]и учётомprocess.
🧹 Nitpick comments (3)
src/OneScript.Core/Contexts/IContextValueConverter.cs (1)
19-21: Статический метод в интерфейсе: проверьте совместимость с версией языка или вынесите в helperНаличие
public staticв интерфейсе требует современных версий C# (C# 11+). Чтобы исключить риски сборки в других конфигурациях, предлагаю вынести проверку в статический helper.Вариант helper (можно разместить рядом, в том же неймспейсе):
namespace OneScript.Contexts { public static class ContextValueConverterHelpers { public static bool ImplementsConverterInterface(Type type) => type.GetInterfaces() .Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IContextValueConverter<>)); } }И заменить вызовы на:
ContextValueConverterHelpers.ImplementsConverterInterface(value)src/ScriptEngine/Machine/NotBslValueWrapper.cs (2)
167-175: Микрооптимизация: убрать LINQ из поиска имени, не создавать итераторы на «горячем пути»Сейчас используется
Where(...)внутриforeach, что создаёт лишние аллокации. Можно обойтись простым циклом.- foreach (var kvp in items.Where(kvp => string.Equals(kvp.Value.Name, name, StringComparison.InvariantCultureIgnoreCase) || - string.Equals(kvp.Value.Alias, name, StringComparison.InvariantCultureIgnoreCase))) - return kvp.Key; + foreach (var kvp in items) + { + var v = kvp.Value; + if (string.Equals(v.Name, name, StringComparison.InvariantCultureIgnoreCase) || + string.Equals(v.Alias, name, StringComparison.InvariantCultureIgnoreCase)) + return kvp.Key; + }
45-68: Детерминированный порядок и кеширование метаданных на тип
- Порядок
GetProperties()/GetMethods()не гарантирован. Если порядок важен для стабильного API — стоит отсортировать (например, поName), либо использовать порядок, задаваемый атрибутом, если он существует.- Инициализация
_properties/_methodsпроизводится на каждый экземпляр. Имеет смысл кешироватьContextPropertyInfo/ContextMethodInfoна уровне типа (статически,ConcurrentDictionary<Type, Cache>), чтобы не гонять рефлексию для каждого объекта.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/OneScript.Core/Contexts/ContextMethodAttribute.cs(3 hunks)src/OneScript.Core/Contexts/ContextMethodInfo.cs(3 hunks)src/OneScript.Core/Contexts/ContextPropertyAttribute.cs(3 hunks)src/OneScript.Core/Contexts/ContextPropertyInfo.cs(3 hunks)src/OneScript.Core/Contexts/IContextValueConverter.cs(1 hunks)src/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs(3 hunks)src/ScriptEngine/Machine/Contexts/ContextValuesMarshaller.cs(5 hunks)src/ScriptEngine/Machine/NotBslValueWrapper.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/OneScript.Core/Contexts/ContextPropertyAttribute.cs
- src/ScriptEngine/Machine/Contexts/ContextValuesMarshaller.cs
- src/OneScript.Core/Contexts/ContextPropertyInfo.cs
- src/OneScript.Core/Contexts/ContextMethodInfo.cs
- src/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-07-25T09:44:11.960Z
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1426
File: src/OneScript.Web.Server/HeaderDictionaryWrapper.cs:43-50
Timestamp: 2024-07-25T09:44:11.960Z
Learning: The `CanWrite` attribute in the `ContextProperty` attribute is used for different purposes and is not related to the presence of a setter in the property.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-29T06:48:46.675Z
Learnt from: Mr-Rm
PR: EvilBeaver/OneScript#1447
File: src/ScriptEngine/Machine/ValueFactory.cs:102-104
Timestamp: 2024-08-29T06:48:46.675Z
Learning: When performing operations that involve type conversion, ensure that the conversion function (e.g., `AsNumber`) handles invalid cases by throwing an appropriate exception, rather than adding redundant type checks in the calling method.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-28T16:51:21.322Z
Learnt from: nixel2007
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2024-08-28T16:51:21.322Z
Learning: В репозитории EvilBeaver/OneScript всегда нужно говорить по-русски.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-05T18:27:08.563Z
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
🔇 Additional comments (3)
src/OneScript.Core/Contexts/IContextValueConverter.cs (1)
13-18: Интерфейс ок, контракт двунаправленного конвертирования сформулирован корректноМетоды ToIValue/ToClr покрывают оба направления, что упрощает использование в рантайме.
src/OneScript.Core/Contexts/ContextMethodAttribute.cs (1)
69-88: ВалидацияConverter: используйте точные типы исключений, проверяйте возможность создания экземпляраСейчас кидается общий
Exception, нет проверки на публичный конструктор без параметров. Это приведёт к падениям позже при активации.
[Suggest_essential_refactor]Рекомендации:
- Использовать
ArgumentException/ArgumentNullException/InvalidOperationExceptionсnameof(value).- Проверить, что тип не абстрактный, не интерфейс и имеет публичный конструктор без параметров (если не создаётся через IoC).
- По возможности — использовать общий helper для проверки реализации интерфейса, чтобы не дублировать логику.
Предлагаемый diff в пределах этого блока:
public Type? Converter { get => _converter; set { if (value != null) { - if (!value.GetInterfaces().Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IContextValueConverter<>))) - throw new Exception("Конвертер должен реализовывать интерфейс IContextValueConverter"); + var implements = value.GetInterfaces() + .Any(i => i.IsGenericType && i.GetGenericTypeDefinition() == typeof(IContextValueConverter<>)); + if (!implements) + throw new ArgumentException("Конвертер должен реализовывать интерфейс IContextValueConverter<>", nameof(value)); - if (value.IsAbstract || value.IsInterface) - throw new Exception("Конвертер не может быть абстрактным типом или интерфейсом"); + if (value.IsAbstract || value.IsInterface) + throw new ArgumentException("Конвертер не может быть абстрактным типом или интерфейсом", nameof(value)); + + // Если экземпляры создаются через Activator: + var ctor = value.GetConstructor(Type.EmptyTypes); + if (ctor == null) + throw new InvalidOperationException("Для конвертера требуется публичный конструктор без параметров (или настройте создание через IoC)."); } _converter = value; } }src/ScriptEngine/Machine/NotBslValueWrapper.cs (1)
79-92: GetPropValue: корректно — проверка геттера и вызов конвертера по имениОтлично: проверяете наличие геттера и вызываете
ToIValueбез жёсткой привязки к типу параметра — это устраняет проблемы с несовпадениемTClrи реальногоPropertyType.
…, а не конкретного экземпляра NotBslValueWrapper
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/ScriptEngine/Machine/NotBslValueWrapper.cs (1)
177-194: CallAsFunction: аргументы конвертируются к ReturnType (ошибка) и ToIValue ищется по строгой сигнатуре
- Сейчас аргументы приводятся к типу возвращаемого значения метода — это неверно.
- Поиск ToIValue с указанием параметров ломается, если T конвертера не совпадает строго с ReturnType.
Исправление:
public override void CallAsFunction(int methodNumber, IValue[] arguments, out IValue retValue, IBslProcess process) { var method = GetMethodsCache()[methodNumber]; - var args = arguments.Select(c => ContextValuesMarshaller.ConvertParam(c, method.ReturnType, process)).ToArray(); + // конвертируем аргументы обобщённо, без привязки к ReturnType + var args = arguments.Select(ContextValuesMarshaller.ConvertToClrObject).ToArray(); var result = method.Invoke(UnderlyingObject, args); - if (method.TryGetConverter(out var converter)) + if (method.TryGetConverter(out var converter)) { - var type = converter.GetType(); - var convertMethod = type - .GetMethod("ToIValue", new [] { method.ReturnType }); - - retValue = (IValue)convertMethod!.Invoke(converter, new[] { result }); + var toIValue = converter.GetType().GetMethod("ToIValue"); + retValue = (IValue)toIValue!.Invoke(converter, new[] { result }); } else retValue = ContextValuesMarshaller.ConvertDynamicValue(result); }Примечание: при необходимости типизированного приведения аргументов лучше брать типы параметров метода, а не ReturnType. Но безопаснее (и проще) — оставить ConvertToClrObject, если ContextMethodInfo.Invoke корректно сопоставляет аргументы.
🧹 Nitpick comments (2)
src/ScriptEngine/Machine/NotBslValueWrapper.cs (2)
67-70: Отбирайте только экземплярные (instance) членыБез BindingFlags сюда попадут и статические свойства/методы с атрибутом, что для обёртки над экземпляром обычно нежелательно.
- var props = objType.GetProperties() + var props = objType.GetProperties(BindingFlags.Public | BindingFlags.Instance) .Where(x => x.GetCustomAttributes(typeof(ContextPropertyAttribute), false).Length != 0) .ToList(); ... - var methods = objType.GetMethods() + var methods = objType.GetMethods(BindingFlags.Public | BindingFlags.Instance) .Where(x => x.GetCustomAttributes(typeof(ContextMethodAttribute), false).Length != 0) .ToList();Also applies to: 79-82
196-209: Неиспользуемый вспомогательный поиск по имени — удалитьGetMemberNumberByName больше не используется после перехода на IndexedNamesCollection и только нагружает код.
- private static int GetMemberNumberByName<T>(Dictionary<int, T> items, string name) - where T : INameAndAliasProvider - { - foreach (var kvp in from kvp in items - let v = kvp.Value - where string.Equals(v.Name, name, StringComparison.InvariantCultureIgnoreCase) || - string.Equals(v.Alias, name, StringComparison.InvariantCultureIgnoreCase) - select kvp) - { - return kvp.Key; - } - - return -1; - } + // метод поиска по имени более не нужен: используем IndexedNamesCollection
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ScriptEngine/Machine/NotBslValueWrapper.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-07-25T09:44:11.960Z
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1426
File: src/OneScript.Web.Server/HeaderDictionaryWrapper.cs:43-50
Timestamp: 2024-07-25T09:44:11.960Z
Learning: The `CanWrite` attribute in the `ContextProperty` attribute is used for different purposes and is not related to the presence of a setter in the property.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-29T06:48:46.675Z
Learnt from: Mr-Rm
PR: EvilBeaver/OneScript#1447
File: src/ScriptEngine/Machine/ValueFactory.cs:102-104
Timestamp: 2024-08-29T06:48:46.675Z
Learning: When performing operations that involve type conversion, ensure that the conversion function (e.g., `AsNumber`) handles invalid cases by throwing an appropriate exception, rather than adding redundant type checks in the calling method.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-28T16:51:21.322Z
Learnt from: nixel2007
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2024-08-28T16:51:21.322Z
Learning: В репозитории EvilBeaver/OneScript всегда нужно говорить по-русски.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-05T18:27:08.563Z
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
🧬 Code Graph Analysis (1)
src/ScriptEngine/Machine/NotBslValueWrapper.cs (10)
src/OneScript.Core/Contexts/ContextPropertyInfo.cs (3)
ContextPropertyInfo(20-127)ContextPropertyInfo(25-29)TryGetConverter(60-70)src/OneScript.Core/Contexts/ContextMethodInfo.cs (5)
ContextMethodInfo(20-131)ContextMethodInfo(27-39)ContextMethodInfo(41-48)Invoke(89-92)TryGetConverter(99-109)src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs (1)
DefineType(36-39)src/OneScript.Core/Contexts/ContextPropertyAttribute.cs (1)
ContextPropertyAttribute(21-33)src/OneScript.Core/Commons/IndexedNamesCollection.cs (2)
RegisterName(28-39)TryGetIdOfName(18-21)src/OneScript.Core/Contexts/ContextMethodAttribute.cs (2)
ContextMethodAttribute(27-37)ContextMethodAttribute(39-43)src/OneScript.Core/Contexts/Internal/IBuildableMember.cs (2)
CanRead(31-31)CanWrite(32-32)src/OneScript.Core/Contexts/IContextValueConverter.cs (1)
IValue(15-15)src/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs (2)
IValue(121-124)T(116-119)src/ScriptEngine/Machine/Contexts/ContextMethodMapper.cs (1)
BslMethodInfo(60-64)
…пляр. Исправлен алгоритм регистрации членов объекта контекста по алиасу. Добавлена передача инстанса bsl процесса в аргументы метода
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/ScriptEngine/Machine/NotBslValueWrapper.cs (1)
185-185: Критическая ошибка: аргументы конвертируются в тип возвращаемого значенияАргументы неправильно конвертируются с использованием
method.ReturnTypeвместо типов параметров метода. Это приведёт к ошибкам преобразования типов при вызове методов.Используйте
ConvertToClrObjectдля универсальной конвертации аргументов, как вCallAsProcedure:- var args = arguments.Select(c => ContextValuesMarshaller.ConvertParam(c, method.ReturnType, process)).ToList(); + var args = arguments.Select(ContextValuesMarshaller.ConvertToClrObject).ToList();
🧹 Nitpick comments (1)
src/ScriptEngine/Machine/NotBslValueWrapper.cs (1)
204-217: Упростите LINQ-запрос для читаемостиТекущий LINQ-запрос излишне сложен. Можно упростить код, сохранив функциональность.
- private static int GetMemberNumberByName<T>(Dictionary<int, T> items, string name) - where T : INameAndAliasProvider - { - foreach (var kvp in from kvp in items - let v = kvp.Value - where string.Equals(v.Name, name, StringComparison.InvariantCultureIgnoreCase) || - string.Equals(v.Alias, name, StringComparison.InvariantCultureIgnoreCase) - select kvp) - { - return kvp.Key; - } - - return -1; - } + private static int GetMemberNumberByName<T>(Dictionary<int, T> items, string name) + where T : INameAndAliasProvider + { + foreach (var kvp in items) + { + var v = kvp.Value; + if (string.Equals(v.Name, name, StringComparison.InvariantCultureIgnoreCase) || + string.Equals(v.Alias, name, StringComparison.InvariantCultureIgnoreCase)) + return kvp.Key; + } + + return -1; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ScriptEngine/Machine/NotBslValueWrapper.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2024-07-25T09:44:11.960Z
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#1426
File: src/OneScript.Web.Server/HeaderDictionaryWrapper.cs:43-50
Timestamp: 2024-07-25T09:44:11.960Z
Learning: The `CanWrite` attribute in the `ContextProperty` attribute is used for different purposes and is not related to the presence of a setter in the property.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-29T06:48:46.675Z
Learnt from: Mr-Rm
PR: EvilBeaver/OneScript#1447
File: src/ScriptEngine/Machine/ValueFactory.cs:102-104
Timestamp: 2024-08-29T06:48:46.675Z
Learning: When performing operations that involve type conversion, ensure that the conversion function (e.g., `AsNumber`) handles invalid cases by throwing an appropriate exception, rather than adding redundant type checks in the calling method.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-28T16:51:21.322Z
Learnt from: nixel2007
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2024-08-28T16:51:21.322Z
Learning: В репозитории EvilBeaver/OneScript всегда нужно говорить по-русски.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
📚 Learning: 2024-08-05T18:27:08.563Z
Learnt from: EvilBeaver
PR: EvilBeaver/OneScript#0
File: :0-0
Timestamp: 2024-08-05T18:27:08.563Z
Learning: For each new pull request in the EvilBeaver/OneScript repository, include a poem in Russian.
Applied to files:
src/ScriptEngine/Machine/NotBslValueWrapper.cs
🧬 Code Graph Analysis (1)
src/ScriptEngine/Machine/NotBslValueWrapper.cs (9)
src/OneScript.Core/Contexts/ContextPropertyInfo.cs (3)
ContextPropertyInfo(20-127)ContextPropertyInfo(25-29)TryGetConverter(60-70)src/OneScript.Core/Contexts/ContextMethodInfo.cs (5)
ContextMethodInfo(20-131)ContextMethodInfo(27-39)ContextMethodInfo(41-48)Invoke(89-92)TryGetConverter(99-109)src/ScriptEngine/Machine/Contexts/ContextIValueImpl.cs (1)
DefineType(36-39)src/OneScript.Core/Contexts/ContextPropertyAttribute.cs (1)
ContextPropertyAttribute(21-33)src/OneScript.Core/Commons/IndexedNamesCollection.cs (2)
RegisterName(28-39)TryGetIdOfName(18-21)src/OneScript.Core/Contexts/ContextMethodAttribute.cs (2)
ContextMethodAttribute(27-37)ContextMethodAttribute(39-43)src/OneScript.Core/Contexts/Internal/IBuildableMember.cs (2)
CanRead(31-31)CanWrite(32-32)src/OneScript.Core/Contexts/IContextValueConverter.cs (1)
IValue(15-15)src/ScriptEngine/Machine/Contexts/ContextValuesMarshaller.cs (15)
IValue(270-311)IValue(313-316)IValue(331-337)IValue(339-345)IValue(347-350)ContextValuesMarshaller(21-390)ContextValuesMarshaller(27-39)ConvertToClrObject(352-374)ConvertParam(49-55)ConvertParam(64-78)ConvertParam(154-157)T(89-92)T(102-106)T(114-142)T(318-329)
2 participants
Добавлено автоматическое оборачивание не AutoContext объекта во враппер и возможность установки конвертера clr-bsl значений для свойств и возвращаемых значений методов
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores