Extend builtin

cli/render.mbt

@@ -36,10 +36,10 @@ fn render_skipped_file_warnings(skipped_files : Array[SkippedFile]) -> String {
 
 ///|
 fn render_skipped_file_warning(skipped : SkippedFile) -> String {
-  match skipped.block_start_line {
-    Some(line) =>
-      "warning: skipping \{skipped.file} block starting at line \{line}: \{skipped.reason}"
-    None => "warning: skipping \{skipped.file}: \{skipped.reason}"
+  if skipped.block_start_line is Some(line) {
+    "warning: skipping \{skipped.file} block starting at line \{line}: \{skipped.reason}"
+  } else {
+    "warning: skipping \{skipped.file}: \{skipped.reason}"
   }
 }
 
@@ -226,7 +226,7 @@ fn format_location(file : String, loc : @basic.Location) -> String {
 fn trim_trailing_newlines(input : String) -> String {
   let mut end = input.length()
   while end > 0 && input[end - 1:end].to_owned() == "\n" {
-    end = end - 1
+    end -= 1
   }
   input[:end].to_owned()
 }

cli/render_wbtest.mbt

@@ -52,8 +52,8 @@ async test "matched source line prefix is not dimmed" {
     content="12 | let value = \u{1b}[93mtarget()\u{1b}[39m",
   )
   let arr = [5, 6, 7]
-  for i = 0; i < arr.length(); i = i + 1 {
-    if arr[i] == 0 {
+  for i, value in arr {
+    if value == 0 {
       @stdio.stdout.write("\{i}\n")
     }
   }

cli/scan_wbtest.mbt

@@ -333,7 +333,7 @@ async test "scan excludes configured rule ids before planning" {
 async test "scan rejects unknown exclude rule ids" {
   try collect_directory_hits(unknown_exclude_rule_scan_options()) catch {
     CliError::Usage(message~, exit_code=found) => {
-      inspect(found, content="2")
+      assert_eq(found, 2)
       inspect(message, content="unknown rule id in --exclude-rule: missing")
     }
     err => fail("unexpected error \{err}")

e2etests/BUILTIN_RULES.md

@@ -73,6 +73,21 @@ source:
 3 >   inspect(flag, content="true")
 4 | }
 
+testdata/builtin-rules-all/simplifiable_assignment.mbt:4:3-4:19
+rule: moonbitlang/simplifiable_assignment
+description:
+  Assignment repeats its target as the operand of a binary expression.
+  Prefer the corresponding augmented assignment, such as foo += 1, foo.bar += 1, foo[i] +=1.
+  Note: Apply this rewrite only when the target expression and any index expression
+  are free of side effects. Augmented assignment may evaluate them a different
+  number of times.
+source:
+2 | fn simplifiable_assignment(step : Int) -> Int {
+3 |   let mut foo = 0
+4 >   foo = foo + step
+5 |   foo
+6 | }
+
 testdata/builtin-rules-all/cstyle_forward_simple_forloop.mbt:3:3-5:4
 rule: moonbitlang/cstyle_forward_simple_forloop
 description:
@@ -135,6 +150,7 @@ $ cd "$TESTDIR"/.. && moonrun "$TESTDIR"/moongrep.wasm -- scan --output-json --e
 {"file":"testdata/builtin-rules-all/inspect_number.mbt","rule_id":"moonbitlang/inspect_number","description":"Found inspect() snapshots whose expected value is a plain number.\nPrefer numeric assertions for numeric checks.","range":{"start":{"line":3,"column":3},"end":{"line":3,"column":26}},"matched_source":"inspect(1, content=\"1\")","source_context":[{"line":1,"text":"///|","is_match":false},{"line":2,"text":"fn number_snapshot() -> Unit {","is_match":false},{"line":3,"text":"  inspect(1, content=\"1\")","is_match":true},{"line":4,"text":"}","is_match":false}]}
 {"file":"testdata/builtin-rules-all/unnessary_else.mbt","rule_id":"moonbitlang/unnessary_else","description":"Found an if expression whose else branch is empty or only returns ().\nPrefer omitting the unnecessary else branch.","range":{"start":{"line":3,"column":3},"end":{"line":6,"column":12}},"matched_source":"if flag {\n    prepare()\n    finish()\n  } else {}","source_context":[{"line":1,"text":"///|","is_match":false},{"line":2,"text":"fn unnecessary_empty_else(flag : Bool) -> Unit {","is_match":false},{"line":3,"text":"  if flag {","is_match":true},{"line":4,"text":"    prepare()","is_match":true},{"line":5,"text":"    finish()","is_match":true},{"line":6,"text":"  } else {}","is_match":true},{"line":7,"text":"}","is_match":false},{"line":8,"text":"///|","is_match":false}]}
 {"file":"testdata/builtin-rules-all/inspect_boolean.mbt","rule_id":"moonbitlang/inspect_boolean","description":"Found inspect(), debug_inspect(), or json_inspect() snapshots whose expected value is true or false.\nPrefer assert_true(...) or assert_false(...) for boolean checks.","range":{"start":{"line":3,"column":3},"end":{"line":3,"column":32}},"matched_source":"inspect(flag, content=\"true\")","source_context":[{"line":1,"text":"///|","is_match":false},{"line":2,"text":"fn boolean_snapshot(flag : Bool) -> Unit {","is_match":false},{"line":3,"text":"  inspect(flag, content=\"true\")","is_match":true},{"line":4,"text":"}","is_match":false}]}
+{"file":"testdata/builtin-rules-all/simplifiable_assignment.mbt","rule_id":"moonbitlang/simplifiable_assignment","description":"Assignment repeats its target as the operand of a binary expression.\nPrefer the corresponding augmented assignment, such as foo += 1, foo.bar += 1, foo[i] +=1.\nNote: Apply this rewrite only when the target expression and any index expression\nare free of side effects. Augmented assignment may evaluate them a different\nnumber of times.","range":{"start":{"line":4,"column":3},"end":{"line":4,"column":19}},"matched_source":"foo = foo + step","source_context":[{"line":2,"text":"fn simplifiable_assignment(step : Int) -> Int {","is_match":false},{"line":3,"text":"  let mut foo = 0","is_match":false},{"line":4,"text":"  foo = foo + step","is_match":true},{"line":5,"text":"  foo","is_match":false},{"line":6,"text":"}","is_match":false}]}
 {"file":"testdata/builtin-rules-all/cstyle_forward_simple_forloop.mbt","rule_id":"moonbitlang/cstyle_forward_simple_forloop","description":"C-style forward for loops that can be rewritten as simple for-in loops.","range":{"start":{"line":3,"column":3},"end":{"line":5,"column":4}},"matched_source":"for i = 0; i < limit; i = i + 1 {\n    tick()\n  }","source_context":[{"line":1,"text":"///|","is_match":false},{"line":2,"text":"fn forward_simple_loop(limit : Int) -> Unit {","is_match":false},{"line":3,"text":"  for i = 0; i < limit; i = i + 1 {","is_match":true},{"line":4,"text":"    tick()","is_match":true},{"line":5,"text":"  }","is_match":true},{"line":6,"text":"}","is_match":false}]}
 {"file":"testdata/builtin-rules-all/cstyle_backward_simple_forloop.mbt","rule_id":"moonbitlang/cstyle_backward_simple_forloop","description":"C-style backward for loops that can be rewritten as simple for-in loops.","range":{"start":{"line":3,"column":3},"end":{"line":5,"column":4}},"matched_source":"for i = limit; i > 0; i = i - 1 {\n    tick_back()\n  }","source_context":[{"line":1,"text":"///|","is_match":false},{"line":2,"text":"fn backward_simple_loop(limit : Int) -> Unit {","is_match":false},{"line":3,"text":"  for i = limit; i > 0; i = i - 1 {","is_match":true},{"line":4,"text":"    tick_back()","is_match":true},{"line":5,"text":"  }","is_match":true},{"line":6,"text":"}","is_match":false}]}
 {"file":"testdata/builtin-rules-all/cstyle_forward_array_iteration.mbt","rule_id":"moonbitlang/cstyle_forward_array_iteration","description":"C-style forward array iteration that can be rewritten as simple for-in loops.","range":{"start":{"line":3,"column":3},"end":{"line":5,"column":4}},"matched_source":"for i = 0; i < items.length(); i = i + 1 {\n    consume(items[i])\n  }","source_context":[{"line":1,"text":"///|","is_match":false},{"line":2,"text":"fn forward_array_loop(items : Array[Int]) -> Unit {","is_match":false},{"line":3,"text":"  for i = 0; i < items.length(); i = i + 1 {","is_match":true},{"line":4,"text":"    consume(items[i])","is_match":true},{"line":5,"text":"  }","is_match":true},{"line":6,"text":"}","is_match":false}]}
@@ -153,3 +169,41 @@ moonbitlang/catch_all 7:3
 moonbitlang/catch_all 11:3
 moonbitlang/catch_all 17:3
 ```
+
+## Simplifiable assignment variants
+
+All eighteen rule shapes are reported: local variables, fields, and indexed
+values using `+`, `-`, `*`, or `/`, including both operand orders supported for
+commutative operators and compound expressions in the other operand.
+
+```mooncram
+$ cd "$TESTDIR"/.. && moonrun "$TESTDIR"/moongrep.wasm -- scan --output-json --enable-builtin-rules testdata/builtin-simplifiable-assignment-variants/positive.mbt | sed -n 's#.*"rule_id":"moonbitlang/simplifiable_assignment".*"range":{"start":{"line":\([0-9][0-9]*\),"column":\([0-9][0-9]*\)}.*"matched_source":"\([^"]*\)".*#\1:\2 \3#p'
+14:3 add_left = add_left + step * 2
+16:3 add_right = step * 2 + add_right
+18:3 subtract = subtract - step
+20:3 multiply_left = multiply_left * (step + 1)
+22:3 multiply_right = (step + 1) * multiply_right
+24:3 divide = divide / step
+32:3 target.add_left = target.add_left + step * 2
+33:3 target.add_right = step * 2 + target.add_right
+34:3 target.subtract = target.subtract - step
+35:3 target.multiply_left = target.multiply_left * (step + 1)
+36:3 target.multiply_right = (step + 1) * target.multiply_right
+37:3 target.divide = target.divide / step
+42:3 target[0] = target[0] + step * 2
+43:3 target[1] = step * 2 + target[1]
+44:3 target[2] = target[2] - step
+45:3 target[3] = target[3] * (step + 1)
+46:3 target[4] = (step + 1) * target[4]
+47:3 target[5] = target[5] / step
+```
+
+The negative fixture checks unsupported operators, different local, field, or
+indexed targets, different field receivers or indexed collections, reversed
+`-` and `/`, and existing augmented assignments. It produces no findings for
+this rule.
+
+```mooncram
+$ cd "$TESTDIR"/.. && moonrun "$TESTDIR"/moongrep.wasm -- scan --output-json --enable-builtin-rules testdata/builtin-simplifiable-assignment-variants/negative.mbt | sed -n 's#"rule_id":"moonbitlang/simplifiable_assignment"#&#p' | wc -l
+0
+```

main.mbt

@@ -9,9 +9,10 @@ extern "c" fn runtime_exit(code : Int) -> Unit = "exit"
 ///|
 async fn write_cli_output(output : String) -> Unit {
   if output != "" {
-    match output.strip_suffix("\n") {
-      Some(_) => @stdio.stdout.write(output)
-      None => @stdio.stdout.write(output + "\n")
+    if output.strip_suffix("\n") is Some(_) {
+      @stdio.stdout.write(output)
+    } else {
+      @stdio.stdout.write(output + "\n")
     }
   }
 }

matching/ellipsis.mbt

@@ -57,9 +57,10 @@ fn ellipsis_item_raw(item : @untyped_ast.Node) -> String? {
       matching_ellipsis_direct_raw(pattern)
     }
     ArrayPattern_Pattern | SpreadableElem_Regular =>
-      match child(item, "value") {
-        Some(value) => matching_ellipsis_direct_raw(value)
-        None => None
+      if child(item, "value") is Some(value) {
+        matching_ellipsis_direct_raw(value)
+      } else {
+        None
       }
     Parameter_Positional => {
       guard child(item, "binder") is Some(binder) else { return None }
@@ -77,9 +78,10 @@ fn matching_ellipsis_direct_raw(node : @untyped_ast.Node) -> String? {
   let raw = match node.kind {
     Expr_Ident => node.normalized_expr_identifier_name()
     Pattern_Var =>
-      match child(node, "value") {
-        Some(binder) => leaf_string_child(binder, "name")
-        None => None
+      if child(node, "value") is Some(binder) {
+        leaf_string_child(binder, "name")
+      } else {
+        None
       }
     Binder => leaf_string_child(node, "name")
     Type_Name => type_placeholder_name(node)

matching/matching.mbt

@@ -181,17 +181,18 @@ fn match_expr_placeholder(
     contains_name(compiled.expr_metavars, name) {
     Some(bind_value(bindings, name, candidate))
   } else if contains_name(compiled.identifier_metavars, name) {
-    match candidate.normalized_expr_identifier_name() {
-      Some(candidate_name) =>
-        Some(
-          bind_value(bindings, name, string_binding(candidate_name, candidate)),
-        )
-      None => Some(false)
+    if candidate.normalized_expr_identifier_name() is Some(candidate_name) {
+      Some(
+        bind_value(bindings, name, string_binding(candidate_name, candidate)),
+      )
+    } else {
+      Some(false)
     }
   } else if contains_name(compiled.constant_metavars, name) {
-    match expr_constant(candidate) {
-      Some(constant) => Some(bind_value(bindings, name, constant))
-      None => Some(false)
+    if expr_constant(candidate) is Some(constant) {
+      Some(bind_value(bindings, name, constant))
+    } else {
+      Some(false)
     }
   } else {
     None
@@ -216,17 +217,18 @@ fn match_pattern_placeholder(
   } else if is_ignore_placeholder(name) {
     Some(true)
   } else if contains_name(compiled.identifier_metavars, name) {
-    match candidate.normalized_pattern_identifier_name() {
-      Some(candidate_name) =>
-        Some(
-          bind_value(bindings, name, string_binding(candidate_name, candidate)),
-        )
-      None => Some(false)
+    if candidate.normalized_pattern_identifier_name() is Some(candidate_name) {
+      Some(
+        bind_value(bindings, name, string_binding(candidate_name, candidate)),
+      )
+    } else {
+      Some(false)
     }
   } else if contains_name(compiled.constant_metavars, name) {
-    match pattern_constant(candidate) {
-      Some(constant) => Some(bind_value(bindings, name, constant))
-      None => Some(false)
+    if pattern_constant(candidate) is Some(constant) {
+      Some(bind_value(bindings, name, constant))
+    } else {
+      Some(false)
     }
   } else {
     None
@@ -783,35 +785,35 @@ fn constructor_name(node : @untyped_ast.Node) -> String? {
 ///|
 fn normalized_constructor(constr : @untyped_ast.Node) -> String {
   let name = constructor_name(constr).unwrap_or("")
-  match child(constr, "extra_info") {
-    Some(extra) =>
-      match extra.kind {
-        ConstructorExtraInfo_NoExtraInfo => name
-        ConstructorExtraInfo_Package =>
-          if leaf_string_child(extra, "value") is Some(pkg) {
-            "@\{pkg}.\{name}"
+  if child(constr, "extra_info") is Some(extra) {
+    match extra.kind {
+      ConstructorExtraInfo_NoExtraInfo => name
+      ConstructorExtraInfo_Package =>
+        if leaf_string_child(extra, "value") is Some(pkg) {
+          "@\{pkg}.\{name}"
+        } else {
+          name
+        }
+      ConstructorExtraInfo_TypeName =>
+        if child(extra, "value") is Some(type_name) {
+          "\{normalized_type_name(type_name)}::\{name}"
+        } else {
+          name
+        }
+      ConstructorExtraInfo_TypeNameWithConstrPackage =>
+        if child(extra, "type_name") is Some(type_name) {
+          if leaf_string_child(extra, "pkg") is Some(pkg) {
+            "\{normalized_type_name(type_name)}::@\{pkg}.\{name}"
           } else {
-            name
-          }
-        ConstructorExtraInfo_TypeName =>
-          if child(extra, "value") is Some(type_name) {
             "\{normalized_type_name(type_name)}::\{name}"
-          } else {
-            name
           }
-        ConstructorExtraInfo_TypeNameWithConstrPackage =>
-          if child(extra, "type_name") is Some(type_name) {
-            if leaf_string_child(extra, "pkg") is Some(pkg) {
-              "\{normalized_type_name(type_name)}::@\{pkg}.\{name}"
-            } else {
-              "\{normalized_type_name(type_name)}::\{name}"
-            }
-          } else {
-            name
-          }
-        _ => name
-      }
-    None => name
+        } else {
+          name
+        }
+      _ => name
+    }
+  } else {
+    name
   }
 }
 

matching/matching_test.mbt

@@ -371,13 +371,13 @@ test "argument metavar captures complete argument nodes" {
     source in [
       "sink(value)", "sink(label=value)", "sink(label~)", "sink(label?=value)", "sink(label?)",
     ] {
-    match match_expr_pattern(compiled, parse_expr(source)) {
-      Some(result) =>
-        match result.bindings.get("arg") {
-          Some(Single(node)) => assert_true(node.kind == Argument)
-          _ => fail("expected argument binding")
-        }
-      None => fail("expected argument metavar to match \{source}")
+    if match_expr_pattern(compiled, parse_expr(source)) is Some(result) {
+      match result.bindings.get("arg") {
+        Some(Single(node)) => assert_true(node.kind == Argument)
+        _ => fail("expected argument binding")
+      }
+    } else {
+      fail("expected argument metavar to match \{source}")
     }
   }
   assert_false(match_expr_pattern(compiled, parse_expr("sink()")) is Some(_))

matching/untyped_matching_test.mbt

@@ -193,9 +193,10 @@ test "default matcher binds repeated expression metavars" {
 ///|
 test "default matcher treats only $_ as ignore placeholder" {
   let compiled = untyped_compiled(parse_untyped_test_metavar_node("foo($_)"))
-  match untyped_match(compiled, "foo(make())") {
-    Some(result) => inspect(result.bindings.length(), content="0")
-    None => fail("expected $_ ignore placeholder to match")
+  if untyped_match(compiled, "foo(make())") is Some(result) {
+    assert_eq(result.bindings.length(), 0)
+  } else {
+    fail("expected $_ ignore placeholder to match")
   }
   assert_true(untyped_match(compiled, "foo(other())") is Some(_))
   assert_true(
@@ -306,14 +307,14 @@ test "default matcher captures complex type nodes" {
       "let value : Array[Int] = input", "let value : Int? = input", "let value : (Int, String) = input",
       "let value : (Int) -> String = input",
     ] {
-    match untyped_match(compiled, source) {
-      Some(result) =>
-        match result.bindings.get("T") {
-          Some(Single(node)) =>
-            assert_true(node.kind.to_string().has_prefix("Type::"))
-          _ => fail("expected type binding for \{source}")
-        }
-      None => fail("expected type metavar to match \{source}")
+    if untyped_match(compiled, source) is Some(result) {
+      match result.bindings.get("T") {
+        Some(Single(node)) =>
+          assert_true(node.kind.to_string().has_prefix("Type::"))
+        _ => fail("expected type binding for \{source}")
+      }
+    } else {
+      fail("expected type metavar to match \{source}")
     }
   }
 }

query/query_test.mbt

@@ -148,7 +148,7 @@ test "query captures identifiers and constants by metavar name" {
     fail("expected second literal value")
   }
   inspect(first_value, content="danger")
-  inspect(second_value, content="42")
+  assert_eq(second_value, "42")
 }
 
 ///|
@@ -225,7 +225,7 @@ test "query raises parse error when relevant source is invalid" {
     #|}
     #|
   try query.captures(source_name="bad.mbt", source) catch {
-    err => inspect("\{Repr(err)}".contains("parse"), content="true")
+    err => assert_true("\{Repr(err)}".contains("parse"))
   } noraise {
     _ => fail("expected parse error")
   }
@@ -234,7 +234,7 @@ test "query raises parse error when relevant source is invalid" {
 ///|
 test "query raises when pattern is invalid" {
   try ExprQuery::ExprQuery("target(") catch {
-    err => inspect("\{Repr(err)}".contains("InvalidRule"), content="true")
+    err => assert_true("\{Repr(err)}".contains("InvalidRule"))
   } noraise {
     _ => fail("expected invalid pattern")
   }
@@ -267,7 +267,7 @@ test "query captures_from_ast matches a direct expression root" {
   guard query_constant_value(literal) is Some(value) else {
     fail("expected literal value")
   }
-  inspect(value, content="42")
+  assert_eq(value, "42")
 }
 
 ///|

rule/apply/apply.mbt

@@ -110,13 +110,12 @@ fn scan_plan_literal_matches(
   literal_cache : Map[String, Bool],
   literal : String,
 ) -> Bool {
-  match literal_cache.get(literal) {
-    Some(matched) => matched
-    None => {
-      let matched = source.contains(literal)
-      literal_cache[literal] = matched
-      matched
-    }
+  if literal_cache.get(literal) is Some(matched) {
+    matched
+  } else {
+    let matched = source.contains(literal)
+    literal_cache[literal] = matched
+    matched
   }
 }