Skip to content

Commit

Permalink
Move iteration logic to get_applied_suggestions_code
Browse files Browse the repository at this point in the history
  • Loading branch information
@chriscerie
chriscerie committed Oct 18, 2023
1 parent ff082cf commit a9ed88b
Show file tree
Hide file tree
Showing 4 changed files with 208 additions and 87 deletions.
83 changes: 83 additions & 0 deletions .vscode/launch.json
View file
Original file line number Diff line number Diff line change
@@ -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}"
}
]
}
107 changes: 64 additions & 43 deletions selene-lib/src/lints.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<F>(
code: &str,
diagnostics: Vec<&Diagnostic>,
get_new_diagnostics: F,
) -> String
where
F: Fn(&str) -> Vec<Diagnostic>,
{
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::<Vec<_>>();
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::<Vec<_>>();
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 {
Expand All @@ -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
}
}

Expand Down
64 changes: 37 additions & 27 deletions selene-lib/src/lints/test_util.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -139,35 +139,45 @@ pub fn test_lint_config_with_output<
|| diagnostic.applicability == Applicability::MaybeIncorrect)
})
.collect::<Vec<_>>();
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::<Vec<_>>();
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::<Vec<_>>();
fixed_diagnostics.sort_by_key(|diagnostic| diagnostic.start_position());

let fixed_diff = generate_diff(
&lua_source,
Expand Down
41 changes: 24 additions & 17 deletions selene/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,27 +295,34 @@ fn read<R: 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::<Vec<_>>(),
);
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::<Vec<_>>(),
|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::<Vec<_>>()
},
);

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() {
Expand Down

0 comments on commit a9ed88b

Please sign in to comment.