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.

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

[Expression] Add array return type #3697

Merged
merged 6 commits into from
Apr 9, 2020
Merged

[Expression] Add array return type #3697

merged 6 commits into from
Apr 9, 2020

Conversation

Danieladu
Copy link
Collaborator

@Danieladu Danieladu commented Apr 7, 2020

close: #3615

Add Array return type to make array type errors be discovered during the static check period.

For example, before :

var expr = Expression.Parse("skip(1, 1)"); // it's ok
var result = expr.TryEvaluate(null); // throw runtime exception, skip can not applied on int

After:

var expr = Expression.Parse("skip(1, 1)"); // 1 is not a valid array

remark

why does Expression.Parse("skip(1, 1)") not throw exception before.
If the method accepts Object type, e.g. count method accepts one parameter, which could be string or array. If the param is a string, return string length, and if the param is an array, return the array size.
So, we can only mark the param object, any return type here is ok during the static checker period.
Same logic with skip(1, 1), skip accepts an object as the first parameters, and all the input types here are treated as valid types in the static checker.

The root cause is that we treat Object as the base type (or any type). So, extract Array type may help us get array related errors in advance.

Additional feature:

Support mixed return type in the static check.
Before, if the expression needs one param that maybe a string or a number. we can only mark it as Object return type, but now, we can use ReturnType.Number | ReturnType.String to mark it.
This can make type matching more precise.

@boydc2014
Copy link
Contributor

Looks good to me, but i can't tell all cases are covered, can you add test cases in both "Good" and "Bad" expression tests, to make things clear.

@@ -3273,7 +3273,7 @@ private static IEnumerable<object> Flatten(IEnumerable<object> list, int dept)
return (result, error);
},
ReturnType.Number,
(expression) => ValidateArityAndAnyType(expression, 2, 2, ReturnType.String, ReturnType.Boolean, ReturnType.Number, ReturnType.Object)),
ValidateBinary),
Copy link
Contributor

@chrimc62 chrimc62 Apr 7, 2020

Choose a reason for hiding this comment

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

ValidateBinary [](start = 20, length = 14)

Do we need more precision than this, i.e. string | array and object? #Resolved

Copy link
Collaborator Author

@Danieladu Danieladu Apr 8, 2020

Choose a reason for hiding this comment

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

Hi chris, I have modified the ReturnType enum to make it has the ability to combine choices or intersect combinations of choices.
So the above code has been changed to

expr => ValidateOrder(expr, null, ReturnType.Array | ReturnType.String, ReturnType.Object)

Looking forward the comments. #Resolved

@@ -3306,7 +3306,7 @@ private static IEnumerable<object> Flatten(IEnumerable<object> list, int dept)
return (result, error);
},
ReturnType.Number,
(expression) => ValidateArityAndAnyType(expression, 2, 2, ReturnType.String, ReturnType.Boolean, ReturnType.Number, ReturnType.Object)),
ValidateBinary),
Copy link
Contributor

@chrimc62 chrimc62 Apr 7, 2020

Choose a reason for hiding this comment

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

ValidateBinary [](start = 20, length = 14)

Same here. #Resolved

Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

🕐

@Danieladu Danieladu requested a review from chrimc62 April 8, 2020 05:58
@chrimc62
Copy link
Contributor

chrimc62 commented Apr 8, 2020

public enum ReturnType

Should tag with flags, i.e. https://docs.microsoft.com/en-us/dotnet/api/system.flagsattribute?view=netframework-4.8


Refers to: libraries/AdaptiveExpressions/Expression.cs:21 in 66e3e1e. [](commit_id = 66e3e1e, deletion_comment = False)

@chrimc62
Copy link
Contributor

chrimc62 commented Apr 8, 2020

Indentation.


Refers to: libraries/AdaptiveExpressions/ExpressionFunctions.cs:3049 in 66e3e1e. [](commit_id = 66e3e1e, deletion_comment = False)

@@ -3197,7 +3210,7 @@ private static IEnumerable<object> Flatten(IEnumerable<object> list, int dept)
new ExpressionEvaluator(
ExpressionType.AddOrdinal,
Apply(args => AddOrdinal(Convert.ToInt32(args[0])), VerifyInteger),
ReturnType.Number,
ReturnType.String,
Copy link
Contributor

@chrimc62 chrimc62 Apr 8, 2020

Choose a reason for hiding this comment

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

String [](start = 31, length = 6)

Good catch. #Resolved

Copy link
Contributor

@chrimc62 chrimc62 left a comment

Choose a reason for hiding this comment

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

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expressions ReturnType and validations should include Array
4 participants