diff --git a/README.md b/README.md index 7d5bbed..1ca0503 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ A Java Language Server that provides three things in one: 1. **Full Java language support** — completions, hover, go-to-definition, compile errors, missing imports — by proxying [Eclipse jdtls](https://github.com/eclipse-jdtls/eclipse.jdt.ls) under the hood -2. **16 functional programming rules** — catches anti-patterns and suggests Vavr/Lombok/Spring alternatives, all before compilation +2. **17 functional programming rules** — catches anti-patterns and suggests Vavr/Lombok/Spring alternatives, all before compilation 3. **Code actions (quick fixes)** — automated refactoring via LSP `textDocument/codeAction`, with machine-readable diagnostic metadata for AI agents Designed for teams using **Vavr**, **Lombok**, and **Spring** with a functional-first approach. @@ -24,7 +24,7 @@ When [jdtls](https://github.com/eclipse-jdtls/eclipse.jdt.ls) is installed, the - Type mismatches - Completions, hover, go-to-definition, find references -Install jdtls separately: `brew install jdtls` (requires JDK 21+). The server auto-detects a Java 21+ installation even when the IDE's project SDK is older (e.g., Java 8) by probing `JDTLS_JAVA_HOME`, `JAVA_HOME`, `/usr/libexec/java_home -v 21+` (macOS), and `java` on PATH. Without jdtls, the server runs in standalone mode — the 16 custom rules still work, but you won't get compile errors or completions. +Install jdtls separately: `brew install jdtls` (requires JDK 21+). The server auto-detects a Java 21+ installation even when the IDE's project SDK is older (e.g., Java 8) by probing `JDTLS_JAVA_HOME`, `JAVA_HOME`, `/usr/libexec/java_home -v 21+` (macOS), and `java` on PATH. Without jdtls, the server runs in standalone mode — the 17 custom rules still work, but you won't get compile errors or completions. ### Functional programming rules @@ -38,14 +38,15 @@ Install jdtls separately: `brew install jdtls` (requires JDK 21+). The server au | `catch-rethrow` | catch block that wraps + rethrows | `Try.of().toEither()` | — | | `mutable-variable` | Local variable reassignment | Final variables + functional transforms | — | | `imperative-loop` | `for`/`while` loops | `.map()`/`.filter()`/`.flatMap()`/`.foldLeft()` | — | -| `mutable-dto` | `@Data` or `@Setter` on class | `@Value` (immutable) | — | -| `imperative-option-unwrap` | `if (opt.isDefined()) { opt.get() }` | `map()`/`flatMap()`/`fold()` | — | +| `mutable-dto` | `@Data` or `@Setter` on class | `@Value` (immutable) | ✅ | +| `imperative-option-unwrap` | `if (opt.isDefined()) { opt.get() }` | `map()`/`flatMap()`/`fold()` | ✅ | | `field-injection` | `@Autowired` on field | Constructor injection | — | | `component-annotation` | `@Component`/`@Service`/`@Repository` | `@Configuration` + `@Bean` | — | | `frozen-mutation` | Mutation on `List.of()`/`Collections.unmodifiable*` | `io.vavr.collection.List` | ✅ | | `null-check-to-monadic` | `if (x != null) { return x.foo(); }` | `Option.of(x).map(...)` | ✅ | | `try-catch-to-monadic` | `try { return x(); } catch (E e) { return d; }` | `Try.of(() -> x()).getOrElse(d)` | ✅ | -| `impure-method` | Method mixing pure logic with side-effects | Extract pure logic; wrap IO in `Try` | — | +| `impure-method` | Method mixing pure logic with side-effects | Extract pure logic; wrap IO in `Try` / return `Either.left` instead of throwing | — | +| `option-map-nullable` | `Option.map(x -> x.get(k))` followed by chained call (`Some(null)` risk) | `.flatMap(x -> Option.of(...))` | — | ## Install @@ -129,7 +130,30 @@ Add `lspServers` to `~/.claude/settings.json` (the plugin handles this automatic claude plugin add https://github.com/aviadshiber/java-functional-lsp.git ``` -This registers the LSP server, adds auto-install hooks, a PostToolUse hook that reminds Claude to fix violations on every `.java` file edit, and the `/lint-java` command. +This registers the LSP server, adds auto-install hooks, a PostToolUse hook that lints every `.java` file after Edit/Write and feeds the violations back to Claude as context (plus a reminder hook on Read), and the `/lint-java` command. + +**Manual hook setup (without the plugin)** — add the lint hook to `~/.claude/settings.json`, pointing at a checkout of this repo: + +```json +{ + "hooks": { + "PostToolUse": [ + { + "matcher": "Edit|MultiEdit|Write", + "hooks": [ + { + "type": "command", + "command": "python3 /path/to/java-functional-lsp/hooks/post_tool_lint.py", + "timeout": 10 + } + ] + } + ] + } +} +``` + +The hook is failure-safe: it only fires on `.java` files, lints just the edited file (well under 2s), stays silent when the file is clean, and always exits 0 so a linter problem can never break the editing session. Or manually add to your Claude Code config: @@ -313,8 +337,10 @@ The server provides LSP code actions (`textDocument/codeAction`) that automatica | `null-check-to-monadic` | Convert to Option monadic flow | Rewrites `if (x != null) { return x.foo(); }` → `Option.of(x).map(...)`, supports chained fallbacks via `.orElse()`, adds import | | `null-return` | Replace with Option.none() | Rewrites `return null` → `return Option.none()`, adds import | | `try-catch-to-monadic` | Convert try/catch to Try monadic flow | Rewrites `try { return expr; } catch (E e) { return default; }` → `Try.of(() -> expr).getOrElse(default)`. Supports 3 patterns: simple default (eager/lazy `.getOrElse`), logging + default (`.onFailure().getOrElse`), and exception-dependent recovery (`.recover(E.class, ...).get()`). Skips try-with-resources, finally, multi-catch, and union types. Adds import. | +| `imperative-option-unwrap` | Convert to Option.map().getOrElse() | Rewrites `if (opt.isDefined()) return opt.get(); else return X;` → `return opt.map(it -> ...).getOrElse(X);` (lazy `getOrElse(() -> ...)` for non-eager defaults). Bails on missing else or complex bodies. | +| `mutable-dto` | Replace @Data with @Value | Replaces the `@Data` annotation with `@Value` and adds `import lombok.Value`. Skips `@Setter`, `@ConfigurationProperties`, and conflicting Lombok constructor annotations. | -Quick fixes automatically add the required Vavr import if it's not already present. Disable auto-import with `"autoImportVavr": false` in config. +Quick fixes automatically add the required Vavr import if it's not already present. Disable auto-import with `"autoImportVavr": false` in config (`"autoImportLombok": false` for the Lombok import added by the `mutable-dto` fix). ## Agent mode (AI integration) @@ -327,12 +353,14 @@ Every diagnostic includes a machine-readable `data` payload designed for AI agen "data": { "fixType": "REPLACE_WITH_VAVR_LIST", "targetLibrary": "io.vavr.collection.List", - "rationale": "Runtime mutation of List.of() causes UnsupportedOperationException. Use Vavr for safe, persistent immutability." + "rationale": "Runtime mutation of List.of() causes UnsupportedOperationException. Use Vavr for safe, persistent immutability.", + "recommendedApi": ".append / .appendAll / .update / .remove (returns a new persistent collection)", + "suggestedSnippet": "list = list.append(\"c\"); // returns a new persistent collection" } } ``` -This lets agents confidently apply fixes without guessing libraries or patterns — the `fixType` tells them *what* to do, `targetLibrary` tells them *which dependency*, and `rationale` tells them *why*. +This lets agents confidently apply fixes without guessing libraries or patterns — the `fixType` tells them *what* to do, `targetLibrary` tells them *which dependency*, and `rationale` tells them *why*. `recommendedApi` names the exact method on the target library (e.g. Vavr `Option` uses `forEach`, **not** `ifPresent`) and `suggestedSnippet` is a paste-able fix built from the real AST variable names. **Agent mode configuration** in `.java-functional-lsp.json`: @@ -346,6 +374,7 @@ This lets agents confidently apply fixes without guessing libraries or patterns | Key | Default | Effect | |-----|---------|--------| | `autoImportVavr` | `true` | Quick fixes auto-add Vavr/Option imports | +| `autoImportLombok` | `true` | The `mutable-dto` quick fix auto-adds `import lombok.Value` | | `strictPurity` | `false` | When `true`, `impure-method` uses WARNING severity instead of HINT | > **Note:** The machine-readable `data` payload is always included in diagnostics when available — no configuration needed. diff --git a/SKILL.md b/SKILL.md index 3e30c6a..4d33cc3 100644 --- a/SKILL.md +++ b/SKILL.md @@ -1,13 +1,13 @@ --- name: java-functional-lsp -description: Java LSP with full language support (completions, hover, go-to-def, compile errors) plus 16 functional programming rules with automated quick fixes. Auto-invoke when setting up Java language support or discussing Java linting configuration. +description: Java LSP with full language support (completions, hover, go-to-def, compile errors) plus 17 functional programming rules with automated quick fixes. Auto-invoke when setting up Java language support or discussing Java linting configuration. allowed-tools: Bash disable-model-invocation: true --- # Java Functional LSP -A Java LSP server that wraps jdtls and adds 16 functional programming rules with code actions (quick fixes). Gives you **full Java language support** (completions, hover, go-to-def, compile errors) **plus** custom diagnostics with machine-readable metadata for AI agents — all before compilation. +A Java LSP server that wraps jdtls and adds 17 functional programming rules with code actions (quick fixes). Gives you **full Java language support** (completions, hover, go-to-def, compile errors) **plus** custom diagnostics with machine-readable metadata for AI agents — all before compilation. ## Prerequisites @@ -22,7 +22,7 @@ brew install jdtls Without jdtls, the server runs in standalone mode — custom rules still work, but no completions/hover/compile errors. -## Rules (16 checks) +## Rules (17 checks) | Rule | Detects | Suggests | Quick Fix | |------|---------|----------|-----------| @@ -34,14 +34,15 @@ Without jdtls, the server runs in standalone mode — custom rules still work, b | `catch-rethrow` | catch wraps + rethrows | `Try.of().toEither()` | — | | `mutable-variable` | Variable reassignment | Final + functional transforms | — | | `imperative-loop` | `for`/`while` loops | `.map()`/`.filter()`/`.flatMap()` | — | -| `mutable-dto` | `@Data` or `@Setter` | `@Value` (immutable) | — | -| `imperative-option-unwrap` | `if (opt.isDefined()) { opt.get() }` | `map()`/`flatMap()`/`fold()` | — | +| `mutable-dto` | `@Data` or `@Setter` | `@Value` (immutable) | ✅ | +| `imperative-option-unwrap` | `if (opt.isDefined()) { opt.get() }` | `map()`/`flatMap()`/`fold()` | ✅ | | `field-injection` | `@Autowired` on field | Constructor injection | — | | `component-annotation` | `@Component`/`@Service`/`@Repository` | `@Configuration` + `@Bean` | — | | `frozen-mutation` | Mutation on `List.of()`/`Collections.unmodifiable*` | `io.vavr.collection.List` | ✅ | | `null-check-to-monadic` | `if (x != null) { return x.foo(); }` | `Option.of(x).map(...)` | ✅ | | `try-catch-to-monadic` | `try { return x(); } catch (E e) { return d; }` | `Try.of(() -> x()).getOrElse(d)` | ✅ | -| `impure-method` | Method mixing pure logic with side-effects | Extract pure logic; wrap IO in `Try` | — | +| `impure-method` | Method mixing pure logic with side-effects | Extract pure logic; wrap IO in `Try` / return `Either.left` instead of throwing | — | +| `option-map-nullable` | `Option.map(x -> x.get(k))` followed by chained call (`Some(null)` risk) | `.flatMap(x -> Option.of(...))` | — | ## Code Actions (Quick Fixes) @@ -51,6 +52,8 @@ Rules marked ✅ provide automated `textDocument/codeAction` fixes: - **null-check-to-monadic** → "Convert to Option monadic flow" — rewrites `if (x != null)` to `Option.of(x).map(...)`, supports chained fallbacks via `.orElse()`, adds import - **null-return** → "Replace with Option.none()" — replaces `null` with `Option.none()`, adds import - **try-catch-to-monadic** → "Convert try/catch to Try monadic flow" — rewrites `try { return expr; } catch (E e) { return default; }` to `Try.of(() -> expr).getOrElse(default)`. Supports 3 patterns: simple default, logging + default (`.onFailure().getOrElse`), and exception-dependent recovery (`.recover(E.class, ...).get()`). Skips try-with-resources, finally, multi-catch, union types. Adds import. +- **imperative-option-unwrap** → "Convert to Option.map().getOrElse()" — rewrites `if (opt.isDefined()) return opt.get(); else return X;` to `return opt.map(it -> ...).getOrElse(X);` (lazy supplier for non-eager defaults). Bails on missing else or complex bodies. +- **mutable-dto** → "Replace @Data with @Value" — swaps the annotation and adds `import lombok.Value` (disable with `"autoImportLombok": false`). Skips `@Setter`, `@ConfigurationProperties`, and conflicting Lombok constructor annotations. ## Agent-Ready Diagnostics @@ -60,11 +63,13 @@ Every diagnostic includes a machine-readable `data` payload: { "fixType": "REPLACE_WITH_VAVR_LIST", "targetLibrary": "io.vavr.collection.List", - "rationale": "Runtime mutation of List.of() causes UnsupportedOperationException." + "rationale": "Runtime mutation of List.of() causes UnsupportedOperationException.", + "recommendedApi": ".append / .appendAll / .update / .remove (returns a new persistent collection)", + "suggestedSnippet": "list = list.append(\"c\"); // returns a new persistent collection" } ``` -This lets AI agents apply fixes with confidence — `fixType` says what to do, `targetLibrary` says which dependency, `rationale` says why. +This lets AI agents apply fixes with confidence — `fixType` says what to do, `targetLibrary` says which dependency, `rationale` says why, `recommendedApi` names the exact method (e.g. Vavr `Option` uses `forEach`, not `ifPresent`), and `suggestedSnippet` is a paste-able fix built from real AST variable names. ## Configuration @@ -93,7 +98,10 @@ Create `.java-functional-lsp.json` in your project root: ## Automatic Enforcement -The plugin includes a PostToolUse hook that fires on every Read/Edit/Write of `.java` files. If diagnostics appear, Claude is prompted to fix them immediately without explanation. +The plugin includes two PostToolUse hooks: + +- **Edit/MultiEdit/Write** → `hooks/post_tool_lint.py` lints the edited `.java` file (single-file, <2s) and injects any violations into Claude's context so they get fixed immediately. Silent on clean files; internal errors are swallowed (always exits 0) so the editing session is never broken. +- **Read** → `hooks/java_linter_reminder.py` reminds Claude to act on LSP diagnostics shown for the file. ## On-Demand Linting diff --git a/editors/intellij/README.md b/editors/intellij/README.md index b8f453c..d0d0820 100644 --- a/editors/intellij/README.md +++ b/editors/intellij/README.md @@ -73,7 +73,7 @@ Project-level rules are configured via `.java-functional-lsp.json` in your proje ## Coexistence with IntelliJ's Java Support -The server automatically detects JetBrains IDEs and disables the jdtls proxy — IntelliJ provides its own Java language features (completions, hover, go-to-definition, compile errors). Only the 16 custom functional programming rules run, and they appear alongside IntelliJ's built-in inspections. +The server automatically detects JetBrains IDEs and disables the jdtls proxy — IntelliJ provides its own Java language features (completions, hover, go-to-definition, compile errors). Only the 17 custom functional programming rules run, and they appear alongside IntelliJ's built-in inspections. ## Troubleshooting diff --git a/editors/vscode/README.md b/editors/vscode/README.md index 68d2ceb..c83b4b3 100644 --- a/editors/vscode/README.md +++ b/editors/vscode/README.md @@ -57,4 +57,4 @@ This extension **coexists** with the Red Hat Java extension (`redhat.java`) and ## Rules -See the [main README](../../README.md) for the full list of 12 rules and configuration options via `.java-functional-lsp.json`. +See the [main README](../../README.md) for the full list of 17 rules and configuration options via `.java-functional-lsp.json`. diff --git a/hooks/hooks.json b/hooks/hooks.json index 3427517..0f8cf6a 100644 --- a/hooks/hooks.json +++ b/hooks/hooks.json @@ -15,13 +15,23 @@ ], "PostToolUse": [ { - "matcher": "Read|Edit|Write", + "matcher": "Read", "hooks": [ { "type": "command", "command": "python3 ${CLAUDE_PLUGIN_ROOT}/hooks/java_linter_reminder.py" } ] + }, + { + "matcher": "Edit|MultiEdit|Write", + "hooks": [ + { + "type": "command", + "command": "python3 ${CLAUDE_PLUGIN_ROOT}/hooks/post_tool_lint.py", + "timeout": 10 + } + ] } ] } diff --git a/hooks/post_tool_lint.py b/hooks/post_tool_lint.py new file mode 100755 index 0000000..ac4dc7e --- /dev/null +++ b/hooks/post_tool_lint.py @@ -0,0 +1,89 @@ +#!/usr/bin/env python3 +"""PostToolUse hook: lint a .java file after Edit/Write and surface violations to Claude. + +Reads the Claude Code PostToolUse JSON payload on stdin, runs java-functional-lsp +on the edited file, and emits diagnostics as ``hookSpecificOutput.additionalContext`` +so the agent sees them in context and can fix them immediately (issue #70). + +Failure-safe by design: every path exits 0 — a linter problem must never break the +editing session. Prefers the fast in-process import; falls back to the CLI when the +package is not importable under this interpreter. +""" + +from __future__ import annotations + +import json +import signal +import subprocess +import sys +from pathlib import Path + +TIMEOUT_SECONDS = 5 # hard cap; single-file analysis is typically <200ms +MAX_DIAGNOSTICS = 25 # keep additionalContext bounded + + +def _lint_in_process(path: Path) -> list[str] | None: + """Lint via direct import. Returns None if the package isn't importable here.""" + try: + from java_functional_lsp.analyzers.base import is_excluded + from java_functional_lsp.cli import check_file, format_diagnostic, load_config + except ImportError: + return None + config = load_config(path) + if is_excluded(path.as_posix(), config.get("excludes", [])): + return [] + return [format_diagnostic(path, d) for d in check_file(path, config)] + + +def _lint_via_cli(path: Path) -> list[str]: + """Fallback: shell out to the installed CLI (exit 1 + stdout lines on violations).""" + proc = subprocess.run( + ["java-functional-lsp", "check", str(path)], + capture_output=True, + text=True, + timeout=TIMEOUT_SECONDS, + check=False, # exit 1 just means violations were found + ) + return [ln for ln in proc.stdout.splitlines() if ln.strip()] + + +def main() -> None: + hook_input = json.load(sys.stdin) + file_path = (hook_input.get("tool_input") or {}).get("file_path", "") + if not file_path.endswith(".java"): + return # silent no-op + path = Path(file_path) + if not path.is_file(): + return # tool call may have failed or the file was deleted + + lines = _lint_in_process(path) + if lines is None: + lines = _lint_via_cli(path) + if not lines: + return # clean file: stay silent, no per-edit context noise + + if len(lines) > MAX_DIAGNOSTICS: + lines = [*lines[:MAX_DIAGNOSTICS], f"... and {len(lines) - MAX_DIAGNOSTICS} more"] + json.dump( + { + "hookSpecificOutput": { + "hookEventName": "PostToolUse", + "additionalContext": ( + "java-functional-lsp found violations in the file you just edited:\n" + + "\n".join(lines) + + "\nFix each violation now with your next Edit. Do not explain or list them." + ), + } + }, + sys.stdout, + ) + + +if __name__ == "__main__": + if hasattr(signal, "SIGALRM"): # POSIX hard runtime cap + signal.signal(signal.SIGALRM, lambda *_: sys.exit(0)) + signal.alarm(TIMEOUT_SECONDS) + try: + main() + except Exception: + sys.exit(0) # hooks must never break the session diff --git a/src/java_functional_lsp/analyzers/base.py b/src/java_functional_lsp/analyzers/base.py index 8e615b3..19ef5bb 100644 --- a/src/java_functional_lsp/analyzers/base.py +++ b/src/java_functional_lsp/analyzers/base.py @@ -3,6 +3,7 @@ from __future__ import annotations import fnmatch +import re from collections.abc import Generator from dataclasses import dataclass from enum import IntEnum @@ -179,6 +180,73 @@ def references_var(node: Node, var_name: bytes) -> bool: return False +def single_return_stmt(branch: Node | None) -> Node | None: + """Return the lone ``return_statement`` of a branch, or None. + + Accepts a block (filters comments, requires exactly one statement) or a bare + statement. Shared by analyzers and fix generators that rewrite + ``if (...) return X;`` shapes. + """ + if branch is None: + return None + if branch.type == "block": + stmts = [c for c in branch.named_children if c.type not in IGNORED_CHILDREN] + else: + stmts = [branch] + if len(stmts) != 1 or stmts[0].type != "return_statement": + return None + return stmts[0] + + +def return_expr_node(return_stmt: Node) -> Node | None: + """Return the expression node of a ``return ;`` statement, or None for a bare return.""" + children = [c for c in return_stmt.named_children if c.type not in IGNORED_CHILDREN] + if not children: + return None + return children[0] + + +def return_expr_text(return_stmt: Node) -> str | None: + """Return the expression text of a ``return ;`` statement, or None for a bare return.""" + expr = return_expr_node(return_stmt) + if expr is None or expr.text is None: + return None + decoded: str = expr.text.decode("utf-8") + return decoded + + +def single_return_expr_text(branch: Node | None) -> str | None: + """Text of the expression in a branch that is exactly ``return ;``, else None.""" + stmt = single_return_stmt(branch) + if stmt is None: + return None + return return_expr_text(stmt) + + +# Java identifiers may contain `$`, which regex `\b` treats as a non-word character: +# `\b\$opt` never matches, and `\bopt` falsely matches inside `$opt`. These lookarounds +# treat `$` as part of the identifier alphabet so both failure modes are excluded. +_IDENT_BOUNDARY_START = r"(? str | None: + """Rewrite ``var_name.get()`` calls in ``text`` to ``replacement``. + + Tolerates whitespace around the ``.`` and parens (``opt .get ()`` is valid Java). + Returns None when nothing was rewritten — callers treat that as "this shape + can't be synthesised safely" and fall back to a placeholder or bail out. + """ + pattern = re.compile(rf"{_IDENT_BOUNDARY_START}{re.escape(var_name)}\s*\.\s*get\s*\(\s*\)") + rewritten = pattern.sub(replacement, text) + return rewritten if rewritten != text else None + + +def rewrite_var_references(text: str, var_name: str, replacement: str) -> str: + """Rewrite standalone references to ``var_name`` in ``text`` to ``replacement``.""" + return re.sub(rf"{_IDENT_BOUNDARY_START}{re.escape(var_name)}{_IDENT_BOUNDARY_END}", replacement, text) + + def has_error_or_missing(node: Node) -> bool: """Return True if the subtree rooted at ``node`` contains any ERROR or MISSING nodes. diff --git a/src/java_functional_lsp/analyzers/functional_checker.py b/src/java_functional_lsp/analyzers/functional_checker.py index 1853c55..c63ef5a 100644 --- a/src/java_functional_lsp/analyzers/functional_checker.py +++ b/src/java_functional_lsp/analyzers/functional_checker.py @@ -9,13 +9,16 @@ from tree_sitter import Node, Tree from .base import ( + IGNORED_CHILDREN, Diagnostic, DiagnosticData, Severity, extract_null_check_var, find_nodes, find_nodes_multi, + rewrite_var_references, severity_from_config, + single_return_expr_text, ) _MESSAGES = { @@ -32,6 +35,11 @@ "Hidden side-effect: Method mixes pure logic with exceptions. " "Extract pure logic; return Either.left(...) or Try.failure(...) instead of throwing." ), + "option-map-nullable": ( + "Possible Some(null): Vavr Option.map() does not collapse null to None " + "(unlike java.util.Optional). The mapper may return null (e.g. Map.get), " + "so the chained call can NPE. Use .flatMap(x -> Option.of(...)) instead." + ), } _DATA = { @@ -75,9 +83,125 @@ ), recommended_api="Either.left(...) / Try.of(() -> ...)", ), + "option-map-nullable": DiagnosticData( + fix_type="USE_FLATMAP_OPTION_OF", + target_library="io.vavr.control.Option", + rationale=( + "Vavr Option.map(f) wraps a null result as Some(null) rather than None. " + "Wrap the nullable expression with Option.of inside flatMap so absence " + "propagates as None." + ), + recommended_api=".flatMap(x -> Option.of(...))", + ), } +def _build_option_map_nullable_data(lambda_node: Node) -> DiagnosticData: + """Build a flatMap snippet using the real lambda parameter and body text. + + Produces e.g. ``.flatMap(m -> Option.of(m.get("author")))`` from the offending + ``.map(m -> m.get("author"))``. The parameter text is reused verbatim, so + ``m``, ``(m)``, and typed ``(Map m)`` forms all yield valid Java. + """ + base = _DATA["option-map-nullable"] + params = lambda_node.child_by_field_name("parameters") + body = lambda_node.child_by_field_name("body") + if params is None or body is None or params.text is None or body.text is None: + return base + param_text = params.text.decode("utf-8") + body_text = body.text.decode("utf-8") + snippet = f".flatMap({param_text} -> Option.of({body_text}))" + return dataclasses.replace(base, suggested_snippet=snippet) + + +# Option factory roots that start a Vavr Option chain. Deliberately excludes +# "Optional" (java.util) — its map() collapses null to empty, so no Some(null) hazard. +_OPTION_FACTORY_NAMES = {b"of", b"ofOptional"} + +# Chained methods whose callback/predicate receives the mapped value and will NPE +# (or misbehave) on Some(null). Conservative: terminal extractors like getOrElse +# and getOrNull never dereference the value, so they are excluded. +_NULL_SENSITIVE_FOLLOWERS = {b"filter", b"map", b"flatMap", b"forEach", b"peek", b"exists", b"forAll"} + +# Bound on receiver-chain walking, mirroring _MAX_CHAIN_DEPTH in fixes.py. +_MAX_OPTION_CHAIN_DEPTH = 10 + +# Integer-literal argument types: x.get(0) on a java.util.List throws rather than +# returning null, so index access is not a Some(null) hazard. All four Java integer +# literal forms — tree-sitter emits a distinct node type per radix. +_INTEGER_LITERAL_TYPES = ( + "decimal_integer_literal", + "hex_integer_literal", + "octal_integer_literal", + "binary_integer_literal", +) + + +def _single_lambda_arg(invocation: Node) -> Node | None: + """Return the lone lambda_expression argument of a method_invocation, or None. + + Method references like ``.map(Map::get)`` are deliberately skipped: there is no + parameter name to build a snippet from, and they are rare in this position. + """ + args = invocation.child_by_field_name("arguments") + if args is None: + return None + named = [c for c in args.named_children if c.type not in IGNORED_CHILDREN] + if len(named) != 1 or named[0].type != "lambda_expression": + return None + return named[0] + + +def _is_nullable_lambda_body(lambda_node: Node) -> bool: + """Conservative nullability heuristic for a .map() lambda body. + + Matches exactly ``recv.get(arg, ...)`` with at least one non-integer argument — + the Map.get(key) / JsonNode.get(name) shape from issue #69. Zero-arg ``.get()`` + (Vavr Option.get / Supplier.get) and integer-index ``List.get(0)`` are excluded. + Broader heuristics (getters without @NonNull, methods lacking @Nonnull) are + future work; this shape covers the real-world NPE incidents with no false + positives on non-nullable lambdas. + """ + body = lambda_node.child_by_field_name("body") + if body is None or body.type != "method_invocation": + return False + name = body.child_by_field_name("name") + obj = body.child_by_field_name("object") + if name is None or name.text != b"get" or obj is None: + return False + args = body.child_by_field_name("arguments") + if args is None: + return False + named = [c for c in args.named_children if c.type not in IGNORED_CHILDREN] + if not named: + return False + return not all(a.type in _INTEGER_LITERAL_TYPES for a in named) + + +def _chain_rooted_in_option(node: Node | None) -> bool: + """Walk a receiver chain looking for Option.of(...) / Option.ofOptional(...) at the root. + + Bare-variable receivers return False: tree-sitter has no type info, so a variable + typed Option cannot be distinguished from java.util.Optional — staying quiet + keeps the rule free of false positives. + """ + depth = 0 + while node is not None and depth < _MAX_OPTION_CHAIN_DEPTH: + if node.type != "method_invocation": + return False + obj = node.child_by_field_name("object") + name = node.child_by_field_name("name") + if obj is not None and name is not None and name.text in _OPTION_FACTORY_NAMES: + # `Option.of(...)` (identifier) or `io.vavr.control.Option.of(...)` (field_access) + if obj.type == "identifier" and obj.text == b"Option": + return True + if obj.type == "field_access" and obj.text is not None and obj.text.endswith(b".Option"): + return True + node = obj + depth += 1 + return False + + def _build_null_check_to_monadic_data( var_name: bytes, consequence: Node | None, alternative: Node | None, if_node: Node | None = None ) -> DiagnosticData: @@ -94,21 +218,21 @@ def _build_null_check_to_monadic_data( return base var = var_name.decode("utf-8") - then_expr = _single_return_expr_text(consequence) + then_expr = single_return_expr_text(consequence) if then_expr is None: return base - # Rewrite references to the checked variable as `it` in the lambda body. Use a word-boundary - # regex so a short var name like `s` doesn't match `s.toString()` inside another identifier. - # Skip the map when the body is exactly the variable itself (identity case). + # Rewrite references to the checked variable as `it` in the lambda body ($-aware + # identifier boundaries, so a short var name like `s` doesn't match inside `$s` or + # `safe`). Skip the map when the body is exactly the variable itself (identity case). if then_expr == var: chain_body = f"Option.of({var})" else: - lambda_body = re.sub(rf"\b{re.escape(var)}\b", "it", then_expr) + lambda_body = rewrite_var_references(then_expr, var, "it") chain_body = f"Option.of({var}).map(it -> {lambda_body})" # Prefer the nested else-branch; otherwise look at the statement immediately following the if # (a common fallthrough pattern: `if (x != null) return ...; return fallback;`). - else_expr = _single_return_expr_text(alternative) + else_expr = single_return_expr_text(alternative) if else_expr is None and if_node is not None: else_expr = _next_statement_return_expr(if_node) @@ -135,25 +259,7 @@ def _next_statement_return_expr(if_node: Node) -> str | None: if idx + 1 >= len(siblings): return None next_stmt = siblings[idx + 1] - return _single_return_expr_text(next_stmt) - - -def _single_return_expr_text(block_or_stmt: Node | None) -> str | None: - """Return the text of a single ``return ;`` statement in a block (or the statement - itself). Returns None for anything else (multiple statements, no return, bare return).""" - if block_or_stmt is None: - return None - if block_or_stmt.type == "block": - stmts = [c for c in block_or_stmt.named_children if c.type not in ("line_comment", "block_comment")] - else: - stmts = [block_or_stmt] - if len(stmts) != 1 or stmts[0].type != "return_statement": - return None - ret_children = [c for c in stmts[0].named_children if c.type not in ("line_comment", "block_comment")] - if not ret_children or not ret_children[0].text: - return None - decoded: str = ret_children[0].text.decode("utf-8") - return decoded + return single_return_expr_text(next_stmt) # Module-scope so it's allocated once at import rather than per diagnostic. @@ -306,6 +412,7 @@ def analyze(self, tree: Tree, source: bytes, config: dict[str, Any]) -> list[Dia self._check_frozen_mutation(tree, diagnostics, config) self._check_null_check_to_monadic(tree, diagnostics, config) + self._check_option_map_nullable(tree, diagnostics, config) self._check_impure_method(tree, diagnostics, config) return diagnostics @@ -464,6 +571,62 @@ def _references_var(self, node: Node, var_name: bytes) -> bool: break return False + def _check_option_map_nullable(self, tree: Tree, diagnostics: list[Diagnostic], config: dict[str, Any]) -> None: + """Detect Vavr Option chains where .map() can produce Some(null) before a chained call. + + Unlike java.util.Optional, Vavr's Option.map() wraps a null mapper result as + Some(null); a following .filter()/.map()/etc. then NPEs on the value (issue #69). + Detection requires all three gates (ordered cheapest-first, measured): + 1. the .map(...) has a value-consuming follower chained after it, + 2. the receiver chain is rooted in a literal Option.of()/Option.ofOptional(), + 3. the map argument is a single-expression lambda matching a known + possibly-null shape (``x.get(key)`` — Map.get / JsonNode.get). + """ + severity = severity_from_config(config, "option-map-nullable") + if severity is None: + return + + for invocation in find_nodes(tree.root_node, "method_invocation"): + name_node = invocation.child_by_field_name("name") + if name_node is None or name_node.text != b"map": + continue + + # Gate 1: something chained after .map(...) consumes the mapped value. + parent = invocation.parent + if parent is None or parent.type != "method_invocation": + continue + if parent.child_by_field_name("object") != invocation: + continue + follower = parent.child_by_field_name("name") + if follower is None or follower.text not in _NULL_SENSITIVE_FOLLOWERS: + continue + + # Gate 2: receiver chain rooted in Option.of(...) / Option.ofOptional(...). + # Measured cheaper than the lambda-shape gate (~3x on fluent-heavy code): + # most chains terminate the walk after one step at a bare identifier. + if not _chain_rooted_in_option(invocation.child_by_field_name("object")): + continue + + # Gate 3: the lambda body is a possibly-null expression. + lambda_node = _single_lambda_arg(invocation) + if lambda_node is None or not _is_nullable_lambda_body(lambda_node): + continue + + # Range = from the `map` identifier to the end of .map(...), not the whole + # chain — keeps the squiggle on the offending call, not Option.of(...). + diagnostics.append( + Diagnostic( + line=name_node.start_point[0], + col=name_node.start_point[1], + end_line=invocation.end_point[0], + end_col=invocation.end_point[1], + severity=severity, + code="option-map-nullable", + message=_MESSAGES["option-map-nullable"], + data=_build_option_map_nullable_data(lambda_node), + ) + ) + def _check_impure_method(self, tree: Tree, diagnostics: list[Diagnostic], config: dict[str, Any]) -> None: """Detect methods mixing pure logic with side-effects. diff --git a/src/java_functional_lsp/analyzers/mutation_checker.py b/src/java_functional_lsp/analyzers/mutation_checker.py index 79b2fa8..7ebaf1f 100644 --- a/src/java_functional_lsp/analyzers/mutation_checker.py +++ b/src/java_functional_lsp/analyzers/mutation_checker.py @@ -12,7 +12,11 @@ find_nodes_multi, has_ancestor, has_sibling_annotation, + return_expr_text, + rewrite_var_get_call, severity_from_config, + single_return_expr_text, + single_return_stmt, ) _MESSAGES = { @@ -78,33 +82,25 @@ def _build_imperative_option_unwrap_data(obj_name: bytes, consequence: Any, else base = _DATA["imperative-option-unwrap"] var = obj_name.decode("utf-8") if obj_name else "opt" - # Detect whether the consequence is a `return opt.get();` shape — then map/getOrElse fits. - # Otherwise (statement-style consumer), forEach is the right hint. - ignored = ("line_comment", "block_comment") - is_return_shape = False - if consequence is not None: - if consequence.type == "block": - stmts = [c for c in consequence.named_children if c.type not in ignored] - else: - stmts = [consequence] - if len(stmts) == 1 and stmts[0].type == "return_statement": - is_return_shape = True - - if is_return_shape: - default_text = "default" - if else_branch is not None: - if else_branch.type == "block": - else_stmts = [c for c in else_branch.named_children if c.type not in ignored] - else: - else_stmts = [else_branch] - if len(else_stmts) == 1 and else_stmts[0].type == "return_statement": - ret_children = [c for c in else_stmts[0].named_children if c.type not in ignored] - if ret_children and ret_children[0].text: - default_text = ret_children[0].text.decode("utf-8") - snippet = f"return {var}.map(value -> value).getOrElse({default_text});" - else: - snippet = f"{var}.forEach(value -> {{ /* use value */ }});" - + # `return opt.get();` shape — map/getOrElse fits. Otherwise (statement-style + # consumer), forEach is the right hint. + then_return = single_return_stmt(consequence) + if then_return is None: + return dataclasses.replace(base, suggested_snippet=f"{var}.forEach(value -> {{ /* use value */ }});") + + default_text = single_return_expr_text(else_branch) or "default" + + # Derive the lambda body from the real then-branch expression (issue #74 #2): + # `return opt.get().toUpperCase();` becomes `.map(value -> value.toUpperCase())` — + # the same rewrite fixes.fix_imperative_option_unwrap applies, via the shared + # base helper. Keep the identity placeholder when no `var.get()` was found to + # rewrite — the shape isn't one we can synthesise safely. + lambda_body = "value" + ret_text = return_expr_text(then_return) + if ret_text is not None and ret_text != f"{var}.get()": + lambda_body = rewrite_var_get_call(ret_text, var, "value") or lambda_body + + snippet = f"return {var}.map(value -> {lambda_body}).getOrElse({default_text});" return dataclasses.replace(base, suggested_snippet=snippet) diff --git a/src/java_functional_lsp/fixes.py b/src/java_functional_lsp/fixes.py index 79068b8..d08889b 100644 --- a/src/java_functional_lsp/fixes.py +++ b/src/java_functional_lsp/fixes.py @@ -21,6 +21,9 @@ get_parser, has_error_or_missing, references_var, + return_expr_node, + rewrite_var_get_call, + single_return_stmt, ) from .analyzers.functional_checker import is_side_effect_invocation @@ -1011,46 +1014,37 @@ def fix_imperative_option_unwrap( var = var_name.decode("utf-8") # Consequence must be a block with exactly: `return .get();` (or similar shape). - cons_stmts = ( - [c for c in consequence.named_children if c.type not in IGNORED_CHILDREN] - if consequence.type == "block" - else [consequence] - ) - if len(cons_stmts) != 1 or cons_stmts[0].type != "return_statement": + cons_return = single_return_stmt(consequence) + if cons_return is None: return None - ret_children = [c for c in cons_stmts[0].named_children if c.type not in IGNORED_CHILDREN] - if not ret_children: + ret_expr = return_expr_node(cons_return) + if ret_expr is None: return None - ret_expr = ret_children[0] # Map the returned expression to a lambda body. If it's literally `var.get()`, the lambda - # is the identity `it -> it`; otherwise rewrite occurrences of `var` to `it`. + # is the identity `it -> it`; otherwise rewrite occurrences of `var.get()` to `it`. ret_text = ret_expr.text.decode("utf-8") if ret_expr.text else var if ret_text == f"{var}.get()": lambda_body = "it" else: - pattern = re.compile(rf"\b{re.escape(var)}\.get\(\)") - lambda_body = pattern.sub("it", ret_text) + rewritten = rewrite_var_get_call(ret_text, var, "it") # If the rewrite did nothing (no .get() call to replace), bail — shape isn't safe. - if lambda_body == ret_text: + if rewritten is None: return None + lambda_body = rewritten # Alternative (else) must be a single return statement. Bail when absent — without an # else-return, `return opt.map(it -> it);` returns Option rather than T, breaking the # method's return type. if alternative is None: return None - alt_stmts = ( - [c for c in alternative.named_children if c.type not in IGNORED_CHILDREN] - if alternative.type == "block" - else [alternative] - ) - if len(alt_stmts) != 1 or alt_stmts[0].type != "return_statement": + alt_return = single_return_stmt(alternative) + if alt_return is None: return None - alt_ret = [c for c in alt_stmts[0].named_children if c.type not in IGNORED_CHILDREN] - if not alt_ret: + alt_expr = return_expr_node(alt_return) + if alt_expr is None: return None - alt_text = alt_ret[0].text.decode("utf-8") if alt_ret[0].text else "null" - if _is_eager(alt_ret[0]): + alt_text = alt_expr.text.decode("utf-8") if alt_expr.text else "null" + if _is_eager(alt_expr): or_else = f".getOrElse({alt_text})" else: or_else = f".getOrElse(() -> {alt_text})" diff --git a/tests/test_base.py b/tests/test_base.py index 63a76a7..9c3e84f 100644 --- a/tests/test_base.py +++ b/tests/test_base.py @@ -9,6 +9,11 @@ has_ancestor, has_sibling_annotation, is_excluded, + return_expr_text, + rewrite_var_get_call, + rewrite_var_references, + single_return_expr_text, + single_return_stmt, ) @@ -155,3 +160,61 @@ def test_no_sibling(self): name = node.child_by_field_name("name") if name and name.text == b"Setter" and node.parent: assert not has_sibling_annotation(node.parent, b"ConfigurationProperties") + + +class TestReturnExtractionHelpers: + """Shared single-return extraction helpers (consolidated from analyzers/fixes).""" + + def _if_branches(self, body: str): + tree = _parse(f"class T {{ String f(Option opt) {{ {body} }} }}") + if_node = next(find_nodes(tree.root_node, "if_statement")) + return if_node.child_by_field_name("consequence"), if_node.child_by_field_name("alternative") + + def test_single_return_stmt_block(self): + cons, _ = self._if_branches('if (opt.isDefined()) { return opt.get(); } else { return "x"; }') + stmt = single_return_stmt(cons) + assert stmt is not None + assert stmt.type == "return_statement" + + def test_single_return_stmt_rejects_multi_statement_block(self): + cons, _ = self._if_branches('if (opt.isDefined()) { log(); return opt.get(); } else { return "x"; }') + assert single_return_stmt(cons) is None + + def test_single_return_stmt_none_branch(self): + assert single_return_stmt(None) is None + + def test_return_expr_text_bare_return_is_none(self): + cons, _ = self._if_branches('if (opt.isDefined()) { return; } else { return "x"; }') + stmt = single_return_stmt(cons) + assert stmt is not None + assert return_expr_text(stmt) is None + + def test_single_return_expr_text_extracts_expression(self): + cons, alt = self._if_branches('if (opt.isDefined()) { return opt.get().trim(); } else { return "x"; }') + assert single_return_expr_text(cons) == "opt.get().trim()" + assert single_return_expr_text(alt) == '"x"' + + +class TestIdentifierRewriteHelpers: + def test_rewrite_var_get_call_basic(self): + assert rewrite_var_get_call("myOpt.get().trim()", "myOpt", "value") == "value.trim()" + + def test_rewrite_var_get_call_no_match_returns_none(self): + assert rewrite_var_get_call('"constant"', "myOpt", "value") is None + + def test_rewrite_var_get_call_tolerates_whitespace(self): + """`opt .get ()` is valid Java; the rewrite must not silently miss it.""" + assert rewrite_var_get_call("myOpt .get() .trim()", "myOpt", "value") == "value .trim()" + + def test_rewrite_var_get_call_dollar_identifier(self): + """Regression: regex \\b never matches before $-prefixed Java identifiers.""" + assert rewrite_var_get_call("$opt.get().trim()", "$opt", "value") == "value.trim()" + + def test_rewrite_var_get_call_does_not_match_inside_longer_identifier(self): + assert rewrite_var_get_call("myOpt2.get()", "myOpt", "value") is None + assert rewrite_var_get_call("$myOpt.get()", "myOpt", "value") is None + + def test_rewrite_var_references_dollar_adjacent(self): + # `s` must not be rewritten inside the distinct identifier `$s` or `s$x`. + assert rewrite_var_references("$s.concat(s)", "s", "it") == "$s.concat(it)" + assert rewrite_var_references("s$x + s", "s", "it") == "s$x + it" diff --git a/tests/test_functional_checker.py b/tests/test_functional_checker.py index 420c065..d91aef9 100644 --- a/tests/test_functional_checker.py +++ b/tests/test_functional_checker.py @@ -664,3 +664,175 @@ class T { f"Diagnostic should point at the side-effect line, not the method declaration " f"(diag_line={diag_line}, method_decl_line={method_decl_line})" ) + + +class TestOptionMapNullable: + """Issue #69: Option.map() producing Some(null) before a chained value-consuming call.""" + + def test_detects_map_get_followed_by_filter(self) -> None: + source = b""" + class T { + Option author(Map metadata) { + return Option.of(metadata) + .map(m -> m.get("author")) + .filter(s -> !s.trim().isEmpty()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + codes = [d.code for d in diags] + assert "option-map-nullable" in codes + + def test_detects_map_get_followed_by_map(self) -> None: + source = b""" + class T { + Option f(Map m0) { + return Option.of(m0).map(m -> m.get("k")).map(s -> s.trim()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" in [d.code for d in diags] + + def test_detects_qualified_option_root(self) -> None: + source = b""" + class T { + void f(Map m0) { + io.vavr.control.Option.of(m0).map(m -> m.get("k")).forEach(s -> use(s)); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" in [d.code for d in diags] + + def test_data_payload_has_flatmap_snippet_with_real_names(self) -> None: + source = b""" + class T { + Option f(Map metadata) { + return Option.of(metadata) + .map(m -> m.get("author")) + .filter(s -> !s.isEmpty()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + diag = next(d for d in diags if d.code == "option-map-nullable") + assert diag.data is not None + assert diag.data.fix_type == "USE_FLATMAP_OPTION_OF" + assert diag.data.target_library == "io.vavr.control.Option" + assert diag.data.suggested_snippet == '.flatMap(m -> Option.of(m.get("author")))' + + def test_range_starts_at_map_not_chain_root(self) -> None: + source = b""" + class T { + Option f(Map m0) { + return Option.of(m0).map(m -> m.get("k")).filter(s -> !s.isEmpty()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + diag = next(d for d in diags if d.code == "option-map-nullable") + line = source.split(b"\n")[diag.line] + assert line[diag.col :].startswith(b"map("), "range should start at the `map` token" + + def test_no_warn_flatmap_version(self) -> None: + source = b""" + class T { + Option f(Map metadata) { + return Option.of(metadata) + .flatMap(m -> Option.of(m.get("author"))) + .filter(s -> !s.isEmpty()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" not in [d.code for d in diags] + + def test_no_warn_non_nullable_lambda(self) -> None: + source = b""" + class T { + Option f(String s0) { + return Option.of(s0).map(s -> s + "!").filter(s -> !s.isEmpty()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" not in [d.code for d in diags] + + def test_no_warn_java_util_optional(self) -> None: + source = b""" + class T { + Optional f(Map m0) { + return Optional.ofNullable(m0).map(m -> m.get("k")).filter(s -> !s.isEmpty()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" not in [d.code for d in diags] + + def test_no_warn_terminal_map(self) -> None: + source = b""" + class T { + Option f(Map m0) { + return Option.of(m0).map(m -> m.get("k")); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" not in [d.code for d in diags] + + def test_no_warn_zero_arg_get(self) -> None: + source = b""" + class T { + Option f(Supplier s0) { + return Option.of(s0).map(s -> s.get()).filter(v -> !v.isEmpty()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" not in [d.code for d in diags] + + def test_no_warn_list_index_get(self) -> None: + source = b""" + class T { + Option f(List xs0) { + return Option.of(xs0).map(xs -> xs.get(0)).filter(v -> !v.isEmpty()); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" not in [d.code for d in diags] + + def test_no_warn_non_decimal_index_get(self) -> None: + """All four Java integer-literal radixes are index access, not nullable keys.""" + for literal in (b"0x1F", b"010", b"0b101"): + source = ( + b"class T { Option f(List xs0) {" + b" return Option.of(xs0).map(xs -> xs.get(" + literal + b")).filter(v -> !v.isEmpty()); } }" + ) + diags = parse_and_analyze(FunctionalChecker(), source) + codes = [d.code for d in diags] + assert "option-map-nullable" not in codes, f"false positive on index literal {literal!r}" + + def test_no_warn_get_or_else_follower(self) -> None: + source = b""" + class T { + String f(Map m0) { + return Option.of(m0).map(m -> m.get("k")).getOrElse("x"); + } + } + """ + diags = parse_and_analyze(FunctionalChecker(), source) + assert "option-map-nullable" not in [d.code for d in diags] + + def test_rule_off_in_config(self) -> None: + source = b""" + class T { + Option f(Map m0) { + return Option.of(m0).map(m -> m.get("k")).filter(s -> !s.isEmpty()); + } + } + """ + config = {"rules": {"option-map-nullable": "off"}} + diags = parse_and_analyze(FunctionalChecker(), source, config) + assert "option-map-nullable" not in [d.code for d in diags] diff --git a/tests/test_hooks.py b/tests/test_hooks.py new file mode 100644 index 0000000..5ca3e97 --- /dev/null +++ b/tests/test_hooks.py @@ -0,0 +1,102 @@ +"""Tests for the PostToolUse lint hook (hooks/post_tool_lint.py, issue #70). + +The hook is exercised as a subprocess — faithful to how Claude Code invokes it — +piping a PostToolUse JSON payload to stdin and asserting on stdout/exit code. +Every case must exit 0: the hook is failure-safe by contract. +""" + +from __future__ import annotations + +import json +import subprocess +import sys +from pathlib import Path +from typing import Any + +HOOK = Path(__file__).parent.parent / "hooks" / "post_tool_lint.py" + + +def run_hook(payload: Any) -> subprocess.CompletedProcess[str]: + raw = payload if isinstance(payload, str) else json.dumps(payload) + return subprocess.run( + [sys.executable, str(HOOK)], + input=raw, + capture_output=True, + text=True, + timeout=30, + check=False, # the hook's exit code is itself under test + ) + + +def test_java_file_with_violation_emits_diagnostics(tmp_path: Path) -> None: + f = tmp_path / "Test.java" + f.write_text("class T { String f() { return null; } }") + proc = run_hook( + { + "tool_name": "Edit", + "tool_input": {"file_path": str(f), "old_string": "x", "new_string": "y"}, + "tool_response": {}, + } + ) + assert proc.returncode == 0 + out = json.loads(proc.stdout) + assert out["hookSpecificOutput"]["hookEventName"] == "PostToolUse" + ctx = out["hookSpecificOutput"]["additionalContext"] + assert "null-return" in ctx + assert str(f) in ctx + + +def test_non_java_file_is_silent_noop(tmp_path: Path) -> None: + f = tmp_path / "notes.md" + f.write_text("hello") + proc = run_hook({"tool_name": "Write", "tool_input": {"file_path": str(f)}}) + assert proc.returncode == 0 + assert proc.stdout == "" + + +def test_clean_java_file_is_silent(tmp_path: Path) -> None: + f = tmp_path / "Clean.java" + f.write_text("final class Clean { static int add(int a, int b) { return a + b; } }") + proc = run_hook({"tool_name": "Edit", "tool_input": {"file_path": str(f)}}) + assert proc.returncode == 0 + assert proc.stdout == "" + + +def test_missing_file_path_key_is_silent() -> None: + proc = run_hook({"tool_name": "Edit", "tool_input": {}}) + assert proc.returncode == 0 + assert proc.stdout == "" + + +def test_malformed_stdin_exits_zero() -> None: + proc = run_hook("not json") + assert proc.returncode == 0 + assert proc.stdout == "" + + +def test_nonexistent_file_exits_zero(tmp_path: Path) -> None: + proc = run_hook({"tool_name": "Edit", "tool_input": {"file_path": str(tmp_path / "Gone.java")}}) + assert proc.returncode == 0 + assert proc.stdout == "" + + +def test_excluded_file_is_silent(tmp_path: Path) -> None: + (tmp_path / ".java-functional-lsp.json").write_text(json.dumps({"excludes": ["**/generated/**"]})) + gen = tmp_path / "generated" + gen.mkdir() + f = gen / "Gen.java" + f.write_text("class T { String f() { return null; } }") + proc = run_hook({"tool_name": "Write", "tool_input": {"file_path": str(f)}}) + assert proc.returncode == 0 + assert proc.stdout == "" + + +def test_diagnostics_are_capped(tmp_path: Path) -> None: + methods = "\n".join(f"String f{i}() {{ return null; }}" for i in range(30)) + f = tmp_path / "Many.java" + f.write_text(f"class T {{\n{methods}\n}}") + proc = run_hook({"tool_name": "Edit", "tool_input": {"file_path": str(f)}}) + assert proc.returncode == 0 + ctx = json.loads(proc.stdout)["hookSpecificOutput"]["additionalContext"] + assert "... and" in ctx + assert ctx.count("null-return") <= 25 diff --git a/tests/test_mutation_checker.py b/tests/test_mutation_checker.py index 04c0587..5f7b34a 100644 --- a/tests/test_mutation_checker.py +++ b/tests/test_mutation_checker.py @@ -186,6 +186,71 @@ class T { assert "myOpt" in snippet # real variable name from AST assert '"fallback"' in snippet # real default value from AST + def test_imperative_option_unwrap_snippet_uses_real_mapped_expression(self) -> None: + """Issue #74 #2: the then-branch expression appears in the lambda body, not a + `value -> value` placeholder — `myOpt.get().toUpperCase()` becomes + `.map(value -> value.toUpperCase())`.""" + source = b""" + class T { + String f(Option myOpt) { + if (myOpt.isDefined()) { + return myOpt.get().toUpperCase(); + } else { + return "x"; + } + } + } + """ + diags = parse_and_analyze(MutationChecker(), source) + unwrap = next(d for d in diags if d.code == "imperative-option-unwrap") + assert unwrap.data is not None + snippet = unwrap.data.suggested_snippet + assert snippet is not None + assert "value -> value.toUpperCase()" in snippet + assert "value -> value)" not in snippet + + def test_imperative_option_unwrap_snippet_handles_dollar_identifier(self) -> None: + """Regression: regex \\b never matches before $-prefixed Java identifiers, which + silently dropped the real transformation from the snippet.""" + source = b""" + class T { + String f(Option $opt) { + if ($opt.isDefined()) { + return $opt.get().trim(); + } else { + return "x"; + } + } + } + """ + diags = parse_and_analyze(MutationChecker(), source) + unwrap = next(d for d in diags if d.code == "imperative-option-unwrap") + assert unwrap.data is not None + snippet = unwrap.data.suggested_snippet + assert snippet is not None + assert "value -> value.trim()" in snippet + + def test_imperative_option_unwrap_snippet_keeps_placeholder_when_get_absent(self) -> None: + """When the then-branch return doesn't contain `var.get()`, the rewrite can't be + synthesised safely — keep the identity placeholder rather than guessing.""" + source = b""" + class T { + String f(Option myOpt) { + if (myOpt.isDefined()) { + return "found"; + } else { + return "missing"; + } + } + } + """ + diags = parse_and_analyze(MutationChecker(), source) + unwraps = [d for d in diags if d.code == "imperative-option-unwrap"] + if unwraps: # rule may legitimately not fire when the body never touches the Option + assert unwraps[0].data is not None + snippet = unwraps[0].data.suggested_snippet + assert snippet is None or "value -> value)" in snippet + def test_mutable_dto_has_recommended_api(self) -> None: """Issue #74 #1: mutable-dto carries the @Value recommendation.""" source = b"@Data class Foo { private String name; }" diff --git a/uv.lock b/uv.lock index 7e9281f..8b6939b 100644 --- a/uv.lock +++ b/uv.lock @@ -184,7 +184,7 @@ wheels = [ [[package]] name = "java-functional-lsp" -version = "0.10.0" +version = "0.11.1" source = { editable = "." } dependencies = [ { name = "pygls" },