diff --git a/selene-lib/src/lints.rs b/selene-lib/src/lints.rs index 1189071d..3949dbdd 100644 --- a/selene-lib/src/lints.rs +++ b/selene-lib/src/lints.rs @@ -129,14 +129,20 @@ impl Diagnostic { fixed_code: Option, applicability: Applicability, ) -> Self { + let notes = if let Some(ref fixed_code_str) = fixed_code { + vec![format!("try: `{}`", fixed_code_str)] + } else { + Vec::new() + }; + Self { code, message, primary_label, fixed_code, applicability, + notes, - notes: Vec::new(), secondary_labels: Vec::new(), } } diff --git a/selene-lib/src/lints/almost_swapped.rs b/selene-lib/src/lints/almost_swapped.rs index 10c81372..7ed9eb91 100644 --- a/selene-lib/src/lints/almost_swapped.rs +++ b/selene-lib/src/lints/almost_swapped.rs @@ -31,7 +31,7 @@ impl Lint for AlmostSwappedLint { .almost_swaps .iter() .map(|almost_swap| { - Diagnostic::new_complete( + Diagnostic::new( "almost_swapped", format!( "this looks like you are trying to swap `{}` and `{}`", @@ -39,14 +39,14 @@ impl Lint for AlmostSwappedLint { (almost_swap.names.1), ), Label::new(almost_swap.range), - vec![format!( - "try: `{name1}, {name2} = {name2}, {name1}`", + Some(format!( + "{name1}, {name2} = {name2}, {name1}", name1 = almost_swap.names.0, name2 = almost_swap.names.1, - )], - Vec::new(), - None, - Applicability::Unspecified, + )), + // FIXME: Ideally `MaybeIncorrect`, but cannot because the end range of swapping `t[1]` and `t[2]` + // doesn't include the closing `]` + Applicability::HasPlaceholders, ) }) .collect() diff --git a/selene-lib/src/lints/compare_nan.rs b/selene-lib/src/lints/compare_nan.rs index 9b05cb2f..56527a78 100644 --- a/selene-lib/src/lints/compare_nan.rs +++ b/selene-lib/src/lints/compare_nan.rs @@ -36,12 +36,10 @@ impl Lint for CompareNanLint { operator = comparisons.operator, ); - Diagnostic::new_complete( + Diagnostic::new( "compare_nan", "comparing things to nan directly is not allowed".to_owned(), Label::new(comparisons.range), - vec![format!("try: `{}` instead", fixed_code.clone())], - Vec::new(), Some(fixed_code), Applicability::MaybeIncorrect, ) diff --git a/selene-lib/src/lints/constant_table_comparison.rs b/selene-lib/src/lints/constant_table_comparison.rs index b19cb53a..356c84b8 100644 --- a/selene-lib/src/lints/constant_table_comparison.rs +++ b/selene-lib/src/lints/constant_table_comparison.rs @@ -32,35 +32,37 @@ impl Lint for ConstantTableComparisonLint { .comparisons .iter() .map(|comparison| { - Diagnostic::new_complete( + let suggestion = comparison.empty_side.as_ref().map(|empty_side| { + format!( + "next({}) {} nil", + match empty_side { + EmptyComparison::CheckEmpty(side) => match side { + EmptyComparisonSide::Left => &comparison.rhs, + EmptyComparisonSide::Right => &comparison.lhs, + }, + + EmptyComparison::CheckNotEmpty(side) => match side { + EmptyComparisonSide::Left => &comparison.rhs, + EmptyComparisonSide::Right => &comparison.lhs, + }, + }, + match empty_side { + EmptyComparison::CheckEmpty(_) => "==", + EmptyComparison::CheckNotEmpty(_) => "~=", + } + ) + }); + + Diagnostic::new( "constant_table_comparison", "comparing to a constant table will always fail".to_owned(), Label::new(comparison.range), - if let Some(empty_side) = comparison.empty_side { - vec![format!( - "try: `next({}) {} nil`", - match empty_side { - EmptyComparison::CheckEmpty(side) => match side { - EmptyComparisonSide::Left => &comparison.rhs, - EmptyComparisonSide::Right => &comparison.lhs, - }, - - EmptyComparison::CheckNotEmpty(side) => match side { - EmptyComparisonSide::Left => &comparison.rhs, - EmptyComparisonSide::Right => &comparison.lhs, - }, - }, - match empty_side { - EmptyComparison::CheckEmpty(_) => "==", - EmptyComparison::CheckNotEmpty(_) => "~=", - } - )] + suggestion.clone(), + if suggestion.is_some() { + Applicability::MaybeIncorrect } else { - Vec::new() + Applicability::Unspecified }, - Vec::new(), - None, - Applicability::Unspecified, ) }) .collect() diff --git a/selene-lib/src/lints/standard_library.rs b/selene-lib/src/lints/standard_library.rs index c1b3e405..2a0795e8 100644 --- a/selene-lib/src/lints/standard_library.rs +++ b/selene-lib/src/lints/standard_library.rs @@ -463,7 +463,7 @@ impl Visitor for StandardLibraryVisitor<'_> { let name = name_path.pop().unwrap(); - self.diagnostics.push(Diagnostic::new_complete( + self.diagnostics.push(Diagnostic::new( "incorrect_standard_library_use", format!( "standard library function `{}{}{}` {}", @@ -473,15 +473,13 @@ impl Visitor for StandardLibraryVisitor<'_> { problem, ), Label::from_node(call, None), - vec![format!( - "try: {}{}{}(...)", + Some(format!( + "{}{}{}(...)", name_path.join("."), use_instead, name - )], - Vec::new(), - None, - Applicability::Unspecified, + )), + Applicability::HasPlaceholders, )); return; diff --git a/selene-lib/src/lints/test_util.rs b/selene-lib/src/lints/test_util.rs index 4cd6e8d7..0000da1b 100644 --- a/selene-lib/src/lints/test_util.rs +++ b/selene-lib/src/lints/test_util.rs @@ -1,4 +1,4 @@ -use super::{AstContext, Context, Diagnostic, Lint}; +use super::{Applicability, AstContext, Context, Diagnostic, Lint}; use crate::{ test_util::{get_standard_library, PrettyString}, StandardLibrary, @@ -48,7 +48,7 @@ fn replace_code_range(code: &str, start: usize, end: usize, replacement: &str) - /// 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 { +fn apply_diagnostics_fixes(code: &str, diagnostics: &[&Diagnostic]) -> String { let mut chosen_diagnostics = Vec::new(); let mut candidate = diagnostics[0]; @@ -77,9 +77,9 @@ fn apply_diagnostics_fixes(code: &str, diagnostics: &Vec<&Diagnostic>) -> String let (start, end) = diagnostic.primary_label.range; let new_code = replace_code_range( code.as_str(), - (start as isize + bytes_offset as isize) as usize, - (end as isize + bytes_offset as isize) as usize, - &fixed.as_str(), + (start as isize + bytes_offset) as usize, + (end as isize + bytes_offset) as usize, + fixed.as_str(), ); bytes_offset += fixed.len() as isize - (end - start) as isize; @@ -148,25 +148,32 @@ pub fn test_lint_config_with_output< let mut fixed_code = lua_source.to_string(); let mut fixed_diagnostics = diagnostics .iter() - .filter(|diagnostic| diagnostic.fixed_code.is_some()) + .filter(|diagnostic| { + diagnostic.fixed_code.is_some() + && (diagnostic.applicability == Applicability::MachineApplicable + || 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()) - { + while fixed_diagnostics.iter().any(|diagnostic| { + diagnostic.fixed_code.is_some() + && (diagnostic.applicability == Applicability::MachineApplicable + || diagnostic.applicability == Applicability::MaybeIncorrect) + }) { fixed_code = apply_diagnostics_fixes(fixed_code.as_str(), &fixed_diagnostics); - let fixed_ast = full_moon::parse(&fixed_code).expect(&format!( - "Fixer generated invalid code:\n\ - ----------------\n\ - {}\n\ - ----------------\n", - fixed_code - )); + 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, diff --git a/selene-lib/tests/lints/almost_swapped/almost_swapped.fixed.diff b/selene-lib/tests/lints/almost_swapped/almost_swapped.fixed.diff new file mode 100644 index 00000000..fb054179 --- /dev/null +++ b/selene-lib/tests/lints/almost_swapped/almost_swapped.fixed.diff @@ -0,0 +1,16 @@ + x, y = y, x + + a = b + b = a + + t[1], t[2] = t[2], t[1] + + t[1] = t[2] + t[2] = t[1] + + foo().a = foo().b + foo().b = foo().a + + -- We use a weird hack so this comment might break something, oh no! + a = b + b = a diff --git a/selene-lib/tests/lints/almost_swapped/panic.fixed.diff b/selene-lib/tests/lints/almost_swapped/panic.fixed.diff new file mode 100644 index 00000000..1c06c479 --- /dev/null +++ b/selene-lib/tests/lints/almost_swapped/panic.fixed.diff @@ -0,0 +1 @@ + foo = "bar" \ No newline at end of file diff --git a/selene-lib/tests/lints/compare_nan/compare_nan_if.stderr b/selene-lib/tests/lints/compare_nan/compare_nan_if.stderr index 97045662..34d34c6f 100644 --- a/selene-lib/tests/lints/compare_nan/compare_nan_if.stderr +++ b/selene-lib/tests/lints/compare_nan/compare_nan_if.stderr @@ -4,7 +4,7 @@ error[compare_nan]: comparing things to nan directly is not allowed 1 │ if x == 0/0 then │ ^^^^^^^^ │ - = try: `x ~= x` instead + = try: `x ~= x` error[compare_nan]: comparing things to nan directly is not allowed ┌─ compare_nan_if.lua:4:4 @@ -12,5 +12,5 @@ error[compare_nan]: comparing things to nan directly is not allowed 4 │ if x ~= 0/0 then │ ^^^^^^^^ │ - = try: `x == x` instead + = try: `x == x` diff --git a/selene-lib/tests/lints/compare_nan/compare_nan_variables.stderr b/selene-lib/tests/lints/compare_nan/compare_nan_variables.stderr index 0aca91c0..776c66bb 100644 --- a/selene-lib/tests/lints/compare_nan/compare_nan_variables.stderr +++ b/selene-lib/tests/lints/compare_nan/compare_nan_variables.stderr @@ -4,7 +4,7 @@ error[compare_nan]: comparing things to nan directly is not allowed 6 │ local _ = x ~= 0/0 │ ^^^^^^^^ │ - = try: `x == x` instead + = try: `x == x` error[compare_nan]: comparing things to nan directly is not allowed ┌─ compare_nan_variables.lua:7:11 @@ -12,5 +12,5 @@ error[compare_nan]: comparing things to nan directly is not allowed 7 │ local _ = x == 0/0 │ ^^^^^^^^ │ - = try: `x ~= x` instead + = try: `x ~= x` diff --git a/selene-lib/tests/lints/constant_table_comparison/constant_table_comparison.fixed.diff b/selene-lib/tests/lints/constant_table_comparison/constant_table_comparison.fixed.diff new file mode 100644 index 00000000..b1004f99 --- /dev/null +++ b/selene-lib/tests/lints/constant_table_comparison/constant_table_comparison.fixed.diff @@ -0,0 +1,20 @@ + print(x == { "a", "b", "c" }) + print({ "a", "b", "c" } == x) + +-print(x == {}) +-print({} == x) +-print(x ~= {}) +-print({} ~= x) ++print(next(x) == nil) ++print(next(x) == nil) ++print(next(x) ~= nil) ++print(next(x) ~= nil) + + print({ "a", "b", "c" } == { "a", "b", "c" }) + print({ "a", "b", "c" } == {}) + print({} == {}) + + print( -- my cool table +- t == {} ++ next(t) == nil + ) diff --git a/selene-lib/tests/lints/roblox_incorrect_roact_usage/old_roblox_std.fixed.diff b/selene-lib/tests/lints/roblox_incorrect_roact_usage/old_roblox_std.fixed.diff new file mode 100644 index 00000000..58f63e35 --- /dev/null +++ b/selene-lib/tests/lints/roblox_incorrect_roact_usage/old_roblox_std.fixed.diff @@ -0,0 +1 @@ + Roact.createElement("Frame") diff --git a/selene-lib/tests/lints/standard_library/any.fixed.diff b/selene-lib/tests/lints/standard_library/any.fixed.diff new file mode 100644 index 00000000..06562162 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/any.fixed.diff @@ -0,0 +1,6 @@ + print(foo) + print(foo.x) + print(foo.x.y.z) + print(foo:x()) + foo.x = 1 + foo.x.y = 2 diff --git a/selene-lib/tests/lints/standard_library/assert.fixed.diff b/selene-lib/tests/lints/standard_library/assert.fixed.diff new file mode 100644 index 00000000..fbbdbff8 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/assert.fixed.diff @@ -0,0 +1,9 @@ + assert(true, "message") + assert(call()) + assert(call(), "this is ok") + assert(...) + + assert(true) + assert(true, "message", call()) + assert(true, "message", ...) + assert() diff --git a/selene-lib/tests/lints/standard_library/bad_call_signatures.fixed.diff b/selene-lib/tests/lints/standard_library/bad_call_signatures.fixed.diff new file mode 100644 index 00000000..54b6bf98 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/bad_call_signatures.fixed.diff @@ -0,0 +1,20 @@ + print() + print("a", 2, "c") + math.sin(3) + math.sin(1 + 1) + math.random() + math.random(1) + print "hello" + -- + math.sin("i'd like the sine of three, please") + math.floor(3.5, "remember to make sure it's 3 :)") + math.sin "pi" + -- + local variable = 3 + math.sin(variable) + math.sin(variable, variable) + -- + math.max(2) + math.pi() + string.format(-2, "foo") + string.format(1 + 1) diff --git a/selene-lib/tests/lints/standard_library/callable_metatables.fixed.diff b/selene-lib/tests/lints/standard_library/callable_metatables.fixed.diff new file mode 100644 index 00000000..5e87e357 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/callable_metatables.fixed.diff @@ -0,0 +1,2 @@ + expect.extend({}) + expect(1).to.be.ok() diff --git a/selene-lib/tests/lints/standard_library/complex.fixed.diff b/selene-lib/tests/lints/standard_library/complex.fixed.diff new file mode 100644 index 00000000..061e23c9 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/complex.fixed.diff @@ -0,0 +1,3 @@ + print(require("foo").bar) + print(coroutine.wrap(print)()) + getmetatable({}).__index = function() end diff --git a/selene-lib/tests/lints/standard_library/constants.fixed.diff b/selene-lib/tests/lints/standard_library/constants.fixed.diff new file mode 100644 index 00000000..1c730fc6 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/constants.fixed.diff @@ -0,0 +1,9 @@ + collectgarbage("count") + collectgarbage "count" + + collectgarbage("doge") + collectgarbage(1) + + collectgarbage("coun" .. "t") + + collectgarbage() diff --git a/selene-lib/tests/lints/standard_library/if_expressions.fixed.diff b/selene-lib/tests/lints/standard_library/if_expressions.fixed.diff new file mode 100644 index 00000000..7a9eed3d --- /dev/null +++ b/selene-lib/tests/lints/standard_library/if_expressions.fixed.diff @@ -0,0 +1,5 @@ + math.floor(if x then 1 else 2) + math.floor(if x then "a" else "b") + + -- TODO: Let's try to capture this, right now the entire expression just resolves to no type + math.floor(if x then 1 else "a") diff --git a/selene-lib/tests/lints/standard_library/lua52.fixed.diff b/selene-lib/tests/lints/standard_library/lua52.fixed.diff new file mode 100644 index 00000000..c48a08c3 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/lua52.fixed.diff @@ -0,0 +1,2 @@ + table.unpack({}) + setfenv() -- This would error if it was defined, since setfenv requires an argument diff --git a/selene-lib/tests/lints/standard_library/math_on_types.fixed.diff b/selene-lib/tests/lints/standard_library/math_on_types.fixed.diff new file mode 100644 index 00000000..e64677c8 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/math_on_types.fixed.diff @@ -0,0 +1,2 @@ + usesType(x * 0.5) + usesType(0.5 * x) diff --git a/selene-lib/tests/lints/standard_library/method_call.fixed.diff b/selene-lib/tests/lints/standard_library/method_call.fixed.diff new file mode 100644 index 00000000..3612cd65 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/method_call.fixed.diff @@ -0,0 +1,4 @@ + foo:bar(1) + foo:bar("a") + foo.bar() + foo:baz() diff --git a/selene-lib/tests/lints/standard_library/method_call.stderr b/selene-lib/tests/lints/standard_library/method_call.stderr index fdbe503e..804b2f80 100644 --- a/selene-lib/tests/lints/standard_library/method_call.stderr +++ b/selene-lib/tests/lints/standard_library/method_call.stderr @@ -10,7 +10,7 @@ error[incorrect_standard_library_use]: standard library function `foo.bar` is a 3 │ foo.bar() │ ^^^^^^^^^ │ - = try: foo:bar(...) + = try: `foo:bar(...)` error[incorrect_standard_library_use]: standard library global `foo` does not contain the field `baz` ┌─ method_call.lua:4:1 diff --git a/selene-lib/tests/lints/standard_library/required.fixed.diff b/selene-lib/tests/lints/standard_library/required.fixed.diff new file mode 100644 index 00000000..b11db297 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/required.fixed.diff @@ -0,0 +1,11 @@ + -- ipairs requires an argument--this should fail. + ipairs() + + -- setmetatable requires a first argument, but not a second one. this should still fail. + setmetatable() + + -- This, however, should not. + setmetatable({}) + + -- nil counts as filling in an unrequired argument. + setmetatable({}, nil) diff --git a/selene-lib/tests/lints/standard_library/shadowing.fixed.diff b/selene-lib/tests/lints/standard_library/shadowing.fixed.diff new file mode 100644 index 00000000..d8219f6c --- /dev/null +++ b/selene-lib/tests/lints/standard_library/shadowing.fixed.diff @@ -0,0 +1,11 @@ + local math = {} + math.dog = 1 + math.sin = print + + math.sin("hi" + math.dog) + + function foo(table) + table.insert() + end + + table.insert() diff --git a/selene-lib/tests/lints/standard_library/string_interpolation.fixed.diff b/selene-lib/tests/lints/standard_library/string_interpolation.fixed.diff new file mode 100644 index 00000000..8e566826 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/string_interpolation.fixed.diff @@ -0,0 +1,7 @@ + collectgarbage(`count`) + collectgarbage(`ohno`) + + string.upper(`foo`) + string.upper(`foo{"bar"}`) + + math.floor(`oh no`) diff --git a/selene-lib/tests/lints/standard_library/ternary.fixed.diff b/selene-lib/tests/lints/standard_library/ternary.fixed.diff new file mode 100644 index 00000000..aec00402 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/ternary.fixed.diff @@ -0,0 +1,3 @@ + math.floor(r > 1 and 1 or 0) + math.floor(r > 1 + 1) + math.floor(not flag and 1 or 0) diff --git a/selene-lib/tests/lints/standard_library/unknown_property.fixed.diff b/selene-lib/tests/lints/standard_library/unknown_property.fixed.diff new file mode 100644 index 00000000..1e58a8ea --- /dev/null +++ b/selene-lib/tests/lints/standard_library/unknown_property.fixed.diff @@ -0,0 +1,9 @@ + print("3.14 = ", math.pie) + print(print.foo) + print(math.huge.big) + math.call() + + -- These aren't correct, but they should be covered under a different lint + call() + local _ = bad + print(foo.bar) diff --git a/selene-lib/tests/lints/standard_library/unpack_function_arguments.fixed.diff b/selene-lib/tests/lints/standard_library/unpack_function_arguments.fixed.diff new file mode 100644 index 00000000..1bbff94f --- /dev/null +++ b/selene-lib/tests/lints/standard_library/unpack_function_arguments.fixed.diff @@ -0,0 +1,12 @@ + -- Since functions can pass multiple parameters through a function if they're the last ones to be called, + -- selene needs to be able to recognize this and not error. + + -- This should not lint, since `unpack(stuff)` could pass in multiple parameters. + math.max(unpack(stuff)) + + -- This should lint, since functions only pass multiple parameters if they are the last argument. + -- debug.setlocal is used because it takes three required parameters. + debug.setlocal(unpack(stuff), 2) + + -- This is still wrong since it has too many parameters. + string.upper("text", unpack(stuff)) diff --git a/selene-lib/tests/lints/standard_library/vararg.fixed.diff b/selene-lib/tests/lints/standard_library/vararg.fixed.diff new file mode 100644 index 00000000..c31aa369 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/vararg.fixed.diff @@ -0,0 +1,3 @@ + return function(...) + table.insert(...) + end diff --git a/selene-lib/tests/lints/standard_library/wildcard.fixed.diff b/selene-lib/tests/lints/standard_library/wildcard.fixed.diff new file mode 100644 index 00000000..5b5d9bc1 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/wildcard.fixed.diff @@ -0,0 +1,12 @@ + script.ClassName = 3 + + script.Name = "Script" + script.Name.UhOh = "Oops" + + script.Child = "Oops" + script.Child.Name = "Okay" + + script.Child.Grandchild.Name = "Okay" + + print(x.y.z) + print(x.y.fail) diff --git a/selene-lib/tests/lints/standard_library/wildcard_structs.fixed.diff b/selene-lib/tests/lints/standard_library/wildcard_structs.fixed.diff new file mode 100644 index 00000000..d56adb03 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/wildcard_structs.fixed.diff @@ -0,0 +1,6 @@ + script.Name = "Okay" + script.Child = "Oops" + script.Child.Name = "Okay" + script.Child.Name.UhOh = "Oops" + script.Child.Grandchild = "Oops" + script.Child.Grandchild.Name = "Okay" diff --git a/selene-lib/tests/lints/standard_library/writing.fixed.diff b/selene-lib/tests/lints/standard_library/writing.fixed.diff new file mode 100644 index 00000000..883e4744 --- /dev/null +++ b/selene-lib/tests/lints/standard_library/writing.fixed.diff @@ -0,0 +1,6 @@ + _G.foo = true + print(_G.foo) + _G = {} + + math.pi = 3 + math.foo = 4