Skip to content

Commit a12cd0b

Browse files
xoofxCopilot
xoofx
and
Copilot
committed
Fix delegate optional defaults
Co-authored-by: Copilot <[email protected]>
1 parent 7327154 commit a12cd0b

File tree

5 files changed

+64
-31
lines changed

5 files changed

+64
-31
lines changed

‎issue-followup-plan.md‎

Lines changed: 10 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22

33
This cleanup pass closed 55 old issues that were stale, already answered, resolved in current Scriban, or outside the scope of core Scriban.
44

5-
Issue `#625` is fixed locally by the documentation change in this pass.
5+
Issues `#625` and `#453` are fixed locally by the changes in this pass.
66

7-
The 10 issues below remain open because they were either reproduced on current head, still point to incorrect documentation, or need an explicit design decision before any code change is worth doing.
7+
The 9 issues below remain open because they were either reproduced on current head, still point to incorrect documentation, or need an explicit design decision before any code change is worth doing.
88

99
Default stance for the next pass:
1010

@@ -14,20 +14,6 @@ Default stance for the next pass:
1414

1515
## 1. Concrete docs and bug fixes
1616

17-
### #453 Default arguments from a delegate definition are ignored
18-
19-
Status: reproduced.
20-
21-
Evidence:
22-
23-
- An imported custom delegate with an optional parameter still fails when called without arguments.
24-
- `src/Scriban/Runtime/DelegateCustomFunction.cs` builds metadata from `del.Method`, which loses defaults from the delegate type's `Invoke` signature in this scenario.
25-
26-
Suggested action:
27-
28-
- Preserve optional/default parameter metadata from the delegate type's `Invoke` signature when wrapping a delegate.
29-
- Add a regression test for an imported custom delegate with an optional parameter.
30-
3117
### #529 Null-Conditional cannot be mixed with indexers
3218

3319
Status: reproduced.
@@ -157,12 +143,11 @@ Suggested action:
157143

158144
## Suggested review order
159145

160-
1. #453
161-
2. #529
162-
3. #553
163-
4. #209
164-
5. #188 and #405 together
165-
6. #368
166-
7. #513
167-
8. #284
168-
9. #446
146+
1. #529
147+
2. #553
148+
3. #209
149+
4. #188 and #405 together
150+
5. #368
151+
6. #513
152+
7. #284
153+
8. #446

‎src/Scriban.Tests/TestRuntime.cs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
namespace Scriban.Tests
2222
{
2323
delegate string Args(object[] args);
24+
delegate string OptionalTextDelegate(string text = "default");
2425

2526
[TestFixture]
2627
public class TestRuntime
@@ -1178,6 +1179,29 @@ public void TestScriptObjectImport()
11781179
}
11791180
}
11801181

1182+
[Test]
1183+
public void TestScriptObjectImportDelegateOptionalParameter()
1184+
{
1185+
var obj = new ScriptObject();
1186+
OptionalTextDelegate formatter = text => text.ToUpperInvariant();
1187+
obj.Import("formatter", formatter);
1188+
1189+
var function = (IScriptCustomFunction)obj["formatter"];
1190+
Assert.AreEqual(0, function.RequiredParameterCount);
1191+
Assert.AreEqual(1, function.ParameterCount);
1192+
1193+
var parameterInfo = function.GetParameterInfo(0);
1194+
Assert.That(parameterInfo.HasDefaultValue, Is.True);
1195+
Assert.AreEqual("default", parameterInfo.DefaultValue);
1196+
1197+
var context = new TemplateContext();
1198+
context.PushGlobal(obj);
1199+
1200+
var result = Template.Parse("{{ formatter() }}").Render(context);
1201+
1202+
Assert.AreEqual("DEFAULT", result);
1203+
}
1204+
11811205

11821206
[Test]
11831207
public void TestScriptObjectAccessor()

‎src/Scriban/Runtime/DelegateCustomFunction.cs‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ partial class DelegateCustomFunction : DynamicCustomFunction
2525
{
2626
private readonly Delegate _del;
2727

28-
public DelegateCustomFunction(Delegate del) : base(del?.Method)
28+
public DelegateCustomFunction(Delegate del) : base(del?.Method, GetDelegateParameterInfos(del))
2929
{
3030
_del = del ?? throw new ArgumentNullException(nameof(del));
3131
Target = del.Target;
@@ -167,6 +167,30 @@ protected virtual object InvokeImpl(TemplateContext context, SourceSpan span, ob
167167
return _del != null ? _del.DynamicInvoke(arguments) : Method.Invoke(Target, arguments);
168168
}
169169

170+
private static ParameterInfo[] GetDelegateParameterInfos(Delegate del)
171+
{
172+
if (del == null) throw new ArgumentNullException(nameof(del));
173+
174+
var delegateInvokeMethod = del.GetType().GetMethod(nameof(Action.Invoke)) ?? throw new ArgumentException("Unable to resolve delegate Invoke method.", nameof(del));
175+
var methodParameters = del.Method.GetParameters();
176+
var invokeParameters = delegateInvokeMethod.GetParameters();
177+
178+
if (methodParameters.Length != invokeParameters.Length)
179+
{
180+
return methodParameters;
181+
}
182+
183+
var mergedParameters = new ParameterInfo[methodParameters.Length];
184+
for (int i = 0; i < methodParameters.Length; i++)
185+
{
186+
var methodParameter = methodParameters[i];
187+
var invokeParameter = invokeParameters[i];
188+
mergedParameters[i] = invokeParameter.HasDefaultValue && !methodParameter.HasDefaultValue ? invokeParameter : methodParameter;
189+
}
190+
191+
return mergedParameters;
192+
}
193+
170194
[UnconditionalSuppressMessage("AOT", "IL3050", Justification = "Array element type is known from the method's parameter type at import time.")]
171195
private object[] PrepareArguments(TemplateContext context, ScriptNode callerContext, ScriptArray scriptArguments, ref Array paramsArguments)
172196
{
@@ -510,4 +534,4 @@ protected override object InvokeImpl(TemplateContext context, SourceSpan span, o
510534
return null;
511535
}
512536
}
513-
}
537+
}

‎src/Scriban/Runtime/DynamicCustomFunction.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,12 @@ abstract partial class DynamicCustomFunction : IScriptCustomFunction
5151
protected readonly int _firstIndexOfUserParameters;
5252

5353
[UnconditionalSuppressMessage("Trimming", "IL2075", Justification = "GetAwaiter is a well-known public method on Task-like types.")]
54-
protected DynamicCustomFunction(MethodInfo method)
54+
protected DynamicCustomFunction(MethodInfo method, ParameterInfo[] parameters = null)
5555
{
5656
Method = method ?? throw new ArgumentNullException(nameof(method));
5757
_returnType = method.ReturnType;
5858

59-
Parameters = method.GetParameters();
59+
Parameters = parameters ?? method.GetParameters();
6060
#if !SCRIBAN_NO_ASYNC
6161
IsAwaitable = method.ReturnType.GetMethod(nameof(Task.GetAwaiter)) != null;
6262
#endif
@@ -287,4 +287,4 @@ public int GetHashCode(MethodInfo method)
287287
}
288288
}
289289
}
290-
}
290+
}

‎src/Scriban/Runtime/ScriptObjectExtensions.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,7 @@ public static void Import(this IScriptObject script, string member, Delegate fun
372372
if (member == null) throw new ArgumentNullException(nameof(member));
373373
if (function == null) throw new ArgumentNullException(nameof(function));
374374

375-
script.TrySetValue(null, new SourceSpan(), member, DynamicCustomFunction.Create(function.Target, function.GetMethodInfo()), true);
375+
script.TrySetValue(null, new SourceSpan(), member, DynamicCustomFunction.Create(function), true);
376376
}
377377

378378
/// <summary>

0 commit comments

Comments
 (0)