work on #77

[lofcz]

Do not merge

@codingseb I've got first version of #77 working without introducing any breaking changes (try toggling static ExpressionEvaluator.OptionReturnOnErrorInsteadOfThrow) but it required a lot of internal reworking (most void methods now return object instead). This is based on commit 179ca99 in your branch MakeScriptEvaluationMoreFlexible. Please take a look and if possible, rebase your branch on this. I need to work on this more (add useful error messages, clear up new enum ExpressionEvaluatorErrors..) but if you'd keep working in your current set we would be more apart with each commit.

When ExpressionEvaluator.OptionReturnOnErrorInsteadOfThrow is turned on, instead of throwing on error, script gracefully returns an instance of ExpressionEvaluatorError.

I've also bumped c# from 7.3 to 8.0 via raising netstandard from 2.0 to 2.1, as I'm using some c# 8.0 features, removed unnecessary else statements and fixed some typos where I was sure about them.

Ps: I love the way you detect dupe variable declarations and var + type / dynamic + type declarations, because we can easily offer a toggle to auto recover from these. Eg:

int a = 1;
int a = 2; // normally this would be an error, but we can can offer a switch to make this valid (a = 2)

Edit2: some tests are failing now, I'm looking into why.

[codingseb]

// normally this would be an error, but we can can offer a switch to make this valid (a = 2)

👍🏻 Good idea

[lofcz]

@codingseb what do you think about this PR? There is definitely some overhead introduced by boxing into objects but by using c# 7 pattern if (X is MyType as MyInst) unboxing overhead is reduced. Do you think this is the way to go or should we bubble every exception to stack and return / throw only on top?

[codingseb]

Hye @lofcz.
Firstly I like the possibility to return Errors objects in main methods Evaluate and ScriptEvaluate

For me the principle of unboxing things is not so annoying but the fact that the user who will customize some stuff have to return different things as object is more annoying for me. Because the meaning of the returned value of methods (such as operators, parsing methods..) become unclear.

I was wondering if modifying a bit signatures of these methods with some kind of encapsulating objects given as argument would be clearer for the user. (Creating something like the current VariableEvaluationEventArg type given in events).
Yes I know it require some refactoring and it will introduce some breaking changes but as the next version for script customization will be a major version we can include this in it and signal a warning on the release note of this version. It will be clearer and more flexible after this. It will also allow to switch off the static of the option.

Some examples here :

// We need to add this
class OperatorEvaluationArgs
{
    public OperatorEvaluationArgs(ExpressionEvaluator evaluator, dynamic left, dynamic right)
    {
        Left = left;
        Right = right;
        Evaluator = evaluator;
    }

    public dynamic Left { get; }
    public dynamic Right { get; }
    public ExpressionEvaluator Evaluator { get; }
    public ExpressionEvaluatorError Error { get; set; }
}

// And so operator dictionaries will become something like :
protected static readonly IList<IDictionary<ExpressionOperator, Func<OperatorEvaluationArgs, object>>> operatorsEvaluations =
            new List<IDictionary<ExpressionOperator, Func<OperatorEvaluationArgs, object>>>()
{
    // ...
    new Dictionary<ExpressionOperator, Func<OperatorEvaluationArgs, object>>()
    {
        {
            ExpressionOperator.ConditionalAnd, (OperatorEvaluationArgs args) => 
            {
                if ( args.Left is BubbleExceptionContainer leftExceptionContainer)
                {
                    if (args.Evaluator.OptionReturnOnErrorInsteadOfThrow)
                    {
                        args.Error = new ExpressionEvaluatorError {Error = ExpressionEvaluatorErrors.AndLeft, Message = leftExceptionContainer.Exception.Message};
                        return null;
                    }
                    
                    throw leftExceptionContainer.Exception;
                }
                if (!args.Left)
                {
                    return false;
                }
                if (args.Right is BubbleExceptionContainer rightExceptionContainer)
                {
                    if (args.Evaluator.OptionReturnOnErrorInsteadOfThrow)
                    {
                        args.Error = new ExpressionEvaluatorError {Error = ExpressionEvaluatorErrors.AndRight, Message = rightExceptionContainer.Exception.Message};
                        return null;
                    }
                    
                    throw rightExceptionContainer.Exception;
                }

                return args.Left && args.Right;
            } 
        },
     // ...
}

// We also need to add this
class ParsingMethodArgs
{
    public ParsingMethodArgs(ExpressionEvaluator Evaluator, string expression, Stack<object> stack, int currentIndex)
    {
        Evaluator = evaluator;
        Expression = expression;
        Stack = stack;
        Index = currentIndex;
    }

    public string Expression { get; }
    public dynamic Right { get; }
    public Stack<object> Stack { get; }
    public int Index { get; set; }
    public ExpressionEvaluatorError Error { get; set; }
}

// So ParsingMethodDelegate become :
protected delegate bool ParsingMethodDelegate(ParsingMethodArgs args);

// Evaluate become :
public object Evaluate(string expression)
{
    expression = expression.Trim();

    Stack<object> stack = new Stack<object>();
    evaluationStackCount++;
    try
    {
        if (GetLambdaExpression(expression, stack))
            return stack.Pop();

        for (int i = 0; i < expression.Length; i++)
        {
            ParsingMethodArgs args = new ParsingMethodArgs(this, expression, stack, i );
            if (!ParsingMethods.Any(parsingMethod => parsingMethod(args) && args.Error == null)
            {
                i = args.Index;
                if(args.Error != null)
                    return args.Error;
                
                string s = expression.Substring(i, 1);

                if (!s.Trim().Equals(string.Empty))
                {
                    throw new ExpressionEvaluatorSyntaxErrorException($"Invalid character [{(int)s[0]}:{s}]");
                }
            }
        }

        return ProcessStack(stack);
    }
    finally
    {
        evaluationStackCount--;
    }
}

// And the begining of EvaluateVarOrFunc for example :
protected virtual object EvaluateVarOrFunc(ParsingMethodArgs args)
{
    Match varFuncMatch = varOrFunctionRegEx.Match(args.Expression.Substring(args.Index));

    if (varFuncMatch.Groups["varKeyword"].Success && !varFuncMatch.Groups["assignationOperator"].Success)
    {
        if (OptionReturnOnErrorInsteadOfThrow)
        {
            args.Error = new ExpressionEvaluatorError {Error = ExpressionEvaluatorErrors.UndefinedVariable, Message = "Implicit variables must be initialized. [var " + varFuncMatch.Groups["name"].Value + "]"};
            return null;
        }
        
        throw new ExpressionEvaluatorSyntaxErrorException("Implicit variables must be initialized. [var " + varFuncMatch.Groups["name"].Value + "]");
    }

    // ...
}

What do you think ?

Also 2 more remarks :

1. I tried to rebase or merge on an local branch based on MakeScriptEvaluationMoreFlexible this PR after resolving conflicts it result in some compile Errors and if I quickly correct them I have a lot of tests that fail after that. So I will wait to have something more stable before rebasing or merging this PR.
2. Passing EE to version 8 of C# will force the user that just copy the file ExpressionEvaluator.cs to convert it's csproj also in version 8. And keeping it at version netstandard2.0 keep compatibility with older project. So as for now the use of version 8 of C# in EE would be mainly for syntaxic sugar and we can easily write things an other way I prefer to keep EE as compatible as possible with older projects.

[lofcz]

Approach you are proposing makes more sense, I will rebase and reimplement this PR in this way.