Validate Rule graph #493

ketkarameya · 2023-06-04T19:21:30Z

This PR introduces very basic validation for our RuleGraphs. At this moment we make sure that (i) All tree-sitter queries are syntactically correct (ii) Filter fields are configured correctly (this is was something we previously checked)

This PR also ensures that the the graph is validated irrespective of where it is loaded from - toml, Rust API, or Python API (Have added test for each API)

ketkarameya

self reviewed

ketkarameya · 2023-06-05T14:09:01Z

src/cleanup_rules/kt/rules.toml

@@ -197,7 +197,7 @@ query = """
 (
 (prefix_expression (boolean_literal) @exp) @prefix_expression
 (#eq? @exp "false")
-(#match @prefix_expression "!.*")  
+(#match? @prefix_expression "!.*")  


Found this bug (and the one below) :)

ketkarameya · 2023-06-05T14:09:37Z

src/lib.rs

    let piranha_args = &self.piranha_arguments;
-    parser


un related to this PR :| , but I observed that i was not using the piranha_args.language() to get the parser

ketkarameya · 2023-06-05T14:11:20Z

src/models/filter.rs

+    }
+
+    if *self.not_contains() != default_not_contains_queries() {
+      self.not_contains().iter().try_for_each(|x| x.validate())?


Such ergonomic APIs make me like Rust :) (try_for_each)

ketkarameya

self reviewed

ketkarameya · 2023-06-05T14:14:52Z

src/models/mod.rs

@@ -24,3 +24,7 @@ pub(crate) mod rule_graph;
 pub(crate) mod rule_store;
 pub(crate) mod scopes;
 pub(crate) mod source_code_unit;
+
+pub(crate) trait Validator {
+  fn validate(&self) -> Result<(), String>;


as our validation enriches, I will modify this Type parameter

lazaroclapp · 2023-06-05T18:05:16Z

src/utilities/tree_sitter_utilities.rs

+  let mut parser = Parser::new();
+  parser
+    .set_language(tree_sitter_query::language())
+    .expect("Could not set the language for the parser.");


I don't understand what's going on here. Are we creating a new parser and expecting it to be set to an invalid language? Why? (This at the very least needs documentation)

aah maybe its just bad api naming in rustlang.
We are setting a language, and if the setter throws an exception we panic with "Could not set the language for the parser."

.expect() ~= .orElseThrow() in Java

Got it. Also, a bit surprised that tree_sitter_query doesn't need any import. For a second I thought this was of the same type piranha_args.language(), i.e. PiranhaLanguage, and was wondering why this wasn't simply tree_sitter_query::language().parser(), but I see the difference now. I still think this all merits a short comment here explaining what this code does (uses tree sitter to construct a parser for its own DSL without all the extensions we have made to Piranha-supported language, correct?)

aah i see the the source of confusion.
Yes u r correct, we are using ts to parse its own dsl.
I will address this as a comment in the code.

Ya it works with out import because we are referring to the crate directly

lazaroclapp · 2023-06-05T18:07:25Z

src/utilities/tree_sitter_utilities.rs

+      .parse(self.get_query(), None)
+      .filter(|x| number_of_errors(&x.root_node()) == 0)
+      .map(|_| Ok(()))
+      .unwrap_or(Err(format!("Cannot parse - {}", self.get_query())))


Do we report what exactly we failed to parse? i.e. for the missing parenthesis "Unexpected end of query string, unmatched ( at position x:y" or similar?

At this moment we do not report why the query is incorrect (or any diagnostics around it)
I will add diagnostics for this as a follow up. I think it just reporting error does not make sense, without diagnostics. Tracking this issue #494

lazaroclapp · 2023-06-05T18:11:46Z

src/models/rule_graph.rs

+  fn validate(&self) -> Result<(), String> {
+    match self.rules().iter().try_for_each(|rule| rule.validate()) {
+      Ok(()) => Ok(()),
+      Err(e) => Err(format!("Incorrect Rule Graph - {}", e)),


Question: Do we report the first failing rule or all syntax errors in every rule in the rule graph? (Neither would be right or wrong, specially at this point, just want to make sure. I'd think it's the first case looking at this, but nor sure what the semantics of try_for_each are.

We are short-circuiting at the first exception.
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.try_for_each

Thinking about it, I should eventually accumulate these errors and report them.
Expecting someone to fix them One-by-one is cruel

ketkarameya

Replies inline

ketkarameya · 2023-06-05T19:08:54Z

src/utilities/tree_sitter_utilities.rs

+  let mut parser = Parser::new();
+  parser
+    .set_language(tree_sitter_query::language())
+    .expect("Could not set the language for the parser.");


aah maybe its just bad api naming in rustlang.
We are setting a language, and if the setter throws an exception we panic with "Could not set the language for the parser."

.expect() ~= .orElseThrow() in Java

ketkarameya · 2023-06-05T19:11:38Z

src/utilities/tree_sitter_utilities.rs

+      .parse(self.get_query(), None)
+      .filter(|x| number_of_errors(&x.root_node()) == 0)
+      .map(|_| Ok(()))
+      .unwrap_or(Err(format!("Cannot parse - {}", self.get_query())))


At this moment we do not report why the query is incorrect (or any diagnostics around it)
I will add diagnostics for this as a follow up. I think it just reporting error does not make sense, without diagnostics. Tracking this issue #494

ketkarameya · 2023-06-05T19:13:35Z

src/models/rule_graph.rs

+  fn validate(&self) -> Result<(), String> {
+    match self.rules().iter().try_for_each(|rule| rule.validate()) {
+      Ok(()) => Ok(()),
+      Err(e) => Err(format!("Incorrect Rule Graph - {}", e)),


We are short-circuiting at the first exception.
https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.try_for_each

Thinking about it, I should eventually accumulate these errors and report them.
Expecting someone to fix them One-by-one is cruel

ketkarameya

Replies inline

ketkarameya

Replies inline

ketkarameya

Replies inline

ketkarameya

Replies inline

lazaroclapp

See comment above about documenting the tree_sitter_query::language() bit, but otherwise this LGTM.

ketkarameya requested a review from lazaroclapp June 4, 2023 19:21

Introduce basic rule graph validation

f742e34

ketkarameya force-pushed the validate_rule_graphs branch from 7faa4eb to f742e34 Compare June 4, 2023 21:39

Add tests - rust api, toml, python

90cd62f

ketkarameya commented Jun 5, 2023

View reviewed changes

lazaroclapp reviewed Jun 5, 2023

View reviewed changes

ketkarameya mentioned this pull request Jun 5, 2023

Report error diagnosis for not parseable tree-sitter queries #494

Open

ketkarameya commented Jun 5, 2023

View reviewed changes

lazaroclapp approved these changes Jun 5, 2023

View reviewed changes

.

52d0793

ketkarameya merged commit 5238295 into master Jun 6, 2023

ketkarameya deleted the validate_rule_graphs branch July 13, 2023 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Rule graph #493

Validate Rule graph #493

ketkarameya commented Jun 4, 2023 •

edited

Loading

ketkarameya left a comment

ketkarameya Jun 5, 2023

ketkarameya Jun 5, 2023

ketkarameya Jun 5, 2023

ketkarameya left a comment

ketkarameya Jun 5, 2023

lazaroclapp Jun 5, 2023

ketkarameya Jun 5, 2023

lazaroclapp Jun 5, 2023

ketkarameya Jun 5, 2023 •

edited

Loading

lazaroclapp Jun 5, 2023

ketkarameya Jun 5, 2023

lazaroclapp Jun 5, 2023

ketkarameya Jun 5, 2023 •

edited

Loading

ketkarameya left a comment

ketkarameya Jun 5, 2023

ketkarameya Jun 5, 2023

ketkarameya Jun 5, 2023 •

edited

Loading

ketkarameya left a comment

ketkarameya left a comment

ketkarameya left a comment

ketkarameya left a comment

lazaroclapp left a comment

Validate Rule graph #493

Validate Rule graph #493

Conversation

ketkarameya commented Jun 4, 2023 • edited Loading

ketkarameya 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

ketkarameya 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

ketkarameya Jun 5, 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

ketkarameya Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

ketkarameya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ketkarameya Jun 5, 2023 • edited Loading

Choose a reason for hiding this comment

ketkarameya left a comment

Choose a reason for hiding this comment

ketkarameya left a comment

Choose a reason for hiding this comment

ketkarameya left a comment

Choose a reason for hiding this comment

ketkarameya left a comment

Choose a reason for hiding this comment

lazaroclapp left a comment

Choose a reason for hiding this comment

ketkarameya commented Jun 4, 2023 •

edited

Loading

ketkarameya Jun 5, 2023 •

edited

Loading

ketkarameya Jun 5, 2023 •

edited

Loading

ketkarameya Jun 5, 2023 •

edited

Loading