From 4ced2311e2689f18beffed4f080b40d3283c6372 Mon Sep 17 00:00:00 2001 From: Liam Pattinson Date: Thu, 21 Nov 2024 19:15:37 +0000 Subject: [PATCH 1/2] Remove Mutex on tree sitter parser Now build a new parser for each file. Barely changes single threaded performance, but greatly improves parallel performance. --- fortitude/src/ast.rs | 45 +++++++----------------------------------- fortitude/src/check.rs | 11 +++++++++-- fortitude/src/lib.rs | 14 ++++++++++--- 3 files changed, 27 insertions(+), 43 deletions(-) diff --git a/fortitude/src/ast.rs b/fortitude/src/ast.rs index 4adedbd..94bf575 100644 --- a/fortitude/src/ast.rs +++ b/fortitude/src/ast.rs @@ -1,38 +1,9 @@ -use anyhow::Context; -use lazy_static::lazy_static; use ruff_diagnostics::Edit; use ruff_source_file::SourceFile; use ruff_text_size::{TextRange, TextSize}; /// Contains methods to parse Fortran code into a tree-sitter Tree and utilites to simplify the /// navigation of a Tree. -use std::sync::Mutex; -use tree_sitter::{Language, Node, Parser, Tree, TreeCursor}; - -lazy_static! { - static ref PARSER: Mutex = { - let parser = Mutex::new(Parser::new()); - parser - .lock() - .unwrap() - .set_language(&tree_sitter_fortran::LANGUAGE.into()) - .expect("Error loading Fortran grammar"); - parser - }; -} - -/// Parse a Fortran string and return the root note to the AST. -pub fn parse>(source: S) -> anyhow::Result { - PARSER - .lock() - .unwrap() - .parse(source.as_ref(), None) - .context("Failed to parse") -} - -/// Access the language of the parser. -pub fn language() -> Language { - PARSER.lock().unwrap().language().unwrap() -} +use tree_sitter::{Node, TreeCursor}; pub struct DepthFirstIterator<'a> { cursor: TreeCursor<'a>, @@ -59,7 +30,7 @@ impl<'a> Iterator for DepthFirstIterator<'a> { pub struct DepthFirstIteratorExcept<'a> { cursor: TreeCursor<'a>, - exceptions: Vec, + exceptions: Vec, // TODO Use kind ids instead } impl<'a> Iterator for DepthFirstIteratorExcept<'a> { @@ -67,7 +38,10 @@ impl<'a> Iterator for DepthFirstIteratorExcept<'a> { fn next(&mut self) -> Option { // ignore exception list if we're at a depth of 0 - if (self.cursor.depth() == 0 || !self.exceptions.contains(&self.cursor.node().kind_id())) + if (self.cursor.depth() == 0 + || !self + .exceptions + .contains(&self.cursor.node().kind().to_string())) && self.cursor.goto_first_child() { return Some(self.cursor.node()); @@ -149,14 +123,9 @@ impl FortitudeNode<'_> for Node<'_> { where I: IntoIterator, { - let lang = language(); - let exception_ids: Vec<_> = exceptions - .into_iter() - .map(|x| lang.id_for_node_kind(x, true)) - .collect(); DepthFirstIteratorExcept { cursor: self.walk(), - exceptions: exception_ids, + exceptions: exceptions.into_iter().map(|x| x.to_string()).collect(), } } diff --git a/fortitude/src/check.rs b/fortitude/src/check.rs index e2062d3..d137f35 100644 --- a/fortitude/src/check.rs +++ b/fortitude/src/check.rs @@ -1,4 +1,4 @@ -use crate::ast::{parse, FortitudeNode}; +use crate::ast::FortitudeNode; use crate::cli::{CheckArgs, GlobalConfigArgs, FORTRAN_EXTS}; use crate::message::DiagnosticMessage; use crate::printer::{Flags as PrinterFlags, Printer}; @@ -20,6 +20,7 @@ use std::path::{Path, PathBuf}; use std::process::ExitCode; use strum::IntoEnumIterator; use toml::Table; +use tree_sitter::Parser; use walkdir::WalkDir; // These are just helper structs to let us quickly work out if there's @@ -245,7 +246,13 @@ pub(crate) fn check_file( } // Perform AST analysis - let tree = parse(file.source_text())?; + let mut parser = Parser::new(); + parser + .set_language(&tree_sitter_fortran::LANGUAGE.into()) + .context("Error loading Fortran grammar")?; + let tree = parser + .parse(file.source_text(), None) + .context("Failed to parse")?; for node in tree.root_node().named_descendants() { if let Some(rules) = ast_entrypoints.get(node.kind()) { for rule in rules { diff --git a/fortitude/src/lib.rs b/fortitude/src/lib.rs index 6d9c44c..68ea003 100644 --- a/fortitude/src/lib.rs +++ b/fortitude/src/lib.rs @@ -15,13 +15,14 @@ mod test; mod text_helpers; pub use crate::registry::clap_completion::RuleParser; pub use crate::rule_selector::clap_completion::RuleSelectorParser; -use ast::{parse, FortitudeNode}; +use anyhow::Context; +use ast::FortitudeNode; use ruff_diagnostics::{Diagnostic, DiagnosticKind}; use ruff_source_file::{OneIndexed, SourceFile}; use ruff_text_size::{TextRange, TextSize}; use settings::Settings; use std::path::Path; -use tree_sitter::Node; +use tree_sitter::{Node, Parser}; // Violation type // -------------- @@ -95,7 +96,14 @@ pub trait AstRule { /// Apply a rule over some text, generating all violations raised as a result. fn apply(source: &SourceFile) -> anyhow::Result> { let entrypoints = Self::entrypoints(); - Ok(parse(source.source_text())? + let mut parser = Parser::new(); + parser + .set_language(&tree_sitter_fortran::LANGUAGE.into()) + .context("Error loading Fortran grammar")?; + let tree = parser + .parse(source.source_text(), None) + .context("Failed to parse")?; + Ok(tree .root_node() .named_descendants() .filter(|x| entrypoints.contains(&x.kind())) From 6234c526e0852c9d2238c37731bf816f6d1c39e6 Mon Sep 17 00:00:00 2001 From: Peter Hill Date: Fri, 22 Nov 2024 09:07:39 +0000 Subject: [PATCH 2/2] Remove `AstRule::apply` Missed removing last unit test when moving to snapshot testing --- fortitude/src/lib.rs | 24 +--------- .../src/rules/style/relational_operators.rs | 48 ------------------- 2 files changed, 2 insertions(+), 70 deletions(-) diff --git a/fortitude/src/lib.rs b/fortitude/src/lib.rs index 68ea003..37ca56b 100644 --- a/fortitude/src/lib.rs +++ b/fortitude/src/lib.rs @@ -15,14 +15,13 @@ mod test; mod text_helpers; pub use crate::registry::clap_completion::RuleParser; pub use crate::rule_selector::clap_completion::RuleSelectorParser; -use anyhow::Context; -use ast::FortitudeNode; + use ruff_diagnostics::{Diagnostic, DiagnosticKind}; use ruff_source_file::{OneIndexed, SourceFile}; use ruff_text_size::{TextRange, TextSize}; use settings::Settings; use std::path::Path; -use tree_sitter::{Node, Parser}; +use tree_sitter::Node; // Violation type // -------------- @@ -92,25 +91,6 @@ pub trait AstRule { /// Return list of tree-sitter node types on which a rule should trigger. fn entrypoints() -> Vec<&'static str>; - - /// Apply a rule over some text, generating all violations raised as a result. - fn apply(source: &SourceFile) -> anyhow::Result> { - let entrypoints = Self::entrypoints(); - let mut parser = Parser::new(); - parser - .set_language(&tree_sitter_fortran::LANGUAGE.into()) - .context("Error loading Fortran grammar")?; - let tree = parser - .parse(source.source_text(), None) - .context("Failed to parse")?; - Ok(tree - .root_node() - .named_descendants() - .filter(|x| entrypoints.contains(&x.kind())) - .filter_map(|x| Self::check(&Settings::default(), &x, source)) - .flatten() - .collect()) - } } /// Simplify making a `SourceFile` in tests diff --git a/fortitude/src/rules/style/relational_operators.rs b/fortitude/src/rules/style/relational_operators.rs index d948454..bb47067 100644 --- a/fortitude/src/rules/style/relational_operators.rs +++ b/fortitude/src/rules/style/relational_operators.rs @@ -56,51 +56,3 @@ impl AstRule for DeprecatedRelationalOperator { vec!["relational_expression"] } } - -#[cfg(test)] -mod tests { - use super::*; - use crate::{test_file, FromStartEndLineCol}; - use pretty_assertions::assert_eq; - - #[test] - fn test_relational_symbol() -> anyhow::Result<()> { - let source = test_file( - " - program test - if (0 .gt. 1) error stop - if (1 .le. 0) error stop - if (a.eq.b.and.a.ne.b) error stop - if (1 == 2) error stop ! OK - if (2 /= 2) error stop ! OK - end program test - ", - ); - let expected: Vec<_> = [ - (2, 8, 2, 12, ".gt.", ">"), - (3, 8, 3, 12, ".le.", "<="), - (4, 7, 4, 11, ".eq.", "=="), - (4, 18, 4, 22, ".ne.", "/="), - ] - .iter() - .map( - |(start_line, start_col, end_line, end_col, symbol, new_symbol)| { - Diagnostic::from_start_end_line_col( - DeprecatedRelationalOperator { - symbol: symbol.to_string(), - new_symbol: new_symbol.to_string(), - }, - &source, - *start_line, - *start_col, - *end_line, - *end_col, - ) - }, - ) - .collect(); - let actual = DeprecatedRelationalOperator::apply(&source)?; - assert_eq!(actual, expected); - Ok(()) - } -}