From 4a7cb3572f8c158aa8ca2e9b1c76bb5b1c1e56ad Mon Sep 17 00:00:00 2001 From: Pavel Koneski Date: Mon, 16 Nov 2020 20:11:57 -0800 Subject: [PATCH 1/2] Prevent ParamArray and ParamDictionary parameters from binding by keyword --- .../Actions/Calls/DefaultOverloadResolver.cs | 33 +++++++++++-------- .../Actions/Calls/OverloadResolver.cs | 24 +++++++++++--- .../Actions/Calls/ParameterMapping.cs | 2 +- .../Actions/Calls/ParamsDictArgBuilder.cs | 10 ++++-- .../Utils/ReflectionUtils.cs | 7 ++++ .../Runtime/ParamDictionaryAttribute.cs | 4 +-- Tests/ClrAssembly/Src/methodargs.cs | 10 +++--- 7 files changed, 62 insertions(+), 28 deletions(-) diff --git a/Src/Microsoft.Dynamic/Actions/Calls/DefaultOverloadResolver.cs b/Src/Microsoft.Dynamic/Actions/Calls/DefaultOverloadResolver.cs index 0de9b934..7fffe256 100644 --- a/Src/Microsoft.Dynamic/Actions/Calls/DefaultOverloadResolver.cs +++ b/Src/Microsoft.Dynamic/Actions/Calls/DefaultOverloadResolver.cs @@ -8,7 +8,7 @@ using System.Diagnostics; using System.Dynamic; using System.Linq.Expressions; - +using System.Reflection; using Microsoft.Scripting.Actions.Calls; using Microsoft.Scripting.Runtime; using Microsoft.Scripting.Utils; @@ -104,30 +104,31 @@ protected internal override Candidate CompareEquivalentCandidates(ApplicableCand #region Actual Arguments private DynamicMetaObject GetArgument(int i) { + Debug.Assert(i >= 0); return _args[(CallType == CallTypes.ImplicitInstance ? 1 : 0) + i]; } protected override void GetNamedArguments(out IList namedArgs, out IList argNames) { - if (_signature.HasNamedArgument() || _signature.HasDictionaryArgument()) { + bool hasNamedArgument = _signature.HasNamedArgument(); + bool hasDictionaryArgument = _signature.HasDictionaryArgument(); + + if (hasNamedArgument || hasDictionaryArgument) { var objects = new List(); var names = new List(); - for (int i = 0; i < _signature.ArgumentCount; i++) { - if (_signature.GetArgumentKind(i) == ArgumentType.Named) { - objects.Add(GetArgument(i)); - names.Add(_signature.GetArgumentName(i)); + if (hasNamedArgument) { + for (int i = 0; i < _signature.ArgumentCount; i++) { + if (_signature.GetArgumentKind(i) == ArgumentType.Named) { + objects.Add(GetArgument(i)); + names.Add(_signature.GetArgumentName(i)); + } } } - if (_signature.HasDictionaryArgument()) { - if (objects == null) { - objects = new List(); - names = new List(); - } + if (hasDictionaryArgument) { SplatDictionaryArgument(names, objects); } - names.TrimExcess(); objects.TrimExcess(); argNames = names; @@ -138,6 +139,11 @@ protected override void GetNamedArguments(out IList namedArgs } } + // TODO: review the signature, add protected + internal override bool AllowByKeywordArgument(OverloadInfo method, ParameterInfo parameter) { + return !parameter.ProhibitsByKeyword(); + } + protected override ActualArguments CreateActualArguments(IList namedArgs, IList argNames, int preSplatLimit, int postSplatLimit) { var res = new List(); @@ -195,8 +201,9 @@ protected override ActualArguments CreateActualArguments(IList splattedNames, IList splattedArgs) { Assert.NotNull(splattedNames, splattedArgs); + Debug.Assert(_signature.HasDictionaryArgument()); - DynamicMetaObject dictMo = GetArgument(_signature.ArgumentCount - 1); + DynamicMetaObject dictMo = GetArgument(_signature.IndexOf(ArgumentType.Dictionary)); IDictionary dict = (IDictionary)dictMo.Value; IDictionaryEnumerator dictEnum = dict.GetEnumerator(); while (dictEnum.MoveNext()) { diff --git a/Src/Microsoft.Dynamic/Actions/Calls/OverloadResolver.cs b/Src/Microsoft.Dynamic/Actions/Calls/OverloadResolver.cs index d65bad02..bd1760da 100644 --- a/Src/Microsoft.Dynamic/Actions/Calls/OverloadResolver.cs +++ b/Src/Microsoft.Dynamic/Actions/Calls/OverloadResolver.cs @@ -183,6 +183,16 @@ protected virtual bool BindToUnexpandedParams(MethodCandidate candidate) { return true; } + /// + /// Checks whether the given parameter may be mapped to by a keyword argument. + /// + /// + /// + // TODO: review the signature, add protected + internal virtual bool AllowByKeywordArgument(OverloadInfo method, ParameterInfo parameter) { + return true; // legacy behavior, but by default OverloadResolver doesn't recognize named arguments anyway + } + /// /// Called before arguments binding. /// @@ -289,7 +299,7 @@ private void AddBasicMethodTargets(OverloadInfo method) { var mapping = new ParameterMapping(this, method, _argNames); - mapping.MapParameters(false); + mapping.MapParameters(reduceByRef: false); foreach (var defaultCandidate in mapping.CreateDefaultCandidates()) { AddSimpleTarget(defaultCandidate); @@ -1188,11 +1198,15 @@ public virtual Type GetGenericInferenceType(DynamicMetaObject dynamicObject) { } public override string ToString() { - string res = string.Empty; - foreach (CandidateSet set in _candidateSets.Values) { - res += set + Environment.NewLine; + if (_candidateSets != null) { + string res = string.Empty; + foreach (CandidateSet set in _candidateSets.Values) { + res += set + Environment.NewLine; + } + return res; + } else { + return $""; } - return res; } } } diff --git a/Src/Microsoft.Dynamic/Actions/Calls/ParameterMapping.cs b/Src/Microsoft.Dynamic/Actions/Calls/ParameterMapping.cs index 9d472f58..595a532f 100644 --- a/Src/Microsoft.Dynamic/Actions/Calls/ParameterMapping.cs +++ b/Src/Microsoft.Dynamic/Actions/Calls/ParameterMapping.cs @@ -103,7 +103,7 @@ public void AddParameter(ParameterWrapper parameter) { public void MapParameter(ParameterInfo pi) { int indexForArgBuilder; - int nameIndex = _argNames.IndexOf(pi.Name); + int nameIndex = _resolver.AllowByKeywordArgument(Overload, pi) ? _argNames.IndexOf(pi.Name) : -1; if (nameIndex == -1) { // positional argument, we simply consume the next argument indexForArgBuilder = ArgIndex++; diff --git a/Src/Microsoft.Dynamic/Actions/Calls/ParamsDictArgBuilder.cs b/Src/Microsoft.Dynamic/Actions/Calls/ParamsDictArgBuilder.cs index 5897a518..dbab56b0 100644 --- a/Src/Microsoft.Dynamic/Actions/Calls/ParamsDictArgBuilder.cs +++ b/Src/Microsoft.Dynamic/Actions/Calls/ParamsDictArgBuilder.cs @@ -5,6 +5,7 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Diagnostics; using System.Linq.Expressions; using System.Reflection; @@ -38,12 +39,15 @@ public ParamsDictArgBuilder(ParameterInfo info, int argIndex, string[] names, in protected internal override Expression ToExpression(OverloadResolver resolver, RestrictedArguments args, bool[] hasBeenUsed) { Type dictType = ParameterInfo.ParameterType; - // TODO: bug: what if ConstantNames().Length > hasBeenUsed.Count(b => !b)? + var names = ConstantNames(); + var expressions = GetParameters(args, hasBeenUsed); + + Debug.Assert(names.Length == expressions.Count); return Expression.Call( GetCreationDelegate(dictType).GetMethodInfo(), - Expression.NewArrayInit(typeof(string), ConstantNames()), - AstUtils.NewArrayHelper(typeof(object), GetParameters(args, hasBeenUsed)) + Expression.NewArrayInit(typeof(string), names), + AstUtils.NewArrayHelper(typeof(object), expressions) ); } diff --git a/Src/Microsoft.Dynamic/Utils/ReflectionUtils.cs b/Src/Microsoft.Dynamic/Utils/ReflectionUtils.cs index 498fd341..cedebcd1 100644 --- a/Src/Microsoft.Dynamic/Utils/ReflectionUtils.cs +++ b/Src/Microsoft.Dynamic/Utils/ReflectionUtils.cs @@ -1004,6 +1004,13 @@ public static bool IsParamDictionary(this ParameterInfo parameter) { return parameter.IsDefined(typeof(ParamDictionaryAttribute), false); } + // TODO: revisit, make public or eliminate + internal static bool ProhibitsByKeyword(this ParameterInfo parameter) { + // params arrays & dictionaries don't allow assignment by keyword + return parameter.IsParamArray() || parameter.IsParamDictionary(); + // TODO: extend if NonKeywordAttribute introduced + } + public static bool IsParamsMethod(MethodBase method) { return IsParamsMethod(method.GetParameters()); } diff --git a/Src/Microsoft.Scripting/Runtime/ParamDictionaryAttribute.cs b/Src/Microsoft.Scripting/Runtime/ParamDictionaryAttribute.cs index ee386a0c..bae6c30f 100644 --- a/Src/Microsoft.Scripting/Runtime/ParamDictionaryAttribute.cs +++ b/Src/Microsoft.Scripting/Runtime/ParamDictionaryAttribute.cs @@ -6,8 +6,8 @@ namespace Microsoft.Scripting { /// - /// This attribute is used to mark a parameter that can accept any keyword parameters that - /// are not bound to normal arguments. The extra keyword parameters will be + /// This attribute is used to mark a parameter that can accept any keyword arguments that + /// are not bound to normal parameters. The extra keyword arguments will be /// passed in a dictionary which is created for the call. /// /// Most languages which support params dictionaries will support the following types: diff --git a/Tests/ClrAssembly/Src/methodargs.cs b/Tests/ClrAssembly/Src/methodargs.cs index bac7f49b..1fc49661 100644 --- a/Tests/ClrAssembly/Src/methodargs.cs +++ b/Tests/ClrAssembly/Src/methodargs.cs @@ -30,7 +30,9 @@ public class VariousParameters { public void M200(int arg) { Flag.Set(arg); } public void M201([DefaultParameterValue(20)] int arg) { Flag.Set(arg); } public void M202(params int[] arg) { Flag.Set(arg.Length); } - public void M203([ParamDictionaryAttribute] IDictionary arg) { Flag.Set(arg.Count); } + public void M203([ParamDictionary] IDictionary arg) { Flag.Set(arg.Count); } + public void M204(params object[] arg) { Flag.Set(arg.Length); } + public void M205([ParamDictionary] IDictionary arg) { Flag.Set(arg.Count); } // optional (get missing value) // - check the value actually passed in @@ -46,11 +48,11 @@ public class VariousParameters { // two parameters public void M300(int x, int y) { } public void M350(int x, params int[] y) { } - public void M351(int x, [ParamDictionary] IDictionary arg) { Flag.Set(arg); } - public void M352([ParamDictionary] IDictionary arg, params int[] x) { Flag.Set(arg); } + public void M351(int x, [ParamDictionary] IDictionary y) { Flag.Set(y); } + public void M352([ParamDictionary] IDictionary x, params object[] y) { Flag.Set(x); } public void M310(int x, [DefaultParameterValue(30)]int y) { Flag.Set(x + y); } - public void M320([DefaultParameterValue(40)] int y, int x) { Flag.Set(x + y); } + public void M320([DefaultParameterValue(40)] int x, int y) { Flag.Set(x + y); } public void M330([DefaultParameterValue(50)] int x, [DefaultParameterValue(60)] int y) { Flag.Set(x + y); } public void M410(int x, [Optional]int y) { } From 087a0e87091e6734ec0085f5997209fabc2326c3 Mon Sep 17 00:00:00 2001 From: Pavel Koneski Date: Tue, 17 Nov 2020 12:21:47 -0800 Subject: [PATCH 2/2] Include OverloadResolver.AllowByKeywordArgument in externally accessible API --- .../Actions/Calls/DefaultOverloadResolver.cs | 7 +++--- .../Actions/Calls/OverloadResolver.cs | 23 +++++++++++++------ .../Utils/ReflectionUtils.cs | 7 ------ 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/Src/Microsoft.Dynamic/Actions/Calls/DefaultOverloadResolver.cs b/Src/Microsoft.Dynamic/Actions/Calls/DefaultOverloadResolver.cs index 7fffe256..21160e2f 100644 --- a/Src/Microsoft.Dynamic/Actions/Calls/DefaultOverloadResolver.cs +++ b/Src/Microsoft.Dynamic/Actions/Calls/DefaultOverloadResolver.cs @@ -139,9 +139,10 @@ protected override void GetNamedArguments(out IList namedArgs } } - // TODO: review the signature, add protected - internal override bool AllowByKeywordArgument(OverloadInfo method, ParameterInfo parameter) { - return !parameter.ProhibitsByKeyword(); + protected internal override bool AllowByKeywordArgument(OverloadInfo method, ParameterInfo parameter) { + // params arrays & dictionaries don't allow assignment by keyword + return base.AllowByKeywordArgument(method, parameter) + && !parameter.IsParamArray() && !parameter.IsParamDictionary(); } protected override ActualArguments CreateActualArguments(IList namedArgs, IList argNames, int preSplatLimit, int postSplatLimit) { diff --git a/Src/Microsoft.Dynamic/Actions/Calls/OverloadResolver.cs b/Src/Microsoft.Dynamic/Actions/Calls/OverloadResolver.cs index bd1760da..b2b512e6 100644 --- a/Src/Microsoft.Dynamic/Actions/Calls/OverloadResolver.cs +++ b/Src/Microsoft.Dynamic/Actions/Calls/OverloadResolver.cs @@ -158,21 +158,21 @@ public BindingTarget ResolveOverload(string methodName, IList meth /// Checks to see if the language allows named arguments to be bound to instance fields or /// properties and turned into setters. By default this is only allowed on contructors. /// - internal protected virtual bool AllowMemberInitialization(OverloadInfo method) { + protected internal virtual bool AllowMemberInitialization(OverloadInfo method) { #pragma warning disable 618 // obsolete return AllowKeywordArgumentSetting(method.ReflectionInfo); #pragma warning restore 618 } [Obsolete("Use OverloadInfo.AllowMemberInitialization instead")] - internal protected virtual bool AllowKeywordArgumentSetting(MethodBase method) { + protected internal virtual bool AllowKeywordArgumentSetting(MethodBase method) { return CompilerHelpers.IsConstructor(method); } /// /// Gets an expression that evaluates to the result of GetByRefArray operation. /// - internal protected virtual Expression GetByRefArrayExpression(Expression argumentArrayExpression) { + protected internal virtual Expression GetByRefArrayExpression(Expression argumentArrayExpression) { return argumentArrayExpression; } @@ -186,11 +186,20 @@ protected virtual bool BindToUnexpandedParams(MethodCandidate candidate) { /// /// Checks whether the given parameter may be mapped to by a keyword argument. /// + /// + /// If overriden, the derived class may only add more constraints to the constraints from the base class. + /// + /// + /// base.AllowByKeywordArgument(method, parameter) + /// && AdditionalCheckIfByKeywordAllowed(method, parameter); + /// ]]> + /// /// /// - // TODO: review the signature, add protected - internal virtual bool AllowByKeywordArgument(OverloadInfo method, ParameterInfo parameter) { - return true; // legacy behavior, but by default OverloadResolver doesn't recognize named arguments anyway + protected internal virtual bool AllowByKeywordArgument(OverloadInfo method, ParameterInfo parameter) { + return true; // no constraints, but by default OverloadResolver doesn't recognize named arguments anyway } /// @@ -201,7 +210,7 @@ internal virtual bool AllowByKeywordArgument(OverloadInfo method, ParameterInfo /// A default mapping will be constructed for the remaining parameters (cleared bits). /// [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1045:DoNotPassTypesByReference", MessageId = "3#")] - internal protected virtual BitArray MapSpecialParameters(ParameterMapping mapping) { + protected internal virtual BitArray MapSpecialParameters(ParameterMapping mapping) { if (!mapping.Overload.IsStatic) { var type = mapping.Overload.DeclaringType; mapping.AddParameter(new ParameterWrapper(null, type, null, ParameterBindingFlags.ProhibitNull)); diff --git a/Src/Microsoft.Dynamic/Utils/ReflectionUtils.cs b/Src/Microsoft.Dynamic/Utils/ReflectionUtils.cs index cedebcd1..498fd341 100644 --- a/Src/Microsoft.Dynamic/Utils/ReflectionUtils.cs +++ b/Src/Microsoft.Dynamic/Utils/ReflectionUtils.cs @@ -1004,13 +1004,6 @@ public static bool IsParamDictionary(this ParameterInfo parameter) { return parameter.IsDefined(typeof(ParamDictionaryAttribute), false); } - // TODO: revisit, make public or eliminate - internal static bool ProhibitsByKeyword(this ParameterInfo parameter) { - // params arrays & dictionaries don't allow assignment by keyword - return parameter.IsParamArray() || parameter.IsParamDictionary(); - // TODO: extend if NonKeywordAttribute introduced - } - public static bool IsParamsMethod(MethodBase method) { return IsParamsMethod(method.GetParameters()); }