Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/OneScript.Core/Contexts/ContextMethodAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ This Source Code Form is subject to the terms of the
at http://mozilla.org/MPL/2.0/.
----------------------------------------------------------*/

#nullable enable
using System;
using System.Runtime.CompilerServices;
using OneScript.Commons;
Expand Down Expand Up @@ -46,5 +47,10 @@ public ContextMethodAttribute(string name, string _ = null,

public string Name => _name;
public string Alias => _alias;

/// <summary>
/// Конвертер возвращаемого значения функции, необходим для неподдерживаемых маршаллингом типов
/// </summary>
public Type? Converter { get; set; }
}
}
28 changes: 28 additions & 0 deletions src/OneScript.Core/Contexts/ContextMethodInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public sealed class ContextMethodInfo : BslMethodInfo, IObjectWrapper
{
private readonly MethodInfo _realMethod;
private readonly ContextMethodAttribute _scriptMark;

public Type ConverterType { get; private set; }

public ContextMethodInfo(MethodInfo realMethod)
{
Expand All @@ -29,6 +31,8 @@ public ContextMethodInfo(MethodInfo realMethod)
{
_scriptMark =
(ContextMethodAttribute) GetCustomAttributes(typeof(ContextMethodAttribute), false).First();

InitConverter();
}
catch (InvalidOperationException e)
{
Expand All @@ -39,8 +43,18 @@ public ContextMethodInfo(MethodInfo realMethod)
public ContextMethodInfo(MethodInfo realMethod, ContextMethodAttribute binding)
{
_realMethod = realMethod;

_scriptMark = binding;
InitConverter();

InjectsProcess = _realMethod.GetParameters().FirstOrDefault()?.ParameterType == typeof(IBslProcess);
HasReturnConverter = binding.Converter != null;
}

@coderabbitai coderabbitai Bot Aug 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Initialize consistently across constructors; compute HasReturnConverter from ConverterType

  • HasReturnConverter is assigned in only one constructor, leaving it false in the other. Prefer computing it from ConverterType to keep logic in one place.
  • InjectsProcess is 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.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akpaevj посмотри, кажется, кролик прав

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


private void InitConverter()
{
if (_scriptMark.Converter != null)
ConverterType = _scriptMark.Converter;
}

public override Type ReturnType => _realMethod.ReturnType;
Expand Down Expand Up @@ -85,6 +99,18 @@ public override MethodInfo GetBaseDefinition()
{
return _realMethod.GetBaseDefinition();
}

public bool TryGetConverter(out object converter)
{
if (ConverterType == null)
{
converter = null;
return false;
}

converter = Activator.CreateInstance(ConverterType);
return true;
}
Comment on lines +99 to +109

@coderabbitai coderabbitai Bot Aug 8, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akpaevj может есть какой-то способ запретить указывать абстрактный тип в атрибуте? Ну или можно забить...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 override ICustomAttributeProvider ReturnTypeCustomAttributes => _realMethod.ReturnTypeCustomAttributes;

Expand All @@ -104,6 +130,8 @@ public override MethodInfo GetBaseDefinition()
public object UnderlyingObject => _realMethod;

public bool InjectsProcess { get; }

public bool HasReturnConverter { get; }

public MethodInfo GetWrappedMethod() => _realMethod;
}
Expand Down
5 changes: 5 additions & 0 deletions src/OneScript.Core/Contexts/ContextPropertyAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,10 @@ public ContextPropertyAttribute(string name, string alias = "")

public string Name => _name;
public string Alias => _alias;

/// <summary>
/// Конвертер значения свойства
/// </summary>
public Type Converter { get; set; }
}
}
30 changes: 29 additions & 1 deletion src/OneScript.Core/Contexts/ContextPropertyInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ This Source Code Form is subject to the terms of the
using System.Linq;
using System.Reflection;
using OneScript.Commons;
using ScriptEngine.Machine;

namespace OneScript.Contexts
{
Expand All @@ -20,11 +21,12 @@ public class ContextPropertyInfo : BslPropertyInfo, IObjectWrapper
{
private readonly PropertyInfo _realProperty;
private readonly ContextPropertyAttribute _scriptMark;

public ContextPropertyInfo(PropertyInfo wrappedInfo)
{
_realProperty = wrappedInfo;
_scriptMark = (ContextPropertyAttribute)_realProperty.GetCustomAttributes(typeof(ContextPropertyAttribute), false).First();
ConverterType = _scriptMark.Converter;
}

public override object[] GetCustomAttributes(bool inherit)
Expand Down Expand Up @@ -54,6 +56,32 @@ public override bool Equals(BslPropertyInfo other)

public override string Alias => _scriptMark.Alias;

public Type ConverterType { get; private set; }

public bool TryGetConverter(out object converter)
{
if (ConverterType == null)
{
converter = null;
return false;
}

converter = Activator.CreateInstance(ConverterType);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не совсем понял, какой смысл в этом методе, что если я укажу в атрибуте в качестве конвертера typeof(Int32) и получу тут число? Может он должен быть private?

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;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Suggested change
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 MethodInfo[] GetAccessors(bool nonPublic)
{
var getter = GetGetMethod(nonPublic);
Expand Down
18 changes: 18 additions & 0 deletions src/OneScript.Core/Contexts/ContextValueConverter.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*----------------------------------------------------------
This Source Code Form is subject to the terms of the
Mozilla Public License, v.2.0. If a copy of the MPL
was not distributed with this file, You can obtain one
at http://mozilla.org/MPL/2.0/.
----------------------------------------------------------*/
using System;
using ScriptEngine.Machine;

namespace OneScript.Contexts
{
public abstract class ContextValueConverter<TClr>

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Не лучше ли интерфейс, раз это полностью абстрактный класс?

{
public abstract IValue ToIValue(TClr obj);

public abstract TClr ToClr(IValue obj);
}
}
46 changes: 26 additions & 20 deletions src/ScriptEngine/Machine/Contexts/ContextMethodMapper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -152,25 +152,35 @@ private static MethodSignature CreateMetadata(MethodInfo target, ContextMethodAt

}

var scriptMethInfo = new MethodSignature();
scriptMethInfo.IsFunction = isFunc;
scriptMethInfo.IsExport = true;
scriptMethInfo.IsDeprecated = binding.IsDeprecated;
scriptMethInfo.ThrowOnUseDeprecated = binding.ThrowOnUse;
scriptMethInfo.Name = binding.Name;
scriptMethInfo.Alias = binding.Alias;

scriptMethInfo.Params = paramDefs;
var scriptMethInfo = new MethodSignature
{
IsFunction = isFunc,
IsExport = true,
IsDeprecated = binding.IsDeprecated,
ThrowOnUseDeprecated = binding.ThrowOnUse,
Name = binding.Name,
Alias = binding.Alias,
Params = paramDefs
};

return scriptMethInfo;
}

private static ContextCallableDelegate<TInstance> CreateFunction(ContextMethodInfo target)
{
var methodCall = MethodCallExpression(target, out var instParam, out var argsParam, out var processParam);

var convertRetMethod = ContextValuesMarshaller.BslReturnValueGenericConverter.MakeGenericMethod(target.ReturnType);
var convertReturnCall = Expression.Call(convertRetMethod, methodCall);

var convertReturnCall = target.ConverterType switch
{
null => Expression.Call(
ContextValuesMarshaller.BslReturnValueGenericConverter.MakeGenericMethod(target.ReturnType),
methodCall),
_ => Expression.Call(
Expression.New(target.ConverterType),
target.ConverterType.GetMethod("ToIValue")!,
methodCall)
};

var body = convertReturnCall;

var l = Expression.Lambda<ContextCallableDelegate<TInstance>>(body, instParam, argsParam, processParam);
Expand Down Expand Up @@ -226,28 +236,24 @@ private static InvocationExpression MethodCallExpression(

var (clrIndexStart, argsLen) = contextMethod.InjectsProcess ? (1, parameters.Length - 1) : (0, parameters.Length);

var argsPass = new List<Expression>();
argsPass.Add(instParam);

var argsPass = new List<Expression> { instParam };

if (contextMethod.InjectsProcess)
argsPass.Add(processParam);

for (int bslIndex = 0,clrIndex = clrIndexStart; bslIndex < argsLen; bslIndex++, clrIndex++)
{
var targetType = parameters[clrIndex].ParameterType;
var convertMethod = ContextValuesMarshaller.BslGenericParameterConverter.MakeGenericMethod(targetType);

Expression defaultArg;
if (parameters[clrIndex].HasDefaultValue)
{
defaultArg = Expression.Constant(parameters[clrIndex].DefaultValue, targetType);
}
else
{
defaultArg = ContextValuesMarshaller.GetDefaultBslValueConstant(targetType);
}

var indexedArg = Expression.ArrayIndex(argsParam, Expression.Constant(bslIndex));

var convertMethod = ContextValuesMarshaller.BslGenericParameterConverter.MakeGenericMethod(targetType);
var conversionCall = Expression.Call(convertMethod,
indexedArg,
defaultArg,
Expand Down
48 changes: 30 additions & 18 deletions src/ScriptEngine/Machine/Contexts/ContextPropertyMapper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ namespace ScriptEngine.Machine.Contexts
{
public class PropertyTarget<TInstance>
{
private readonly BslPropertyInfo _propertyInfo;

private readonly ContextPropertyInfo _propertyInfo;
public PropertyTarget(PropertyInfo propInfo)
{
_propertyInfo = new ContextPropertyInfo(propInfo);
Expand All @@ -27,16 +27,6 @@ public PropertyTarget(PropertyInfo propInfo)
if (string.IsNullOrEmpty(Alias))
Alias = propInfo.Name;

IValue CantReadAction(TInstance inst)
{
throw PropertyAccessException.PropIsNotReadableException(Name);
}

void CantWriteAction(TInstance inst, IValue val)
{
throw PropertyAccessException.PropIsNotWritableException(Name);
}

if (_propertyInfo.CanRead)
{
var getMethodInfo = propInfo.GetGetMethod();
Expand Down Expand Up @@ -84,6 +74,18 @@ void CantWriteAction(TInstance inst, IValue val)
{
Setter = CantWriteAction;
}

return;

void CantWriteAction(TInstance inst, IValue val)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

а чем помешали в качестве локальных методов?

{
throw PropertyAccessException.PropIsNotWritableException(Name);
}

IValue CantReadAction(TInstance inst)
{
throw PropertyAccessException.PropIsNotReadableException(Name);
}
}

public Func<TInstance, IValue> Getter { get; }
Expand All @@ -108,19 +110,29 @@ private Func<TInstance, IValue> CreateGetter<T>(MethodInfo methInfo)
private Action<TInstance, IValue> CreateSetter<T>(MethodInfo methInfo)
{
var method = (Action<TInstance, T>)Delegate.CreateDelegate(typeof(Action<TInstance, T>), methInfo);
return (inst, val) => method(inst, ConvertParam<T>(val));
return (inst, val) => method(inst, ConvertValue<T>(val));
}

private static T ConvertParam<T>(IValue value)
private T ConvertValue<T>(IValue value)
{
return ContextValuesMarshaller.ConvertValueStrict<T>(value);
if (value is NotBslValueWrapper wrapper && wrapper.UnderlyingObject.GetType() == _propertyInfo.PropertyType)
return (T)wrapper.UnderlyingObject;

if (_propertyInfo.ConverterType == null)
return ContextValuesMarshaller.ConvertValueStrict<T>(value);

var converter = Activator.CreateInstance(_propertyInfo.ConverterType!) as ContextValueConverter<T>;
return converter!.ToClr(value);
}

private static IValue ConvertReturnValue<TRet>(TRet param)
private IValue ConvertReturnValue<TRet>(TRet param)
{
return ContextValuesMarshaller.ConvertReturnValue(param);
if (_propertyInfo.ConverterType == null)
return ContextValuesMarshaller.ConvertReturnValue(param);

var converter = Activator.CreateInstance(_propertyInfo.ConverterType!) as ContextValueConverter<TRet>;
return converter!.ToIValue(param);
}

}

public class ContextPropertyMapper<TInstance>
Expand Down
Loading