diff --git a/CHANGELOG.md b/CHANGELOG.md index a8cbc3a..13db497 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## [1.7.0](https://github.com/wrightkit/language-provider-protocol/compare/v1.6.0...v1.7.0) (2026-10-10) +## [1.7.0](https://github.com/wrightkit/language-provider-protocol/compare/v1.6.0...v1.7.0) (2026-10-11) ### Features diff --git a/conformance/mock-provider/src/main.rs b/conformance/mock-provider/src/main.rs index dd59cc7..2954ed5 100644 --- a/conformance/mock-provider/src/main.rs +++ b/conformance/mock-provider/src/main.rs @@ -15,6 +15,8 @@ mod wire; use std::collections::HashSet; use std::io::{self, BufRead, BufWriter, Write}; +use serde_json::Value; + use server::{DEFAULT_PROTOCOL_VERSION, PROTOCOL_VERSIONS, Server}; fn main() { @@ -24,29 +26,44 @@ fn main() { let stdout = io::stdout(); let mut out = BufWriter::new(stdout.lock()); let mut server = Server::new(supported_version, disabled); - let mut line = String::new(); + let mut buf = Vec::new(); loop { - line.clear(); - let read = reader - .read_line(&mut line) - .unwrap_or_else(|e| panic!("failed to read stdin: {e}")); + buf.clear(); + let read = match reader.read_until(b'\n', &mut buf) { + Ok(n) => n, + Err(_) => break, + }; if read == 0 { break; } - let message = line.trim_end_matches(['\r', '\n']); - if message.is_empty() { - continue; + let response = match std::str::from_utf8(&buf) { + Ok(line) => { + let message = line.trim_end_matches(['\r', '\n']); + if message.is_empty() { + continue; + } + server.handle_message(message) + } + // A line that is not valid UTF-8 cannot be valid JSON: -32700 and + // the session continues. + Err(_) => rpc::std_error(Value::Null, -32700, "Parse error"), + }; + if write_response(&mut out, &response).is_err() { + break; } - let response = server.handle_message(message); - let serialized = serde_json::to_string(&response).expect("response serializes"); - writeln!(out, "{serialized}").expect("write stdout"); - out.flush().expect("flush stdout"); if server.exiting { break; } } } +fn write_response(out: &mut impl Write, response: &Value) -> io::Result<()> { + let serialized = serde_json::to_string(response).expect("response serializes"); + out.write_all(serialized.as_bytes())?; + out.write_all(b"\n")?; + out.flush() +} + /// `--protocol-version ` pins the supported version; `--without a,b,c` /// disables capabilities (used by the capability-negotiation fixtures). fn parse_args() -> (String, HashSet<&'static str>) { diff --git a/conformance/mock-provider/src/methods.rs b/conformance/mock-provider/src/methods.rs index 4ed1e82..9007631 100644 --- a/conformance/mock-provider/src/methods.rs +++ b/conformance/mock-provider/src/methods.rs @@ -209,7 +209,7 @@ impl Server { ¶ms.document.uri, )?; let locations: Vec = puzzle - .occurrences(symbol.name(), params.include_declaration) + .symbol_occurrences(&symbol, params.include_declaration) .into_iter() .map(|range| location_json(¶ms.document.uri, range)) .collect(); @@ -255,21 +255,33 @@ impl Server { let edits: Vec = sorted_entries(¶ms.documents) .into_iter() - .filter_map(|(key, doc)| { - let puzzle = parse_document(&doc.text).puzzle?; + .map(|(key, doc)| { + // §15.2: every reference in every received document must be + // covered, so a document that did not parse cleanly makes the + // rename unsafe — refuse instead of emitting partial edits. + let Some(puzzle) = parse_document(&doc.text).into_puzzle() else { + return Err(HandlerError::lpp( + "invalidDocument", + json!({ "uri": doc.uri, "reason": "documentHasErrors" }), + "document has errors and cannot be renamed safely", + )); + }; let edits: Vec = puzzle .rename_ranges(&symbol, key == ¶ms.position_document_uri) .into_iter() .map(|range| json!({ "range": range, "newText": params.new_name })) .collect(); - (!edits.is_empty()).then(|| { + Ok((!edits.is_empty()).then(|| { json!({ "documentUri": doc.uri, "version": doc.version, "textEdits": edits, }) - }) + })) }) + .collect::, _>>()? + .into_iter() + .flatten() .collect(); if edits.is_empty() { return Err(HandlerError::refusal( diff --git a/conformance/mock-provider/src/project.rs b/conformance/mock-provider/src/project.rs index 6156f2d..4bddbf4 100644 --- a/conformance/mock-provider/src/project.rs +++ b/conformance/mock-provider/src/project.rs @@ -172,7 +172,9 @@ impl Loader<'_> { })?; // The project is the `.xdl` files in the selected entry's directory. - let mut paths: Vec = std::fs::read_dir(&root) + // A failed directory entry fails the load rather than silently + // dropping a required source (spec section 8.1). + let entries: Vec<_> = std::fs::read_dir(&root) .map_err(|error| { self.fail( "projectDirectoryUnreadable", @@ -180,18 +182,50 @@ impl Loader<'_> { format!("project directory could not be read: {error}"), ) })? - .filter_map(Result::ok) - .map(|entry| entry.path()) - .filter(|path| path == &entry_path || path.extension().is_some_and(|ext| ext == "xdl")) - .collect(); + .collect::, _>>() + .map_err(|error| { + self.fail( + "projectDirectoryUnreadable", + None, + format!("project directory could not be read: {error}"), + ) + })?; + let mut paths: Vec = Vec::new(); + for entry in entries { + let path = entry.path(); + if path != entry_path { + if path.extension().is_none_or(|ext| ext != "xdl") { + continue; + } + // A directory named `*.xdl` is not a source file; symlinks + // still resolve (or fail) through canonicalization below. + let is_dir = entry + .file_type() + .map_err(|error| { + self.fail( + "sourceFileUnreadable", + path_to_file_uri(&path).as_deref(), + format!("source file type could not be read: {error}"), + ) + })? + .is_dir(); + if is_dir { + continue; + } + } + paths.push(path); + } paths.sort(); let mut documents = HashMap::new(); for path in paths { + // The failing file's URI is knowable before canonicalization; + // `details.uri` should name it (spec section 8.1). + let unresolved_uri = path_to_file_uri(&path); let path = std::fs::canonicalize(&path).map_err(|error| { self.fail( "sourceFileUnreadable", - None, + unresolved_uri.as_deref(), format!("source file could not be resolved: {error}"), ) })?; diff --git a/conformance/mock-provider/src/puzzle/artifact.rs b/conformance/mock-provider/src/puzzle/artifact.rs index 4935e30..ade6065 100644 --- a/conformance/mock-provider/src/puzzle/artifact.rs +++ b/conformance/mock-provider/src/puzzle/artifact.rs @@ -48,9 +48,24 @@ struct SheetOp { arg: i64, } +/// The `x-demo-lang` identifier grammar: `[A-Za-z_][A-Za-z0-9_]*`. +fn is_ident(name: &str) -> bool { + let mut chars = name.chars(); + matches!(chars.next(), Some(c) if c.is_ascii_alphabetic() || c == '_') + && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') +} + /// Canonical source text regenerated from a puzzle evaluation sheet. +/// Names are emitted verbatim, so every one must be a valid identifier or +/// the output would not parse (or would carry injected syntax). pub(crate) fn reconstruct_source(content: &str) -> Option { let sheet: Sheet = serde_json::from_str(content).ok()?; + if !is_ident(&sheet.name) + || sheet.ops.iter().any(|op| !is_ident(&op.name)) + || sheet.solution.iter().any(|name| !is_ident(name)) + { + return None; + } let mut out = format!( "puzzle {} {{\n target = {}\n start = {}\n ops {{\n", sheet.name, sheet.target, sheet.start diff --git a/conformance/mock-provider/src/puzzle/lex.rs b/conformance/mock-provider/src/puzzle/lex.rs index 1a6c457..573c7f0 100644 --- a/conformance/mock-provider/src/puzzle/lex.rs +++ b/conformance/mock-provider/src/puzzle/lex.rs @@ -24,6 +24,11 @@ pub(super) enum TokenKind { Minus, Slash, Newline, + /// A character outside the token grammar. Carries the char so diagnostics + /// can name it; the parser never accepts it as an identifier. + Unknown(char), + /// An integer literal outside the i64 range. + BadNumber, } impl TokenKind { @@ -85,7 +90,7 @@ pub(super) fn tokenize(src: &SourceText<'_>, text: &str) -> Vec { b'-' if bytes.get(i + 1).is_some_and(|b| b.is_ascii_digit()) => { let (n, end) = read_number(text, i); i = end; - TokenKind::Number(n) + n.map_or(TokenKind::BadNumber, TokenKind::Number) } b'-' => { i += 1; @@ -94,7 +99,7 @@ pub(super) fn tokenize(src: &SourceText<'_>, text: &str) -> Vec { b if b.is_ascii_digit() => { let (n, end) = read_number(text, i); i = end; - TokenKind::Number(n) + n.map_or(TokenKind::BadNumber, TokenKind::Number) } b if b.is_ascii_alphabetic() || b == b'_' => { while i < bytes.len() && (bytes[i].is_ascii_alphanumeric() || bytes[i] == b'_') { @@ -108,8 +113,11 @@ pub(super) fn tokenize(src: &SourceText<'_>, text: &str) -> Vec { kind.clone() } None => { - i += 1; - TokenKind::Ident(format!("")) + // Consume the whole character so token ranges stay on + // UTF-8 boundaries; `SourceText` positions slice `text`. + let ch = text[i..].chars().next().expect("in-bounds byte"); + i += ch.len_utf8(); + TokenKind::Unknown(ch) } }, }; @@ -121,16 +129,15 @@ pub(super) fn tokenize(src: &SourceText<'_>, text: &str) -> Vec { tokens } -fn read_number(text: &str, i: usize) -> (i64, usize) { +fn read_number(text: &str, i: usize) -> (Option, usize) { let bytes = text.as_bytes(); - let negative = bytes[i] == b'-'; - let mut end = i + usize::from(negative); + let mut end = i + usize::from(bytes[i] == b'-'); while end < bytes.len() && bytes[end].is_ascii_digit() { end += 1; } - let digits = &text[i + usize::from(negative)..end]; - let value = digits.parse::().unwrap_or(0); - (if negative { -value } else { value }, end) + // Parse the signed slice so -9223372036854775808 (i64::MIN) stays + // representable; out-of-range literals surface as `None`. + (text[i..end].parse::().ok(), end) } /// Human-readable token description for parse diagnostics. @@ -139,6 +146,8 @@ pub(super) fn describe(tok: &Token) -> String { TokenKind::Ident(name) => format!("'{name}'"), TokenKind::Number(n) => format!("number {n}"), TokenKind::Newline => "end of line".to_string(), + TokenKind::Unknown(ch) => format!("unrecognized '{}'", ch.escape_default()), + TokenKind::BadNumber => "out-of-range integer literal".to_string(), other => format!("'{other:?}'"), } } diff --git a/conformance/mock-provider/src/puzzle/parse.rs b/conformance/mock-provider/src/puzzle/parse.rs index 6565a9f..5eb94aa 100644 --- a/conformance/mock-provider/src/puzzle/parse.rs +++ b/conformance/mock-provider/src/puzzle/parse.rs @@ -17,6 +17,12 @@ pub(crate) fn parse_document(text: &str) -> ParseOutput { diagnostics: Vec::new(), }; let puzzle = parse_top(&mut parser); + // Spec section 9.2: diagnostics are sorted by range.start. Diagnostics + // are pushed in analysis phases (body errors, then missing-section at the + // puzzle name, then semantic checks), so a stable sort is required. + parser + .diagnostics + .sort_by_key(|d| (d.range.start.line, d.range.start.character)); ParseOutput { puzzle, diagnostics: parser.diagnostics, @@ -56,6 +62,16 @@ impl Parser { } } + /// Report an unexpected end of input just past the last token. + fn eof_error(&mut self, what: &str) { + let at = self + .tokens + .last() + .map(|t| Range::point(t.range.end)) + .unwrap_or_else(|| Range::point(Position::ORIGIN)); + self.error_at(at, format!("unexpected end of input: expected {what}")); + } + /// Report a syntax error at `range` without consuming input. fn error_at(&mut self, range: Range, message: impl Into) { self.diagnostics @@ -98,12 +114,19 @@ impl Parser { /// silently at end of input. fn expect_number(&mut self, what: &str) -> Option { let tok = self.peek().cloned()?; - if let TokenKind::Number(n) = tok.kind { - self.pos += 1; - Some(n) - } else { - self.error(&tok, format!("expected {what}")); - None + match tok.kind { + TokenKind::Number(n) => { + self.pos += 1; + Some(n) + } + TokenKind::BadNumber => { + self.error(&tok, "integer literal out of range"); + None + } + _ => { + self.error(&tok, format!("expected {what}")); + None + } } } @@ -124,8 +147,8 @@ impl Parser { } /// Consume the required `x` parameter name in an op declaration, or report - /// an error and recover. A failed ident skips an extra line, matching the - /// reference behavior for malformed op declarations. + /// an error and recover. `expect_ident` has already recovered to the next + /// line on a wrong token; at end of input there is nothing to recover. fn expect_x(&mut self, what: &str) -> bool { match self.expect_ident() { Some((name, _)) if name == "x" => true, @@ -133,10 +156,7 @@ impl Parser { self.error(&tok, format!("{what}, found '{name}'")); false } - None => { - self.recover_line(); - false - } + None => false, } } } @@ -160,6 +180,10 @@ fn parse_top(parser: &mut Parser) -> Option { return None; } } + if parser.peek().is_none() { + parser.eof_error("a puzzle name"); + return None; + } let (name, name_tok) = parser.expect_ident()?; let mut puzzle = Puzzle { name, @@ -170,6 +194,10 @@ fn parse_top(parser: &mut Parser) -> Option { solution: Vec::new(), solution_range: Range::point(Position::ORIGIN), }; + if parser.peek().is_none() { + parser.eof_error("'{'"); + return None; + } if !parser.expect_punct(TokenKind::LBrace, "'{'") { return None; } @@ -277,7 +305,21 @@ fn parse_top(parser: &mut Parser) -> Option { puzzle.target, value ), )), - _ => None, + Some(_) => None, + // All ops resolved here (unresolved references are already + // errors), so a failed simulation is an arithmetic failure: + // division by zero or integer overflow. It must be an error — + // compile cannot produce an artifact without a value. + None => { + parser.diagnostics.push(diagnostic( + puzzle.solution_range, + "error", + "x-demo/simulation-failed", + "solution cannot be evaluated: division by zero or integer overflow" + .to_string(), + )); + None + } } }; if let Some((code, message)) = warning { @@ -322,11 +364,17 @@ fn parse_op(parser: &mut Parser, puzzle: &mut Puzzle, name_tok: &Token) { return; }; let name = name.clone(); - parser.expect_punct(TokenKind::Colon, "':'"); + // A failed expect_* has already reported and recovered to the next line; + // continuing would cascade the same diagnostic into the following op. + if !parser.expect_punct(TokenKind::Colon, "':'") { + return; + } if !parser.expect_x("op parameter must be 'x'") { return; } - parser.expect_punct(TokenKind::Arrow, "'=>'"); + if !parser.expect_punct(TokenKind::Arrow, "'=>'") { + return; + } if !parser.expect_x("op body must apply to 'x'") { return; } @@ -342,7 +390,6 @@ fn parse_op(parser: &mut Parser, puzzle: &mut Puzzle, name_tok: &Token) { return; }; let Some(arg) = parser.expect_number("a numeric argument") else { - parser.recover_line(); return; }; puzzle.ops.push(Op { diff --git a/conformance/mock-provider/src/puzzle/symbols.rs b/conformance/mock-provider/src/puzzle/symbols.rs index 37fb4c5..514ae0c 100644 --- a/conformance/mock-provider/src/puzzle/symbols.rs +++ b/conformance/mock-provider/src/puzzle/symbols.rs @@ -34,15 +34,6 @@ pub(crate) enum Symbol { Op { name: String }, } -impl Symbol { - /// The name the symbol binds to. - pub(crate) fn name(&self) -> &str { - match self { - Symbol::Puzzle { name } | Symbol::Op { name } => name, - } - } -} - impl Puzzle { /// The symbol under `byte`: the puzzle name, an op declaration, or a /// solution reference, in that order. @@ -103,6 +94,25 @@ impl Puzzle { ranges } + /// Ranges where `symbol` occurs, resolved by symbol kind: a puzzle name + /// is only ever its own declaration (puzzle names are never referenced + /// elsewhere); an op is its declaration then references in document + /// order. Looking an op up by name is wrong for a puzzle symbol that + /// happens to share the op's name. + pub(crate) fn symbol_occurrences( + &self, + symbol: &Symbol, + include_declaration: bool, + ) -> Vec { + match symbol { + Symbol::Puzzle { .. } => include_declaration + .then_some(self.name_range) + .into_iter() + .collect(), + Symbol::Op { name } => self.occurrences(name, include_declaration), + } + } + /// Whether renaming `symbol` to `new_name` would collide with an /// existing op. Only op renames can collide. pub(crate) fn rename_collides(&self, symbol: &Symbol, new_name: &str) -> bool { @@ -124,7 +134,14 @@ impl Puzzle { Vec::new() } } - Symbol::Op { name } => self.occurrences(name, true), + Symbol::Op { name } => { + // §15.2 requires edits sorted by range.start; occurrences + // lists the declaration first (a §14.2 carve-out for + // references), which misorders when `solution` precedes `ops`. + let mut ranges = self.occurrences(name, true); + ranges.sort_by_key(|r| (r.start.line, r.start.character)); + ranges + } } } } diff --git a/conformance/mock-provider/src/puzzle/tests.rs b/conformance/mock-provider/src/puzzle/tests.rs index f2c091d..3f94197 100644 --- a/conformance/mock-provider/src/puzzle/tests.rs +++ b/conformance/mock-provider/src/puzzle/tests.rs @@ -53,14 +53,116 @@ fn reports_duplicate_and_unresolved() { } #[test] -fn malformed_op_declaration_skips_following_line() { - // Reference recovery behavior: a bad op parameter consumes the rest of - // that line AND the next line of the ops block, so `later` is never - // declared and the solution reference stays unresolved. +fn malformed_op_declaration_does_not_consume_the_next_line() { + // Recovery is line-oriented: a bad op parameter reports one diagnostic + // and skips to the next newline; the following op still parses, so the + // solution reference to it resolves. const SRC: &str = "puzzle t {\n target = 2\n start = 0\n ops {\n bad: 5 => x + 1\n later: x => x * 2\n }\n solution = [ later ]\n}"; let out = parse_document(SRC); let codes: Vec<&str> = out.diagnostics.iter().map(|d| d.code.as_str()).collect(); + assert_eq!(codes, ["x-demo/syntax"]); + let puzzle = out.puzzle.expect("partial parse keeps declarations"); + assert!(puzzle.op("later").is_some()); +} + +#[test] +fn missing_op_punctuation_reports_once_without_swallowing_the_next_op() { + // `b` is missing ':' — the error must not cascade into `c`'s line. + const SRC: &str = "puzzle p {\n target = 1\n start = 0\n ops {\n b x => x + 1\n c: x => x + 2\n }\n solution = [ b, c ]\n}"; + let out = parse_document(SRC); + let codes: Vec<&str> = out.diagnostics.iter().map(|d| d.code.as_str()).collect(); assert_eq!(codes, ["x-demo/syntax", "x-demo/unresolved-op"]); + let puzzle = out.puzzle.expect("partial parse keeps declarations"); + assert!(puzzle.op("c").is_some()); + assert!(puzzle.op("b").is_none()); +} + +#[test] +fn unknown_bytes_are_syntax_errors_not_identifiers() { + // '@', '$', and multi-byte characters are not part of the token grammar: + // they must surface as syntax errors, never parse as identifiers, and + // must not panic the diagnostic path. + for src in [ + "puzzle @ {\n target = 1\n start = 0\n ops {}\n solution = []\n}", + "puzzle p {\n target = 1\n start = 0\n ops {\n $: x => x + 1\n }\n solution = [ $ ]\n}", + "puzzle α {\n target = 1\n start = 0\n ops {}\n solution = []\n}", + ] { + let out = parse_document(src); + assert!( + out.diagnostics.iter().any(|d| d.severity == "error"), + "expected a syntax error for {src:?}" + ); + } +} + +#[test] +fn truncated_document_reports_an_error_instead_of_no_puzzle() { + // A document ending right after `puzzle` or `puzzle ` must produce + // an error diagnostic, not a silent `puzzle: None` — `lpp/compile` + // assumes no errors implies a puzzle. + for src in ["puzzle", "puzzle p", "puzzle p "] { + let out = parse_document(src); + assert!(out.puzzle.is_none(), "unexpectedly parsed {src:?}"); + assert!( + out.diagnostics.iter().any(|d| d.severity == "error"), + "expected a diagnostic for {src:?}" + ); + } +} + +#[test] +fn out_of_range_integer_literal_is_an_error() { + // An integer literal outside the i64 range must be diagnosed, not + // silently coerced to 0 and compiled into the artifact. + let out = parse_document( + "puzzle p {\n target = 99999999999999999999\n start = 0\n ops {}\n solution = []\n}", + ); + assert!( + out.diagnostics.iter().any(|d| d.severity == "error"), + "expected an error for an out-of-range literal" + ); + + // i64::MIN is a valid literal. + let out = parse_document( + "puzzle p {\n target = -9223372036854775808\n start = 0\n ops {}\n solution = []\n}", + ); + assert!( + !out.diagnostics.iter().any(|d| d.severity == "error"), + "i64::MIN is representable: {out:?}" + ); +} + +#[test] +fn diagnostics_are_sorted_by_range_start() { + // A syntax error late in the body plus a missing-section diagnostic at + // the puzzle name must still come out ordered by range.start (§9.2). + const SRC: &str = + "puzzle p {\n start = 1\n ops {\n d: x => x + 1\n }\n solution = [ d ]\n ???\n}"; + let out = parse_document(SRC); + let mut sorted = out.diagnostics.clone(); + sorted.sort_by_key(|d| (d.range.start.line, d.range.start.character)); + assert_eq!( + out.diagnostics.iter().map(|d| &d.code).collect::>(), + sorted.iter().map(|d| &d.code).collect::>(), + "diagnostics are not sorted by range.start" + ); +} + +#[test] +fn arithmetic_failure_is_an_error_not_silence() { + // Division by zero and i64 overflow make simulate() return None; that + // must surface as an error diagnostic or compile's "no errors implies + // resolvable ops" invariant breaks. + for src in [ + "puzzle p {\n target = 1\n start = 1\n ops {\n d: x => x / 0\n }\n solution = [ d ]\n}", + "puzzle p {\n target = 1\n start = 9223372036854775807\n ops {\n d: x => x + 1\n }\n solution = [ d ]\n}", + ] { + let out = parse_document(src); + assert!( + out.diagnostics.iter().any(|d| d.severity == "error"), + "expected an error diagnostic for {src:?}" + ); + } } #[test] @@ -86,6 +188,38 @@ fn utf16_positions_are_exact() { assert!(src.byte_of(pos(0, 12)).is_none()); } +#[test] +fn trailing_newline_phantom_line_is_a_valid_position() { + // A document ending in '\n' has a phantom empty last line; a position + // there is valid (spec §6.4) and must not invert the line's byte range. + let src = SourceText::new("a\n"); + assert_eq!(src.line_text(0), "a"); + assert_eq!(src.line_text(1), ""); + assert_eq!(src.byte_of(pos(1, 0)), Some(2)); + + let src = SourceText::new("a\r\n"); + assert_eq!(src.line_text(1), ""); + assert_eq!(src.byte_of(pos(1, 0)), Some(3)); + + // An empty line between two newlines stays empty, not inverted. + let src = SourceText::new("a\n\nb"); + assert_eq!(src.line_text(1), ""); + assert_eq!(src.byte_of(pos(1, 0)), Some(2)); +} + +#[test] +fn crlf_terminator_does_not_count_toward_character() { + // "\r\n" is the line terminator, not content: positions at its bytes map + // to end-of-line, never past the visible line length. + let src = SourceText::new("aaaaaaaaaa\r\nb"); + assert_eq!(src.position_of(10), pos(0, 10)); // '\r' byte -> end of line 0 + assert_eq!(src.position_of(11), pos(0, 10)); // '\n' byte -> end of line 0 + assert_eq!(src.position_of(12), pos(1, 0)); + // A lone '\r' mid-line is content. + let src = SourceText::new("a\rb\nc"); + assert_eq!(src.position_of(3), pos(0, 3)); +} + #[test] fn compile_reconstruct_round_trip() { let out = parse_document(CLEAN); @@ -96,6 +230,25 @@ fn compile_reconstruct_round_trip() { assert_eq!(source, CLEAN); } +#[test] +fn reconstruct_rejects_names_the_parser_would_not_accept() { + // Names are written into generated source verbatim; anything outside + // the identifier grammar would emit text our own parser rejects (or + // inject extra syntax). + for bad in ["has space", "a\nb", "", "x-y", "9lives"] { + for content in [ + serde_json::json!({"name": bad, "target": 1, "start": 0, "ops": [], "solution": [], "value": 0}), + serde_json::json!({"name": "p", "target": 1, "start": 0, "ops": [{"name": bad, "op": "+", "arg": 1}], "solution": [], "value": 1}), + serde_json::json!({"name": "p", "target": 1, "start": 0, "ops": [], "solution": [bad], "value": 0}), + ] { + assert!( + reconstruct_source(&content.to_string()).is_none(), + "name {bad:?} must be rejected" + ); + } + } +} + #[test] fn validate_edits_detects_overlap_in_original_order() { let edits = vec![edit(7, 15, 7, 21, "a"), edit(7, 18, 7, 23, "b")]; @@ -291,6 +444,35 @@ fn occurrences_list_declaration_first_then_references() { assert!(puzzle.occurrences("triple", true).is_empty()); } +#[test] +fn rename_ranges_are_sorted_when_solution_precedes_ops() { + // `solution` may precede `ops` in a document; rename edits must still be + // sorted by range.start (§15.2) even though occurrences lists the + // declaration first. + const SRC: &str = "puzzle p {\n target = 2\n start = 1\n solution = [ d ]\n ops {\n d: x => x + 1\n }\n}"; + let (src, puzzle) = puzzle_of(SRC); + let symbol = symbol_at(&src, &puzzle, 5, 4).expect("op d"); + let ranges = puzzle.rename_ranges(&symbol, true); + let mut sorted = ranges.clone(); + sorted.sort_by_key(|r| (r.start.line, r.start.character)); + assert_eq!(ranges, sorted, "rename ranges are not sorted"); +} + +#[test] +fn puzzle_symbol_occurrences_do_not_include_a_same_named_op() { + // Position on the puzzle name `a` while an op `a` exists: the puzzle + // symbol's occurrences are its own declaration only — never the op's. + const SRC: &str = "puzzle a {\n target = 2\n start = 1\n ops {\n a: x => x + 1\n }\n solution = [ a ]\n}"; + let (src, puzzle) = puzzle_of(SRC); + let symbol = symbol_at(&src, &puzzle, 0, 7).expect("puzzle name"); + assert!(matches!(symbol, Symbol::Puzzle { .. })); + assert_eq!( + puzzle.symbol_occurrences(&symbol, true), + vec![rng(0, 7, 0, 8)] + ); + assert!(puzzle.symbol_occurrences(&symbol, false).is_empty()); +} + #[test] fn rename_ranges_for_op_cover_declaration_and_references() { let (src, puzzle) = puzzle_of(CLEAN); diff --git a/conformance/mock-provider/src/puzzle/text.rs b/conformance/mock-provider/src/puzzle/text.rs index 8f0a155..8cf0881 100644 --- a/conformance/mock-provider/src/puzzle/text.rs +++ b/conformance/mock-provider/src/puzzle/text.rs @@ -60,9 +60,12 @@ impl<'a> SourceText<'a> { .get(line + 1) .copied() .unwrap_or(self.text.len()); - if self.text.as_bytes().get(end.wrapping_sub(1)) == Some(&b'\n') { + // The terminator counts only when it sits inside this line's span — + // a phantom final line after a trailing newline has an empty span + // and must not strip the previous line's terminator. + if end > start && self.text.as_bytes()[end - 1] == b'\n' { end -= 1; - if self.text.as_bytes().get(end.wrapping_sub(1)) == Some(&b'\r') { + if end > start && self.text.as_bytes()[end - 1] == b'\r' { end -= 1; } } @@ -102,8 +105,10 @@ impl<'a> SourceText<'a> { Ok(i) => i, Err(i) => i - 1, }; - let (start, _) = self.line_byte_range(line); - let units = self.text[start..byte] + // Positions count content only: a byte inside the line terminator + // ("\r\n" or "\n") maps to the end of the line. + let (start, end) = self.line_byte_range(line); + let units = self.text[start..byte.min(end)] .chars() .map(char::len_utf16) .sum::(); diff --git a/conformance/mock-provider/src/server.rs b/conformance/mock-provider/src/server.rs index f1bbabd..95c5ea6 100644 --- a/conformance/mock-provider/src/server.rs +++ b/conformance/mock-provider/src/server.rs @@ -125,10 +125,13 @@ impl Server { return invalid_request(); }; let id = match object.get("id") { - Some(id @ (Value::Number(_) | Value::String(_))) => id.clone(), + Some(id @ Value::String(_)) => id.clone(), + // Request ids are integers or strings (spec section 4); a + // fractional number is not an integer. + Some(id @ Value::Number(_)) if id.is_i64() || id.is_u64() => id.clone(), // No notifications in LPP v1: a message without an id (or with a // null id) is a protocol violation. - _ => { + None | Some(Value::Null) => { return lpp_error( Value::Null, "invalidRequest", @@ -136,8 +139,15 @@ impl Server { "invalid request: LPP v1 defines no notifications", ); } + // Any other id shape (bool, float, array, object) is not a valid + // JSON-RPC request. + Some(_) => return invalid_request(), + }; + // `params` is a REQUIRED object in every LPP v1 request. + let params = match object.get("params") { + Some(params @ Value::Object(_)) => params.clone(), + _ => return std_error(id.clone(), -32602, "Invalid params"), }; - let params = object.get("params").cloned().unwrap_or_else(|| json!({})); match method { "lpp/initialize" => self.initialize(&id, params), "lpp/shutdown" => self.shutdown(&id), diff --git a/conformance/mock-provider/tests/stdio.rs b/conformance/mock-provider/tests/stdio.rs new file mode 100644 index 0000000..73bf348 --- /dev/null +++ b/conformance/mock-provider/tests/stdio.rs @@ -0,0 +1,173 @@ +//! Stdio-framing regressions: malformed bytes on the wire must not crash the +//! provider, and EOF must end the session cleanly. + +use std::io::Write; +use std::process::{Command, Stdio}; + +fn provider() -> Command { + Command::new(env!("CARGO_BIN_EXE_lpp-mock-provider")) +} + +/// Feed `input` to a fresh provider, then close stdin and collect the +/// response messages. +fn run_session(input: &[u8]) -> Vec { + run_session_args(&[], input) +} + +/// `run_session` with extra provider command-line arguments. +fn run_session_args(args: &[&str], input: &[u8]) -> Vec { + let mut child = provider() + .args(args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .expect("spawn provider"); + child + .stdin + .take() + .expect("stdin") + .write_all(input) + .expect("write input"); + let out = child.wait_with_output().expect("wait"); + assert!(out.status.success(), "provider exited {:?}", out.status); + String::from_utf8(out.stdout) + .expect("utf8 stdout") + .lines() + .map(|line| serde_json::from_str(line).expect("json response")) + .collect() +} + +const INIT: &str = + r#"{"jsonrpc":"2.0","id":1,"method":"lpp/initialize","params":{"protocolVersion":"1.0"}}"#; + +#[test] +fn non_utf8_line_gets_parse_error_and_session_continues() { + let mut child = provider() + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .spawn() + .expect("spawn provider"); + { + let mut stdin = child.stdin.take().expect("stdin"); + stdin.write_all(b"\xff\n").expect("write invalid bytes"); + // A well-formed request after the bad line: the session must survive. + stdin + .write_all( + br#"{"jsonrpc":"2.0","id":1,"method":"lpp/initialize","params":{"protocolVersion":"1.0"}}"# + .as_slice(), + ) + .and_then(|()| stdin.write_all(b"\n")) + .expect("write initialize"); + } + let out = child.wait_with_output().expect("wait"); + assert!(out.status.success(), "provider exited {:?}", out.status); + let text = String::from_utf8(out.stdout).expect("utf8 stdout"); + let mut lines = text.lines(); + let parse_error: serde_json::Value = + serde_json::from_str(lines.next().expect("first line")).expect("json response"); + assert_eq!(parse_error["error"]["code"], -32700); + assert_eq!(parse_error["id"], serde_json::Value::Null); + let init: serde_json::Value = + serde_json::from_str(lines.next().expect("second line")).expect("json response"); + assert_eq!(init["id"], 1); + assert!(init["result"]["protocolVersion"].is_string()); +} + +#[test] +fn request_shape_strictness() { + // Spec section 4: `id` is an integer or string; `params` is REQUIRED. + let requests = [ + // Fractional and boolean ids are not integer/string ids. + r#"{"jsonrpc":"2.0","id":1.5,"method":"lpp/initialize","params":{"protocolVersion":"1.0"}}"#, + r#"{"jsonrpc":"2.0","id":true,"method":"lpp/initialize","params":{"protocolVersion":"1.0"}}"#, + // Missing `params`, and a non-object `params`. + r#"{"jsonrpc":"2.0","id":3,"method":"lpp/initialize"}"#, + r#"{"jsonrpc":"2.0","id":4,"method":"lpp/initialize","params":[]}"#, + ]; + let responses = run_session(format!("{}\n", requests.join("\n")).as_bytes()); + assert_eq!(responses[0]["error"]["code"], -32600, "{responses:?}"); + assert_eq!(responses[1]["error"]["code"], -32600, "{responses:?}"); + assert_eq!(responses[2]["error"]["code"], -32602, "{responses:?}"); + assert_eq!(responses[3]["error"]["code"], -32602, "{responses:?}"); +} + +#[test] +fn rename_refuses_when_a_document_has_errors() { + // A document whose parse produced error diagnostics cannot be renamed + // atomically — spec section 15.2 requires covering every reference in + // every received document — so the provider must refuse rather than + // emit partial edits. + let clean = "puzzle c {\n target = 2\n start = 1\n ops {\n a: x => x + 1\n }\n solution = [ a ]\n}"; + let dup = "puzzle p {\n target = 1\n start = 0\n ops {\n a: x => x + 1\n a: x => x + 2\n }\n solution = [ a ]\n}"; + let request = format!( + r#"{{"jsonrpc":"2.0","id":2,"method":"lpp/rename","params":{{"documents":{{"c":{{"uri":"file:///c.xdl","languageId":"x-demo-lang","version":1,"text":{clean}}},"d":{{"uri":"file:///d.xdl","languageId":"x-demo-lang","version":1,"text":{dup}}}}},"positionDocumentUri":"c","position":{{"line":4,"character":4}},"newName":"b"}}}}"#, + clean = serde_json::to_string(clean).unwrap(), + dup = serde_json::to_string(dup).unwrap(), + ); + let responses = run_session(format!("{INIT}\n{request}\n").as_bytes()); + let lpp = &responses[1]["error"]["data"]["lpp"]; + assert_eq!( + lpp["kind"], "invalidDocument", + "expected refusal, got {responses:?}" + ); +} + +/// Scratch directory under the repo's ignored `target/` for project-loader +/// tests. Removed at test start and end so reruns stay clean. +fn project_dir(name: &str) -> std::path::PathBuf { + let root = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../target/project-tests") + .join(format!("{name}-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).expect("create project dir"); + root +} + +const VALID: &str = + "puzzle p {\n target = 1\n start = 0\n ops {\n a: x => x + 1\n }\n solution = [ a ]\n}"; + +fn check_directory(root: &std::path::Path) -> serde_json::Value { + let init = + r#"{"jsonrpc":"2.0","id":1,"method":"lpp/initialize","params":{"protocolVersion":"1.6"}}"#; + let request = format!( + r#"{{"jsonrpc":"2.0","id":2,"method":"lpp/check","params":{{"entry":{{"uri":"file://{}","languageId":"x-demo-lang","version":1,"kind":"directory"}}}}}}"#, + root.display() + ); + // Directory entries need LPP 1.2+, so pin the provider's version. + let responses = run_session_args( + &["--protocol-version", "1.6"], + format!("{init}\n{request}\n").as_bytes(), + ); + responses.into_iter().nth(1).expect("check response") +} + +#[test] +fn project_loader_ignores_xdl_directories() { + // A directory named `*.xdl` is not a source file; loading must skip it + // rather than fail the whole request trying to read it. + let root = project_dir("xdl-dir"); + std::fs::write(root.join("entry.xdl"), VALID).unwrap(); + std::fs::create_dir(root.join("junk.xdl")).unwrap(); + let response = check_directory(&root); + std::fs::remove_dir_all(&root).ok(); + assert!( + response["result"].is_object(), + "expected check result, got {response}" + ); +} + +#[cfg(unix)] +#[test] +fn project_load_failure_names_the_broken_source_uri() { + // A dangling symlink canonicalizes to nothing: the failure's + // `details.uri` must name the file that could not be loaded. + let root = project_dir("dangling"); + std::fs::write(root.join("entry.xdl"), VALID).unwrap(); + std::os::unix::fs::symlink("missing-target.xdl", root.join("broken.xdl")).unwrap(); + let response = check_directory(&root); + std::fs::remove_dir_all(&root).ok(); + let lpp = &response["error"]["data"]["lpp"]; + assert_eq!(lpp["kind"], "projectLoadFailed", "got {response}"); + let uri = lpp["details"]["uri"].as_str().unwrap_or_default(); + assert!(uri.ends_with("/broken.xdl"), "details.uri: {uri}"); +} diff --git a/conformance/runner/src/main.rs b/conformance/runner/src/main.rs index 92d7d9d..031252b 100644 --- a/conformance/runner/src/main.rs +++ b/conformance/runner/src/main.rs @@ -36,9 +36,16 @@ fn main() { let mut paths: Vec = match std::fs::read_dir(&fixtures_dir) { Ok(entries) => entries - .filter_map(Result::ok) + .collect::, _>>() + .unwrap_or_else(|error| { + usage_error(&format!( + "cannot read fixtures dir '{}': {error}", + fixtures_dir.display() + )) + }) + .into_iter() .map(|entry| entry.path()) - .filter(|path| path.extension().is_some_and(|ext| ext == "json")) + .filter(|path| path.is_file() && path.extension().is_some_and(|ext| ext == "json")) .collect(), Err(error) => { usage_error(&format!( @@ -114,10 +121,10 @@ fn parse_args() -> Args { let mut iter = std::env::args().skip(1); while let Some(arg) = iter.next() { match arg.as_str() { - "--fixtures" => args.fixtures = iter.next(), - "--provider" => args.provider = iter.next(), + "--fixtures" => args.fixtures = Some(arg_value(&mut iter, "--fixtures")), + "--provider" => args.provider = Some(arg_value(&mut iter, "--provider")), "--validate-only" => args.validate_only = true, - "--scope" => args.scope = iter.next(), + "--scope" => args.scope = Some(arg_value(&mut iter, "--scope")), other => usage_error(&format!("unknown argument '{other}'")), } } @@ -132,6 +139,14 @@ fn parse_args() -> Args { args } +fn arg_value(iter: &mut impl Iterator, flag: &str) -> String { + match iter.next() { + // A missing value must not silently swallow the next flag. + Some(value) if !value.starts_with("--") => value, + _ => usage_error(&format!("{flag} requires a value")), + } +} + fn usage_error(message: &str) -> ! { eprintln!("lpp-conformance-runner: {message}"); std::process::exit(2); diff --git a/conformance/runner/src/scenario.rs b/conformance/runner/src/scenario.rs index 6c19071..9af605a 100644 --- a/conformance/runner/src/scenario.rs +++ b/conformance/runner/src/scenario.rs @@ -223,7 +223,7 @@ fn uses_project_uri(scenario: &Scenario) -> bool { scenario.steps.iter().any(|step| { let request_uses_uri = match &step.request { StepRequest::Request(request) => contains_project_uri(request), - StepRequest::RawLine(_) => false, + StepRequest::RawLine(raw) => raw.contains("${PROJECT_URI}"), }; request_uses_uri || contains_project_uri(&step.expect_response) }) diff --git a/conformance/runner/src/session.rs b/conformance/runner/src/session.rs index 62c1c16..6b7a6fc 100644 --- a/conformance/runner/src/session.rs +++ b/conformance/runner/src/session.rs @@ -34,12 +34,14 @@ pub(crate) fn run_scenario( let stdout = child.stdout.take().ok_or("provider stdout not available")?; let (response_tx, response_rx) = mpsc::channel(); + let (done_tx, done_rx) = mpsc::channel(); let reader_thread = thread::spawn(move || { for line in BufReader::new(stdout).lines().map_while(Result::ok) { if !line.trim().is_empty() && response_tx.send(line).is_err() { break; } } + let _ = done_tx.send(()); }); for (i, step) in scenario.steps.iter().enumerate() { @@ -64,16 +66,24 @@ pub(crate) fn run_scenario( )); } Err(mpsc::RecvTimeoutError::Disconnected) => { - let status = wait_exit(&mut child, EXIT_TIMEOUT); + // stdout is closed, so no response can ever arrive; reap the + // provider immediately instead of waiting out the timeout. + let status = kill(&mut child); return Err(format!( "step {i}: provider closed stdout without responding (exit status {status:?})" )); } }; - let actual: Value = serde_json::from_str(&response_line) - .map_err(|e| format!("step {i}: response is not valid JSON: {e}"))?; + let actual: Value = match serde_json::from_str(&response_line) { + Ok(actual) => actual, + Err(e) => { + kill(&mut child); + return Err(format!("step {i}: response is not valid JSON: {e}")); + } + }; let expected = substitute_project_uri(&step.expect_response, project.as_ref()); if let Err(detail) = crate::matcher::matches(&expected, &actual) { + kill(&mut child); return Err(format!( "step {i}: response mismatch at {detail}\n expected: {}\n actual: {}", serde_json::to_string_pretty(&expected).expect("serializes"), @@ -101,7 +111,22 @@ pub(crate) fn run_scenario( "expected exit code {expected_exit}, got {actual_exit}" )); } - let _ = reader_thread.join(); + // Output the provider emitted past the last step must surface. Give the + // reader a short grace period for already-buffered lines; a descendant + // can keep the pipe open, so the wait stays bounded. + const DRAIN_GRACE: Duration = Duration::from_millis(500); + let _ = done_rx.recv_timeout(DRAIN_GRACE); + let extra: Vec = response_rx.try_iter().collect(); + if !extra.is_empty() { + return Err(format!( + "provider emitted {} unexpected line(s) after the last step: {extra:?}", + extra.len() + )); + } + // Do not join the reader: a provider descendant inheriting stdout keeps + // the pipe open past the child's exit and a join could block forever. + // The thread ends on EOF or a failed send, and dies with the process. + drop(reader_thread); Ok(()) } @@ -111,7 +136,12 @@ fn step_request_line(step: &Step, project: Option<&ProjectFixture>) -> String { serde_json::to_string(&substitute_project_uri(request, project)) .expect("request serializes") } - StepRequest::RawLine(raw) => raw.clone(), + // `${PROJECT_URI}` inside a raw line substitutes literally, matching + // the request-object path. + StepRequest::RawLine(raw) => raw.replace( + "${PROJECT_URI}", + project.map_or("", |project| project.uri.as_str()), + ), } } @@ -137,3 +167,170 @@ fn kill(child: &mut Child) -> std::process::ExitStatus { let _ = child.kill(); child.wait().unwrap_or_default() } + +#[cfg(all(test, unix))] +mod tests { + use super::*; + use serde_json::json; + use std::fs; + use std::path::{Path, PathBuf}; + + /// Scratch dir for fake-provider scripts under the repo's ignored + /// `target/`; removed at test start and end so reruns stay clean. + fn scratch(name: &str) -> PathBuf { + let dir = Path::new(env!("CARGO_MANIFEST_DIR")) + .join("../../target/runner-tests") + .join(format!("{name}-{}", std::process::id())); + let _ = fs::remove_dir_all(&dir); + fs::create_dir_all(&dir).expect("create scratch dir"); + dir + } + + fn scenario(script: &Path, extra_args: Vec, steps: Vec) -> Scenario { + Scenario { + name: "test".into(), + description: None, + scope: "test".into(), + session: None, + provider_args: std::iter::once(script.to_str().unwrap().to_string()) + .chain(extra_args) + .collect(), + project_files: None, + steps, + expect_exit_code: Some(0), + } + } + + fn check_step(expect_response: Value) -> Step { + Step { + request: StepRequest::Request( + json!({"jsonrpc":"2.0","id":1,"method":"lpp/check","params":{}}), + ), + expect_response, + } + } + + /// Run the script as `/bin/sh