Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change error message when throw when same functions are registered #229

Closed
wants to merge 1 commit into from
Closed

Change error message when throw when same functions are registered #229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 3 additions & 3 deletions src/DynamicExpresso.Core/Identifier.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ public Identifier(string name, Expression expression)
}
}

internal class FunctionIdentifier : Identifier
public class FunctionIdentifier : Identifier
sagilio marked this conversation as resolved.
Show resolved Hide resolved
{
internal FunctionIdentifier(string name, Delegate value) : base(name, new MethodGroupExpression(value))
public FunctionIdentifier(string name, Delegate value) : base(name, new MethodGroupExpression(value))
{
}

internal void AddOverload(Delegate overload)
public void AddOverload(Delegate overload)
{
((MethodGroupExpression)Expression).AddOverload(overload);
}
Expand Down
14 changes: 7 additions & 7 deletions src/DynamicExpresso.Core/Parsing/Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ public bool IsShiftOperator(out ExpressionType shiftType)
// << could be a token, but is not for symmetry
else if (_token.id == TokenId.LessThan && _parseChar == '<')
{
NextToken(); // consume next <
NextToken(); // consume next <
shiftType = ExpressionType.LeftShift;
return true;
}
Expand Down Expand Up @@ -1019,7 +1019,7 @@ private Expression ParseIdentifier()
{
return ParseTypeKeyword(knownType);
}

// Working context implementation
//if (it != null)
// return ParseMemberAccess(null, it);
Expand Down Expand Up @@ -1228,7 +1228,7 @@ private Expression ParseMethodGroupInvocation(MethodGroupExpression methodGroup,
applicableMethods = FindBestMethod(candidates.Select(_ => _.Method), args);

if (applicableMethods.Length == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Given the issue description I thought that we should throw AmbiguousDelegateInvocation when applicableMethods.Length > 1 . Why we throw when == 0?
Maybe I missing something .

@metoule maybe you can help us ;-)

Copy link
Contributor

@metoule metoule Feb 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

That's precisely the issue I highlighted, and why it's actually quite difficult to fix properly :D
The issue resides in the FindBestMethod method. When there are multiple applicable overloads, they're filtered thanks to the MethodHasPriority method, which can also result in an empty array if no overload has the priority:

private static MethodData[] FindBestMethod(IEnumerable<MethodData> methods, Expression[] args)
{
  // empty array if there are no applicable methods
  var applicable = methods
    .Where(m => CheckIfMethodIsApplicableAndPrepareIt(m, args))
    .ToArray();
  if (applicable.Length > 1)
  {
    // also returns an empty array if no overload has priority (ie. if there are multiple applicable methods)!
    return applicable
      .Where(m => applicable.All(n => m == n || MethodHasPriority(args, m, n)))
      .ToArray();
  }

  return applicable;
}

The issue is that the caller of FindBestMethod doesn't know if the resulting array is empty because no function was found, or if multiple applicable overloads were found.

We might be able to fix the issue like this, but it would require checking all calls to FindBestMethod to be sure:

private static MethodData[] FindBestMethod(IEnumerable<MethodData> methods, Expression[] args)
{
  // empty array if there are no applicable methods
  var applicable = methods
    .Where(m => CheckIfMethodIsApplicableAndPrepareIt(m, args))
    .ToArray();
  if (applicable.Length > 1)
  {
    var bestCandidates = applicable
      .Where(m => applicable.All(n => m == n || MethodHasPriority(args, m, n)))
      .ToArray();
    if (bestCandidates.length > 0)
      return bestCandidates; // should always have a length of 1 
  }

  return applicable;
}

Copy link
Member

Choose a reason for hiding this comment

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

I like it. Thank you for the explanation. @sagilio Do you think you can review the code to follow this suggestion?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the explanation, I already understand why to return 0 length array here. I think #231 has fixed this well, and will not have much performance impact when the overloads count is not very large.

throw CreateParseException(errorPos, ErrorMessages.ArgsIncompatibleWithDelegate);
throw CreateParseException(errorPos, ErrorMessages.AmbiguousDelegateInvocation);

if (applicableMethods.Length > 1)
throw CreateParseException(errorPos, ErrorMessages.AmbiguousDelegateInvocation);
Expand All @@ -1250,7 +1250,7 @@ private Type ParseKnownType()

private bool TryParseKnownType(string name, out Type type)
{
// if the type is unknown, we need to restart parsing
// if the type is unknown, we need to restart parsing
var originalPos = _token.pos;
_arguments.TryGetKnownType(name, out type);

Expand Down Expand Up @@ -2362,7 +2362,7 @@ private static bool IsWritable(Expression expression)
}
case ExpressionType.Parameter:
return true;

}

return false;
Expand Down Expand Up @@ -2497,7 +2497,7 @@ private Expression GenerateGreaterThan(Expression left, Expression right)
return GenerateBinary(ExpressionType.GreaterThan, left, right);
}



private Expression GenerateGreaterThanEqual(Expression left, Expression right)
{
Expand Down Expand Up @@ -3090,7 +3090,7 @@ private static Expression GenerateNullableTypeConversion(Expression expr)
}

/// <summary>
/// Expression that wraps over an interpreter. This is used when parsing a lambda expression
/// Expression that wraps over an interpreter. This is used when parsing a lambda expression
/// definition, because we don't know the parameters type before resolution.
/// </summary>
private class InterpreterExpression : Expression
Expand Down
32 changes: 30 additions & 2 deletions test/DynamicExpresso.UnitTest/GithubIssues.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,11 +221,40 @@ static bool GetGFunction2(string arg = null)
Assert.Throws<ParseException>(() => interpreter.Eval("GFunction(arg)"));

// there should be an ambiguous call exception, but GFunction1 is used
// because gFunc1.Method.GetParameters()[0].HasDefaultValue == true
// because gFunc1.Method.GetParameters()[0].HasDefaultValue == true
// and gFunc2.Method.GetParameters()[0].HasDefaultValue == false
Assert.False((bool)interpreter.Eval("GFunction()"));
}

[Test]
public void Github_Issue_159()
{
// GetGFunction2 is defined inside the test function
static bool GetGFunction2(string arg = null)
{
return arg == null;
}

GFunction gFunc1 = GetGFunction1;
GFunction gFunc2 = GetGFunction2;

var interpreter = new Interpreter();
interpreter.SetFunction("GFunction", gFunc1);
interpreter.SetFunction("GFunction", gFunc2);
interpreter.SetVariable("arg", "arg");

// ambiguous call
var exception = Assert.Throws<ParseException>(() => interpreter.Eval("GFunction(arg)"));
davideicardi marked this conversation as resolved.
Show resolved Hide resolved
Assert.AreEqual("Ambiguous invocation of delegate (multiple overloads found) (at index 0).", exception.Message);

interpreter = new Interpreter();
interpreter.SetIdentifier(new FunctionIdentifier("GFunction", gFunc1));
interpreter.SetIdentifier(new FunctionIdentifier("GFunction", gFunc2));
interpreter.SetVariable("arg", "arg");

// normal call
Assert.False((bool)interpreter.Eval("GFunction(arg)"));
}
#endif

[Test]
Expand Down Expand Up @@ -497,7 +526,6 @@ public void GitHub_Issue_221_Reflection_not_allowed()
Assert.Throws<ReflectionNotAllowedException>(() => interpreter.Parse("list.SelectMany(t => t.GetMethods())"));
}


public static class Utils
{
public static List<T> Array<T>(IEnumerable<T> collection) => new List<T>(collection);
Expand Down