⏺ Phase 10 is complete. Here's the summary: #13

Merged

navicore merged 2 commits from phase10 into main 2026-02-10 06:38:58 +00:00

Conversation 2 Commits 2 Files changed 28 +2693 -50

navicore commented 2026-02-10 06:09:01 +00:00 (Migrated from github.com)

Copy link

Phase 10: Language Server (LSP) — Complete

What was done

Project restructuring:

• Created src/lib.rs as library root re-exporting all modules
• Updated src/main.rs to use library imports instead of mod declarations
• Updated Cargo.toml with [features] lsp, optional deps (tower-lsp, tokio, serde_json), and bin targets

14 new files created:
┌───────────────────────────┬──────────────────────────────────────────────────────┐
│ File │ Purpose │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lib.rs │ Library root with feature-gated pub mod lsp │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/bin/patch-rexx-lsp.rs │ LSP binary entry point │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/mod.rs │ LSP module root │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/server.rs │ LanguageServer trait impl with tower-lsp wiring │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/analysis.rs │ Static document analysis (lex + parse + AST walk) │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/diagnostics.rs │ RexxDiagnostic → LSP Diagnostic mapping │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/completion.rs │ Keywords, BIFs, labels, variables, stems │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/hover.rs │ BIF docs, subroutine info, keyword docs │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/definition.rs │ Go-to label/subroutine definition │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/symbols.rs │ Document outline (labels, subroutines) │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/actions.rs │ Typo suggestions with edit distance │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/hints.rs │ PARSE template and NUMERIC inlay hints │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ src/lsp/bif_docs.rs │ Static BIF signature + description data (48 entries) │
├───────────────────────────┼──────────────────────────────────────────────────────┤
│ tests/phase10.rs │ Feature-gated integration tests (24 tests) │
└───────────────────────────┴──────────────────────────────────────────────────────┘
Verification

• cargo test — 411 tests pass (no lsp feature, all existing phases)
• cargo test --features lsp — 435 tests pass (411 existing + 24 new phase10)
• cargo clippy --all-targets -- -D warnings — clean
• cargo clippy --all-targets --features lsp -- -D warnings — clean
• cargo run --bin patch-rexx -- -e "say 'hello'" — interpreter works
• cargo build --features lsp --bin patch-rexx-lsp — LSP binary builds

Phase 10: Language Server (LSP) — Complete What was done Project restructuring: - Created src/lib.rs as library root re-exporting all modules - Updated src/main.rs to use library imports instead of mod declarations - Updated Cargo.toml with [features] lsp, optional deps (tower-lsp, tokio, serde_json), and [[bin]] targets 14 new files created: ┌───────────────────────────┬──────────────────────────────────────────────────────┐ │ File │ Purpose │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lib.rs │ Library root with feature-gated pub mod lsp │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/bin/patch-rexx-lsp.rs │ LSP binary entry point │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/mod.rs │ LSP module root │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/server.rs │ LanguageServer trait impl with tower-lsp wiring │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/analysis.rs │ Static document analysis (lex + parse + AST walk) │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/diagnostics.rs │ RexxDiagnostic → LSP Diagnostic mapping │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/completion.rs │ Keywords, BIFs, labels, variables, stems │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/hover.rs │ BIF docs, subroutine info, keyword docs │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/definition.rs │ Go-to label/subroutine definition │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/symbols.rs │ Document outline (labels, subroutines) │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/actions.rs │ Typo suggestions with edit distance │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/hints.rs │ PARSE template and NUMERIC inlay hints │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ src/lsp/bif_docs.rs │ Static BIF signature + description data (48 entries) │ ├───────────────────────────┼──────────────────────────────────────────────────────┤ │ tests/phase10.rs │ Feature-gated integration tests (24 tests) │ └───────────────────────────┴──────────────────────────────────────────────────────┘ Verification - cargo test — 411 tests pass (no lsp feature, all existing phases) - cargo test --features lsp — 435 tests pass (411 existing + 24 new phase10) - cargo clippy --all-targets -- -D warnings — clean - cargo clippy --all-targets --features lsp -- -D warnings — clean - cargo run --bin patch-rexx -- -e "say 'hello'" — interpreter works - cargo build --features lsp --bin patch-rexx-lsp — LSP binary builds

claude[bot] commented 2026-02-10 06:12:31 +00:00 (Migrated from github.com)

Copy link

PR #13 Review: Language Server (LSP) Implementation

Summary

This PR implements a complete LSP for patch-rexx with 14 new files, feature-gated compilation, and comprehensive test coverage (24 new tests). All 435 tests pass.

Critical Issues

1. Missing SourceLoc in AST Walker (src/lsp/analysis.rs)

Severity: Medium — Affects accuracy of LSP features

Lines: 230, 256, 262, 269

The AstCollector::walk_* methods create VariableInfo and CallInfo with placeholder SourceLoc::new(0, 0) instead of actual source locations:

// analysis.rs:230
self.assignments.push(VariableInfo {
    name: ctrl.var.clone(),
    loc: SourceLoc::new(0, 0),  // ❌ Wrong location
});

// analysis.rs:256
self.references.push(VariableInfo {
    name: name.clone(),
    loc: SourceLoc::new(0, 0),  // ❌ Wrong location
});

Impact:

• Go-to-definition, hover, and code actions won't work correctly for variable references and function calls inside expressions
• Only top-level assignments from clause locations are accurate

Fix: The AST nodes (Expr, DoKind::Controlled, etc.) need to carry their own SourceLoc. This is a deeper architectural issue — expressions currently don't have location info.

Recommendation: Either:

1. Add loc: SourceLoc to all Expr variants (breaking change, correct solution)
2. Document this limitation and defer to a future phase
3. Track byte offsets during parsing and back-compute locations (fragile)

---

2. Case-Sensitivity in is_rexx_word_char (src/lsp/hover.rs:174, definition.rs:61)

Severity: Low — Minor deviation from REXX semantics

Lines: hover.rs:174-176, definition.rs:61-63

fn is_rexx_word_char(ch: char) -> bool {
    ch.is_alphanumeric() || ch == '_' || ch == '.' || ch == '\!' || ch == '?' || ch == '@'
}

Issue: REXX variable names can contain \!, ?, @ per ANSI X3.274-1996 §2.2.3, so this is correct. However, including . means word-at-position will match ARR.1 as a single word instead of stopping at ARR.

Impact:

• Hovering over ARR.1 won't match stem variable ARR. correctly
• Go-to-definition for compound variables may fail

Fix: Exclude . from word characters for LSP purposes, or special-case stem lookups.

---

3. Potential Off-by-One in End Position (src/lsp/diagnostics.rs:19)

Severity: Low — Minor display issue

Line: diagnostics.rs:19

let end_col = diag
    .location
    .as_ref()
    .and_then(|loc| loc.source_line.as_ref().map(String::len))
    .unwrap_or(col + 1);

Issue: If source_line is present, this underlines the entire line regardless of where the error actually is. For a parse error at column 5, the squiggle should cover just the erroneous token, not columns 5–80.

Fix: Store token length in SourceLoc or in RexxDiagnostic, then use col + token_len instead of line.len().

---

4. Subroutine End Line Heuristic (src/lsp/analysis.rs:312-317)

Severity: Low — Affects document outline accuracy

Lines: 312-317

let end_line = labels
    .iter()
    .filter(|l| l.loc.line > clause.loc.line)
    .map(|l| l.loc.line.saturating_sub(1))
    .min()
    .unwrap_or(total_lines);

Issue: This assumes subroutines end at the next label. But REXX allows:

foo:
  say 'in foo'
  return
say 'more code'
bar:

The heuristic will include say 'more code' in foo's range, which is incorrect.

Correct behavior: Subroutine ends at the first RETURN or EXIT after the label, OR at the next label if no RETURN is found.

Impact: Document symbol outline (foo function) includes extra code.

Fix: Walk the AST to find the first Return/Exit clause after the label.

---

Minor Issues

5. Unnecessary Clone in detect_subroutines (src/lsp/analysis.rs:308)

Severity: Pedantic

Line: 308

exposed.clone_from(names);

Should be:

exposed = names.clone();

Both work, but clone_from is for reusing allocations, which doesn't apply here since exposed is empty.

---

6. Duplicate word_at_position (hover.rs:146, definition.rs:33)

Severity: Pedantic

The word_at_position and is_rexx_word_char functions are duplicated in hover.rs and definition.rs. Extract to a shared utils module.

---

7. Hard-Coded Keyword List (completion.rs:6-53)

Severity: Pedantic

The REXX_KEYWORDS list is manually maintained. If the parser gains support for new keywords (e.g., OPTIONS, NUMERIC FORM ENGINEERING), this list must be updated manually.

Suggestion: Generate from the parser's keyword matching logic or keep a canonical list in src/keywords.rs.

---

Non-Issues (Correct Behavior)

✅ Error Number Mapping (diagnostics.rs:41)

Correctly converts RexxError to LSP diagnostic code. Error 41 = Bad arithmetic, Error 42 = overflow, etc. ✅

✅ REXX Case-Insensitivity (hover.rs:89, completion.rs:94)

All symbol comparisons use .to_uppercase(). Correct per REXX semantics. ✅

✅ Edit Distance (actions.rs:73-98)

Classic Levenshtein algorithm for typo suggestions. Correct implementation. ✅

✅ Feature-Gating (lib.rs:13-14, Cargo.toml)

LSP is correctly feature-gated. Binary builds without --features lsp don't include tokio/tower-lsp dependencies. ✅

✅ Async Handling (server.rs:184-193)

Tokio runtime is correctly created and used to serve the LSP. ✅

---

Test Coverage

All 24 new LSP tests pass:

• Label/subroutine detection ✅
• PROCEDURE/EXPOSE tracking ✅
• Variable and function call collection ✅
• PARSE template variable extraction ✅
• Error propagation (lex/parse errors) ✅
• BIF documentation lookup ✅
• Complex programs (Fibonacci, nested loops) ✅

Suggested additional tests:

1. Compound variable handling (arr.1 vs ARR.)
2. Subroutine end detection with RETURN statements
3. Variable references inside expressions (once locations are fixed)
4. Multi-line PARSE templates
5. Nested subroutine calls

---

Performance Notes

Potential Allocations

1. analysis.rs:48 — source.lines().map(String::from).collect() allocates every line. For large files, consider using &str slices or lazy iteration.
2. actions.rs:73-98 — Edit distance allocates O(m*n) matrix on every typo check. Pre-filter by length difference to avoid unnecessary work.

No Panics

✅ All indexing uses .get() or .saturating_sub() to avoid panics on malformed input.

---

Architecture Quality

Strengths:

• Clean separation: analysis.rs walks AST, LSP modules consume DocumentAnalysis
• Feature-gated: LSP doesn't bloat the core interpreter
• Error-tolerant: LSP provides partial results even with parse errors
• Correct REXX semantics: case-insensitive, no reserved words

Weaknesses:

• AST lacks source locations for expressions (blocker for accurate LSP features)
• Heuristic-based subroutine detection (fixable with AST walk)
• Manual keyword list maintenance

---

Verdict

Approve with recommendations.

The LSP implementation is structurally sound and demonstrates good understanding of both REXX semantics and LSP architecture. The feature-gating is correct, tests are comprehensive, and clippy is clean.

Critical work:

1. Fix missing SourceLoc in expressions (defer to Phase 11 or accept as known limitation)
2. Fix subroutine end detection to respect RETURN/EXIT
3. Fix . in word char matching for stem variables

Nice-to-have:

• Extract word_at_position to utils
• Generate keywords from parser
• Pre-filter edit distance by length

All tests pass (435/435). No panics, no REXX semantic violations.

# PR #13 Review: Language Server (LSP) Implementation ## Summary This PR implements a complete LSP for patch-rexx with 14 new files, feature-gated compilation, and comprehensive test coverage (24 new tests). All 435 tests pass. ## Critical Issues ### 1. **Missing SourceLoc in AST Walker** (src/lsp/analysis.rs) **Severity: Medium** — Affects accuracy of LSP features **Lines:** 230, 256, 262, 269 The `AstCollector::walk_*` methods create `VariableInfo` and `CallInfo` with placeholder `SourceLoc::new(0, 0)` instead of actual source locations: ```rust // analysis.rs:230 self.assignments.push(VariableInfo { name: ctrl.var.clone(), loc: SourceLoc::new(0, 0), // ❌ Wrong location }); // analysis.rs:256 self.references.push(VariableInfo { name: name.clone(), loc: SourceLoc::new(0, 0), // ❌ Wrong location }); ``` **Impact:** - Go-to-definition, hover, and code actions won't work correctly for variable references and function calls inside expressions - Only top-level assignments from clause locations are accurate **Fix:** The AST nodes (`Expr`, `DoKind::Controlled`, etc.) need to carry their own `SourceLoc`. This is a deeper architectural issue — expressions currently don't have location info. **Recommendation:** Either: 1. Add `loc: SourceLoc` to all `Expr` variants (breaking change, correct solution) 2. Document this limitation and defer to a future phase 3. Track byte offsets during parsing and back-compute locations (fragile) --- ### 2. **Case-Sensitivity in is_rexx_word_char** (src/lsp/hover.rs:174, definition.rs:61) **Severity: Low** — Minor deviation from REXX semantics **Lines:** hover.rs:174-176, definition.rs:61-63 ```rust fn is_rexx_word_char(ch: char) -> bool { ch.is_alphanumeric() || ch == '_' || ch == '.' || ch == '\!' || ch == '?' || ch == '@' } ``` **Issue:** REXX variable names can contain `\!`, `?`, `@` per ANSI X3.274-1996 §2.2.3, so this is correct. However, including `.` means word-at-position will match `ARR.1` as a single word instead of stopping at `ARR`. **Impact:** - Hovering over `ARR.1` won't match stem variable `ARR.` correctly - Go-to-definition for compound variables may fail **Fix:** Exclude `.` from word characters for LSP purposes, or special-case stem lookups. --- ### 3. **Potential Off-by-One in End Position** (src/lsp/diagnostics.rs:19) **Severity: Low** — Minor display issue **Line:** diagnostics.rs:19 ```rust let end_col = diag .location .as_ref() .and_then(|loc| loc.source_line.as_ref().map(String::len)) .unwrap_or(col + 1); ``` **Issue:** If `source_line` is present, this underlines the **entire line** regardless of where the error actually is. For a parse error at column 5, the squiggle should cover just the erroneous token, not columns 5–80. **Fix:** Store token length in `SourceLoc` or in `RexxDiagnostic`, then use `col + token_len` instead of `line.len()`. --- ### 4. **Subroutine End Line Heuristic** (src/lsp/analysis.rs:312-317) **Severity: Low** — Affects document outline accuracy **Lines:** 312-317 ```rust let end_line = labels .iter() .filter(|l| l.loc.line > clause.loc.line) .map(|l| l.loc.line.saturating_sub(1)) .min() .unwrap_or(total_lines); ``` **Issue:** This assumes subroutines end at the next label. But REXX allows: ```rexx foo: say 'in foo' return say 'more code' bar: ``` The heuristic will include `say 'more code'` in `foo`'s range, which is incorrect. **Correct behavior:** Subroutine ends at the first `RETURN` or `EXIT` after the label, OR at the next label if no `RETURN` is found. **Impact:** Document symbol outline (`foo` function) includes extra code. **Fix:** Walk the AST to find the first `Return`/`Exit` clause after the label. --- ## Minor Issues ### 5. **Unnecessary Clone in detect_subroutines** (src/lsp/analysis.rs:308) **Severity: Pedantic** **Line:** 308 ```rust exposed.clone_from(names); ``` Should be: ```rust exposed = names.clone(); ``` Both work, but `clone_from` is for reusing allocations, which doesn't apply here since `exposed` is empty. --- ### 6. **Duplicate word_at_position** (hover.rs:146, definition.rs:33) **Severity: Pedantic** The `word_at_position` and `is_rexx_word_char` functions are duplicated in `hover.rs` and `definition.rs`. Extract to a shared `utils` module. --- ### 7. **Hard-Coded Keyword List** (completion.rs:6-53) **Severity: Pedantic** The `REXX_KEYWORDS` list is manually maintained. If the parser gains support for new keywords (e.g., `OPTIONS`, `NUMERIC FORM ENGINEERING`), this list must be updated manually. **Suggestion:** Generate from the parser's keyword matching logic or keep a canonical list in `src/keywords.rs`. --- ## Non-Issues (Correct Behavior) ### ✅ Error Number Mapping (diagnostics.rs:41) Correctly converts `RexxError` to LSP diagnostic code. Error 41 = Bad arithmetic, Error 42 = overflow, etc. ✅ ### ✅ REXX Case-Insensitivity (hover.rs:89, completion.rs:94) All symbol comparisons use `.to_uppercase()`. Correct per REXX semantics. ✅ ### ✅ Edit Distance (actions.rs:73-98) Classic Levenshtein algorithm for typo suggestions. Correct implementation. ✅ ### ✅ Feature-Gating (lib.rs:13-14, Cargo.toml) LSP is correctly feature-gated. Binary builds without `--features lsp` don't include tokio/tower-lsp dependencies. ✅ ### ✅ Async Handling (server.rs:184-193) Tokio runtime is correctly created and used to serve the LSP. ✅ --- ## Test Coverage All 24 new LSP tests pass: - Label/subroutine detection ✅ - PROCEDURE/EXPOSE tracking ✅ - Variable and function call collection ✅ - PARSE template variable extraction ✅ - Error propagation (lex/parse errors) ✅ - BIF documentation lookup ✅ - Complex programs (Fibonacci, nested loops) ✅ **Suggested additional tests:** 1. Compound variable handling (`arr.1` vs `ARR.`) 2. Subroutine end detection with `RETURN` statements 3. Variable references inside expressions (once locations are fixed) 4. Multi-line PARSE templates 5. Nested subroutine calls --- ## Performance Notes ### Potential Allocations 1. **analysis.rs:48** — `source.lines().map(String::from).collect()` allocates every line. For large files, consider using `&str` slices or lazy iteration. 2. **actions.rs:73-98** — Edit distance allocates `O(m*n)` matrix on every typo check. Pre-filter by length difference to avoid unnecessary work. ### No Panics ✅ All indexing uses `.get()` or `.saturating_sub()` to avoid panics on malformed input. --- ## Architecture Quality **Strengths:** - Clean separation: `analysis.rs` walks AST, LSP modules consume `DocumentAnalysis` - Feature-gated: LSP doesn't bloat the core interpreter - Error-tolerant: LSP provides partial results even with parse errors - Correct REXX semantics: case-insensitive, no reserved words **Weaknesses:** - AST lacks source locations for expressions (blocker for accurate LSP features) - Heuristic-based subroutine detection (fixable with AST walk) - Manual keyword list maintenance --- ## Verdict **Approve with recommendations.** The LSP implementation is structurally sound and demonstrates good understanding of both REXX semantics and LSP architecture. The feature-gating is correct, tests are comprehensive, and clippy is clean. **Critical work:** 1. Fix missing `SourceLoc` in expressions (defer to Phase 11 or accept as known limitation) 2. Fix subroutine end detection to respect `RETURN`/`EXIT` 3. Fix `.` in word char matching for stem variables **Nice-to-have:** - Extract `word_at_position` to utils - Generate keywords from parser - Pre-filter edit distance by length All tests pass (435/435). No panics, no REXX semantic violations.

claude[bot] commented 2026-02-10 06:30:02 +00:00 (Migrated from github.com)

Copy link

PR Review: Phase 10 LSP Implementation

I've reviewed the Phase 10 LSP implementation for the REXX interpreter. Overall, this is solid work with good attention to REXX semantics and error handling. The code is well-structured and follows the project's design principles.

✅ Strengths

REXX Semantics - Correct

• Case-insensitive keywords: Properly handled throughout (hover.rs:90, completion.rs)
• Everything-is-a-string: LSP analysis correctly interfaces with the parser/AST without making assumptions about value types
• No reserved words: Analysis correctly treats keywords as contextual (subroutines can shadow keywords)
• Uppercase normalization: Labels and symbols correctly uppercased (analysis.rs:180, hover.rs:109)

Arithmetic & Error Handling - Excellent

• Division by zero: Correctly returns ArithmeticOverflow (Error 42) in all division paths (eval.rs:1606, 1624, 1639)
• Error numbers: Proper ANSI X3.274-1996 error mappings throughout (error.rs:119-160)
• BigDecimal usage: Consistent use of arbitrary-precision arithmetic with NUMERIC DIGITS compliance
• Power edge cases: Handles 0^(-n) correctly (eval.rs:1670-1672)

Safety - No Panics Found

• Parser integration: Safe error propagation in analysis.rs:53-67
• LSP operations: No unwrap/panic paths in LSP code (only one acceptable expect in server.rs:185 for runtime initialization)
• Bounds checking: Proper saturating_sub usage throughout to prevent underflow

Architecture

• Clean separation: LSP module properly feature-gated (#[cfg(feature = "lsp")])
• Utility extraction: Removed duplication with shared util.rs module (util.rs:9-39)
• SourceLoc threading: The commit notes mention fixing SourceLoc(0,0) placeholders by threading ctx_loc through walk_expr/walk_do_block — this is good practice

🔍 Observations (not issues, just notes)

1. Analysis.rs: SourceLoc Usage

The LSP analysis correctly threads ctx_loc through expression walkers (analysis.rs:252). This is a smart design decision since expressions don't carry their own SourceLoc in the AST, and using the parent clause's location is the right fallback for diagnostics.

2. Word Character Definition

The fix mentioned in commit 2 (removing . from is_rexx_word_char) is correct. REXX compound variables like ARR.1 should resolve to stem ARR, not the full compound (util.rs:37-39).

3. Test Coverage

24 new integration tests in phase10.rs covering:

• Label/subroutine detection with PROCEDURE/EXPOSE
• Variable assignments, references, stems
• Parse template variables
• Error capture (lex/parse errors)
• BIF documentation lookup
• Complex nested programs

This is solid coverage for the LSP features.

4. Diagnostic Mapping

The single-character underline (diagnostics.rs:16) is correct for pinpoint error location. The error code mapping properly converts RexxError enum values to LSP diagnostic codes.

📊 Summary

No issues found. This PR demonstrates:

• Correct REXX semantics understanding
• Proper error handling with no panic paths
• Good test coverage for LSP functionality
• Clean architecture with appropriate abstractions

The implementation correctly handles:

• Parser edge cases (unterminated comments, incomplete blocks)
• Operator precedence (delegated to parser, LSP just walks AST)
• NUMERIC DIGITS compliance (inherits from evaluator)
• Division by zero (Error 42)
• Deep nesting (parser handles via recursion limits)

Recommendation: ✅ Approve and merge

The Phase 10 LSP implementation is production-ready and maintains the high quality standards established in earlier phases.

---

Review performed using Claude Code CLI

# PR Review: Phase 10 LSP Implementation I've reviewed the Phase 10 LSP implementation for the REXX interpreter. Overall, this is **solid work** with good attention to REXX semantics and error handling. The code is well-structured and follows the project's design principles. ## ✅ Strengths ### REXX Semantics - Correct - **Case-insensitive keywords**: Properly handled throughout (hover.rs:90, completion.rs) - **Everything-is-a-string**: LSP analysis correctly interfaces with the parser/AST without making assumptions about value types - **No reserved words**: Analysis correctly treats keywords as contextual (subroutines can shadow keywords) - **Uppercase normalization**: Labels and symbols correctly uppercased (analysis.rs:180, hover.rs:109) ### Arithmetic & Error Handling - Excellent - **Division by zero**: Correctly returns `ArithmeticOverflow` (Error 42) in all division paths (eval.rs:1606, 1624, 1639) - **Error numbers**: Proper ANSI X3.274-1996 error mappings throughout (error.rs:119-160) - **BigDecimal usage**: Consistent use of arbitrary-precision arithmetic with NUMERIC DIGITS compliance - **Power edge cases**: Handles 0^(-n) correctly (eval.rs:1670-1672) ### Safety - No Panics Found - **Parser integration**: Safe error propagation in analysis.rs:53-67 - **LSP operations**: No unwrap/panic paths in LSP code (only one acceptable `expect` in server.rs:185 for runtime initialization) - **Bounds checking**: Proper `saturating_sub` usage throughout to prevent underflow ### Architecture - **Clean separation**: LSP module properly feature-gated (#[cfg(feature = "lsp")]) - **Utility extraction**: Removed duplication with shared `util.rs` module (util.rs:9-39) - **SourceLoc threading**: The commit notes mention fixing `SourceLoc(0,0)` placeholders by threading `ctx_loc` through `walk_expr`/`walk_do_block` — this is good practice ## 🔍 Observations (not issues, just notes) ### 1. Analysis.rs: SourceLoc Usage The LSP analysis correctly threads `ctx_loc` through expression walkers (analysis.rs:252). This is a **smart design decision** since expressions don't carry their own SourceLoc in the AST, and using the parent clause's location is the right fallback for diagnostics. ### 2. Word Character Definition The fix mentioned in commit 2 (removing `.` from `is_rexx_word_char`) is correct. REXX compound variables like `ARR.1` should resolve to stem `ARR`, not the full compound (util.rs:37-39). ### 3. Test Coverage 24 new integration tests in phase10.rs covering: - Label/subroutine detection with PROCEDURE/EXPOSE - Variable assignments, references, stems - Parse template variables - Error capture (lex/parse errors) - BIF documentation lookup - Complex nested programs This is **solid coverage** for the LSP features. ### 4. Diagnostic Mapping The single-character underline (diagnostics.rs:16) is correct for pinpoint error location. The error code mapping properly converts RexxError enum values to LSP diagnostic codes. ## 📊 Summary **No issues found.** This PR demonstrates: - Correct REXX semantics understanding - Proper error handling with no panic paths - Good test coverage for LSP functionality - Clean architecture with appropriate abstractions The implementation correctly handles: - Parser edge cases (unterminated comments, incomplete blocks) - Operator precedence (delegated to parser, LSP just walks AST) - NUMERIC DIGITS compliance (inherits from evaluator) - Division by zero (Error 42) - Deep nesting (parser handles via recursion limits) **Recommendation: ✅ Approve and merge** The Phase 10 LSP implementation is production-ready and maintains the high quality standards established in earlier phases. --- *Review performed using Claude Code CLI*

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

Something isn't working

  

dependencies

Pull requests that update a dependency file

  

documentation

Improvements or additions to documentation

  

duplicate

This issue or pull request already exists

  

enhancement

New feature or request

  

good first issue

Good for newcomers

  

help wanted

Extra attention is needed

  

invalid

This doesn't seem right

  

question

Further information is requested

  

rust

Pull requests that update rust code

  

wontfix

This will not be worked on

No labels

bug

dependencies

documentation

duplicate

enhancement

good first issue

help wanted

invalid

question

rust

wontfix

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/patch-rexx!13

No description provided.