There are a number of issues with the DLR overload resolver, which I am looking into now, but the code is so wonderfully complex that I prefer to start with something simple, to build confidence. Here is such change: it is not functional (it doesn't change how the resolving happens), it merely makes the error messages more specific in case of duplicate arguments.
Currently, for duplicate arguments, the message is "MethodName() got multiple values for keyword argument 'arg'". To me it suggests that a keyword argument arg is duplicated, but it can also be reported when an argument is provided by position and keyword, which is also a clash.
To be more accurate, positional/keyword clash is not handled well at all, sometimes resulting in strange messages as "MethodName() takes at least 2 arguments (6 given)", IndexOutOfRangeException, or simply no error but incorrect behaviour. Looking into the code, it seems to me that handling all those cases is not quite finished. But in this PR, I only want to establish what the appropiate error message should be in such cases, and than later build on it when resolving the actual problems.
For reference, Python (CPython) will issue "funcname() got multiple values for keyword argument 'arg'" only when there is a keyword/keyword clash. For positional/keyword clash, CPython uses "funcname() got multiple values for argument 'arg'" (for user defined functions) and "argument for funcname() given by name ('arg') and position (n)" (for built-in functions). Not that DLR has to follow Python, but lacking better ideas I borrowed the builtin message from Python here.
Some notes about the implementation choices: the reason for a specific method binding failure is being carried around in an object of type CallFailure. This class has a property Reason of enum type CallFailureReason. I was tempted to add a new constant to that enum, say, DuplicateArgument, but it is in public API, so worth some consideration. On one hand, .NET Core guidelines allow it, but what would clients do when they read a value that falls outside the expected enum range? Also, reading comments on the DuplicateKeyword contant, it seems that the original intent was to cover both cases with the same enum. So I settled on a change that adds an extra property enabling to differentiate the two cases further down the road. Besides, the extra property carries useful information for error reporting. (Although, looking ahead, I see opportunity/need to add more enum values for some new cases). The type of the property could be better IReadOnlyList<int>, but that is also true for the other like-like properties, so I stayed with List<int> for consistency.
Arguably, the error message could have simply been "MethodName() got multiple values for argument 'arg'", but since the information abouth the positional argument is there, why not show it to the user. It will either help the user to diagnose their problem (if the reported position correct), or help us to spot bugs in the DLR (if incorrect).
This brings me to the last point: about the meaning of the position. As I see it, it can be interpreted as either a position of the offending argument that is given without a name, or a position of the parameter, which is being claimed by a positional argument and a keyword argument. Typically they are the same, but not always. Personally, as a user, I would expect the position of the argument being reported, but the DLR reports the position of the parameter. Also, interestingly, CPython reports the position of the parameter, so I assume this is fine. Here is an example:
Python 3.4.4 (v3.4.4:737efcadf5a6, Dec 20 2015, 20:20:57) [MSC v.1600 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import datetime
>>> datetime.date(year=2020, *(11, 2))
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
TypeError: Argument given by name ('year') and position (1)
There are a number of issues with the DLR overload resolver, which I am looking into now, but the code is so wonderfully complex that I prefer to start with something simple, to build confidence. Here is such change: it is not functional (it doesn't change how the resolving happens), it merely makes the error messages more specific in case of duplicate arguments.
Currently, for duplicate arguments, the message is "MethodName() got multiple values for keyword argument 'arg'". To me it suggests that a keyword argument arg is duplicated, but it can also be reported when an argument is provided by position and keyword, which is also a clash.
To be more accurate, positional/keyword clash is not handled well at all, sometimes resulting in strange messages as "MethodName() takes at least 2 arguments (6 given)",
IndexOutOfRangeException, or simply no error but incorrect behaviour. Looking into the code, it seems to me that handling all those cases is not quite finished. But in this PR, I only want to establish what the appropiate error message should be in such cases, and than later build on it when resolving the actual problems.For reference, Python (CPython) will issue "funcname() got multiple values for keyword argument 'arg'" only when there is a keyword/keyword clash. For positional/keyword clash, CPython uses "funcname() got multiple values for argument 'arg'" (for user defined functions) and "argument for funcname() given by name ('arg') and position (n)" (for built-in functions). Not that DLR has to follow Python, but lacking better ideas I borrowed the builtin message from Python here.
Some notes about the implementation choices: the reason for a specific method binding failure is being carried around in an object of type
CallFailure. This class has a propertyReasonof enum typeCallFailureReason. I was tempted to add a new constant to that enum, say,DuplicateArgument, but it is in public API, so worth some consideration. On one hand, .NET Core guidelines allow it, but what would clients do when they read a value that falls outside the expected enum range? Also, reading comments on theDuplicateKeywordcontant, it seems that the original intent was to cover both cases with the same enum. So I settled on a change that adds an extra property enabling to differentiate the two cases further down the road. Besides, the extra property carries useful information for error reporting. (Although, looking ahead, I see opportunity/need to add more enum values for some new cases). The type of the property could be betterIReadOnlyList<int>, but that is also true for the other like-like properties, so I stayed withList<int>for consistency.Arguably, the error message could have simply been "MethodName() got multiple values for argument 'arg'", but since the information abouth the positional argument is there, why not show it to the user. It will either help the user to diagnose their problem (if the reported position correct), or help us to spot bugs in the DLR (if incorrect).
This brings me to the last point: about the meaning of the position. As I see it, it can be interpreted as either a position of the offending argument that is given without a name, or a position of the parameter, which is being claimed by a positional argument and a keyword argument. Typically they are the same, but not always. Personally, as a user, I would expect the position of the argument being reported, but the DLR reports the position of the parameter. Also, interestingly, CPython reports the position of the parameter, so I assume this is fine. Here is an example: