Merge pull request #4029 from vangel76/fix/scriptcheck-fixpoint-always

fix(scriptcheck): same report with and without a loader for scripts without includes

crates/scriptcheck/src/lib.rs

@@ -15,8 +15,10 @@
 //!
 //! The checker never recurses: it reads each file once, and each scan of a file goes through its
 //! tokens front to back, first for the names its functions and its global code declare and then
-//! for the analysis. When it follows includes, it scans each file for the includes and what the
-//! variables hold, again while that grows (at most three more times), and then for the report. It
+//! for the analysis. It scans the script for the includes and what the variables hold, and each
+//! file it includes when it follows them, again while that grows (at most three more times), and
+//! then for the report; a scan that finds nothing more is the report scan of a script read without
+//! its included files. It
 //! keeps open brackets and the levels of an XML literal on capped stacks. Its work is linear in the
 //! input's size, and no input can overflow the stack.
 #![forbid(unsafe_code)]
@@ -146,7 +148,8 @@ pub use report::PROBLEMS;
 /// `loader`, the files its `#include` lines name are read through it and checked too; without one
 /// they are listed as [`IncludeStatus::NotRead`]. When the script or one of those files assigns a
 /// member chain or a File object to a variable, the check treats the variable as holding it in
-/// every file. Nothing in the script runs.
+/// every file, also in code before the assignment. A script that includes nothing gets the same
+/// report with a loader and without one. Nothing in the script runs.
 pub fn check(name: &str, bytes: &[u8], loader: Option<&mut dyn Loader>, limits: &Limits) -> Report {
     let mut acc = report::Acc::new(limits);
     if bytes.len() > limits.file_bytes {
@@ -169,6 +172,7 @@ pub fn check(name: &str, bytes: &[u8], loader: Option<&mut dyn Loader>, limits:
     // With a loader, this first scan finds the includes and what the script's variables hold; its
     // tallies are left out.
     let mut tallies = report::Acc::new(limits);
+    let pre = acc.clone();
     let scan = analyze::scan(&text, &declared, None, std::rc::Rc::from([]), &none, if loader.is_some() { &mut tallies } else { &mut acc });
     acc.steps = acc.steps.saturating_add(tallies.steps);
     if scan.module {
@@ -179,6 +183,7 @@ pub fn check(name: &str, bytes: &[u8], loader: Option<&mut dyn Loader>, limits:
         return acc.finish(Kind::Module, scan.lines);
     }
     let Some(loader) = loader else {
+        let mut acc = settle(acc, pre, &text, &declared, scan.known);
         includes::not_read(&mut acc, scan.includes);
         return acc.finish(Kind::Script, scan.lines);
     };
@@ -215,6 +220,40 @@ pub fn check(name: &str, bytes: &[u8], loader: Option<&mut dyn Loader>, limits:
     acc.finish(Kind::Script, lines)
 }
 
+/// The report of a script read without its included files: what the scans above find for a script
+/// that includes nothing, so a variable a function uses before the global code assigns it holds
+/// what it is assigned. `first` holds the scan made with nothing known, which found `known`, and
+/// `pre` what came before it. A scan made with what the scans before it found, and that finds
+/// nothing more, is the report scan; no other scan is repeated.
+fn settle<'l>(
+    first: report::Acc<'l>,
+    pre: report::Acc<'l>,
+    text: &str,
+    declared: &analyze::Declared<'_>,
+    mut known: analyze::Known,
+) -> report::Acc<'l> {
+    if known.len() == 0 {
+        return first;
+    }
+    let entries = pre.limits.entries;
+    let mut spent = first.steps.saturating_sub(pre.steps);
+    for _ in 0..3 {
+        let before = known.len();
+        let mut acc = pre.clone();
+        let more = analyze::scan(text, declared, None, std::rc::Rc::from([]), &known, &mut acc).known;
+        known.add(more, entries);
+        if known.len() == before {
+            acc.steps = acc.steps.saturating_add(spent);
+            return acc;
+        }
+        spent = spent.saturating_add(acc.steps.saturating_sub(pre.steps));
+    }
+    let mut acc = pre;
+    analyze::scan(text, declared, None, std::rc::Rc::from([]), &known, &mut acc);
+    acc.steps = acc.steps.saturating_add(spent);
+    acc
+}
+
 /// The last part of a path (`lib/Tidy.jsx` → `Tidy.jsx`), whichever separator it uses.
 pub(crate) fn base_name(name: &str) -> &str {
     name.rsplit(['/', '\\']).next().unwrap_or(name)

crates/scriptcheck/src/report.rs

@@ -253,6 +253,7 @@ impl Tally {
 }
 
 /// What a check has found so far, across the script and its included files.
+#[derive(Clone)]
 pub(crate) struct Acc<'l> {
     pub limits: &'l Limits,
     pub steps: usize,

crates/scriptcheck/src/tests.rs

@@ -1757,3 +1757,63 @@ proptest! {
         prop_assert!(r.includes.iter().all(|i| i.status != IncludeStatus::NotRead));
     }
 }
+
+/// A function that writes to a variable the global code assigns a File object after it: the
+/// variable declaration is hoisted, so the function writes a file, with a loader and without one.
+#[test]
+fn a_variable_assigned_after_the_function_using_it_holds_its_value() {
+    let src = "function g(){ f.write('x'); }  var f = new File('a');";
+    let without = report(src);
+    let with = check("test.jsx", src.as_bytes(), Some(&mut mem(&[])), &Limits::default());
+    assert!(feature(&without, "file.write").is_some(), "{:?}", without.features);
+    assert!(feature(&with, "file.write").is_some(), "{:?}", with.features);
+}
+
+/// Script-like text with functions that use variables the code after them assigns, a chain of them
+/// taking more than one scan to settle.
+fn hoisting_text() -> impl Strategy<Value = String> {
+    let pieces = prop::sample::select(vec![
+        "function g(){ ",
+        "function h(){ ",
+        "}",
+        "f.write('x'); ",
+        "b.remove(); ",
+        "e.acmeItems.add(); ",
+        "b = a; ",
+        "var a = f; ",
+        "var f = new File('a'); ",
+        "var d = app.activeDocument; ",
+        "var e = d; ",
+        "e = d.acmeLayers; ",
+        "f = d; ",
+    ]);
+    (prop::collection::vec(pieces, 0..30), script_text()).prop_map(|(v, tail)| v.concat() + &tail)
+}
+
+proptest! {
+    #![proptest_config(ProptestConfig::with_cases(512))]
+
+    /// A script that includes nothing gets the same report with a loader and without one.
+    #[test]
+    fn a_script_without_includes_reports_the_same_with_and_without_a_loader(text in hoisting_text()) {
+        let mut without = report(&text);
+        prop_assume!(without.includes.is_empty());
+        let mut with = check("test.jsx", text.as_bytes(), Some(&mut mem(&[])), &Limits::default());
+        without.steps = 0;
+        with.steps = 0;
+        prop_assert_eq!(without, with);
+    }
+}
+
+/// Chains of variables a function assigns before the code after it assigns their source take a
+/// scan each to settle: the script is read at most seven times.
+#[test]
+fn variables_that_settle_over_several_scans_stay_linear() {
+    let mut src = String::new();
+    for i in 0..150 {
+        src += &format!("function a{i}(){{ e{i} = d{i}; }} function b{i}(){{ d{i} = c{i}; }} function c{i}(){{ c{i} = b{i}; }} ");
+        src += &format!("function d{i}(){{ b{i} = a{i}; }} var a{i} = new File('x');\n");
+    }
+    let r = report(&src);
+    assert!(r.steps <= 9 * src.len() + 16, "{} steps for {} bytes", r.steps, src.len());
+}