fix: parser and semantic correctness gaps, enforce benchmark parity

Parser:
- lock: parse and discard the optional trailing `on fail` clause, so
  `lock { } on fail e { }` no longer leaves `on` to expression parsing
- member assignment: retain the compound operator on Expr::MemberAssign
  via a new `op` field; `self.f += self.f` was being reported as a
  self-assignment because the `+` was dropped
- let expression: accept `let var v = ... in ...`, matching
  query_let_clause and the let-var-decl grammar
- correct the parse_leading_qualifiers doc: readonly/distinct are
  consumed when a class follows, transactional via contextual keywords

Semantic:
- union annotations resolve to Unknown unless every member resolves to
  the same type; picking types[0] made `int|string x = "s"` an error

Benchmark script:
- fail immediately when `cargo build --release` fails or the binary is
  missing, so a stale binary cannot be benchmarked
- gate the timing on a findings diff between bal scan and blazelint,
  and reject timeouts or nonzero exits in the timed runs

Docs:
- relabel the stale "current state" figures in both plans as baselines
- record unused-parameters as shipping `off`, an intentional divergence

Corpus: diagnostic-free files 149 -> 151 of 163, false positives still 0.

Author: Chamal1120

.blazerc

@@ -11,6 +11,14 @@ self-assignment = "warn"
 invalid-range = "warn"
 unused-variables = "error"
 
+# Advisory scan rules — opt in by raising these to "warn".
+# A rule omitted from this file does not run at all, so listing them as "off"
+# is documentation, not a behaviour change.
+unused-parameters = "off"          # ballerina:2
+isolated-public-function = "off"   # ballerina:3
+isolated-public-method = "off"     # ballerina:4
+isolated-public-class = "off"      # ballerina:5
+
 [settings]
 max-line-length = 120
 max-function-length = 50

docs/SCAN_RULES_PLAN.md

@@ -21,7 +21,8 @@ change and no new analysis capability.
 | | Count |
 |---|---|
 | Official scan rules | 27 (12 language + 15 library) |
-| Implemented | **1** — `ballerina:2` unused function parameter |
+| Baseline (before this plan) | **1** — `ballerina:2` unused function parameter |
+| Implemented now | **7** |
 | After this plan | **7** (26%) |
 
 Our other six rules (`camel-case`, `constant-case`, `line-length`,
@@ -130,13 +131,12 @@ Add to `Config::default()`:
 | `isolated-public-function` | `off` | Advisory; noisy outside concurrent code |
 | `isolated-public-method` | `off` | ” |
 | `isolated-public-class` | `off` | ” |
-| `unused-parameters` | `off` → **`warn`** | Reconsider: `bal scan` ships this on as `ballerina:2`. Aligning improves parity honestly, at the cost of noise on callback signatures. |
+| `unused-parameters` | `off` | Intentional divergence from `bal scan`, which ships this on as `ballerina:2`: it fires on callback signatures that cannot drop a parameter. |
 
-Note the `unused-parameters` question is a genuine trade-off, not an oversight:
-we turned it off last session precisely because it fired 27 times on the example
-corpus, nearly all on signatures that cannot drop a parameter. Recommend keeping
-it `off` by default and documenting the divergence, rather than importing the
-noise to match a number.
+The `unused-parameters` divergence is a deliberate trade-off, not an oversight:
+we turned it off because it fired 27 times on the example corpus, nearly all on
+signatures that cannot drop a parameter. It stays `off` by default and the
+divergence is documented, rather than importing the noise to match a number.
 
 ## 6. Verification
 

docs/SEMANTIC_PLAN.md

@@ -4,9 +4,10 @@
 parser now structures, so Blazelint's *output* is trustworthy on real code, not
 just its parsing.
 
-**Current state** — 99/163 corpus files (60%) are diagnostic-free. The other 64
-produce **207 diagnostics, and the sampled ones are all false positives**: the
-official Ballerina 2201.10.0 compiler accepts every file Blazelint rejects.
+**Baseline before this plan** — 99/163 corpus files (60%) were diagnostic-free.
+The other 64 produced **207 diagnostics, and the sampled ones were all false
+positives**: the official Ballerina 2201.10.0 compiler accepted every file
+Blazelint rejected. (Phases A–E have since shipped — see §4 for the results.)
 
 **Target** — ≥95% of corpus files diagnostic-free, with zero false positives
 maintained, and every parser AST node reachable by both analysis passes.

scripts/benchmark_vs_scan.sh

@@ -36,7 +36,11 @@ WORK="$(mktemp -d)"
 trap 'rm -rf "$WORK"' EXIT
 
 [ -x "$BAL" ] || { echo "Ballerina not found at $BAL (set BAL_HOME)"; exit 1; }
-cargo build --release --quiet
+command -v jq >/dev/null || { echo "jq is required to compare findings"; exit 1; }
+
+# A stale or missing binary must never be benchmarked: fail here, not later.
+cargo build --release --quiet || { echo "cargo build --release failed"; exit 1; }
+[ -x "$BLZ" ] || { echo "blazelint binary not found at $BLZ"; exit 1; }
 
 # ---------------------------------------------------------------- corpus setup
 PKG="$WORK/bench"
@@ -95,29 +99,84 @@ echo "Corpus: $FILES files in a single package"
 echo "Rule set: ballerina:1 (avoid-checkpanic), ballerina:2 (unused parameter)"
 echo
 
+# ------------------------------------------------------- correctness gate
+# A timing comparison is only meaningful if both tools actually did the work and
+# found the same things. Run each once, unsuppressed, and diff the findings
+# before any number is reported.
+run_or_die() { # label timeout cmd...
+    local label="$1" limit="$2"; shift 2
+    local out status
+    out=$(timeout "$limit" "$@" 2>&1); status=$?
+    if [ "$status" -eq 124 ]; then
+        echo "$label timed out after ${limit}s" >&2; exit 1
+    elif [ "$status" -ne 0 ]; then
+        echo "$label failed (exit $status):" >&2; echo "$out" >&2; exit 1
+    fi
+    printf '%s\n' "$out"
+}
+
+# Canonical finding: file:line:rule-id. Columns differ between the tools'
+# anchor points, so equivalence is compared at line granularity.
+(cd "$PKG" && run_or_die "bal scan" 900 "$BAL" scan) >/dev/null
+jq -r '.[] | "\(.location.filePath):\(.location.startLine + 1):\(.rule.id)"' \
+    "$PKG/target/report/scan_results.json" | sort > "$WORK/scan.findings"
+
+: > "$WORK/blz.findings"
+for f in "$PKG"/*.bal; do
+    (cd "$PKG" && run_or_die "blazelint $(basename "$f")" 300 "$BLZ" "$(basename "$f")")
+done | awk '
+    /^(Warning|Error):/ { msg = $0 }
+    /^[[:space:]]*--> / {
+        rule = "unmatched"
+        if (msg ~ /never used/) rule = "ballerina:2"
+        else if (msg ~ /checkpanic/) rule = "ballerina:1"
+        split($2, p, ":"); print p[1] ":" p[2] ":" rule
+    }' | sort > "$WORK/blz.findings"
+
+if ! diff -u "$WORK/scan.findings" "$WORK/blz.findings" > "$WORK/findings.diff"; then
+    echo "Findings differ between bal scan and blazelint; refusing to benchmark." >&2
+    echo "(-) bal scan only, (+) blazelint only:" >&2
+    sed -n '3,$p' "$WORK/findings.diff" >&2
+    exit 1
+fi
+FINDINGS=$(wc -l < "$WORK/scan.findings")
+[ "$FINDINGS" -gt 0 ] || { echo "No findings produced; corpus or rule set is wrong" >&2; exit 1; }
+echo "Findings: $FINDINGS, identical between both tools"
+
 # --------------------------------------------------------------- measure tools
 median() { sort -n | awk '{a[NR]=$1} END {print (NR%2) ? a[(NR+1)/2] : (a[NR/2]+a[NR/2+1])/2}'; }
 
+# Timed runs stay quiet for clean measurement, but a nonzero exit or a timeout
+# invalidates the sample rather than being recorded as a fast run.
+check_timed() { # label status
+    if [ "$2" -eq 124 ]; then echo "$1 timed out during timing run" >&2; exit 1
+    elif [ "$2" -ne 0 ]; then echo "$1 failed during timing run (exit $2)" >&2; exit 1; fi
+}
+
 # JVM floor: what `bal` costs before doing any analysis at all.
 jvm_times=()
 for _ in $(seq 1 "$RUNS"); do
-    s=$(date +%s%N); (cd "$PKG" && timeout 300 "$BAL" version >/dev/null 2>&1); e=$(date +%s%N)
+    s=$(date +%s%N); (cd "$PKG" && timeout 300 "$BAL" version >/dev/null 2>&1); status=$?; e=$(date +%s%N)
+    check_timed "bal version" "$status"
     jvm_times+=( $(( (e - s) / 1000000 )) )
 done
 JVM=$(printf '%s\n' "${jvm_times[@]}" | median)
 
 scan_times=()
 for _ in $(seq 1 "$RUNS"); do
-    s=$(date +%s%N); (cd "$PKG" && timeout 900 "$BAL" scan >/dev/null 2>&1); e=$(date +%s%N)
+    s=$(date +%s%N); (cd "$PKG" && timeout 900 "$BAL" scan >/dev/null 2>&1); status=$?; e=$(date +%s%N)
+    check_timed "bal scan" "$status"
     scan_times+=( $(( (e - s) / 1000000 )) )
 done
 SCAN=$(printf '%s\n' "${scan_times[@]}" | median)
 
 blz_times=()
 for _ in $(seq 1 "$RUNS"); do
     s=$(date +%s%N)
-    (cd "$PKG" && for f in *.bal; do "$BLZ" "$f" >/dev/null 2>&1; done)
+    (cd "$PKG" && for f in *.bal; do timeout 300 "$BLZ" "$f" >/dev/null 2>&1 || exit $?; done)
+    status=$?
     e=$(date +%s%N)
+    check_timed "blazelint" "$status"
     blz_times+=( $(( (e - s) / 1000000 )) )
 done
 BLZ_MS=$(printf '%s\n' "${blz_times[@]}" | median)

src/ast.rs

@@ -134,9 +134,13 @@ pub enum Expr {
     },
     /// Assignment to a non-variable lvalue (field or index target), e.g.
     /// `self.count = 1` or `arr[i] = x`. Simple-variable assignment uses `Assign`.
+    ///
+    /// `op` is `None` for a plain `=` and carries the operator for a compound
+    /// assignment (`arr[i] += 1`), which reads `target = target op value`.
     MemberAssign {
         target: Box<Expr>,
         value: Box<Expr>,
+        op: Option<BinaryOp>,
         span: Span,
     },
     /// Method call expression (e.g., obj.method()).

src/linter/rules/self_assignment.rs

@@ -39,8 +39,14 @@ impl LintRule for SelfAssignmentRule {
                 Expr::Assign { name, value, .. } => {
                     matches!(value.as_ref(), Expr::Variable { name: v, .. } if v == name)
                 }
-                // `self.f = self.f`, `a[0] = a[0]`
-                Expr::MemberAssign { target, value, .. } => same_lvalue(target, value),
+                // `self.f = self.f`, `a[0] = a[0]`. A compound assignment such as
+                // `self.f += self.f` does change the value, so only plain `=` counts.
+                Expr::MemberAssign {
+                    target,
+                    value,
+                    op: None,
+                    ..
+                } => same_lvalue(target, value),
                 _ => false,
             };
             if redundant {

src/parser.rs

@@ -413,10 +413,13 @@ impl Parser {
     /// Consumes a run of module-level qualifier keywords, returning whether
     /// `public` was seen and the list of qualifier lexemes (for AST retention).
     ///
-    /// Only `public` and `isolated` are treated as leading qualifiers here.
-    /// `readonly`, `distinct`, and `transactional` are deliberately excluded
-    /// because they are primarily type-level constructors (`readonly & T`,
-    /// `distinct T`) that must reach `parse_type_descriptor` at declaration start.
+    /// `public` and `isolated` are always consumed here. `readonly` and
+    /// `distinct` are consumed only when `qualifies_a_class` confirms a class
+    /// declaration follows, so the type-level constructors (`readonly & T`,
+    /// `distinct T`) still reach `parse_type_descriptor` at declaration start.
+    /// `transactional`, `client`, and `service` are consumed as contextual
+    /// keywords, with `service` excluded when a `/` marks a service declaration
+    /// (`service /path on ...`) rather than a class qualifier.
     fn parse_leading_qualifiers(&mut self) -> ParseResult<(bool, Vec<String>)> {
         let mut is_public = false;
         let mut qualifiers = Vec::new();
@@ -1054,6 +1057,10 @@ impl Parser {
         self.advance()?; // 'lock'
         let start = self.previous_span().start;
         let body = self.braced_block()?;
+        // `lock { } on fail e { }` is legal. `Stmt::Lock` has no on-fail fields,
+        // so the clause is parsed and discarded — otherwise `on` would reach
+        // expression parsing as a stray statement.
+        let _on_fail = self.parse_on_fail()?;
         let end = self.previous_span().end;
         Ok(Stmt::Lock {
             body,
@@ -1737,14 +1744,24 @@ impl Parser {
             }
 
             // Field/index lvalues (`self.count = x`, `arr[i] += 1`) are valid
-            // assignment targets. The compound-operator distinction is not retained
-            // here (member-assignment semantics are deferred).
+            // assignment targets. The compound operator is retained on the node
+            // rather than desugared, since `Expr` is not `Clone` and the target
+            // would have to be duplicated to build the binary form.
             if matches!(expr, Expr::FieldAccess { .. } | Expr::MemberAccess { .. }) {
                 let span_start = expr.span().start.min(assign_span.start);
                 let span_end = value_span_end.max(assign_span.end);
+
+                let op = match op_token {
+                    Token::Eq => None,
+                    Token::PlusEq => Some(BinaryOp::PlusAssign),
+                    Token::MinusEq => Some(BinaryOp::MinusAssign),
+                    _ => unreachable!(),
+                };
+
                 return Ok(Expr::MemberAssign {
                     target: Box::new(expr),
                     value: Box::new(value),
+                    op,
                     span: span_start..span_end,
                 });
             }
@@ -2506,7 +2523,10 @@ impl Parser {
         let mut bindings = Vec::new();
         loop {
             self.match_token(&[Token::Final])?; // optional 'final'
-            let _ty = self.parse_type_descriptor()?;
+                                                // A binding is `var name` or `<type> name`, as in `query_let_clause`.
+            if !self.match_token(&[Token::Var])? {
+                let _ty = self.parse_type_descriptor()?;
+            }
             let name = self.expect_ident("Expected variable name in let binding")?;
             self.consume(Token::Eq, "Expected '=' in let binding", Some("'='"))?;
             let value = self.expression()?;

src/semantic.rs

@@ -1465,10 +1465,20 @@ impl Analyzer {
             }
             TypeDescriptor::Optional(inner) => self.type_from_annotation(inner),
             TypeDescriptor::Union(types) => {
-                if !types.is_empty() {
-                    self.type_from_annotation(&types[0])
-                } else {
-                    Type::Unknown("union".to_string())
+                // Picking the first member would let `int|string x = "s"` be
+                // reported as a type error. Only a union whose members all resolve
+                // to the same type carries usable information; anything wider stays
+                // Unknown so downstream checks accept every valid member.
+                let mut distinct: Vec<Type> = Vec::new();
+                for ty in types {
+                    let resolved = self.type_from_annotation(ty);
+                    if !distinct.contains(&resolved) {
+                        distinct.push(resolved);
+                    }
+                }
+                match distinct.len() {
+                    1 => distinct.remove(0),
+                    _ => Type::Unknown("union".to_string()),
                 }
             }
             // Parse-tolerant, deferred-semantics types: represented but not fully