Skip to content

Commit

Permalink
Handle conflicting fixes
Browse files Browse the repository at this point in the history
  • Loading branch information
@chriscerie
chriscerie committed Oct 14, 2023
1 parent 0560a86 commit 5417142
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 39 deletions.
78 changes: 57 additions & 21 deletions selene-lib/src/lints/test_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,34 @@ fn replace_code_range(code: &str, start: usize, end: usize, replacement: &str) -
return format!("{}{}{}", &code[..start], replacement, &code[end..]);
}

// Assumes diagnostics is sorted by starting ranges and that there are no overlapping ranges
/// Assumes diagnostics is sorted by starting positions
///
/// 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
fn apply_diagnostics_fixes(code: &str, diagnostics: &Vec<&Diagnostic>) -> String {
let mut chosen_diagnostics = Vec::new();

let mut candidate = diagnostics[0];
for diagnostic in diagnostics.iter().skip(1) {
let this_range = diagnostic.primary_label.range;

if this_range.1 <= candidate.primary_label.range.1 {
// Completely contained inside
candidate = diagnostic;
} else if this_range.0 <= candidate.primary_label.range.1 {
// Partially overlapping
continue;
} else {
chosen_diagnostics.push(candidate);
candidate = diagnostic;
}
}
chosen_diagnostics.push(candidate);

let mut bytes_offset = 0;

let new_code = diagnostics
let new_code = chosen_diagnostics
.iter()
.fold(code.to_string(), |code, diagnostic| {
if let Some(fixed) = &diagnostic.fixed_code {
Expand All @@ -66,8 +89,6 @@ fn apply_diagnostics_fixes(code: &str, diagnostics: &Vec<&Diagnostic>) -> String
}
});

full_moon::parse(&new_code).expect("Failed to parse fixed code");

new_code
}

Expand Down Expand Up @@ -111,28 +132,38 @@ pub fn test_lint_config_with_output<
fs::read_to_string(path_base.with_extension("lua")).expect("Cannot find lua file");

let ast = full_moon::parse(&lua_source).expect("Cannot parse lua file");
let mut diagnostics = lint.pass(
&ast,
&Context {
standard_library: config.standard_library,
user_set_standard_library: if standard_library_is_set {
Some(vec!["test-set".to_owned()])
} else {
None
},
},
&AstContext::from_ast(&ast),
);

let mut files = codespan::Files::new();
let source_id = files.add(format!("{test_name}.lua"), lua_source.clone());
let context = Context {
standard_library: config.standard_library,
user_set_standard_library: if standard_library_is_set {
Some(vec!["test-set".to_owned()])
} else {
None
},
};

let mut diagnostics = lint.pass(&ast, &context, &AstContext::from_ast(&ast));
diagnostics.sort_by_key(|diagnostic| diagnostic.primary_label.range);

let mut output = termcolor::NoColor::new(Vec::new());
let mut fixed_code = lua_source.to_string();
let mut fixed_diagnostics = diagnostics.iter().collect::<Vec<_>>();
let mut lint_results;

let fixed_lua_code = apply_diagnostics_fixes(&lua_source, &diagnostics.iter().collect());
let fixed_diff = generate_diff(&lua_source, &fixed_lua_code);
// 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())
{
fixed_code = apply_diagnostics_fixes(fixed_code.as_str(), &fixed_diagnostics);

let fixed_ast = full_moon::parse(&fixed_code).expect("Fix generated invalid code");
lint_results = lint.pass(&fixed_ast, &context, &AstContext::from_ast(&fixed_ast));
fixed_diagnostics = lint_results.iter().collect::<Vec<_>>();
fixed_diagnostics.sort_by_key(|diagnostic| diagnostic.primary_label.range);
}

let fixed_diff = generate_diff(&lua_source, &fixed_code);
let diff_output_path = path_base.with_extension("fixed.diff");

if let Ok(expected) = fs::read_to_string(&diff_output_path) {
Expand All @@ -145,6 +176,11 @@ pub fn test_lint_config_with_output<
.expect("couldn't write fixed.diff to output file");
}

let mut files = codespan::Files::new();
let source_id = files.add(format!("{test_name}.lua"), lua_source.clone());

let mut output = termcolor::NoColor::new(Vec::new());

for diagnostic in diagnostics
.into_iter()
.map(|diagnostic| diagnostic.into_codespan_diagnostic(source_id, CodespanSeverity::Error))
Expand Down
73 changes: 55 additions & 18 deletions selene/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ use std::{

use codespan_reporting::{
diagnostic::{
self, Diagnostic as CodespanDiagnostic, Label as CodespanLabel,
Severity as CodespanSeverity,
Diagnostic as CodespanDiagnostic, Label as CodespanLabel, Severity as CodespanSeverity,
},
term::DisplayStyle as CodespanDisplayStyle,
};
Expand Down Expand Up @@ -191,13 +190,36 @@ fn replace_code_range(code: &str, start: usize, end: usize, replacement: &str) -
return format!("{}{}{}", &code[..start], replacement, &code[end..]);
}

// Assumes diagnostics is sorted by starting ranges and that there are no overlapping ranges
/// Assumes diagnostics is sorted by starting positions
///
/// 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
///
// This is just copied from `test_util`. Can we get a better abstraction?
// FIXME: handle the overlapping ranges
fn apply_diagnostics_fixes(code: &str, diagnostics: &Vec<&Diagnostic>) -> String {
let mut chosen_diagnostics = Vec::new();

let mut candidate = diagnostics[0];
for diagnostic in diagnostics.iter().skip(1) {
let this_range = diagnostic.primary_label.range;

if this_range.1 <= candidate.primary_label.range.1 {
// Completely contained inside
candidate = diagnostic;
} else if this_range.0 <= candidate.primary_label.range.1 {
// Partially overlapping
continue;
} else {
chosen_diagnostics.push(candidate);
candidate = diagnostic;
}
}
chosen_diagnostics.push(candidate);

let mut bytes_offset = 0;

let new_code = diagnostics
let new_code = chosen_diagnostics
.iter()
.fold(code.to_string(), |code, diagnostic| {
if let Some(fixed) = &diagnostic.fixed_code {
Expand All @@ -216,8 +238,6 @@ fn apply_diagnostics_fixes(code: &str, diagnostics: &Vec<&Diagnostic>) -> String
}
});

full_moon::parse(&new_code).expect("Failed to parse fixed code");

new_code
}

Expand Down Expand Up @@ -321,6 +341,34 @@ fn read<R: Read>(
let mut diagnostics = checker.test_on(&ast);
diagnostics.sort_by_key(|diagnostic| diagnostic.diagnostic.start_position());

if is_fix {
let mut fixed_code = contents.as_ref().to_string();

// 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.fixed_code.is_some())
{
fixed_code = apply_diagnostics_fixes(
fixed_code.as_str(),
&diagnostics
.iter()
.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);
diagnostics.sort_by_key(|diagnostic| diagnostic.diagnostic.start_position());
}

let _ = fs::write(filename, fixed_code);
}

let (mut errors, mut warnings) = (0, 0);
for diagnostic in &diagnostics {
match diagnostic.severity {
Expand All @@ -336,17 +384,6 @@ fn read<R: Read>(
let stdout = termcolor::StandardStream::stdout(get_color());
let mut stdout = stdout.lock();

if is_fix {
let fixed_code = apply_diagnostics_fixes(
contents.as_ref(),
&diagnostics
.iter()
.map(|diagnostic| &diagnostic.diagnostic)
.collect(),
);
let _ = fs::write(filename, fixed_code);
}

for diagnostic in diagnostics {
if opts.luacheck {
// Existing Luacheck consumers presumably use --formatter plain
Expand Down

0 comments on commit 5417142

Please sign in to comment.