From a9ed88b600b74f6851b46313c8ec2292c6458137 Mon Sep 17 00:00:00 2001 From: Christopher Chang <51393127+chriscerie@users.noreply.github.com> Date: Wed, 18 Oct 2023 12:34:14 -0700 Subject: [PATCH] Move iteration logic to get_applied_suggestions_code --- .vscode/launch.json | 83 +++++++++++++++++++++++ selene-lib/src/lints.rs | 107 ++++++++++++++++++------------ selene-lib/src/lints/test_util.rs | 64 ++++++++++-------- selene/src/main.rs | 41 +++++++----- 4 files changed, 208 insertions(+), 87 deletions(-) create mode 100644 .vscode/launch.json diff --git a/.vscode/launch.json b/.vscode/launch.json new file mode 100644 index 00000000..f93ddc80 --- /dev/null +++ b/.vscode/launch.json @@ -0,0 +1,83 @@ +{ + // Use IntelliSense to learn about possible attributes. + // Hover to view descriptions of existing attributes. + // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387 + "version": "0.2.0", + "configurations": [ + { + "type": "lldb", + "request": "launch", + "name": "Debug executable 'selene'", + "cargo": { + "args": [ + "build", + "--bin=selene", + "--package=selene" + ], + "filter": { + "name": "selene", + "kind": "bin" + } + }, + "args": [], + "cwd": "${workspaceFolder}/selene-lib" + }, + { + "type": "lldb", + "request": "launch", + "name": "Debug unit tests in executable 'selene'", + "cargo": { + "args": [ + "test", + "--no-run", + "--bin=selene", + "--package=selene" + ], + "filter": { + "name": "selene", + "kind": "bin" + } + }, + "args": [], + "cwd": "${workspaceFolder}" + }, + { + "type": "lldb", + "request": "launch", + "name": "Debug unit tests in library 'selene-lib'", + "cargo": { + "args": [ + "test", + "--no-run", + "--lib", + "--package=selene-lib" + ], + "filter": { + "name": "selene-lib", + "kind": "lib" + } + }, + "args": [], + "cwd": "${workspaceFolder}" + }, + { + "type": "lldb", + "request": "launch", + "name": "Debug integration test 'checker'", + "cargo": { + "args": [ + "test", + "--no-run", + "--test=checker", + "--package=selene-lib" + ], + "filter": { + "name": "checker", + "kind": "test" + } + }, + "args": [], + "cwd": "${workspaceFolder}" + } + ] +} \ No newline at end of file diff --git a/selene-lib/src/lints.rs b/selene-lib/src/lints.rs index 153cc3a1..514528b1 100644 --- a/selene-lib/src/lints.rs +++ b/selene-lib/src/lints.rs @@ -196,24 +196,79 @@ impl Diagnostic { self.fixed_code.is_some() && self.applicability == Applicability::MachineApplicable } - /// 1. Applies all disjoint fixes - /// 1. If a fix is completely contained inside another fix, uses the inner one and discard the outer one - /// 2. If fixes partially overlap, chooses the fix that starts first and discard the other one - pub fn get_applied_suggestions_code(code: &str, diagnostics: &[&Diagnostic]) -> String { + pub fn has_maybe_incorrect_fix(&self) -> bool { + self.fixed_code.is_some() && self.applicability == Applicability::MaybeIncorrect + } + + /// After applying suggestions, calls `get_new_diagnostics` and reruns to ensure fixes didn't produce new errors + pub fn get_applied_suggestions_code( + code: &str, + diagnostics: Vec<&Diagnostic>, + get_new_diagnostics: F, + ) -> String + where + F: Fn(&str) -> Vec, + { + let mut chosen_diagnostics = Self::get_independent_suggestions(&diagnostics); + let mut owned_diagnostics_ref; + let mut owned_diagnostics; + + let mut fixed_code = code.to_string(); + + while !chosen_diagnostics.is_empty() { + let mut bytes_offset = 0; + + for diagnostic in chosen_diagnostics { + if let Some(suggestion) = &diagnostic.fixed_code { + let (start, end) = diagnostic.primary_label.range; + + // This conversion can theoretically overflow, but it's tied to string length so the user + // would face memory constraints of such large strings long before this is an issue + let start_with_offset = start as isize + bytes_offset; + let end_with_offset = end as isize + bytes_offset; + + fixed_code = format!( + "{}{}{}", + // Conversion is safe as this algorithm guarantees range + offset will never be negative + &fixed_code[..start_with_offset as usize], + suggestion.as_str(), + &fixed_code[(end_with_offset as usize)..] + ); + + bytes_offset += + suggestion.len() as isize - (end_with_offset - start_with_offset); + } + } + + owned_diagnostics = get_new_diagnostics(&fixed_code); + owned_diagnostics_ref = owned_diagnostics.iter().collect::>(); + chosen_diagnostics = Self::get_independent_suggestions(&owned_diagnostics_ref); + } + + fixed_code + } + + /// * Chooses all disjoint suggestions + /// * If a suggestion is completely contained inside others, uses the innermost one and discards the outer ones + /// * If suggestions partially overlap, chooses the one that starts first and discard the other ones + fn get_independent_suggestions<'a>(diagnostics: &'a [&'a Diagnostic]) -> Vec<&'a Diagnostic> { let mut sorted_diagnostics = diagnostics .iter() - .filter(|diagnostic| diagnostic.has_machine_applicable_fix()) + .filter(|&&diagnostic| { + diagnostic.has_machine_applicable_fix() || diagnostic.has_maybe_incorrect_fix() + }) + .copied() .collect::>(); sorted_diagnostics.sort_by_key(|diagnostic| diagnostic.start_position()); let mut chosen_diagnostics = Vec::new(); let mut candidate = match sorted_diagnostics.first() { - Some(first) => first, - None => return code.to_string(), + Some(first) => *first, + None => return Vec::new(), }; - for diagnostic in sorted_diagnostics.iter().skip(1) { + for &diagnostic in sorted_diagnostics.iter().skip(1) { let this_range = diagnostic.primary_label.range; if this_range.1 <= candidate.primary_label.range.1 { @@ -229,41 +284,7 @@ impl Diagnostic { } chosen_diagnostics.push(candidate); - let mut bytes_offset = 0; - - let new_code = chosen_diagnostics - .iter() - .fold(code.to_string(), |code, diagnostic| { - if let Some(fixed) = &diagnostic.fixed_code { - let (start, end) = diagnostic.primary_label.range; - - // This can theoretically overflow, but the user would face memory constraints - // of such large strings long before - let start_with_offset = start as isize + bytes_offset; - let end_with_offset = end as isize + bytes_offset; - - let new_code = if start_with_offset > end_with_offset - || end_with_offset > code.len().try_into().unwrap() - { - code.to_string() - } else { - format!( - "{}{}{}", - // Conversion is safe as range with offset will never be negative - &code[..start_with_offset as usize], - fixed.as_str(), - &code[(end_with_offset as usize)..] - ) - }; - - bytes_offset += fixed.len() as isize - (end_with_offset - start_with_offset); - new_code - } else { - code - } - }); - - new_code + chosen_diagnostics } } diff --git a/selene-lib/src/lints/test_util.rs b/selene-lib/src/lints/test_util.rs index de819bb9..8122316e 100644 --- a/selene-lib/src/lints/test_util.rs +++ b/selene-lib/src/lints/test_util.rs @@ -45,7 +45,7 @@ fn generate_diff(source1: &str, source2: &str, diagnostics: &[&Diagnostic]) -> S for change in TextDiff::from_lines(source1, source2).iter_all_changes() { let change_length = change.value().len() as u32; - let change_end_byte = byte_offset + change_length as u32; + let change_end_byte = byte_offset + change_length; let mut applicability_prefix = " "; for diagnostic in diagnostics { @@ -139,35 +139,45 @@ pub fn test_lint_config_with_output< || diagnostic.applicability == Applicability::MaybeIncorrect) }) .collect::>(); - let mut lint_results; - - // To handle potential conflicts with different lint suggestions, we apply conflicting fixes one at a time. - // Then we re-evaluate the lints with the new code until there are no more fixes to apply - while fixed_diagnostics.iter().any(|diagnostic| { - diagnostic.fixed_code.is_some() - && (diagnostic.applicability == Applicability::MachineApplicable - || diagnostic.applicability == Applicability::MaybeIncorrect) - }) { - fixed_code = - Diagnostic::get_applied_suggestions_code(fixed_code.as_str(), &fixed_diagnostics); - - let fixed_ast = full_moon::parse(&fixed_code).unwrap_or_else(|_| { - panic!( - "Fixer generated invalid code:\n\ + + fixed_code = Diagnostic::get_applied_suggestions_code( + fixed_code.as_str(), + fixed_diagnostics, + |new_code| { + println!("Fixer generated code:\n{}", new_code); + let fixed_ast = full_moon::parse(new_code).unwrap_or_else(|_| { + panic!( + "Fixer generated invalid code:\n\ + ----------------\n\ + {}\n\ + ----------------\n", + new_code + ) + }); + lint.pass( + &fixed_ast, + &context, + &AstContext::from_ast(&fixed_ast, &new_code.to_string()), + ) + }, + ); + + let fixed_ast = full_moon::parse(&fixed_code).unwrap_or_else(|_| { + panic!( + "Fixer generated invalid code:\n\ ----------------\n\ {}\n\ ----------------\n", - fixed_code - ) - }); - lint_results = lint.pass( - &fixed_ast, - &context, - &AstContext::from_ast(&fixed_ast, &fixed_code), - ); - fixed_diagnostics = lint_results.iter().collect::>(); - fixed_diagnostics.sort_by_key(|diagnostic| diagnostic.start_position()); - } + fixed_code + ) + }); + let lint_results = lint.pass( + &fixed_ast, + &context, + &AstContext::from_ast(&fixed_ast, &fixed_code), + ); + fixed_diagnostics = lint_results.iter().collect::>(); + fixed_diagnostics.sort_by_key(|diagnostic| diagnostic.start_position()); let fixed_diff = generate_diff( &lua_source, diff --git a/selene/src/main.rs b/selene/src/main.rs index 6b2d2f8c..375ea57c 100644 --- a/selene/src/main.rs +++ b/selene/src/main.rs @@ -295,27 +295,34 @@ fn read( .filter(|diagnostic| diagnostic.diagnostic.has_machine_applicable_fix()) .count(); - // To handle potential conflicts with different lint suggestions, we apply conflicting fixes one at a time. - // Then we re-evaluate the lints with the new code until there are no more fixes to apply - while diagnostics - .iter() - .any(|diagnostic| diagnostic.diagnostic.has_machine_applicable_fix()) - { - fixed_code = Diagnostic::get_applied_suggestions_code( - fixed_code.as_str(), - &diagnostics - .iter() - .map(|diagnostic| &diagnostic.diagnostic) - .collect::>(), - ); + fixed_code = Diagnostic::get_applied_suggestions_code( + fixed_code.as_str(), + diagnostics + .iter() + .filter(|diagnostic| diagnostic.diagnostic.has_machine_applicable_fix()) + .map(|diagnostic| &diagnostic.diagnostic) + .collect::>(), + |new_code| { + let new_ast = full_moon::parse(&new_code).expect( + "selene tried applying lint suggestions, but it generated invalid code that could not be parsed; \ + this is likely a selene bug", + ); - let fixed_ast = full_moon::parse(&fixed_code).expect( + checker + .test_on(&new_ast, &new_code.to_string()) + .into_iter() + .filter(|diagnostic| diagnostic.diagnostic.has_machine_applicable_fix()) + .map(|diagnostic| diagnostic.diagnostic) + .collect::>() + }, + ); + + let fixed_ast = full_moon::parse(&fixed_code).expect( "selene tried applying lint suggestions, but it generated invalid code that could not be parsed; \ this is likely a selene bug", ); - diagnostics = checker.test_on(&fixed_ast, &fixed_code); - diagnostics.sort_by_key(|diagnostic| diagnostic.diagnostic.start_position()); - } + diagnostics = checker.test_on(&fixed_ast, &fixed_code); + diagnostics.sort_by_key(|diagnostic| diagnostic.diagnostic.start_position()); if num_fixes > 0 { if fs::write(filename, fixed_code.clone()).is_ok() {