diff --git a/crates/mq-lint/README.md b/crates/mq-lint/README.md index e94476413..b2279bbe6 100644 --- a/crates/mq-lint/README.md +++ b/crates/mq-lint/README.md @@ -32,6 +32,21 @@ for d in diagnostics { Each diagnostic's `rule_id()` and `message()` are derived from `d.kind`, a [`LintMessage`](src/message.rs) enum with one variant per rule. Rule identity (`RuleId`) and message text are both enums rather than free-form strings, so adding or renaming a rule is a compile-time-checked change in one place (`src/message.rs`). +### Applying Fixes + +Some rules (marked "Fixable" in the tables below) attach a [`Fix`](src/fix.rs) that a caller can resolve against the original source and apply: + +```rust +let code = "s\"${x}\""; +let edits: Vec<_> = diagnostics + .iter() + .filter_map(|d| d.fix.as_ref().and_then(|fix| fix.resolve(code))) + .collect(); +let fixed = mq_lint::fix::apply_edits(code, &edits); +``` + +A `Fix` only records source ranges (not text), so it stays independent of any one source string until `resolve` is called — the same diagnostics can be resolved against the CLI's file contents or the language server's in-editor buffer. + ### As a CLI Build with the `cli` feature to get the `mq-lint` binary (also invocable as `mq lint` if placed on your `PATH`, see [External Subcommands](https://mqlang.org/book/start/external_subcommands.html)): @@ -47,9 +62,13 @@ echo "let x = .h1" | mq-lint # read from stdin mq-lint --disable naming_convention script.mq mq-lint --min-severity warn script.mq # only show warn/error diagnostics mq-lint --list-rules # print all rule IDs and their severity +mq-lint --fix script.mq # rewrite the file, applying every fixable diagnostic +echo "let x = .h1" | mq-lint --fix # write the fixed code to stdout ``` -Exits with a non-zero status if any diagnostic (at or above `--min-severity`) was reported. +Exits with a non-zero status if any diagnostic (at or above `--min-severity`) was reported. `--fix` only +rewrites diagnostics that have a machine-applicable fix (see "Fixable" in the rules tables below); it then +reports any remaining diagnostics as usual. ### Disabling Rules @@ -76,17 +95,20 @@ config.complexity.max_interpolation_exprs = 4; ### Correctness -| Rule ID | Severity | Description | -| ----------------------- | -------- | ------------------------------------------------------------------------------ | -| `unused_variable` | warn | `let`/`var` variable declared but never referenced | -| `unused_function` | warn | `def` function defined but never called | -| `unused_import` | warn | `import` module declared but never accessed | -| `unreachable_code` | error | Code following `break`/`continue` that can never execute | -| `infinite_loop` | warn | `loop` body without a `break` | -| `duplicate_match_arm` | error | Same pattern appears more than once in a `match` | -| `shadow_variable` | warn | Variable re-declared in an inner scope with the same name as an outer variable | -| `missing_else_in_expr` | warn | `if` expression with no `else` branch (evaluates to `none` on false) | -| `always_true_condition` | warn | `if` condition is a literal `true` or `false` | +| Rule ID | Severity | Fixable | Description | +| ----------------------- | -------- | ------- | ------------------------------------------------------------------------------ | +| `unused_variable` | warn | ✓ | `let`/`var` variable declared but never referenced | +| `unused_function` | warn | | `def` function defined but never called | +| `unused_import` | warn | | `import` module declared but never accessed | +| `unused_parameter` | warn | ✓ | Function parameter declared but never referenced | +| `unreachable_code` | error | | Code following `break`/`continue` that can never execute | +| `infinite_loop` | warn | | `loop` body without a `break` | +| `duplicate_match_arm` | error | | Same pattern appears more than once in a `match` | +| `shadow_variable` | warn | | Variable re-declared in an inner scope with the same name as an outer variable | +| `missing_else_in_expr` | warn | | `if` expression with no `else` branch (evaluates to `none` on false) | +| `always_true_condition` | warn | | `if` condition is a literal `true` or `false` | + +"Fixable" rules can be auto-corrected with `mq-lint --fix` or their language server quick fix. **Example — `unused_variable`** @@ -95,6 +117,12 @@ let x = .h1; # warn: x is never used .text ``` +**Example — `unused_parameter`** + +```mq +def greet(name, unused): s"Hello ${name}"; # warn: unused is never used +``` + **Example — `missing_else_in_expr`** ```mq @@ -109,16 +137,20 @@ if (true): 1 else: 2; # warn: condition is always `true` ### Style / Best Practices -| Rule ID | Severity | Description | -| --------------------------- | -------- | -------------------------------------------------------------------- | -| `prefer_let_over_var` | warn | `var` variable never reassigned — prefer `let` | -| `naming_convention` | style | Function or variable name is not `snake_case` | -| `boolean_comparison` | style | `x == true` → `x`, `x == false` → `not(x)` | -| `redundant_boolean_literal` | style | `if (cond): true else: false` simplifies to `cond` | -| `prefer_specific_heading` | style | `.h` without a level — prefer `.h1`–`.h6` | -| `prefer_coalesce` | style | `if (x == none): fallback else: x` simplifies to `x ?? fallback` | -| `prefer_pipe_style` | style | Nested unary call `f(g(x))` reads better as a pipe `x \| g() \| f()` | -| `redundant_try` | style | `try: ... catch: none` is exactly what the `?` operator does | +| Rule ID | Severity | Fixable | Description | +| --------------------------- | -------- | ------- | --------------------------------------------------------------------- | +| `prefer_let_over_var` | warn | ✓ | `var` variable never reassigned — prefer `let` | +| `naming_convention` | style | | Function or variable name is not `snake_case` | +| `boolean_comparison` | style | ✓ | `x == true` → `x`, `x == false` → `not(x)` | +| `redundant_boolean_literal` | style | ✓ | `if (cond): true else: false` simplifies to `cond` | +| `prefer_specific_heading` | style | | `.h` without a level — prefer `.h1`–`.h6` | +| `prefer_coalesce` | style | ✓ | `if (x == none): fallback else: x` simplifies to `x ?? fallback` | +| `prefer_pipe_style` | style | | Nested unary call `f(g(x))` reads better as a pipe `x \| g() \| f()` | +| `redundant_try` | style | ✓ | `try: ... catch: none` is exactly what the `?` operator does | +| `unnecessary_interpolation` | style | ✓ | `s"${x}"` (a single interpolation, nothing else) → `x` | +| `constant_string_concat` | style | ✓ | `"a" + "b"` (both literals) → `"ab"` | + +"Fixable" rules can be auto-corrected with `mq-lint --fix` or their language server quick fix. **Example — `prefer_let_over_var`** @@ -162,6 +194,18 @@ to_text(to_upper(x)) # style: rewrite as `x | to_upper() | to_text()` try: get("x") catch: none # style: rewrite as `get("x")?` ``` +**Example — `unnecessary_interpolation`** + +```mq +s"${.h1}" # style: rewrite as `.h1` — no interpolation needed for a single expr +``` + +**Example — `constant_string_concat`** + +```mq +"hello" + " world" # style: rewrite as `"hello world"` +``` + ### Complexity | Rule ID | Default Threshold | Description | diff --git a/crates/mq-lint/src/fix.rs b/crates/mq-lint/src/fix.rs new file mode 100644 index 000000000..72d2644e8 --- /dev/null +++ b/crates/mq-lint/src/fix.rs @@ -0,0 +1,246 @@ +//! Textual auto-fixes for lint diagnostics. +//! +//! Rules only see the HIR, not raw source text, so a [`Fix`] records ranges rather than strings +//! and is resolved against the source later, wherever it's available (the CLI or the LSP). + +/// The core replacement text for a [`Fix`], before `prefix`/`suffix` are applied. +#[derive(Debug, Clone, PartialEq)] +pub(crate) enum Core { + Literal(String), + Verbatim(mq_lang::Range), + Concat(Vec), +} + +impl Core { + fn resolve(&self, source: &str) -> Option { + match self { + Core::Literal(text) => Some(text.clone()), + Core::Verbatim(range) => Some(slice(source, *range)?.trim().to_string()), + Core::Concat(parts) => parts + .iter() + .map(|part| part.resolve(source)) + .collect::>>() + .map(|parts| parts.concat()), + } + } +} + +/// A suggested rewrite for the span of a diagnostic: `range` gets replaced with `prefix` + +/// resolved core text + `suffix`. +#[derive(Debug, Clone, PartialEq)] +pub struct Fix { + pub range: mq_lang::Range, + core: Core, + pub prefix: String, + pub suffix: String, +} + +impl Fix { + /// A fix that replaces `range` with the source text spanned by `verbatim`, unchanged. + pub fn verbatim(range: mq_lang::Range, verbatim: mq_lang::Range) -> Self { + Self { + range, + core: Core::Verbatim(verbatim), + prefix: String::new(), + suffix: String::new(), + } + } + + /// A fix that replaces `range` with fixed `text` known at rule-check time. + pub fn literal(range: mq_lang::Range, text: impl Into) -> Self { + Self { + range, + core: Core::Literal(text.into()), + prefix: String::new(), + suffix: String::new(), + } + } + + /// A fix that replaces `range` with several literal/verbatim `parts` concatenated together, + /// for rewrites that reorder or splice more than one sub-expression (see + /// [`crate::fix::Core`]). + pub(crate) fn concat(range: mq_lang::Range, parts: Vec) -> Self { + Self { + range, + core: Core::Concat(parts), + prefix: String::new(), + suffix: String::new(), + } + } + + pub fn with_prefix(mut self, prefix: impl Into) -> Self { + self.prefix = prefix.into(); + self + } + + pub fn with_suffix(mut self, suffix: impl Into) -> Self { + self.suffix = suffix.into(); + self + } + + /// Resolves this fix against `source`, returning the range to replace and its replacement + /// text. + /// + /// A `Verbatim` core is trimmed, since rules can often only bound one side of such a range by + /// a sibling token rather than the expression's own true end (see [`crate::LintContext::full_range`]). + pub fn resolve(&self, source: &str) -> Option<(mq_lang::Range, String)> { + let core = self.core.resolve(source)?; + Some((self.range, format!("{}{}{}", self.prefix, core, self.suffix))) + } +} + +/// Converts a 1-based line/column [`mq_lang::Position`] (column counted in `char`s) into a byte +/// offset into `source`. +fn position_to_byte_offset(source: &str, position: mq_lang::Position) -> Option { + let mut offset = 0; + let mut lines = source.split_inclusive('\n'); + + for _ in 1..position.line { + offset += lines.next()?.len(); + } + let line = lines.next().unwrap_or(""); + + let mut column = 1; + for (i, _) in line.char_indices() { + if column == position.column { + return Some(offset + i); + } + column += 1; + } + if column == position.column { + return Some(offset + line.trim_end_matches('\n').len()); + } + + None +} + +/// The smallest range spanning both `a` and `b`. +pub fn union(a: mq_lang::Range, b: mq_lang::Range) -> mq_lang::Range { + let start = if (a.start.line, a.start.column) <= (b.start.line, b.start.column) { + a.start + } else { + b.start + }; + let end = if (a.end.line, a.end.column) >= (b.end.line, b.end.column) { + a.end + } else { + b.end + }; + mq_lang::Range { start, end } +} + +/// Extracts the substring of `source` spanned by `range`. +pub fn slice(source: &str, range: mq_lang::Range) -> Option<&str> { + let start = position_to_byte_offset(source, range.start)?; + let end = position_to_byte_offset(source, range.end)?; + if start > end { + return None; + } + source.get(start..end) +} + +/// Applies a set of resolved `(range, replacement)` edits to `source`, returning the new text. +/// +/// Edits are applied from the end of the source towards the start so earlier byte offsets stay +/// valid; if two edits overlap, the one starting earlier wins and the other is dropped. +pub fn apply_edits(source: &str, edits: &[(mq_lang::Range, String)]) -> String { + let mut spans: Vec<(usize, usize, &str)> = edits + .iter() + .filter_map(|(range, text)| { + let start = position_to_byte_offset(source, range.start)?; + let end = position_to_byte_offset(source, range.end)?; + (start <= end).then_some((start, end, text.as_str())) + }) + .collect(); + spans.sort_by_key(|(start, ..)| std::cmp::Reverse(*start)); + + let mut result = source.to_string(); + let mut applied_start = usize::MAX; + for (start, end, text) in spans { + if end > applied_start { + continue; + } + result.replace_range(start..end, text); + applied_start = start; + } + result +} + +#[cfg(test)] +mod tests { + use super::*; + + fn range(start_line: u32, start_col: usize, end_line: u32, end_col: usize) -> mq_lang::Range { + mq_lang::Range { + start: mq_lang::Position { + line: start_line, + column: start_col, + }, + end: mq_lang::Position { + line: end_line, + column: end_col, + }, + } + } + + #[test] + fn slice_extracts_single_line_span() { + let source = ".checked == false"; + assert_eq!(slice(source, range(1, 1, 1, 9)), Some(".checked")); + } + + #[test] + fn slice_extracts_across_lines() { + let source = "let x =\n .h1\n| x"; + assert_eq!(slice(source, range(2, 3, 2, 6)), Some(".h1")); + } + + #[test] + fn slice_handles_multibyte_columns() { + let source = r#"s"${あ}""#; + // The interpolated expr `あ` starts at char column 5 (after `s`, `"`, `$`, `{`). + assert_eq!(slice(source, range(1, 5, 1, 6)), Some("あ")); + } + + #[test] + fn resolve_applies_prefix_and_suffix() { + let source = ".checked == false"; + let fix = Fix::verbatim(range(1, 1, 1, 18), range(1, 1, 1, 9)).with_prefix("!"); + assert_eq!(fix.resolve(source), Some((range(1, 1, 1, 18), "!.checked".to_string()))); + } + + #[test] + fn resolve_uses_literal_core() { + let source = r#"s"${x}""#; + let fix = Fix::literal(range(1, 1, 1, 8), "x"); + assert_eq!(fix.resolve(source), Some((range(1, 1, 1, 8), "x".to_string()))); + } + + #[test] + fn apply_edits_splices_single_edit() { + let source = ".checked == false"; + let edits = vec![(range(1, 1, 1, 18), "!.checked".to_string())]; + assert_eq!(apply_edits(source, &edits), "!.checked"); + } + + #[test] + fn apply_edits_handles_multiple_non_overlapping_edits() { + let source = "try: get(\"x\") catch: none\n| .checked == true"; + let edits = vec![ + (range(1, 1, 1, 26), "get(\"x\")?".to_string()), + (range(2, 3, 2, 19), ".checked".to_string()), + ]; + assert_eq!(apply_edits(source, &edits), "get(\"x\")?\n| .checked"); + } + + #[test] + fn apply_edits_drops_overlapping_edit() { + let source = ".checked == false"; + let edits = vec![ + (range(1, 1, 1, 18), "!.checked".to_string()), + (range(1, 1, 1, 9), ".checked".to_string()), + ]; + // The two edits overlap; only the one starting first (lowest offset) is kept. + assert_eq!(apply_edits(source, &edits), "!.checked"); + } +} diff --git a/crates/mq-lint/src/lib.rs b/crates/mq-lint/src/lib.rs index 41d254095..08ceba90c 100644 --- a/crates/mq-lint/src/lib.rs +++ b/crates/mq-lint/src/lib.rs @@ -19,10 +19,12 @@ //! ``` pub mod config; +pub mod fix; pub mod message; pub mod rules; pub use config::LintConfig; +pub use fix::Fix; pub use message::{LintMessage, RuleId}; use mq_hir::{Hir, SourceId}; @@ -59,6 +61,8 @@ pub struct Diagnostic { pub severity: Severity, /// Source location of the finding, if available. pub range: Option, + /// A machine-applicable rewrite, if the rule can suggest one. + pub fix: Option, } impl Diagnostic { @@ -67,6 +71,7 @@ impl Diagnostic { kind, severity, range: None, + fix: None, } } @@ -75,6 +80,11 @@ impl Diagnostic { self } + pub fn with_fix(mut self, fix: Fix) -> Self { + self.fix = Some(fix); + self + } + /// The rule that produced this diagnostic. pub fn rule_id(&self) -> RuleId { self.kind.rule_id() @@ -115,6 +125,27 @@ impl<'a> LintContext<'a> { .symbols() .filter(move |(_, s)| s.source.source_id == Some(source_id)) } + + /// The full source range spanned by a symbol and all of its descendants. + /// + /// A symbol's own `source.text_range` often covers only its own token (e.g. a `BinaryOp` + /// symbol's range is just the operator), not the whole subtree, so rules building a [`Fix`] + /// should use this instead. + pub fn full_range(&self, symbol_id: mq_hir::SymbolId) -> Option { + let symbol = self.hir.symbol(symbol_id)?; + let mut range = symbol.source.text_range; + + for (child_id, _) in self.all_symbols().filter(|(_, s)| s.parent == Some(symbol_id)) { + if let Some(child_range) = self.full_range(child_id) { + range = Some(match range { + Some(r) => fix::union(r, child_range), + None => child_range, + }); + } + } + + range + } } /// A single lint rule. diff --git a/crates/mq-lint/src/main.rs b/crates/mq-lint/src/main.rs index edcbd6984..82ddb0ae9 100644 --- a/crates/mq-lint/src/main.rs +++ b/crates/mq-lint/src/main.rs @@ -26,6 +26,11 @@ struct Cli { /// Print all available rule IDs and their default severity, then exit #[arg(long)] list_rules: bool, + + /// Rewrite files in place, applying every diagnostic with a machine-applicable fix + /// (reads stdin if no files are given, writing the fixed code to stdout) + #[arg(long)] + fix: bool, } #[derive(Clone, Copy)] @@ -82,6 +87,13 @@ fn run() -> io::Result { if cli.files.is_empty() { let mut code = String::new(); io::stdin().read_to_string(&mut code)?; + + if cli.fix { + let (fixed, _) = fix_source(&code, &linter, &config); + write!(w, "{fixed}")?; + return Ok(false); + } + let had_diagnostics = lint_source(&mut w, &code, "", &linter, &config, min_severity)?; return Ok(had_diagnostics); } @@ -92,6 +104,24 @@ fn run() -> io::Result { let code = std::fs::read_to_string(path) .map_err(|e| io::Error::other(format!("reading file {}: {}", path.display(), e)))?; let label = path.display().to_string(); + + let code = if cli.fix { + let (fixed, fix_count) = fix_source(&code, &linter, &config); + if fixed != code { + std::fs::write(path, &fixed) + .map_err(|e| io::Error::other(format!("writing file {}: {}", path.display(), e)))?; + let issue_word = if fix_count == 1 { "issue" } else { "issues" }; + writeln!( + w, + "{} {fix_count} {issue_word} in {label}", + "fixed".bright_green().bold(), + )?; + } + fixed + } else { + code + }; + if lint_source(&mut w, &code, &label, &linter, &config, min_severity)? { had_diagnostics = true; } @@ -100,6 +130,23 @@ fn run() -> io::Result { Ok(had_diagnostics) } +/// Applies every diagnostic with a fix to `code`, returning the rewritten source and how many +/// fixes were applied. +fn fix_source(code: &str, linter: &Linter, config: &LintConfig) -> (String, usize) { + let mut hir = Hir::default(); + let (source_id, _) = hir.add_code(None, code); + let ctx = LintContext::new(&hir, source_id, config); + + let edits: Vec<(mq_lang::Range, String)> = linter + .run(&ctx) + .into_iter() + .filter_map(|d| d.fix.as_ref().and_then(|fix| fix.resolve(code))) + .collect(); + + let fix_count = edits.len(); + (mq_lint::fix::apply_edits(code, &edits), fix_count) +} + fn list_rules(w: &mut impl Write) -> io::Result<()> { let mut rules: Vec<_> = mq_lint::rules::all_rules(); rules.sort_by_key(|r| r.id()); @@ -296,4 +343,34 @@ mod tests { let result = Cli::try_parse_from(["mq-lint", "--min-severity", "bogus"]); assert!(result.is_err()); } + + #[test] + fn test_cli_fix_flag() { + let cli = Cli::try_parse_from(["mq-lint", "--fix", "test.mq"]).unwrap(); + assert!(cli.fix); + + let cli = Cli::try_parse_from(["mq-lint", "test.mq"]).unwrap(); + assert!(!cli.fix); + } + + #[rstest] + #[case(r#".checked == true"#, ".checked")] + #[case(r#"try: get("x") catch: none"#, r#"get("x")?"#)] + #[case(r#"s"${x}""#, "x")] + fn test_fix_source_applies_known_fixable_rules(#[case] code: &str, #[case] expected: &str) { + let config = LintConfig::default(); + let linter = Linter::with_default_rules(); + let (fixed, fix_count) = fix_source(code, &linter, &config); + assert_eq!(fixed, expected); + assert_eq!(fix_count, 1); + } + + #[test] + fn test_fix_source_is_a_noop_when_nothing_is_fixable() { + let config = LintConfig::default(); + let linter = Linter::with_default_rules(); + let (fixed, fix_count) = fix_source(".h1", &linter, &config); + assert_eq!(fixed, ".h1"); + assert_eq!(fix_count, 0); + } } diff --git a/crates/mq-lint/src/rules/correctness/unused_parameter.rs b/crates/mq-lint/src/rules/correctness/unused_parameter.rs index 27797defa..5bc48a6fe 100644 --- a/crates/mq-lint/src/rules/correctness/unused_parameter.rs +++ b/crates/mq-lint/src/rules/correctness/unused_parameter.rs @@ -1,6 +1,6 @@ use rustc_hash::FxHashSet; -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; use mq_hir::{ScopeId, ScopeKind, SymbolId, SymbolKind}; pub struct UnusedParameter; @@ -16,6 +16,14 @@ fn is_in_scope_subtree(ctx: &LintContext<'_>, scope_id: ScopeId, target_scope_id false } +/// True if `sym` is a `${name}` interpolation embed (which reuses `SymbolKind::Variable` for the +/// embedded text) rather than an actual reference symbol. +fn is_interpolation_embed(ctx: &LintContext<'_>, sym: &mq_hir::Symbol) -> bool { + sym.parent + .and_then(|id| ctx.hir.symbol(id)) + .is_some_and(|p| matches!(p.kind, SymbolKind::InterpolatedString)) +} + impl LintRule for UnusedParameter { fn id(&self) -> RuleId { RuleId::UnusedParameter @@ -41,7 +49,10 @@ impl LintRule for UnusedParameter { let used_names: FxHashSet<&str> = ctx .all_symbols() - .filter(|(_, s)| matches!(s.kind, SymbolKind::Ref | SymbolKind::Ident | SymbolKind::Call)) + .filter(|(_, s)| { + matches!(s.kind, SymbolKind::Ref | SymbolKind::Ident | SymbolKind::Call) + || is_interpolation_embed(ctx, s) + }) .filter(|(_, s)| is_in_scope_subtree(ctx, s.scope, fn_scope_id)) .filter_map(|(_, s)| s.value.as_deref()) .collect(); @@ -61,7 +72,7 @@ impl LintRule for UnusedParameter { let mut d = Diagnostic::new(LintMessage::UnusedParameter { name: name.to_string() }, self.severity()); if let Some(range) = param_sym.source.text_range { - d = d.with_range(range); + d = d.with_range(range).with_fix(Fix::literal(range, format!("_{name}"))); } diagnostics.push(d); } @@ -100,6 +111,7 @@ mod tests { #[case("def f(a, b): a + b")] #[case("def f(a): a")] #[case("def f(): .h1")] + #[case(r#"def f(name): s"Hello ${name}""#)] fn no_diagnostic_when_all_params_used(#[case] code: &str) { let diags = check(code); assert_eq!(diags.len(), 0); @@ -116,4 +128,16 @@ mod tests { let diags = check("def f(a, b, c): .h1"); assert_eq!(diags.len(), 3); } + + #[test] + fn fix_prefixes_name_with_underscore() { + let code = "def f(a, b): a"; + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!( + crate::fix::apply_edits(code, &[(range, replacement)]), + "def f(a, _b): a" + ); + } } diff --git a/crates/mq-lint/src/rules/correctness/unused_variable.rs b/crates/mq-lint/src/rules/correctness/unused_variable.rs index b35a0dc36..a7a5b7fc0 100644 --- a/crates/mq-lint/src/rules/correctness/unused_variable.rs +++ b/crates/mq-lint/src/rules/correctness/unused_variable.rs @@ -1,10 +1,18 @@ use rustc_hash::FxHashSet; -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; use mq_hir::SymbolKind; pub struct UnusedVariable; +/// True if `sym` is a `${name}` interpolation embed rather than an actual `let`/`var` declaration +/// (both reuse `SymbolKind::Variable`; see `Hir::add_interpolated_string`). +fn is_interpolation_embed(ctx: &LintContext<'_>, sym: &mq_hir::Symbol) -> bool { + sym.parent + .and_then(|id| ctx.hir.symbol(id)) + .is_some_and(|p| matches!(p.kind, SymbolKind::InterpolatedString)) +} + impl LintRule for UnusedVariable { fn id(&self) -> RuleId { RuleId::UnusedVariable @@ -15,10 +23,14 @@ impl LintRule for UnusedVariable { } fn check(&self, ctx: &LintContext<'_>) -> Vec { - // Build a set of all names referenced by Ref/Ident/Call symbols + // Build a set of all names referenced by Ref/Ident/Call symbols, plus bare `${name}` + // interpolation embeds (which reuse `SymbolKind::Variable` for the embedded text). let used_names: FxHashSet<&str> = ctx .all_symbols() - .filter(|(_, s)| matches!(s.kind, SymbolKind::Ref | SymbolKind::Ident | SymbolKind::Call)) + .filter(|(_, s)| { + matches!(s.kind, SymbolKind::Ref | SymbolKind::Ident | SymbolKind::Call) + || is_interpolation_embed(ctx, s) + }) .filter_map(|(_, s)| s.value.as_deref()) .collect(); @@ -30,12 +42,17 @@ impl LintRule for UnusedVariable { if name.starts_with('_') { return None; } + // An interpolation embed is itself a reference, not a declaration, so it can't be + // "unused" or renamed. + if is_interpolation_embed(ctx, sym) { + return None; + } if used_names.contains(name) { return None; } let mut d = Diagnostic::new(LintMessage::UnusedVariable { name: name.to_string() }, self.severity()); if let Some(range) = sym.source.text_range { - d = d.with_range(range); + d = d.with_range(range).with_fix(Fix::literal(range, format!("_{name}"))); } Some(d) }) @@ -72,8 +89,19 @@ mod tests { #[case("let x = .h1 | x")] #[case("let _x = .h1")] #[case("let _ignored = .h1")] + #[case(r#"s"${x}""#)] + #[case(r#"let x = 1 | s"${x}""#)] fn no_diagnostic(#[case] code: &str) { let diags = check(code); assert_eq!(diags.len(), 0); } + + #[test] + fn fix_prefixes_name_with_underscore() { + let code = "let x = .h1"; + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), "let _x = .h1"); + } } diff --git a/crates/mq-lint/src/rules/style/boolean_comparison.rs b/crates/mq-lint/src/rules/style/boolean_comparison.rs index fdc024d05..e229fff9f 100644 --- a/crates/mq-lint/src/rules/style/boolean_comparison.rs +++ b/crates/mq-lint/src/rules/style/boolean_comparison.rs @@ -1,4 +1,4 @@ -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; use mq_hir::SymbolKind; pub struct BooleanComparison; @@ -30,7 +30,7 @@ impl LintRule for BooleanComparison { .all_symbols() .find(|(_, s)| s.parent == Some(bop_id) && matches!(s.kind, SymbolKind::Boolean)); - let Some((_, bool_sym)) = bool_child else { + let Some((bool_id, bool_sym)) = bool_child else { continue; }; @@ -43,7 +43,36 @@ impl LintRule for BooleanComparison { }, self.severity(), ); - if let Some(range) = bop_sym.source.text_range { + // The other operand: the child of `bop_id` that isn't the boolean literal. Bound its + // end by the operator's own start when it precedes the operator, since a compound + // operand's trailing `)` isn't tracked by any HIR symbol; `Fix::resolve` trims the + // resulting whitespace. + let operand_id = ctx + .all_symbols() + .find(|(id, s)| s.parent == Some(bop_id) && *id != bool_id) + .map(|(id, _)| id); + let operand_range = operand_id + .and_then(|id| ctx.full_range(id)) + .map(|r| match bop_sym.source.text_range { + Some(op_range) if r.start <= op_range.start => mq_lang::Range { + start: r.start, + end: op_range.start, + }, + _ => r, + }); + + if let (Some(bool_range), Some(operand_range)) = (ctx.full_range(bool_id), operand_range) { + let range = crate::fix::union(bool_range, operand_range); + d = d.with_range(range); + + // `== true` / `!= false` keep the operand as-is; `== false` / `!= true` negate it. + let negate = (op == "==" && bool_val == "false") || (op == "!=" && bool_val == "true"); + let mut fix = Fix::verbatim(range, operand_range); + if negate { + fix = fix.with_prefix("!"); + } + d = d.with_fix(fix); + } else if let Some(range) = bop_sym.source.text_range { d = d.with_range(range); } diagnostics.push(d); @@ -86,4 +115,27 @@ mod tests { let diags = check(code); assert_eq!(diags.len(), 0); } + + #[rstest] + #[case(".checked == true", ".checked")] + #[case(".checked == false", "!.checked")] + #[case(".checked != true", "!.checked")] + #[case(".checked != false", ".checked")] + fn fix_simplifies_boolean_comparison(#[case] code: &str, #[case] expected: &str) { + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), expected); + } + + #[test] + fn fix_keeps_trailing_call_parens_when_operand_ends_in_punctuation() { + // The operand's own HIR range doesn't track its closing `)`, so the fix must recover it + // rather than truncating to `to_bool(x`. + let code = r#"to_bool(x) == false"#; + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), "!to_bool(x)"); + } } diff --git a/crates/mq-lint/src/rules/style/constant_string_concat.rs b/crates/mq-lint/src/rules/style/constant_string_concat.rs index 426b7179f..485795a64 100644 --- a/crates/mq-lint/src/rules/style/constant_string_concat.rs +++ b/crates/mq-lint/src/rules/style/constant_string_concat.rs @@ -1,4 +1,4 @@ -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; use mq_hir::SymbolKind; pub struct ConstantStringConcat; @@ -15,16 +15,24 @@ impl LintRule for ConstantStringConcat { fn check(&self, ctx: &LintContext<'_>) -> Vec { ctx.all_symbols() .filter(|(_, sym)| matches!(sym.kind, SymbolKind::BinaryOp) && sym.value.as_deref() == Some("+")) - .filter_map(|(op_id, op_sym)| { - let children: Vec<_> = ctx.all_symbols().filter(|(_, s)| s.parent == Some(op_id)).collect(); + .filter_map(|(op_id, _)| { + let mut children: Vec<_> = ctx.all_symbols().filter(|(_, s)| s.parent == Some(op_id)).collect(); if children.len() < 2 || !children.iter().all(|(_, s)| matches!(s.kind, SymbolKind::String)) { return None; } + children.sort_by_key(|(_, s)| s.source.text_range.map(|r| (r.start.line, r.start.column))); let mut d = Diagnostic::new(LintMessage::ConstantStringConcat, self.severity()); - if let Some(range) = op_sym.source.text_range { - d = d.with_range(range); + if let (Some(first), Some(last)) = ( + children.first().and_then(|(_, s)| s.source.text_range), + children.last().and_then(|(_, s)| s.source.text_range), + ) { + let range = crate::fix::union(first, last); + let combined: String = children.iter().filter_map(|(_, s)| s.value.as_deref()).collect(); + d = d + .with_range(range) + .with_fix(Fix::literal(range, format!("\"{}\"", escape_string_literal(&combined)))); } Some(d) }) @@ -32,6 +40,19 @@ impl LintRule for ConstantStringConcat { } } +/// Escapes `\` and `"` so `s` round-trips as a valid mq string literal body. +fn escape_string_literal(s: &str) -> String { + let mut out = String::with_capacity(s.len()); + for c in s.chars() { + match c { + '\\' => out.push_str("\\\\"), + '"' => out.push_str("\\\""), + _ => out.push(c), + } + } + out +} + #[cfg(test)] mod tests { use mq_hir::Hir; @@ -64,4 +85,33 @@ mod tests { let diags = check(code); assert_eq!(diags.len(), 0); } + + #[rstest] + #[case(r#""hello" + " world""#, r#""hello world""#)] + #[case(r#""foo" + "bar""#, r#""foobar""#)] + fn fix_combines_string_literals(#[case] code: &str, #[case] expected: &str) { + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), expected); + } + + #[test] + fn fix_escapes_quotes_and_backslashes_in_combined_literal() { + let code = r#""a\"b" + "c\\d""#; + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + let fixed = crate::fix::apply_edits(code, &[(range, replacement)]); + assert_eq!(fixed, r#""a\"bc\\d""#); + + // The escaped literal must itself lex back to the original concatenated value. + let mut hir = mq_hir::Hir::default(); + let (source_id, _) = hir.add_code(None, &fixed); + let sym = hir + .symbols() + .find(|(_, s)| s.source.source_id == Some(source_id) && matches!(s.kind, SymbolKind::String)) + .map(|(_, s)| s.value.clone().unwrap()); + assert_eq!(sym.as_deref(), Some("a\"bc\\d")); + } } diff --git a/crates/mq-lint/src/rules/style/prefer_coalesce.rs b/crates/mq-lint/src/rules/style/prefer_coalesce.rs index 9f689a3ca..c1dffdd02 100644 --- a/crates/mq-lint/src/rules/style/prefer_coalesce.rs +++ b/crates/mq-lint/src/rules/style/prefer_coalesce.rs @@ -1,6 +1,7 @@ use mq_hir::{Symbol, SymbolId, SymbolKind}; -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::fix::Core; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; pub struct PreferCoalesce; @@ -19,34 +20,38 @@ fn children_sorted_by_position<'a>(ctx: &'a LintContext<'_>, parent: SymbolId) - children } +/// A confirmed `if (x none): ... else: ...` null-check, equivalent to `x ?? fallback`. +struct CoalesceMatch { + /// `"=="` or `"!="`. + op: &'static str, + then_id: SymbolId, + else_kw_id: SymbolId, + else_expr_id: SymbolId, + /// Whichever branch (`then_id` or `else_expr_id`) repeats the null-checked value; it's + /// confirmed to be a leaf, so its range is safe to use verbatim. + value_branch_id: SymbolId, +} + /// Detects `if (x == none): fallback else: x` and its `!=` form, both equivalent to `x ?? fallback`. -fn matches_null_check_pattern(ctx: &LintContext<'_>, if_id: SymbolId) -> bool { +fn find_coalesce_match(ctx: &LintContext<'_>, if_id: SymbolId) -> Option { let children = children_sorted_by_position(ctx, if_id); - let Some(&(cond_id, cond)) = children.first() else { - return false; - }; - let Some(&(then_id, _)) = children.get(1) else { - return false; - }; - let Some(&(else_kw_id, _)) = children.iter().find(|(_, s)| matches!(s.kind, SymbolKind::Else)) else { - return false; - }; - let Some(&(else_expr_id, _)) = children_sorted_by_position(ctx, else_kw_id).first() else { - return false; - }; + let &(cond_id, cond) = children.first()?; + let &(then_id, _) = children.get(1)?; + let &(else_kw_id, _) = children.iter().find(|(_, s)| matches!(s.kind, SymbolKind::Else))?; + let &(else_expr_id, _) = children_sorted_by_position(ctx, else_kw_id).first()?; if !matches!(cond.kind, SymbolKind::BinaryOp) { - return false; - } - let Some(op) = cond.value.as_deref() else { return false }; - if op != "==" && op != "!=" { - return false; + return None; } + let op = cond.value.as_deref()?; + let op: &'static str = match op { + "==" => "==", + "!=" => "!=", + _ => return None, + }; let cond_children = children_sorted_by_position(ctx, cond_id); - let (Some(&(lhs_id, lhs)), Some(&(rhs_id, rhs))) = (cond_children.first(), cond_children.get(1)) else { - return false; - }; + let (&(lhs_id, lhs), &(rhs_id, rhs)) = (cond_children.first()?, cond_children.get(1)?); // Whichever side isn't the `none` literal is the value being null-checked. let value_id = if is_none_literal(rhs) && is_leaf(ctx, lhs_id) { @@ -54,22 +59,72 @@ fn matches_null_check_pattern(ctx: &LintContext<'_>, if_id: SymbolId) -> bool { } else if is_none_literal(lhs) && is_leaf(ctx, rhs_id) { rhs_id } else { - return false; - }; - let Some(value_sym) = ctx.hir.symbol(value_id) else { - return false; + return None; }; + let value_sym = ctx.hir.symbol(value_id)?; // `==`: the value must be repeated in the else-branch; `!=`: in the then-branch. let value_branch_id = if op == "==" { else_expr_id } else { then_id }; if !is_leaf(ctx, value_branch_id) { - return false; + return None; + } + let value_branch = ctx.hir.symbol(value_branch_id)?; + if value_branch.kind != value_sym.kind || value_branch.value != value_sym.value { + return None; + } + + Some(CoalesceMatch { + op, + then_id, + else_kw_id, + else_expr_id, + value_branch_id, + }) +} + +/// Builds the `value ?? fallback` fix for a confirmed match. +/// +/// The fallback branch is a leaf (a bare literal/selector) or not, but either way its own end +/// isn't reliably tracked once it has children (e.g. a call's closing `)`). So instead of slicing +/// the fallback's own range, this bounds it by whichever neighboring token *is* reliable: `else`'s +/// own start when the fallback comes first (the `==` shape), or — since nothing follows it when +/// the fallback comes last (the `!=` shape) — by simply not replacing it at all, leaving its +/// original text in place right after the edit. +fn build_fix(ctx: &LintContext<'_>, if_start: mq_lang::Position, m: &CoalesceMatch) -> Option<(mq_lang::Range, Fix)> { + let value_range = ctx.hir.symbol(m.value_branch_id)?.source.text_range?; + + if m.op == "==" { + let else_kw_start = ctx.hir.symbol(m.else_kw_id)?.source.text_range?.start; + let fallback_range = mq_lang::Range { + start: ctx.full_range(m.then_id)?.start, + end: else_kw_start, + }; + let range = mq_lang::Range { + start: if_start, + end: value_range.end, + }; + let fix = Fix::concat( + range, + vec![ + Core::Verbatim(value_range), + Core::Literal(" ?? ".to_string()), + Core::Verbatim(fallback_range), + ], + ); + Some((range, fix)) + } else { + let fallback_start = ctx.full_range(m.else_expr_id)?.start; + let range = mq_lang::Range { + start: if_start, + end: fallback_start, + }; + let fix = Fix::concat( + range, + vec![Core::Verbatim(value_range), Core::Literal(" ?? ".to_string())], + ); + Some((range, fix)) } - let Some(value_branch) = ctx.hir.symbol(value_branch_id) else { - return false; - }; - value_branch.kind == value_sym.kind && value_branch.value == value_sym.value } impl LintRule for PreferCoalesce { @@ -84,13 +139,18 @@ impl LintRule for PreferCoalesce { fn check(&self, ctx: &LintContext<'_>) -> Vec { ctx.all_symbols() .filter(|(_, sym)| matches!(sym.kind, SymbolKind::If)) - .filter(|(if_id, _)| matches_null_check_pattern(ctx, *if_id)) - .map(|(_, if_sym)| { + .filter_map(|(if_id, if_sym)| { + let m = find_coalesce_match(ctx, if_id)?; let mut d = Diagnostic::new(LintMessage::PreferCoalesce, self.severity()); - if let Some(range) = if_sym.source.text_range { - d = d.with_range(range); + match if_sym.source.text_range.and_then(|r| build_fix(ctx, r.start, &m)) { + Some((range, fix)) => d = d.with_range(range).with_fix(fix), + None => { + if let Some(range) = if_sym.source.text_range { + d = d.with_range(range); + } + } } - d + Some(d) }) .collect() } @@ -127,4 +187,28 @@ mod tests { let diags = check(code); assert_eq!(diags.len(), 0); } + + #[rstest] + #[case(r#"if (.value == none): "default" else: .value"#, r#".value ?? "default""#)] + #[case(r#"if (x != none): x else: "default""#, r#"x ?? "default""#)] + fn fix_rewrites_as_coalesce(#[case] code: &str, #[case] expected: &str) { + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), expected); + } + + #[rstest] + // `==` shape: the fallback is the then-branch, which is *not* the source's last token, so its + // end must be bounded by `else`'s start rather than its own (potentially truncated) end. + #[case(r#"if (x == none): get_default(1, 2) else: x"#, "x ?? get_default(1, 2)")] + // `!=` shape: the fallback is the else-branch and the source's last token; its closing `)` is + // recovered by leaving it untouched rather than slicing it out. + #[case(r#"if (x != none): x else: get_default()"#, "x ?? get_default()")] + fn fix_preserves_compound_fallback_with_trailing_punctuation(#[case] code: &str, #[case] expected: &str) { + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), expected); + } } diff --git a/crates/mq-lint/src/rules/style/prefer_let_over_var.rs b/crates/mq-lint/src/rules/style/prefer_let_over_var.rs index 2c3b2efc4..286215297 100644 --- a/crates/mq-lint/src/rules/style/prefer_let_over_var.rs +++ b/crates/mq-lint/src/rules/style/prefer_let_over_var.rs @@ -1,6 +1,6 @@ use rustc_hash::FxHashSet; -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; use mq_hir::{SymbolId, SymbolKind}; use mq_lang::Range; @@ -68,7 +68,7 @@ impl LintRule for PreferLetOverVar { LintMessage::PreferLetOverVar { name: name.to_string() }, self.severity(), ); - d = d.with_range(kw_range); + d = d.with_range(kw_range).with_fix(Fix::literal(kw_range, "let")); diagnostics.push(d); } @@ -108,4 +108,16 @@ mod tests { let diags = check(code); assert_eq!(diags.len(), 0); } + + #[test] + fn fix_replaces_var_with_let() { + let code = "var x = .h1 | x"; + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!( + crate::fix::apply_edits(code, &[(range, replacement)]), + "let x = .h1 | x" + ); + } } diff --git a/crates/mq-lint/src/rules/style/redundant_boolean_literal.rs b/crates/mq-lint/src/rules/style/redundant_boolean_literal.rs index 3cff50633..ae11bf68c 100644 --- a/crates/mq-lint/src/rules/style/redundant_boolean_literal.rs +++ b/crates/mq-lint/src/rules/style/redundant_boolean_literal.rs @@ -1,4 +1,4 @@ -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; use mq_hir::{SymbolId, SymbolKind}; pub struct RedundantBooleanLiteral; @@ -27,14 +27,20 @@ impl LintRule for RedundantBooleanLiteral { continue; }; - // The then-body is the second child of If by position (first is the condition). - let then_bool = second_child_boolean(ctx, if_id); - // The else-body is the first (only) child of Else. - let else_bool = first_child_boolean(ctx, else_id); + // Condition and then-body are the first two children of If by position. + let if_children = sorted_children(ctx, if_id, true); + let Some(&(cond_id, _)) = if_children.first() else { + continue; + }; + let Some(&(_, then_sym)) = if_children.get(1) else { + continue; + }; + let Some(&(_, else_sym)) = sorted_children(ctx, else_id, false).first() else { + continue; + }; - let (then_val, else_val) = match (then_bool, else_bool) { - (Some(t), Some(e)) => (t, e), - _ => continue, + let (Some(then_val), Some(else_val)) = (boolean_value(then_sym), boolean_value(else_sym)) else { + continue; }; // Pattern: if (cond): true else: false → cond @@ -46,7 +52,21 @@ impl LintRule for RedundantBooleanLiteral { }, self.severity(), ); - if let Some(range) = if_sym.source.text_range { + if let (Some(if_start), Some(else_range), Some(cond_range)) = ( + if_sym.source.text_range, + else_sym.source.text_range, + ctx.full_range(cond_id), + ) { + let range = mq_lang::Range { + start: if_start.start, + end: else_range.end, + }; + let mut fix = Fix::verbatim(range, cond_range); + if then_val == "false" { + fix = fix.with_prefix("!"); + } + d = d.with_range(range).with_fix(fix); + } else if let Some(range) = if_sym.source.text_range { d = d.with_range(range); } diagnostics.push(d); @@ -64,42 +84,30 @@ fn find_else_child(ctx: &LintContext<'_>, if_id: SymbolId) -> Option { .map(|(id, _)| id) } -/// Returns the value of the second child (by source position) of `parent_id` if it is Boolean. -fn second_child_boolean<'a>(ctx: &'a LintContext<'_>, parent_id: SymbolId) -> Option<&'a str> { +fn boolean_value(sym: &mq_hir::Symbol) -> Option<&str> { + matches!(sym.kind, SymbolKind::Boolean) + .then(|| sym.value.as_deref()) + .flatten() +} + +/// Children of `parent_id`, sorted by source position. When `exclude_branches` is set, `Else` +/// and `Elif` children are skipped (used for `If`'s children, where they aren't part of the +/// condition/then-body pair). +fn sorted_children<'a>( + ctx: &'a LintContext<'_>, + parent_id: SymbolId, + exclude_branches: bool, +) -> Vec<(SymbolId, &'a mq_hir::Symbol)> { let mut children: Vec<_> = ctx .all_symbols() .filter(|(_, s)| { s.parent == Some(parent_id) - && !matches!(s.kind, SymbolKind::Else | SymbolKind::Elif) && s.source.text_range.is_some() + && !(exclude_branches && matches!(s.kind, SymbolKind::Else | SymbolKind::Elif)) }) - .filter_map(|(id, s)| s.source.text_range.map(|r| (id, r, s))) - .collect(); - children.sort_by_key(|(_, r, _)| (r.start.line, r.start.column)); - - let (_, _, sym) = children.get(1)?; - if matches!(sym.kind, SymbolKind::Boolean) { - sym.value.as_deref() - } else { - None - } -} - -/// Returns the value of the first child (by source position) of `parent_id` if it is Boolean. -fn first_child_boolean<'a>(ctx: &'a LintContext<'_>, parent_id: SymbolId) -> Option<&'a str> { - let mut children: Vec<_> = ctx - .all_symbols() - .filter(|(_, s)| s.parent == Some(parent_id) && s.source.text_range.is_some()) - .filter_map(|(id, s)| s.source.text_range.map(|r| (id, r, s))) .collect(); - children.sort_by_key(|(_, r, _)| (r.start.line, r.start.column)); - - let (_, _, sym) = children.first()?; - if matches!(sym.kind, SymbolKind::Boolean) { - sym.value.as_deref() - } else { - None - } + children.sort_by_key(|(_, s)| s.source.text_range.map(|r| (r.start.line, r.start.column))); + children } #[cfg(test)] @@ -135,4 +143,14 @@ mod tests { let diags = check(code); assert_eq!(diags.len(), 0); } + + #[rstest] + #[case("if (.h1): true else: false;", ".h1;")] + #[case("if (.h1): false else: true;", "!.h1;")] + fn fix_simplifies_to_condition(#[case] code: &str, #[case] expected: &str) { + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), expected); + } } diff --git a/crates/mq-lint/src/rules/style/redundant_try.rs b/crates/mq-lint/src/rules/style/redundant_try.rs index 5e0028199..7789165d0 100644 --- a/crates/mq-lint/src/rules/style/redundant_try.rs +++ b/crates/mq-lint/src/rules/style/redundant_try.rs @@ -1,6 +1,6 @@ use mq_hir::SymbolKind; -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; pub struct RedundantTry; @@ -18,7 +18,7 @@ impl LintRule for RedundantTry { ctx.all_symbols() .filter(|(_, sym)| matches!(sym.kind, SymbolKind::Try)) .filter_map(|(try_id, try_sym)| { - let (catch_id, _) = ctx + let (catch_id, catch_sym) = ctx .all_symbols() .find(|(_, s)| s.parent == Some(try_id) && matches!(s.kind, SymbolKind::Catch))?; let (catch_expr_id, catch_expr) = ctx.all_symbols().find(|(_, s)| s.parent == Some(catch_id))?; @@ -32,7 +32,34 @@ impl LintRule for RedundantTry { } let mut d = Diagnostic::new(LintMessage::RedundantTry, self.severity()); - if let Some(range) = try_sym.source.text_range { + + // The try body is the one child of `try_id` that isn't `catch`; bound its end by + // where `catch` starts, since a compound body's trailing `)` isn't tracked by any + // HIR symbol. + let body_start = ctx + .all_symbols() + .find(|(id, s)| s.parent == Some(try_id) && *id != catch_id) + .and_then(|(id, _)| ctx.full_range(id)) + .map(|r| r.start); + + if let (Some(try_start), Some(catch_expr_range), Some(body_start), Some(catch_start)) = ( + try_sym.source.text_range, + catch_expr.source.text_range, + body_start, + catch_sym.source.text_range, + ) { + let range = mq_lang::Range { + start: try_start.start, + end: catch_expr_range.end, + }; + let body_range = mq_lang::Range { + start: body_start, + end: catch_start.start, + }; + d = d + .with_range(range) + .with_fix(Fix::verbatim(range, body_range).with_suffix("?")); + } else if let Some(range) = try_sym.source.text_range { d = d.with_range(range); } Some(d) @@ -64,6 +91,16 @@ mod tests { assert_eq!(diags.len(), 1); } + #[test] + fn fix_replaces_try_catch_with_error_suppression_operator() { + let code = r#"try: get("x") catch: none"#; + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(range, diags[0].range.unwrap()); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), r#"get("x")?"#); + } + #[rstest] #[case(r#"try: get("x") catch: "default""#)] #[case(r#"try: get("x") catch: 0"#)] diff --git a/crates/mq-lint/src/rules/style/unnecessary_interpolation.rs b/crates/mq-lint/src/rules/style/unnecessary_interpolation.rs index 3a18b48d7..6e6458551 100644 --- a/crates/mq-lint/src/rules/style/unnecessary_interpolation.rs +++ b/crates/mq-lint/src/rules/style/unnecessary_interpolation.rs @@ -1,4 +1,4 @@ -use crate::{Diagnostic, LintContext, LintMessage, LintRule, RuleId, Severity}; +use crate::{Diagnostic, Fix, LintContext, LintMessage, LintRule, RuleId, Severity}; use mq_hir::SymbolKind; use rustc_hash::FxHashMap; @@ -26,13 +26,23 @@ impl LintRule for UnnecessaryInterpolation { .filter(|(_, sym)| matches!(sym.kind, SymbolKind::InterpolatedString)) .filter_map(|(sym_id, sym)| { let count = child_counts.get(&sym_id).copied().unwrap_or(0); - if count == 1 { - sym.source.text_range.map(|range| { - Diagnostic::new(LintMessage::UnnecessaryInterpolation, self.severity()).with_range(range) - }) - } else { - None + if count != 1 { + return None; } + let range = sym.source.text_range?; + let mut d = Diagnostic::new(LintMessage::UnnecessaryInterpolation, self.severity()).with_range(range); + + // The lone segment's text is already captured verbatim as a `Variable` symbol's + // value, so use it directly rather than slicing `${...}`'s span (which includes + // the delimiters). + let expr_text = ctx + .all_symbols() + .find(|(_, s)| s.parent == Some(sym_id) && matches!(s.kind, SymbolKind::Variable)) + .and_then(|(_, s)| s.value.clone()); + if let Some(expr_text) = expr_text { + d = d.with_fix(Fix::literal(range, expr_text)); + } + Some(d) }) .collect() } @@ -62,6 +72,16 @@ mod tests { assert_eq!(diags.len(), 1); } + #[rstest] + #[case(r#"s"${x}""#, "x")] + #[case(r#"s"${.h1}""#, ".h1")] + fn fix_replaces_interpolation_with_inner_expr(#[case] code: &str, #[case] expected: &str) { + let diags = check(code); + let fix = diags[0].fix.as_ref().unwrap(); + let (range, replacement) = fix.resolve(code).unwrap(); + assert_eq!(crate::fix::apply_edits(code, &[(range, replacement)]), expected); + } + #[rstest] #[case(r#"s"${x}${y}""#)] #[case(r#"s"prefix ${x}""#)] diff --git a/crates/mq-lsp/src/code_action.rs b/crates/mq-lsp/src/code_action.rs index 2d61e9212..a9b010bfb 100644 --- a/crates/mq-lsp/src/code_action.rs +++ b/crates/mq-lsp/src/code_action.rs @@ -6,7 +6,8 @@ use std::{ use rustc_hash::FxHashMap; use smol_str::SmolStr; use tower_lsp_server::ls_types::{ - self, CodeAction, CodeActionKind, CodeActionOrCommand, CodeActionParams, Position, Range, TextEdit, WorkspaceEdit, + self, CodeAction, CodeActionKind, CodeActionOrCommand, CodeActionParams, NumberOrString, Position, Range, TextEdit, + WorkspaceEdit, }; use url::Url; @@ -64,11 +65,44 @@ fn add_module_statement_action( }) } +/// Builds a quick fix from a resolved `mq-lint` [`Fix`](mq_lint::Fix), replacing the diagnostic's +/// span with the fix's suggested text. +fn lint_fix_action( + uri: &ls_types::Uri, + diagnostic: &ls_types::Diagnostic, + title: String, + range: mq_lang::Range, + replacement: String, +) -> CodeActionOrCommand { + let mut changes = HashMap::new(); + changes.insert( + uri.clone(), + vec![TextEdit { + range: to_range(range), + new_text: replacement, + }], + ); + + CodeActionOrCommand::CodeAction(CodeAction { + title, + kind: Some(CodeActionKind::QUICKFIX), + diagnostics: Some(vec![diagnostic.clone()]), + edit: Some(WorkspaceEdit { + changes: Some(changes), + ..Default::default() + }), + ..Default::default() + }) +} + /// Builds quick fixes for diagnostics in the request range: pub(crate) fn response( hir: Arc>, url: Url, params: CodeActionParams, + source_id: Option, + lint_config: Option<&mq_lint::LintConfig>, + source_text: Option<&str>, ) -> Option> { let hir_guard = hir.read().unwrap(); hir_guard.source_by_url(&url)?; @@ -117,6 +151,30 @@ pub(crate) fn response( } } + if let (Some(source_id), Some(lint_config), Some(source_text)) = (source_id, lint_config, source_text) { + let ctx = mq_lint::LintContext::new(&hir_guard, source_id, lint_config); + let lint_diagnostics = mq_lint::Linter::with_default_rules().run(&ctx); + + for diagnostic in ¶ms.context.diagnostics { + let Some(NumberOrString::String(rule_id)) = &diagnostic.code else { + continue; + }; + let Some(matched) = lint_diagnostics + .iter() + .find(|d| d.rule_id().as_str() == rule_id.as_str() && d.range.map(to_range) == Some(diagnostic.range)) + else { + continue; + }; + let Some(fix) = matched.fix.as_ref() else { continue }; + let Some((range, replacement)) = fix.resolve(source_text) else { + continue; + }; + + let title = format!("Fix: {}", matched.help().unwrap_or_else(|| matched.message())); + actions.push(lint_fix_action(uri, diagnostic, title, range, replacement)); + } + } + if actions.is_empty() { None } else { Some(actions) } } @@ -151,7 +209,7 @@ mod tests { let range = Range::new(Position::new(0, 0), Position::new(0, 9)); let params = params_for(&uri, range, "Unresolved symbol: csv_parse"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_some()); let actions = actions.unwrap(); @@ -171,7 +229,7 @@ mod tests { let range = Range::new(Position::new(0, 19), Position::new(0, 24)); let params = params_for(&uri, range, "Unresolved symbol: func1"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_none()); } @@ -185,7 +243,7 @@ mod tests { let range = Range::new(Position::new(0, 0), Position::new(0, 17)); let params = params_for(&uri, range, "Unresolved symbol: totally_unknown_fn"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_none()); } @@ -203,7 +261,7 @@ mod tests { let range = Range::new(Position::new(0, 0), Position::new(0, 9)); let params = params_for(&uri, range, "Unresolved symbol: csv_parse"); - let actions = response(Arc::new(RwLock::new(hir)), url, params).unwrap(); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None).unwrap(); assert_eq!(actions.len(), exporters.len()); let titles: Vec = actions @@ -228,7 +286,7 @@ mod tests { let range = Range::new(Position::new(0, 0), Position::new(0, 9)); let params = params_for(&uri, range, "Unresolved symbol: csv_parse"); - let actions = response(Arc::new(RwLock::new(hir)), url, params).unwrap(); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None).unwrap(); let CodeActionOrCommand::CodeAction(action) = &actions[0] else { panic!("expected a CodeAction"); }; @@ -258,7 +316,7 @@ mod tests { let range = Range::new(Position::new(0, 0), Position::new(0, 9)); let params = params_for(&uri, range, "Unresolved symbol: csv_parse"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_some()); } @@ -273,7 +331,7 @@ mod tests { let range = Range::new(Position::new(5, 0), Position::new(5, 9)); let params = params_for(&uri, range, "Unresolved symbol: csv_parse"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_none()); } @@ -296,7 +354,7 @@ mod tests { partial_result_params: Default::default(), }; - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_none()); } @@ -308,7 +366,7 @@ mod tests { let range = Range::new(Position::new(0, 0), Position::new(0, 9)); let params = params_for(&uri, range, "Unresolved symbol: csv_parse"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_none()); } @@ -350,7 +408,7 @@ mod tests { partial_result_params: Default::default(), }; - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_some()); let actions = actions.unwrap(); // Only `json_parse` resolves to a std module export; `totally_unknown_fn` contributes nothing. @@ -392,7 +450,7 @@ mod tests { let uri = ls_types::Uri::from_str(url.as_str()).unwrap(); let params = params_for(&uri, range, "Unresolved symbol: csv_parse"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_some()); let actions = actions.unwrap(); @@ -425,7 +483,7 @@ mod tests { let uri = ls_types::Uri::from_str(url.as_str()).unwrap(); let params = params_for(&uri, range, "Unresolved symbol: some_fn"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_none()); } @@ -443,7 +501,89 @@ mod tests { let range = Range::new(Position::new(0, 0), Position::new(0, 1)); let params = params_for(&uri, range, "Unresolved symbol: csv_parse"); - let actions = response(Arc::new(RwLock::new(hir)), url, params); + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); + assert!(actions.is_none()); + } + + /// Builds `CodeActionParams` carrying a single diagnostic shaped like the ones + /// `LspError::LintWarning` produces (range + `code` set to the rule id), so the response's + /// lint-fix matching (which keys off `diagnostic.code` and `diagnostic.range`) has something + /// to match against. + fn lint_diagnostic_params(uri: &ls_types::Uri, range: Range, rule_id: mq_lint::RuleId) -> CodeActionParams { + CodeActionParams { + text_document: TextDocumentIdentifier { uri: uri.clone() }, + range, + context: CodeActionContext { + diagnostics: vec![ls_types::Diagnostic::new( + range, + None, + Some(NumberOrString::String(rule_id.to_string())), + Some("mq-lint".to_string()), + "diagnostic message is irrelevant to matching".to_string(), + None, + None, + )], + only: None, + trigger_kind: None, + }, + work_done_progress_params: WorkDoneProgressParams::default(), + partial_result_params: Default::default(), + } + } + + #[test] + fn test_offers_fix_for_boolean_comparison_lint_diagnostic() { + let mut hir = mq_hir::Hir::default(); + let url = Url::parse("file:///test.mq").unwrap(); + let code = ".checked == true"; + let (source_id, _) = hir.add_code(Some(url.clone()), code); + + let uri = ls_types::Uri::from_str(url.as_str()).unwrap(); + let range = Range::new(Position::new(0, 0), Position::new(0, code.len() as u32)); + let params = lint_diagnostic_params(&uri, range, mq_lint::RuleId::BooleanComparison); + let lint_config = mq_lint::LintConfig::default(); + + let actions = response( + Arc::new(RwLock::new(hir)), + url, + params, + Some(source_id), + Some(&lint_config), + Some(code), + ) + .unwrap(); + + assert_eq!(actions.len(), 1); + let CodeActionOrCommand::CodeAction(action) = &actions[0] else { + panic!("expected a CodeAction"); + }; + assert_eq!(action.kind, Some(CodeActionKind::QUICKFIX)); + let edits = action + .edit + .as_ref() + .unwrap() + .changes + .as_ref() + .unwrap() + .get(&uri) + .unwrap(); + assert_eq!(edits.len(), 1); + assert_eq!(edits[0].new_text, ".checked"); + } + + #[test] + fn test_no_fix_action_when_lint_disabled() { + let mut hir = mq_hir::Hir::default(); + let url = Url::parse("file:///test.mq").unwrap(); + let code = ".checked == true"; + hir.add_code(Some(url.clone()), code); + + let uri = ls_types::Uri::from_str(url.as_str()).unwrap(); + let range = Range::new(Position::new(0, 0), Position::new(0, code.len() as u32)); + let params = lint_diagnostic_params(&uri, range, mq_lint::RuleId::BooleanComparison); + + // No `source_id`/`lint_config`/`source_text` supplied (mirrors `enable_lint: false`). + let actions = response(Arc::new(RwLock::new(hir)), url, params, None, None, None); assert!(actions.is_none()); } } diff --git a/crates/mq-lsp/src/server.rs b/crates/mq-lsp/src/server.rs index 6db8612c4..12f1ccae6 100644 --- a/crates/mq-lsp/src/server.rs +++ b/crates/mq-lsp/src/server.rs @@ -202,7 +202,20 @@ impl LanguageServer for Backend { params: ls_types::CodeActionParams, ) -> jsonrpc::Result> { let url = to_url(¶ms.text_document.uri.clone()); - Ok(code_action::response(Arc::clone(&self.hir), url, params)) + let uri_string = params.text_document.uri.to_string(); + + let source_id = self.source_map.read().unwrap().get_by_left(&uri_string).copied(); + let source_text = self.text_map.get(&uri_string).map(|text| Arc::clone(text.value())); + let lint_config = self.config.enable_lint.then(|| self.config.lint_config.clone()); + + Ok(code_action::response( + Arc::clone(&self.hir), + url, + params, + source_id, + lint_config.as_ref(), + source_text.as_deref().map(String::as_str), + )) } async fn rename(&self, params: ls_types::RenameParams) -> jsonrpc::Result> {