From 618fdcaa02cc623d88dc421949af0bcc8f0c014a Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Wed, 24 Jul 2024 14:05:00 -0400 Subject: [PATCH] Fix invalidation when file changed during analysis --- src/cli/lib.rs | 45 +++++++++++++++---- src/cli/test_runners/test_runner.rs | 23 ++++++++-- src/code_info/analysis_result.rs | 8 +++- src/file_scanner_analyzer/analyzer.rs | 16 +++---- src/file_scanner_analyzer/file.rs | 18 ++++---- src/file_scanner_analyzer/lib.rs | 15 ++++--- src/file_scanner_analyzer/unused_symbols.rs | 9 +++- .../a-before-analysis/input.hack | 14 ++++++ .../beforeAnalysisMethodChange/a/input.hack | 14 ++++++ .../beforeAnalysisMethodChange/output.txt | 1 + .../a-before-analysis/input.hack | 19 ++++++++ .../a/input.hack | 19 ++++++++ .../b/input.hack | 19 ++++++++ .../output.txt | 0 .../a-before-analysis/input.hack | 19 ++++++++ .../a/input.hack | 19 ++++++++ .../b/input.hack | 19 ++++++++ .../output.txt | 0 .../nestedAssignment/input.hack | 11 +++++ .../nestedAssignment/output.txt | 2 + 20 files changed, 253 insertions(+), 37 deletions(-) create mode 100644 tests/diff/beforeAnalysisMethodChange/a-before-analysis/input.hack create mode 100644 tests/diff/beforeAnalysisMethodChange/a/input.hack create mode 100644 tests/diff/beforeAnalysisMethodChange/output.txt create mode 100644 tests/diff/beforeAnalysisMethodChangeThenBack/a-before-analysis/input.hack create mode 100644 tests/diff/beforeAnalysisMethodChangeThenBack/a/input.hack create mode 100644 tests/diff/beforeAnalysisMethodChangeThenBack/b/input.hack create mode 100644 tests/diff/beforeAnalysisMethodChangeThenBack/output.txt create mode 100644 tests/diff/beforeAnalysisMethodChangeThenForward/a-before-analysis/input.hack create mode 100644 tests/diff/beforeAnalysisMethodChangeThenForward/a/input.hack create mode 100644 tests/diff/beforeAnalysisMethodChangeThenForward/b/input.hack create mode 100644 tests/diff/beforeAnalysisMethodChangeThenForward/output.txt create mode 100644 tests/unused/UnusedExpression/nestedAssignment/input.hack create mode 100644 tests/unused/UnusedExpression/nestedAssignment/output.txt diff --git a/src/cli/lib.rs b/src/cli/lib.rs index 9312bb1f..1f6a18a2 100644 --- a/src/cli/lib.rs +++ b/src/cli/lib.rs @@ -686,7 +686,9 @@ fn do_fix( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(&cwd, config_path, &mut interner).ok(); + config + .update_from_file(&cwd, config_path, &mut interner) + .ok(); } config.allowed_issues = None; @@ -704,6 +706,7 @@ fn do_fix( None, None, None, + || {}, ); if let Ok((mut analysis_result, successfull_run_data)) = result { @@ -737,7 +740,9 @@ fn do_remove_unused_fixmes( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(cwd, config_path, &mut interner).ok(); + config + .update_from_file(cwd, config_path, &mut interner) + .ok(); } config.allowed_issues = None; @@ -759,6 +764,7 @@ fn do_remove_unused_fixmes( None, None, None, + || {}, ); if let Ok((mut analysis_result, successful_run_data)) = result { @@ -814,7 +820,9 @@ fn do_add_fixmes( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(cwd, config_path, &mut interner).ok(); + config + .update_from_file(cwd, config_path, &mut interner) + .ok(); } config.allowed_issues = None; @@ -833,6 +841,7 @@ fn do_add_fixmes( None, None, None, + || {}, ); if let Ok((mut analysis_result, successful_run_data)) = result { @@ -880,7 +889,9 @@ fn do_migrate( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(cwd, config_path, &mut interner).ok(); + config + .update_from_file(cwd, config_path, &mut interner) + .ok(); } config.allowed_issues = None; @@ -921,6 +932,7 @@ fn do_migrate( None, None, None, + || {}, ); if let Ok((mut analysis_result, successful_run_data)) = result { @@ -969,7 +981,9 @@ fn do_migration_candidates( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(cwd, config_path, &mut interner).ok(); + config + .update_from_file(cwd, config_path, &mut interner) + .ok(); } config.allowed_issues = None; @@ -990,6 +1004,7 @@ fn do_migration_candidates( None, None, None, + || {}, ); if let Ok(result) = result { @@ -1047,7 +1062,9 @@ fn do_codegen( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(cwd, config_path, &mut interner).ok(); + config + .update_from_file(cwd, config_path, &mut interner) + .ok(); } config.allowed_issues = None; @@ -1066,6 +1083,7 @@ fn do_codegen( None, None, None, + || {}, ); if let Ok(result) = result { @@ -1159,7 +1177,9 @@ fn do_find_paths( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(cwd, config_path, &mut interner).ok(); + config + .update_from_file(cwd, config_path, &mut interner) + .ok(); } config.allowed_issues = None; @@ -1187,6 +1207,7 @@ fn do_find_paths( None, None, None, + || {}, ); if let Ok((analysis_result, successful_run_data)) = result { @@ -1224,7 +1245,9 @@ fn do_security_check( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(cwd, config_path, &mut interner).ok(); + config + .update_from_file(cwd, config_path, &mut interner) + .ok(); } config.allowed_issues = None; @@ -1254,6 +1277,7 @@ fn do_security_check( None, None, None, + || {}, ); if let Ok((analysis_result, successful_run_data)) = result { @@ -1345,7 +1369,9 @@ fn do_analysis( let mut interner = Interner::default(); if config_path.exists() { - config.update_from_file(cwd, config_path, &mut interner).ok(); + config + .update_from_file(cwd, config_path, &mut interner) + .ok(); } // do this after we've loaded from file, as they can be overridden @@ -1374,6 +1400,7 @@ fn do_analysis( None, None, None, + || {}, ); if let Ok((analysis_result, successful_run_data)) = result { diff --git a/src/cli/test_runners/test_runner.rs b/src/cli/test_runners/test_runner.rs index bcb818b3..183a98db 100644 --- a/src/cli/test_runners/test_runner.rs +++ b/src/cli/test_runners/test_runner.rs @@ -255,6 +255,7 @@ impl TestRunner { previous_scan_data, previous_analysis_result, None, + || {}, ); if dir.contains("/migrations/") @@ -445,14 +446,22 @@ impl TestRunner { let workdir_base = dir.clone() + "/workdir"; - let mut folders = vec![dir.clone() + "/a", dir.clone() + "/b"]; + let mut folders = vec![(dir.clone() + "/a", false)]; + + if Path::new(&(dir.clone() + "/a-before-analysis")).exists() { + folders[0].1 = true; + } + + if Path::new(&(dir.clone() + "/b")).exists() { + folders.push((dir.clone() + "/b", false)); + } if Path::new(&(dir.clone() + "/c")).exists() { - folders.push(dir.clone() + "/c"); + folders.push((dir.clone() + "/c", false)); } if Path::new(&(dir.clone() + "/d")).exists() { - folders.push(dir.clone() + "/d"); + folders.push((dir.clone() + "/d", false)); } let mut previous_scan_data = None; @@ -469,7 +478,7 @@ impl TestRunner { stub_dirs.push(cwd.clone() + "/third-party/xhp-lib/src"); } - for folder in folders { + for (folder, change_after_scan) in folders { copy_recursively(folder.clone(), workdir_base.clone()).unwrap(); let run_result = hakana_workhorse::scan_and_analyze( @@ -488,6 +497,12 @@ impl TestRunner { previous_scan_data, previous_analysis_result, None, + || { + if change_after_scan { + copy_recursively(folder.clone() + "-before-analysis", workdir_base.clone()) + .unwrap(); + } + }, ); fs::remove_dir_all(&workdir_base).unwrap(); diff --git a/src/code_info/analysis_result.rs b/src/code_info/analysis_result.rs index 6e325f37..47926cef 100644 --- a/src/code_info/analysis_result.rs +++ b/src/code_info/analysis_result.rs @@ -6,7 +6,10 @@ use serde::Serialize; use crate::{ code_location::FilePath, - data_flow::{graph::{DataFlowGraph, GraphKind}, node::DataFlowNodeId}, + data_flow::{ + graph::{DataFlowGraph, GraphKind}, + node::DataFlowNodeId, + }, function_context::FunctionLikeIdentifier, issue::{Issue, IssueKind}, symbol_references::SymbolReferences, @@ -35,6 +38,7 @@ pub struct AnalysisResult { pub time_in_analysis: Duration, pub functions_to_migrate: FxHashMap, pub has_invalid_hack_files: bool, + pub changed_during_analysis_files: FxHashSet, } impl AnalysisResult { @@ -55,6 +59,7 @@ impl AnalysisResult { functions_to_migrate: FxHashMap::default(), codegen: BTreeMap::default(), has_invalid_hack_files: false, + changed_during_analysis_files: FxHashSet::default(), } } @@ -78,6 +83,7 @@ impl AnalysisResult { } self.functions_to_migrate.extend(other.functions_to_migrate); self.codegen.extend(other.codegen); + self.changed_during_analysis_files.extend(other.changed_during_analysis_files); self.has_invalid_hack_files = self.has_invalid_hack_files || other.has_invalid_hack_files; } diff --git a/src/file_scanner_analyzer/analyzer.rs b/src/file_scanner_analyzer/analyzer.rs index cd8ddfaa..cfd36c21 100644 --- a/src/file_scanner_analyzer/analyzer.rs +++ b/src/file_scanner_analyzer/analyzer.rs @@ -1,3 +1,4 @@ +use crate::file::get_file_contents_hash; use crate::{get_aast_for_path, update_progressbar, SuccessfulScanData}; use hakana_analyzer::config::Config; use hakana_analyzer::file_analyzer; @@ -108,11 +109,7 @@ pub fn analyze_files( file_analysis_time += analyze_file( file_path, str_path, - scan_data - .file_system - .file_hashes_and_times - .get(&file_path) - .map(|k| k.1), + scan_data.file_system.file_hashes_and_times.get(&file_path), codebase, interner, &analysis_config, @@ -155,7 +152,7 @@ pub fn analyze_files( fn analyze_file( file_path: FilePath, str_path: &String, - last_updated_time: Option, + last_hash_and_time: Option<&(u64, u64)>, codebase: &CodebaseInfo, interner: &Interner, config: &Arc, @@ -173,9 +170,12 @@ fn analyze_file( .unwrap() .as_micros() as u64; - if let Some(last_updated_time) = last_updated_time { - if updated_time != last_updated_time { + if let Some((file_hash, last_updated_time)) = last_hash_and_time { + if updated_time != *last_updated_time + && get_file_contents_hash(&str_path).unwrap_or(0) != *file_hash + { analysis_result.has_invalid_hack_files = true; + analysis_result.changed_during_analysis_files.insert(file_path); analysis_result.emitted_issues.insert( file_path, vec![Issue::new( diff --git a/src/file_scanner_analyzer/file.rs b/src/file_scanner_analyzer/file.rs index 43360cda..087bd063 100644 --- a/src/file_scanner_analyzer/file.rs +++ b/src/file_scanner_analyzer/file.rs @@ -22,13 +22,6 @@ pub struct VirtualFileSystem { } impl VirtualFileSystem { - pub fn get_contents_hash(&self, file_path: &String) -> Result { - match fs::read_to_string(file_path) { - Ok(file_contents) => Ok(xxhash_rust::xxh3::xxh3_64(file_contents.as_bytes())), - Err(error) => Err(error), - } - } - pub(crate) fn apply_language_server_changes( &mut self, language_server_changes: FxHashMap, @@ -265,7 +258,7 @@ impl VirtualFileSystem { if old_update_time == &updated_time { *old_contents_hash } else if calculate_file_hashes { - self.get_contents_hash(&str_path).unwrap_or(0) + get_file_contents_hash(&str_path).unwrap_or(0) } else { 0 } @@ -273,7 +266,7 @@ impl VirtualFileSystem { 0 } } else if calculate_file_hashes { - self.get_contents_hash(&str_path).unwrap_or(0) + get_file_contents_hash(&str_path).unwrap_or(0) } else { 0 }; @@ -297,3 +290,10 @@ impl VirtualFileSystem { } } } + +pub fn get_file_contents_hash(file_path: &String) -> Result { + match fs::read_to_string(file_path) { + Ok(file_contents) => Ok(xxhash_rust::xxh3::xxh3_64(file_contents.as_bytes())), + Err(error) => Err(error), + } +} diff --git a/src/file_scanner_analyzer/lib.rs b/src/file_scanner_analyzer/lib.rs index 9bdb1942..0fbbddfa 100644 --- a/src/file_scanner_analyzer/lib.rs +++ b/src/file_scanner_analyzer/lib.rs @@ -184,7 +184,7 @@ pub async fn scan_and_analyze_async( let mut analysis_result = (*analysis_result.lock().unwrap()).clone(); - let scan_data = Arc::try_unwrap(arc_scan_data).unwrap(); + let mut scan_data = Arc::try_unwrap(arc_scan_data).unwrap(); add_invalid_files(&scan_data, &mut analysis_result); @@ -192,16 +192,17 @@ pub async fn scan_and_analyze_async( find_unused_definitions( &mut analysis_result, &config, - &scan_data.codebase, + &mut scan_data.codebase, &scan_data.interner, &ignored_paths, + &mut scan_data.file_system, ); } Ok((analysis_result, scan_data)) } -pub fn scan_and_analyze( +pub fn scan_and_analyze ()>( stubs_dirs: Vec, filter: Option, ignored_paths: Option>, @@ -214,6 +215,7 @@ pub fn scan_and_analyze( previous_scan_data: Option, previous_analysis_result: Option, language_server_changes: Option>, + chaos_monkey: F, ) -> io::Result<(AnalysisResult, SuccessfulScanData)> { let mut all_scanned_dirs = stubs_dirs.clone(); all_scanned_dirs.push(config.root_dir.clone()); @@ -319,6 +321,8 @@ pub fn scan_and_analyze( let mut pure_file_analysis_time = Duration::default(); + chaos_monkey(); + analyze_files( files_to_analyze, arc_scan_data.clone(), @@ -344,7 +348,7 @@ pub fn scan_and_analyze( cache_analysis_data(cache_dir, &analysis_result)?; - let scan_data = Arc::try_unwrap(arc_scan_data).unwrap(); + let mut scan_data = Arc::try_unwrap(arc_scan_data).unwrap(); add_invalid_files(&scan_data, &mut analysis_result); @@ -352,9 +356,10 @@ pub fn scan_and_analyze( find_unused_definitions( &mut analysis_result, &config, - &scan_data.codebase, + &mut scan_data.codebase, &scan_data.interner, &ignored_paths, + &mut scan_data.file_system, ); } diff --git a/src/file_scanner_analyzer/unused_symbols.rs b/src/file_scanner_analyzer/unused_symbols.rs index bc3bdd54..716004e2 100644 --- a/src/file_scanner_analyzer/unused_symbols.rs +++ b/src/file_scanner_analyzer/unused_symbols.rs @@ -14,15 +14,22 @@ use rustc_hash::{FxHashMap, FxHashSet}; use std::sync::Arc; +use crate::file::VirtualFileSystem; + pub(crate) fn find_unused_definitions( analysis_result: &mut AnalysisResult, config: &Arc, - codebase: &CodebaseInfo, + codebase: &mut CodebaseInfo, interner: &Interner, ignored_paths: &Option>, + file_system: &mut VirtualFileSystem, ) { // don’t show unused definitions if we have any invalid Hack files if analysis_result.has_invalid_hack_files { + for file_path in &analysis_result.changed_during_analysis_files { + file_system.file_hashes_and_times.remove(file_path); + codebase.files.remove(file_path); + } return; } diff --git a/tests/diff/beforeAnalysisMethodChange/a-before-analysis/input.hack b/tests/diff/beforeAnalysisMethodChange/a-before-analysis/input.hack new file mode 100644 index 00000000..c679feca --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChange/a-before-analysis/input.hack @@ -0,0 +1,14 @@ +final class A { + public static function foo(): void { + self::bar(); + self::baz(); + } + + protected static function bar(): void {} + + protected static function baz(): void { + echo 1; + echo 3; + echo 2; + } +} \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChange/a/input.hack b/tests/diff/beforeAnalysisMethodChange/a/input.hack new file mode 100644 index 00000000..cca2bffa --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChange/a/input.hack @@ -0,0 +1,14 @@ +final class A { + public static function foo(): void { + self::bar(); + self::baz(); + } + + protected static function bar(): void {} + + protected static function baz(): void { + echo 1; + + echo 2; + } +} \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChange/output.txt b/tests/diff/beforeAnalysisMethodChange/output.txt new file mode 100644 index 00000000..6772b095 --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChange/output.txt @@ -0,0 +1 @@ +InvalidHackFile \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChangeThenBack/a-before-analysis/input.hack b/tests/diff/beforeAnalysisMethodChangeThenBack/a-before-analysis/input.hack new file mode 100644 index 00000000..c9d1aa86 --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChangeThenBack/a-before-analysis/input.hack @@ -0,0 +1,19 @@ +final class A { + public static function foo(): void { + self::bar(); + self::baz(); + } + + protected static function bar(): void {} + + protected static function baz(): void { + echo 1; + echo 3; + echo 2; + } +} + +<<__EntryPoint>> +function main(): void { + A::foo(); +} \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChangeThenBack/a/input.hack b/tests/diff/beforeAnalysisMethodChangeThenBack/a/input.hack new file mode 100644 index 00000000..06201fdc --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChangeThenBack/a/input.hack @@ -0,0 +1,19 @@ +final class A { + public static function foo(): void { + self::bar(); + self::baz(); + } + + protected static function bar(): void {} + + protected static function baz(): void { + echo 1; + + echo 2; + } +} + +<<__EntryPoint>> +function main(): void { + A::foo(); +} \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChangeThenBack/b/input.hack b/tests/diff/beforeAnalysisMethodChangeThenBack/b/input.hack new file mode 100644 index 00000000..06201fdc --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChangeThenBack/b/input.hack @@ -0,0 +1,19 @@ +final class A { + public static function foo(): void { + self::bar(); + self::baz(); + } + + protected static function bar(): void {} + + protected static function baz(): void { + echo 1; + + echo 2; + } +} + +<<__EntryPoint>> +function main(): void { + A::foo(); +} \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChangeThenBack/output.txt b/tests/diff/beforeAnalysisMethodChangeThenBack/output.txt new file mode 100644 index 00000000..e69de29b diff --git a/tests/diff/beforeAnalysisMethodChangeThenForward/a-before-analysis/input.hack b/tests/diff/beforeAnalysisMethodChangeThenForward/a-before-analysis/input.hack new file mode 100644 index 00000000..c9d1aa86 --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChangeThenForward/a-before-analysis/input.hack @@ -0,0 +1,19 @@ +final class A { + public static function foo(): void { + self::bar(); + self::baz(); + } + + protected static function bar(): void {} + + protected static function baz(): void { + echo 1; + echo 3; + echo 2; + } +} + +<<__EntryPoint>> +function main(): void { + A::foo(); +} \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChangeThenForward/a/input.hack b/tests/diff/beforeAnalysisMethodChangeThenForward/a/input.hack new file mode 100644 index 00000000..06201fdc --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChangeThenForward/a/input.hack @@ -0,0 +1,19 @@ +final class A { + public static function foo(): void { + self::bar(); + self::baz(); + } + + protected static function bar(): void {} + + protected static function baz(): void { + echo 1; + + echo 2; + } +} + +<<__EntryPoint>> +function main(): void { + A::foo(); +} \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChangeThenForward/b/input.hack b/tests/diff/beforeAnalysisMethodChangeThenForward/b/input.hack new file mode 100644 index 00000000..382f2ddf --- /dev/null +++ b/tests/diff/beforeAnalysisMethodChangeThenForward/b/input.hack @@ -0,0 +1,19 @@ +final class A { + public static function foo(): void { + self::bar(); + self::baz(); + } + + protected static function bar(): void {} + + protected static function baz(): void { + echo 1; + echo 4; + echo 2; + } +} + +<<__EntryPoint>> +function main(): void { + A::foo(); +} \ No newline at end of file diff --git a/tests/diff/beforeAnalysisMethodChangeThenForward/output.txt b/tests/diff/beforeAnalysisMethodChangeThenForward/output.txt new file mode 100644 index 00000000..e69de29b diff --git a/tests/unused/UnusedExpression/nestedAssignment/input.hack b/tests/unused/UnusedExpression/nestedAssignment/input.hack new file mode 100644 index 00000000..6d2bc336 --- /dev/null +++ b/tests/unused/UnusedExpression/nestedAssignment/input.hack @@ -0,0 +1,11 @@ +function foo() : void { + $a = 0; + $b = 0; + + while (rand(0, 1)) { + $a = $a + 1; + $b = $b + 1; + } + + echo $a; +} \ No newline at end of file diff --git a/tests/unused/UnusedExpression/nestedAssignment/output.txt b/tests/unused/UnusedExpression/nestedAssignment/output.txt new file mode 100644 index 00000000..4d1b8388 --- /dev/null +++ b/tests/unused/UnusedExpression/nestedAssignment/output.txt @@ -0,0 +1,2 @@ +ERROR: UnusedAssignment - input.hack:3:3 - Assignment to $b is unused +ERROR: UnusedAssignment - input.hack:7:5 - Assignment to $b is unused \ No newline at end of file