Revert old changes and update change to how we handle error nodes to be python specific on imports

Author: jdelgo

crates/static-analysis-kernel/src/analysis/analyze.rs

@@ -3,7 +3,7 @@ use crate::analysis::ddsa_lib::js::flow::java::{ClassGraph, FileGraph};
 use crate::analysis::ddsa_lib::runtime::ExecutionResult;
 use crate::analysis::ddsa_lib::JsRuntime;
 use crate::analysis::generated_content::{is_generated_file, is_minified_file};
-use crate::analysis::tree_sitter::{get_tree, get_tree_sitter_language, has_missing, TSQuery};
+use crate::analysis::tree_sitter::{get_tree, get_tree_sitter_language, TSQuery};
 use crate::model::analysis::{
     FileIgnoreBehavior, LinesToIgnore, ERROR_RULE_EXECUTION, ERROR_RULE_TIMEOUT,
 };
@@ -183,12 +183,6 @@ where
         }
         return vec![];
     };
-
-    // skip the file if there is an error or missing node
-    if tree.root_node().has_error() || has_missing(&tree.root_node()) {
-        return vec![];
-    }
-
     let tree = Arc::new(tree);
     let cst_parsing_time = now.elapsed();
 
@@ -444,52 +438,6 @@ def foo(arg1):
         )
     }
 
-    #[test]
-    fn test_error_node_skips_file() {
-        let rule = RuleInternal {
-            name: "myrule".to_string(),
-            short_description: None,
-            description: None,
-            category: RuleCategory::CodeStyle,
-            severity: RuleSeverity::Notice,
-            language: Language::Python,
-            code: "function visit(node, filename, code) {}".to_string(),
-            tree_sitter_query: get_query(QUERY_CODE, &Language::Python).unwrap(),
-        };
-        let results = analyze(
-            &Language::Python,
-            &vec![rule],
-            &Arc::from("myfile.py"),
-            &Arc::from("x = "), // incomplete assignment produces an ERROR node
-            &RuleConfig::default(),
-            &AnalysisOptions::default(),
-        );
-        assert!(results.is_empty());
-    }
-
-    #[test]
-    fn test_missing_node_skips_file() {
-        let rule = RuleInternal {
-            name: "myrule".to_string(),
-            short_description: None,
-            description: None,
-            category: RuleCategory::CodeStyle,
-            severity: RuleSeverity::Notice,
-            language: Language::JavaScript,
-            code: "function visit(node, filename, code) {}".to_string(),
-            tree_sitter_query: get_query("(identifier) @name", &Language::JavaScript).unwrap(),
-        };
-        let results = analyze(
-            &Language::JavaScript,
-            &vec![rule],
-            &Arc::from("myfile.js"),
-            &Arc::from("function foo() {"), // missing closing "}" produces a MISSING node
-            &RuleConfig::default(),
-            &AnalysisOptions::default(),
-        );
-        assert!(results.is_empty());
-    }
-
     // execution time must be more than 0
     #[test]
     fn test_execution_time() {
@@ -866,6 +814,56 @@ def foo2(arg1):
         assert_eq!(2, suppressed.len());
     }
 
+    #[test]
+    fn test_violation_ignore_taint_flow() {
+        // language=java
+        let text = "\
+class Test {
+    // An ignore on a taint flow region (not the base region of the violation):
+    // no-dd-sa
+    void test(String input) {
+        String a = input;
+        var b = a;
+        execute(b);
+    }
+}
+";
+        let ts_query = "\
+(argument_list (identifier) @arg)
+";
+        // language=javascript
+        let rule_code = r#"
+function visit(captures) {
+    const arg = captures.get("arg");
+    const sourceFlows = ddsa.getTaintSources(arg);
+    const v = Violation.new("flow violation", sourceFlows[0]);
+    addError(v);
+}
+"#;
+
+        let rule = RuleInternal {
+            name: "java-security/flow-rule".to_string(),
+            short_description: None,
+            description: None,
+            category: RuleCategory::Security,
+            severity: RuleSeverity::Error,
+            language: Language::Java,
+            code: rule_code.to_string(),
+            tree_sitter_query: get_query(ts_query, &Language::Java).unwrap(),
+        };
+
+        let analysis_options = AnalysisOptions::default();
+        let results = analyze(
+            &Language::Python,
+            &vec![rule],
+            &Arc::from("file.java"),
+            &Arc::from(text),
+            &RuleConfig::default(),
+            &analysis_options,
+        );
+        assert!(results[0].violations.is_empty());
+    }
+
     fn assert_lines_to_ignore(code: String, language: Language, rule: &'static str) {
         let lines_to_ignore = get_lines_to_ignore(code.as_str(), &language);
         assert_eq!(1, lines_to_ignore.lines_to_ignore_per_rule.len());

crates/static-analysis-kernel/src/analysis/languages/python/imports.rs

@@ -232,14 +232,17 @@ fn parse_import_statement<'tree, 'text: 'tree>(
         // We loop so that if creating the `Import` fails, we can try any subsequent imports within
         // this node (e.g. in a comma-separated list) to avoid prematurely yielding `None` on the iterator.
         while idx < node.named_child_count() {
-            debug_assert_eq!(node.field_name_for_named_child(idx as u32), Some("name"));
+            let field_name = node.field_name_for_named_child(idx as u32);
             let name_node = node.named_child(idx).expect("should be in-bounds");
-            let parsed = parse_field_child_node(source_code, name_node);
-
             idx += 1;
 
-            if let Ok(import) = Import::try_new(parsed.full_text, parsed.alias, None, false) {
-                return Some(import);
+            if field_name != Some("name") {
+                continue;
+            }
+            if let Some(parsed) = parse_field_child_node(source_code, name_node) {
+                if let Ok(import) = Import::try_new(parsed.full_text, parsed.alias, None, false) {
+                    return Some(import);
+                }
             }
         }
         None
@@ -254,7 +257,8 @@ fn parse_import_from_statement<'text>(
     debug_assert_eq!(node.kind(), "import_from_statement");
     let module_name_node = node.child_by_field_name("module_name").expect(FIELD_EXISTS);
     let is_relative = module_name_node.kind() == "relative_import";
-    let parsed_module = parse_field_child_node(source_code, module_name_node);
+    let parsed_module = parse_field_child_node(source_code, module_name_node)
+        .ok_or_else(|| format!("invalid node type `{}`", module_name_node.kind()))?;
     // Python syntax invariant: the module of a "from" import can't have an alias.
     debug_assert!(parsed_module.alias.is_none());
     // tree-sitter grammar invariant: a valid wildcard import must end in a `wildcard_import` node.
@@ -269,13 +273,13 @@ fn parse_import_from_statement<'text>(
         let field_children = node.children_by_field_name("name", &mut cursor);
         let entities = field_children
             .into_iter()
-            .map(|child| {
-                let parsed = parse_field_child_node(source_code, child);
+            .filter_map(|child| {
+                let parsed = parse_field_child_node(source_code, child)?;
                 // tree-sitter grammar invariant: these children represent entities, never a module.
-                Entity {
+                Some(Entity {
                     name: parsed.full_text,
                     alias: parsed.alias,
-                }
+                })
             })
             .collect::<Vec<_>>();
         ImportEntities::Specific(entities)
@@ -298,13 +302,13 @@ fn parse_future_import_statement<'text>(
     let field_children = node.children_by_field_name("name", &mut cursor);
     let entities = field_children
         .into_iter()
-        .map(|child| {
-            let parsed = parse_field_child_node(source_code, child);
+        .filter_map(|child| {
+            let parsed = parse_field_child_node(source_code, child)?;
             // tree-sitter grammar invariant: these children represent entities, never a module.
-            Entity {
+            Some(Entity {
                 name: parsed.full_text,
                 alias: parsed.alias,
-            }
+            })
         })
         .collect::<Vec<_>>();
     Import::try_new(
@@ -328,32 +332,32 @@ fn parse_future_import_statement<'text>(
 fn parse_field_child_node<'text>(
     source_code: &'text str,
     node: tree_sitter::Node,
-) -> MaybeAliased<'text> {
+) -> Option<MaybeAliased<'text>> {
     match node.kind() {
         "relative_import" => {
             // (relative_import (import_prefix) (dotted_name))
             for i in 0..node.child_count() {
                 let child = node.child(i).expect("i should be in-bounds");
                 if child.kind() == "dotted_name" {
-                    return MaybeAliased {
+                    return Some(MaybeAliased {
                         full_text: ts_node_text(source_code, child),
                         alias: None,
-                    };
+                    });
                 }
             }
             // Otherwise, this `relative_import` only contains an `import_prefix` node,
             // so return the entire node's text (which will consist of one or more "."):
-            MaybeAliased {
+            Some(MaybeAliased {
                 full_text: ts_node_text(source_code, node),
                 alias: None,
-            }
+            })
         }
         "dotted_name" => {
             // (dotted_name (identifier)+)
-            MaybeAliased {
+            Some(MaybeAliased {
                 full_text: ts_node_text(source_code, node),
                 alias: None,
-            }
+            })
         }
         "aliased_import" => {
             // (aliased_import name: (dotted_name) alias: (identifier))
@@ -363,18 +367,20 @@ fn parse_field_child_node<'text>(
             let alias_node = node.child_by_field_name("alias").expect(FIELD_EXISTS);
             debug_assert_eq!(alias_node.kind(), "identifier");
             let alias = ts_node_text(source_code, alias_node);
-            MaybeAliased {
+            Some(MaybeAliased {
                 full_text,
                 alias: Some(alias),
-            }
+            })
         }
-        other => panic!("invalid node type `{other}`"),
+        _ => None,
     }
 }
 
 #[cfg(test)]
 mod tests {
-    use super::{parse_imports, Entity, Import, ImportEntities};
+    use super::{parse_imports, parse_imports_with_tree, Entity, Import, ImportEntities};
+    use crate::analysis::tree_sitter::get_tree;
+    use crate::model::common::Language;
 
     /// A shorthand to build an [`Entity`] without an alias.
     pub fn ent(name: &str) -> Entity<'_> {
@@ -477,6 +483,30 @@ mod tests {
             }
         }
     }
+
+    #[test]
+    fn error_node_does_not_panic() {
+        let code = "import foo, + bar";
+        let tree = get_tree(code, &Language::Python).unwrap();
+        assert!(tree.root_node().has_error());
+        let imports = parse_imports_with_tree(code, &tree);
+        assert_eq!(
+            imports,
+            vec![
+                Import::try_new("foo", None, None, false).unwrap(),
+                Import::try_new("bar", None, None, false).unwrap(),
+            ]
+        );
+    }
+
+    #[test]
+    fn all_error_nodes_does_not_panic() {
+        let code = "import +, +";
+        let tree = get_tree(code, &Language::Python).unwrap();
+        assert!(tree.root_node().has_error());
+        let imports = parse_imports_with_tree(code, &tree);
+        assert!(imports.is_empty());
+    }
 }
 
 /// mod for documenting (intentionally) "incorrect" parsing behavior.

crates/static-analysis-kernel/src/analysis/tree_sitter.rs

@@ -6,7 +6,7 @@ use std::collections::HashMap;
 use std::sync::Arc;
 use std::time::Duration;
 use streaming_iterator::StreamingIterator;
-use tree_sitter::{CaptureQuantifier, Node};
+use tree_sitter::CaptureQuantifier;
 
 pub fn get_tree_sitter_language(language: &Language) -> tree_sitter::Language {
     extern "C" {
@@ -70,21 +70,6 @@ pub fn get_tree(code: &str, language: &Language) -> Option<tree_sitter::Tree> {
     tree_sitter_parser.parse(code, None)
 }
 
-// traverse from a node and return whether there is a missing node or not
-pub fn has_missing(node: &Node) -> bool {
-    if node.is_missing() {
-        return true;
-    }
-
-    for i in 0..node.child_count() {
-        if has_missing(&node.child(i).unwrap()) {
-            return true;
-        }
-    }
-
-    false
-}
-
 // build the query from tree-sitter
 pub fn get_query(
     query_code: &str,
@@ -736,17 +721,4 @@ SELECT * FROM table WHERE column = 'value';
         assert_eq!(None, superclasses.field_name);
         assert!(query_node.captures.contains_key("classname"));
     }
-
-    #[test]
-    fn test_has_missing_detects_missing_node() {
-        // "function foo() {" is missing the closing "}" and here tree-sitter inserts a MISSING node
-        let tree = get_tree("function foo() {", &Language::JavaScript).unwrap();
-        assert!(has_missing(&tree.root_node()));
-    }
-
-    #[test]
-    fn test_has_missing_returns_false_for_valid_tree() {
-        let tree = get_tree("def foo(): pass", &Language::Python).unwrap();
-        assert!(!has_missing(&tree.root_node()));
-    }
 }