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

Validate Rule graph #493

Merged
merged 3 commits into from
Jun 6, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ tree-sitter-typescript = "0.20.1"
tree-sitter-go = { git = "https://github.com/uber/tree-sitter-go.git", rev = "8f807196afab4a1a1256dbf62a011020c6fe7745" }
tree-sitter-thrift = "0.5.0"
tree-sitter-strings = { git = "https://github.com/ketkarameya/tree-sitter-strings.git" }
tree-sitter-query = "0.1.0"
derive_builder = "0.12.0"
getset = "0.1.2"
pyo3 = "0.19.0"
Expand Down
4 changes: 2 additions & 2 deletions src/cleanup_rules/kt/rules.toml
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ query = """
(
(prefix_expression (boolean_literal) @exp) @prefix_expression
(#eq? @exp "false")
(#match @prefix_expression "!.*")
(#match? @prefix_expression "!.*")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Found this bug (and the one below) :)

)
"""
replace = "true"
Expand All @@ -216,7 +216,7 @@ query = """
(
(prefix_expression (boolean_literal) @exp) @prefix_expression
(#eq? @exp "true")
(#match @prefix_expression "!.*")
(#match? @prefix_expression "!.*")
)
"""
replace = "false"
Expand Down
7 changes: 2 additions & 5 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ use std::{collections::HashMap, fs::File, io::Write, path::PathBuf};

use itertools::Itertools;
use log::{debug, info};
use tree_sitter::Parser;

use crate::models::rule_store::RuleStore;

Expand Down Expand Up @@ -111,11 +110,9 @@ impl Piranha {
/// Performs cleanup related to stale flags
fn perform_cleanup(&mut self) {
// Setup the parser for the specific language
let mut parser = Parser::new();
let piranha_args = &self.piranha_arguments;
parser
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

.set_language(*piranha_args.language().language())
.expect("Could not set the language for the parser.");

let mut parser = piranha_args.language().parser();

let mut path_to_codebase = self.piranha_arguments.path_to_codebase().to_string();

Expand Down
65 changes: 44 additions & 21 deletions src/models/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ use crate::utilities::{
tree_sitter_utilities::{get_all_matches_for_query, get_match_for_query, get_node_for_range},
};

use super::{rule::InstantiatedRule, rule_store::RuleStore, source_code_unit::SourceCodeUnit};
use super::{
rule::InstantiatedRule, rule_store::RuleStore, source_code_unit::SourceCodeUnit, Validator,
};

use crate::utilities::{tree_sitter_utilities::TSQuery, Instantiate};

Expand Down Expand Up @@ -103,47 +105,68 @@ impl Filter {
gen_py_str_methods!();
}

impl FilterBuilder {
/// Builds Filter from FilterBuilder
/// * create Filter from the builder
/// * validates new argument combinations
pub fn build(&self) -> Filter {
match &self._validate() {
Ok(filter) => filter.clone(),
Err(e) => panic!("Invalid filter - {}", e),
}
}

fn _validate(&self) -> Result<Filter, String> {
let _filter: Filter = self.create().unwrap();

impl Validator for Filter {
fn validate(&self) -> Result<(), String> {
// Only allow users to set either contains or not_contains, but not both
if !_filter.contains().get_query().is_empty() && !_filter.not_contains().is_empty() {
if *self.contains() != default_contains_query()
&& *self.not_contains() != default_not_contains_queries()
{
return Err(
"Invalid Filter Argument. `contains` and `not_contains` cannot be set at the same time !!! Please use two filters instead."
.to_string(),
);
}

if _filter.at_least > _filter.at_most {
if self.at_least > self.at_most {
return Err(
"Invalid Filter Argument. `at_least` should be less than or equal to `at_most` !!!"
.to_string(),
);
}

// If the user set `at_least` or `at_most`, then the contains query cannot be empty
if (_filter.at_least != default_contains_at_least()
|| _filter.at_most != default_contains_at_most())
&& _filter.contains().get_query().is_empty()
if (self.at_least != default_contains_at_least() || self.at_most != default_contains_at_most())
&& self.contains().get_query().is_empty()
{
return Err(
"Invalid Filter Argument. `at_least` or `at_most` is set, but `contains` is empty !!!"
.to_string(),
);
}

Ok(_filter)
if *self.enclosing_node() != default_enclosing_node() {
self.enclosing_node().validate()?
}

if *self.not_enclosing_node() != default_not_enclosing_node() {
self.not_enclosing_node().validate()?
}

if *self.contains() != default_contains_query() {
self.contains().validate()?
}

if *self.not_contains() != default_not_contains_queries() {
self.not_contains().iter().try_for_each(|x| x.validate())?
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

}
Ok(())
}
}

impl FilterBuilder {
/// Builds Filter from FilterBuilder
/// * create Filter from the builder
/// * validates new argument combinations
pub fn build(&self) -> Filter {
match &self._validate() {
Ok(filter) => filter.clone(),
Err(e) => panic!("Invalid filter - {}", e),
}
}

fn _validate(&self) -> Result<Filter, String> {
let _filter: Filter = self.create().unwrap();
_filter.validate().map(|_| _filter)
}
}

Expand Down
4 changes: 4 additions & 0 deletions src/models/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as our validation enriches, I will modify this Type parameter

}
11 changes: 11 additions & 0 deletions src/models/rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ use super::{
default_replace, default_replace_node, default_rule_name,
},
filter::Filter,
Validator,
};

#[derive(Deserialize, Debug, Clone, Default, PartialEq)]
Expand Down Expand Up @@ -184,6 +185,16 @@ impl Rule {
gen_py_str_methods!();
}

impl Validator for Rule {
fn validate(&self) -> Result<(), String> {
let validation = self
.query()
.validate()
.and_then(|_: ()| self.filters().iter().try_for_each(|f| f.validate()));
validation
}
}

pub use piranha_rule;

#[derive(Debug, Getters, Clone)]
Expand Down
28 changes: 22 additions & 6 deletions src/models/rule_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,21 @@ Copyright (c) 2023 Uber Technologies, Inc.
limitations under the License.
*/

use derive_builder::Builder;
use getset::{Getters, MutGetters};
use itertools::Itertools;

use crate::{
models::{outgoing_edges::OutgoingEdges, rule::Rule},
utilities::{gen_py_str_methods, read_toml, MapOfVec},
};
use colored::Colorize;
use derive_builder::Builder;
use getset::{Getters, MutGetters};
use itertools::Itertools;
use std::{collections::HashMap, path::Path};

use super::{
default_configs::{default_edges, default_rule_graph_map, default_rules},
outgoing_edges::Edges,
rule::{InstantiatedRule, Rules},
Validator,
};
use pyo3::prelude::{pyclass, pymethods};

Expand Down Expand Up @@ -54,6 +55,15 @@ pub struct RuleGraph {
graph: HashMap<String, Vec<(String, String)>>,
}

impl Validator for RuleGraph {
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)),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@ketkarameya ketkarameya Jun 5, 2023

Choose a reason for hiding this comment

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

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

}
}
}

#[pymethods]
impl RuleGraph {
#[new]
Expand Down Expand Up @@ -92,12 +102,18 @@ impl RuleGraphBuilder {
}
}

RuleGraphBuilder::default()
let graph = RuleGraphBuilder::default()
.edges(_rule_graph.edges().clone())
.rules(_rule_graph.rules().clone())
.graph(graph)
.create()
.unwrap()
.unwrap();

if let Err(err) = graph.validate() {
panic!("{}", err.as_str().red());
}

graph
}
}

Expand Down
10 changes: 4 additions & 6 deletions src/models/source_code_unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ use itertools::Itertools;
use log::{debug, error};

use tree_sitter::{InputEdit, Node, Parser, Range, Tree};
use tree_sitter_traversal::{traverse, Order};

use crate::{
models::rule_graph::{GLOBAL, PARENT},
utilities::tree_sitter_utilities::{
get_match_for_query, get_node_for_range, get_replace_range, get_tree_sitter_edit, TSQuery,
get_match_for_query, get_node_for_range, get_replace_range, get_tree_sitter_edit,
number_of_errors, TSQuery,
},
};

Expand Down Expand Up @@ -358,11 +358,9 @@ impl SourceCodeUnit {
panic!("{}", msg);
}

/// Returns the number of errors in the AST
/// Returns the number of errors in this source code unit
fn _number_of_errors(&self) -> usize {
traverse(self.root_node().walk(), Order::Post)
.filter(|node| node.is_error() || node.is_missing())
.count()
number_of_errors(&self.root_node())
}

// Replaces the content of the current file with the new content and re-parses the AST
Expand Down
44 changes: 44 additions & 0 deletions src/models/unit_tests/source_code_unit_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ Copyright (c) 2023 Uber Technologies, Inc.
use tree_sitter::{Parser, Point};

use crate::models::filter::FilterBuilder;

use crate::models::rule_graph::RuleGraphBuilder;
use crate::utilities::tree_sitter_utilities::TSQuery;
use crate::{
filter,
Expand Down Expand Up @@ -607,3 +609,45 @@ fn test_filter_bad_range() {
.at_most(4)
.build();
}

#[test]
#[should_panic(expected = "Cannot parse")]
fn test_filter_syntactically_incorrect_contains() {
FilterBuilder::default()
.contains(TSQuery::new(String::from("(if_statement @if_stmt")))
.build();
}

#[test]
#[should_panic(expected = "Cannot parse")]
fn test_filter_syntactically_incorrect_not_contains() {
FilterBuilder::default()
.not_contains(vec![TSQuery::new(String::from("(if_statement @if_stmt"))])
.build();
}

#[test]
#[should_panic(expected = "Cannot parse")]
fn test_filter_syntactically_incorrect_enclosing_node() {
FilterBuilder::default()
.enclosing_node(TSQuery::new(String::from("(if_statement @if_stmt")))
.build();
}

#[test]
#[should_panic(expected = "Cannot parse")]
fn test_filter_syntactically_incorrect_not_enclosing_node() {
FilterBuilder::default()
.not_enclosing_node(TSQuery::new(String::from("(if_statement @if_stmt")))
.build();
}

#[test]
#[should_panic(expected = "Cannot parse")]
fn test_rule_graph_incorrect_query() {
RuleGraphBuilder::default()
.rules(vec![
piranha_rule! {name = "Test rule", query = "(if_statement"},
])
.build();
}
18 changes: 18 additions & 0 deletions src/tests/test_piranha_java.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,3 +376,21 @@ fn test_handle_syntactically_incorrect_tree_panic() {
// Delete temp_dir
temp_dir.close().unwrap();
}

#[test]
#[should_panic(expected = "Cannot parse")]
fn test_incorrect_rule() {
initialize();
let _path = PathBuf::from("test-resources")
.join(JAVA)
.join("incorrect_rule");
let path_to_codebase = _path.join("input").to_str().unwrap().to_string();
let path_to_configurations = _path.join("configurations").to_str().unwrap().to_string();
let piranha_arguments = PiranhaArgumentsBuilder::default()
.path_to_codebase(path_to_codebase)
.path_to_configurations(path_to_configurations)
.language(PiranhaLanguage::from(JAVA))
.build();

let _ = execute_piranha(&piranha_arguments);
}
Loading