support of or-functions #18

eric-therond · 2023-05-14T16:10:24Z

Probably not a clean code, I am learning both rust and rego languages ...

I tried to add support of mutiple definitions of a function

eric-therond · 2023-05-14T19:39:20Z

@microsoft-github-policy-service agree

anakrish · 2023-05-27T01:44:18Z

@eric-therond Thanks for the contribution. Can you sign off you commit?

anakrish · 2023-05-27T01:44:58Z

The PR looks good to me. Are there other tests that you can think of?

eric-therond · 2023-05-27T19:08:45Z

@anakrish I signed-off the commit
I'am ok with the tests for now

mingweishih · 2023-05-30T16:24:56Z

src/interpreter.rs

-            let a = match a {
-                Expr::Var(s) => s.text(),
-                _ => unimplemented!("destructuring function arguments"),
+        let mut results: Vec<Result<Value>> = Vec::new();


Can the type just be Vec<Value>?

It makes sense to me, thanks

mingweishih · 2023-05-30T16:26:26Z

src/interpreter.rs

+            self.scopes.push(args_scope);
+            let value = self.eval_rule_bodies(ctx, span, bodies)?;
+            let result = match &value {
+                Value::Set(s) if s.len() == 1 => Ok(s.iter().next().unwrap().clone()),


If the results is Vec<Value>, you can remove Ok(...) wrapping here and below.

mingweishih · 2023-05-30T16:55:44Z

src/interpreter.rs

+            results.push(result);
+        }
+
+        if let Ok(value0) = &results[0] {


Assuming results is Vec<Value>, you could do something like:

if results.is_empty() { bail!("internal error"); } if results.iter().any(|value| value != results[0]) { return Err(span.source.error( span.line, span.col, "functions must not produce multiple outputs for same inputs", )); } Ok(results[0])

anakrish

LGTM.

Thanks for the contribution.

anakrish · 2023-05-30T17:13:48Z

@eric-therond Kindly address suggestions from @mingweishih

anakrish · 2023-05-31T15:44:14Z

tests/interpreter/cases/call/or.yaml

+      - |
+        package test
+
+        inc(x) = x + 1


We also need to add another test case for cases where one of the function definitions evaluates to undefined:

inc(x) = x + 1 inc(x) = x + 1 inc(x) = y { x > 10 # This will evaluate to false. y = 100 # y will be undefined. } a1 = inc(5)

The logic needs to be fixed to handle this. Undefined definitions are ignored.

anakrish · 2023-05-31T15:46:54Z

tests/interpreter/cases/call/or.yaml

+        a1 = inc(2)
+    query: data.test
+    want_result:
+      a1: true


The above example produces eval_conflict_error: functions must not produce multiple outputs for same inputs in Rego playground.

anakrish · 2023-05-31T15:51:44Z

src/interpreter.rs

+            // Restore local variables for current context.
+            self.scopes = scopes;
+
+            // If the function successfully executed and returned true, not necessary to execute the other


This assumption may not be correct. It seems that Rego requires all the definitions to be executed even if one definition returned true.

You're absolutely right, I think I mixed up two different rego objects: "incremental rules" and "multiple function definitions" (I told you I was new to rego !).

To my understanding, rules with the same name should be executed one after another until true is returned, but for functions, as you said, all functions with the same name should be executed, and the return value should be the same, regardless of the value set (defined as in your example).

I will fix that tomorrow

src/interpreter.rs

anakrish · 2023-06-01T17:56:13Z

src/interpreter.rs

-                )
-                .as_str(),
-            ));
+        let mut fcns: Vec<&Rule> = Vec::new();


We don't need this copying to fcns anymore.

I have a borrowing error without the copy that I am not able to fix by myself

Ok. The compiler thinks that fcns_rules could be modified by the subsequent code.

You can simplify the code using the clone method.

Suggested change

let mut fcns: Vec<&Rule> = Vec::new();

let fcns = fcns_rules.clone();

anakrish · 2023-06-01T17:59:04Z

src/interpreter.rs

+
+        if results
+            .iter()
+            .any(|value| *value != results[0] && *value != Value::Undefined)


If the first result is Undefined, then this logic won't work.

anakrish · 2023-06-01T18:00:06Z

tests/interpreter/cases/call/or.yaml

+    query: data.test
+    want_result:
+      a1: 6
+


Add test cases for the following scenarios:
1, First definition produces undefined value.
2. All definitions produce undefined value.

Confirm expected behavior in Rego playground.

I changed a bit my approach to cover all the cases

anakrish

Do think about:

How we can simplify the logic and possibly avoid extra copying
Any more test cases that will lock down the logic.

anakrish · 2023-06-05T18:53:27Z

src/interpreter.rs

+                Value::Set(s) if s.is_empty() && output_expr.is_none() => Value::Bool(true),
+
+                // If the function execution resulted in undefined, then propagate it.
+                Value::Undefined => Value::Undefined,


We could have a strategy where we don't gather Undefined values. In that case,

If results is empty, that means all the rules evaluated to Undefined.

If results is not empty, we need to check that all the values are the same.

anakrish · 2023-06-05T18:54:02Z

src/interpreter.rs

+            0 => bail!("internal error"),
+            1 => Ok(results[0].clone()),
+            _ => {
+                let mut defined_results = results.clone();


We can simplify the logic by avoiding pushing Undefined values altogether.

anakrish · 2023-06-05T18:54:36Z

tests/interpreter/cases/call/or.yaml

+      a1: 6
+
+
+  - note: or-all-undefined


Good tests.

anakrish

@eric-therond Please squash your commits.

anakrish · 2023-06-08T21:03:58Z

tests/interpreter/cases/call/or.yaml

+
+        a1 = inc(5)
+    query: data.test
+    want_result: {}


There is one more case to handle:

fcn(x) = y { y = x + 1 } fcn(x) = y { y = concat(" ", ["hello", x]) } a1 = fcn("world") a2 = fcn(5)

This produces:

{ "a1": "hello world", "a2": 6 }

The definitions that produce an error are ignored. If all definitions produce errors, then one of the errors is propagated.

I added the test case without being able to fix it in this PR.
I left a comment to not forget about it.

Unless I missed a simple solution, there is a lot of code to rewrite to support that ? especially how errors are handled.

For minimal changes, we could slightly tweak your current logic:

Also push errors into a vector errors within the loop.

Then

If results is not empty then ignore errors and return the result after asserting that all the values are same.

If results is empty, then

If errors is not empty, then return the first error.

If errors is empty, then return Undefined.

I didn't know that errors are propagated like that in Rust ... thanks for the hint, it's super clear.

I still had a problem, the test case or-one-error returns

{ "a1": [ "hello world" ], "a2": [ 6 ] }

but in rego playground it is:

{ "a1": "hello world", "a2": 6 }

I didn't investigate why this difference.

Thanks for pointing it out. I will take a look at it.

Signed-off-by: eric-therond <[email protected]>

anakrish · 2023-06-12T17:43:02Z

src/interpreter.rs

-        let scopes = std::mem::take(&mut self.scopes);
+            // Set the arguments scope.
+            self.scopes.push(args_scope);
+            let value = self.eval_rule_bodies(ctx, span, bodies)?;


You could do something like this to accumulate errors

Suggested change

let value = self.eval_rule_bodies(ctx, span, bodies)?;

let value = match self.eval_rule_bodies(ctx, span, bodies) {

Ok(v) => v

Err(e) => {

// If the rule produces an error, save the error.

errors.push(Err(e));

continue;

}

};

anakrish · 2023-06-12T17:43:40Z

src/interpreter.rs

-                .as_str(),
-            ));
-        }
+        let mut results: Vec<Value> = Vec::new();


Suggested change

let mut results: Vec<Value> = Vec::new();

let mut results = Vec::new();

let mut errors = Vec::new();

anakrish · 2023-06-12T17:44:38Z

src/interpreter.rs

-            Value::Set(s) if s.len() == 1 => Ok(s.iter().next().unwrap().clone()),
-            Value::Set(s) if !s.is_empty() => Err(span.source.error(
+        if results.is_empty() {
+            return Ok(Value::Undefined);


Suggested change

return Ok(Value::Undefined);

if errors.is_empty() {

return Ok(Value::Undefined);

} else {

return errors[0];

}

Signed-off-by: eric-therond <[email protected]>

anakrish · 2023-06-14T16:21:07Z

@eric-therond Can you squash you commits?

I will then merge the PR and then investigate fixing the return values in the case you pointed out.

Thanks for your contribution!

anakrish requested a review from mingweishih May 27, 2023 01:45

eric-therond force-pushed the orfunctions branch from c5cecb7 to 8758266 Compare May 27, 2023 18:49

mingweishih reviewed May 30, 2023

View reviewed changes

anakrish approved these changes May 30, 2023

View reviewed changes

anakrish reviewed May 31, 2023

View reviewed changes

anakrish reviewed Jun 1, 2023

View reviewed changes

anakrish reviewed Jun 5, 2023

View reviewed changes

anakrish reviewed Jun 8, 2023

View reviewed changes

support of multiple definitions of the same function

db3d5f7

Signed-off-by: eric-therond <[email protected]>

eric-therond force-pushed the orfunctions branch from ae7bd01 to db3d5f7 Compare June 12, 2023 16:10

anakrish reviewed Jun 12, 2023

View reviewed changes

handle functions producing an error

154557d

Signed-off-by: eric-therond <[email protected]>

anakrish merged commit 910ef32 into microsoft:main Jun 16, 2023

	let mut fcns: Vec<&Rule> = Vec::new();
	let fcns = fcns_rules.clone();

-            let value = self.eval_rule_bodies(ctx, span, bodies)?;
+            let value = match self.eval_rule_bodies(ctx, span, bodies) {
+               Ok(v) => v
+               Err(e) => {
+                    // If the rule produces an error, save the error.
+                    errors.push(Err(e));
+                    continue;
+               }
+            };

	let mut results: Vec<Value> = Vec::new();
	let mut results = Vec::new();
	let mut errors = Vec::new();

-            return Ok(Value::Undefined);
+           if errors.is_empty() {
+               return Ok(Value::Undefined);
+           } else {
+               return errors[0];
+           }

support of or-functions #18

support of or-functions #18

Conversation

eric-therond commented May 14, 2023

eric-therond commented May 14, 2023

anakrish commented May 27, 2023

anakrish commented May 27, 2023

eric-therond commented May 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anakrish left a comment • edited Loading

Choose a reason for hiding this comment

anakrish commented May 30, 2023

anakrish May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-therond May 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anakrish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anakrish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anakrish commented Jun 14, 2023

anakrish left a comment •

edited

Loading

anakrish May 31, 2023 •

edited

Loading

eric-therond May 31, 2023 •

edited

Loading