diff --git a/crates/omnigraph-api-types/src/lib.rs b/crates/omnigraph-api-types/src/lib.rs index d35b745d0..36900a0a5 100644 --- a/crates/omnigraph-api-types/src/lib.rs +++ b/crates/omnigraph-api-types/src/lib.rs @@ -1489,6 +1489,105 @@ pub struct FullTextIndexRebuildRequiredOutput { pub reason: String, } +/// A source position: 1-based line and column (in characters) and the byte +/// offset. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, ToSchema)] +pub struct PositionOutput { + pub line: u32, + pub column: u32, + pub byte: u32, +} + +/// Whether a suggested source edit can be applied mechanically. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, ToSchema)] +#[serde(rename_all = "snake_case")] +pub enum ApplicabilityOutput { + MachineApplicable, + NeedsReview, +} + +/// A UTF-8 byte range in the original source, with an exclusive end. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, ToSchema)] +pub struct TextEditOutput { + pub start: usize, + pub end: usize, + pub replacement: String, +} + +/// Non-overlapping edits against the original request source. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, ToSchema)] +pub struct SuggestionOutput { + pub applicability: ApplicabilityOutput, + pub edits: Vec, +} + +/// The diagnostics contract for a refused query (RFC 0047): a stable code +/// (`Q…` parse, `T…` typecheck); where the failure is, as a source position +/// or as the stage and expression when it is post-parse; what was expected or +/// violated; and one concrete fix, absent when `expected` names the decision. +/// Rides `ErrorOutput.diagnostic` as an additive detail because +/// [`ErrorCode`] is a closed compatibility contract. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, ToSchema)] +pub struct DiagnosticOutput { + pub code: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub position: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub stage: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub expression: Option, + pub expected: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub fix: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub suggestion: Option, +} + +impl From<&omnigraph_compiler::QueryDiagnostic> for DiagnosticOutput { + fn from(diagnostic: &omnigraph_compiler::QueryDiagnostic) -> Self { + Self { + code: diagnostic.code.as_str().to_string(), + position: diagnostic.position.map(|at| PositionOutput { + line: at.line, + column: at.column, + byte: at.byte, + }), + stage: diagnostic + .stage + .as_ref() + .map(|stage| stage.name.to_string()), + expression: diagnostic + .stage + .as_ref() + .and_then(|stage| stage.expression.clone()), + expected: diagnostic.message.clone(), + fix: diagnostic.fix.clone(), + suggestion: diagnostic + .suggestion + .as_ref() + .map(|suggestion| SuggestionOutput { + applicability: match suggestion.applicability { + omnigraph_compiler::Applicability::MachineApplicable => { + ApplicabilityOutput::MachineApplicable + } + omnigraph_compiler::Applicability::NeedsReview => { + ApplicabilityOutput::NeedsReview + } + }, + edits: suggestion + .edits + .iter() + .map(|edit| TextEditOutput { + start: edit.start, + end: edit.end, + replacement: edit.replacement.clone(), + }) + .collect(), + }), + } + } +} + #[derive(Debug, Clone, Serialize, Deserialize, ToSchema)] pub struct ErrorOutput { pub error: String, @@ -1546,6 +1645,34 @@ pub struct ErrorOutput { /// preserves the closed [`ErrorCode`] contract. #[serde(default, skip_serializing_if = "Option::is_none")] pub full_text_index_rebuild_required: Option, + /// Set for a refused query: the diagnostics contract's code, position or + /// stage, expectation and fix. `error` keeps the one-line rendering. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub diagnostic: Option, +} + +impl ErrorOutput { + /// An error body carrying `error` and nothing else; every typed detail is + /// absent. + pub fn message(error: impl Into) -> Self { + Self { + error: error.into(), + code: None, + merge_conflicts: Vec::new(), + published_dataset_version_conflict: None, + read_set_conflict: None, + key_conflict: None, + resource_limit: None, + blob_range: None, + external_blob_source: None, + recovery_required: None, + precondition_failure: None, + change_feed_gap: None, + change_diff_refusal: None, + full_text_index_rebuild_required: None, + diagnostic: None, + } + } } pub fn snapshot_payload( @@ -1983,6 +2110,37 @@ mod tests { use omnigraph_compiler::settings::SettingScope; use serde_json::json; + #[test] + fn diagnostic_suggestions_round_trip_and_old_payloads_remain_valid() { + let old = json!({"code": "Q002", "expected": "missing parameters", "fix": "query q()"}); + let decoded: DiagnosticOutput = serde_json::from_value(old.clone()).unwrap(); + assert_eq!(serde_json::to_value(decoded).unwrap(), old); + + let source = "query q { match { $p: Person } return { $p.name } }"; + let error = omnigraph_compiler::query::parser::parse_query(source).unwrap_err(); + let output = DiagnosticOutput::from(error.diagnostic().unwrap()); + let json = serde_json::to_value(&output).unwrap(); + assert_eq!( + json["suggestion"], + json!({ + "applicability": "machine_applicable", + "edits": [{"start": 7, "end": 7, "replacement": "()"}] + }) + ); + assert_eq!( + serde_json::from_value::(json).unwrap(), + output + ); + let mut reviewed = output; + reviewed.suggestion.as_mut().unwrap().applicability = ApplicabilityOutput::NeedsReview; + let json = serde_json::to_value(&reviewed).unwrap(); + assert_eq!(json["suggestion"]["applicability"], "needs_review"); + assert_eq!( + serde_json::from_value::(json).unwrap(), + reviewed + ); + } + /// `SettingsRequest` has one field per `request` row of the definition, /// in definition order, spelled as the row's name; a `process` row has /// none and is refused as an unknown field. diff --git a/crates/omnigraph-cli/src/helpers.rs b/crates/omnigraph-cli/src/helpers.rs index 279f05082..46b680904 100644 --- a/crates/omnigraph-cli/src/helpers.rs +++ b/crates/omnigraph-cli/src/helpers.rs @@ -482,27 +482,10 @@ pub(crate) fn precondition_failed_cli( expected: String, actual: Option, ) -> PreconditionFailedCli { - PreconditionFailedCli { - output: ErrorOutput { - error: message, - code: None, - merge_conflicts: Vec::new(), - published_dataset_version_conflict: None, - read_set_conflict: None, - key_conflict: None, - resource_limit: None, - blob_range: None, - external_blob_source: None, - recovery_required: None, - precondition_failure: Some(omnigraph_api_types::PreconditionFailureOutput { - expected, - actual, - }), - change_feed_gap: None, - change_diff_refusal: None, - full_text_index_rebuild_required: None, - }, - } + let mut output = ErrorOutput::message(message); + output.precondition_failure = + Some(omnigraph_api_types::PreconditionFailureOutput { expected, actual }); + PreconditionFailedCli { output } } pub(crate) async fn remote_json( @@ -1036,7 +1019,7 @@ pub(crate) async fn execute_query_lint( fn registry_from_serving_queries( queries: &[omnigraph_cluster::ServingQuery], graph: Option<&str>, -) -> Result { +) -> std::result::Result> { let specs: Vec = queries .iter() .filter(|q| graph.is_none_or(|g| q.graph_id == g)) @@ -1047,16 +1030,21 @@ fn registry_from_serving_queries( tool_name: None, }) .collect(); - QueryRegistry::from_specs(specs).map_err(|errors| { - color_eyre::eyre::eyre!( - "stored-query registry failed to load:\n {}", - errors - .iter() - .map(|e| e.to_string()) - .collect::>() - .join("\n ") - ) - }) + QueryRegistry::from_specs(specs) +} + +fn registry_load_issues(errors: Vec) -> Vec { + errors + .into_iter() + .map(|error| QueriesIssue { + query: error.query.unwrap_or_else(|| "".to_string()), + message: error.message, + diagnostic: error + .diagnostic + .as_deref() + .map(omnigraph_api_types::DiagnosticOutput::from), + }) + .collect() } /// `queries validate --cluster ` (RFC-011): type-check every stored query @@ -1083,7 +1071,13 @@ pub(crate) async fn execute_queries_validate( } matched_any = true; let registry = - registry_from_serving_queries(&snapshot.queries, Some(&serving_graph.graph_id))?; + match registry_from_serving_queries(&snapshot.queries, Some(&serving_graph.graph_id)) { + Ok(registry) => registry, + Err(errors) => { + breakages.extend(registry_load_issues(errors)); + continue; + } + }; let db = Omnigraph::open(&serving_graph.root.to_string_lossy()).await?; let report = check(®istry, &db.catalog()); total += registry.len(); @@ -1091,12 +1085,17 @@ pub(crate) async fn execute_queries_validate( breakages.push(QueriesIssue { query: b.query.clone(), message: b.message.clone(), + diagnostic: b + .diagnostic + .as_ref() + .map(omnigraph_api_types::DiagnosticOutput::from), }); } for w in &report.warnings { warnings.push(QueriesIssue { query: w.query.clone(), message: w.message.clone(), + diagnostic: None, }); } } @@ -1125,6 +1124,11 @@ pub(crate) async fn execute_queries_validate( } for issue in &output.breakages { println!("ERROR query '{}': {}", issue.query, issue.message); + if let Some(diagnostic) = &issue.diagnostic { + for line in crate::output::diagnostic_detail_lines(diagnostic) { + println!("{line}"); + } + } } for issue in &output.warnings { println!("WARN query '{}': {}", issue.query, issue.message); @@ -1165,7 +1169,16 @@ pub(crate) async fn execute_queries_list( json: bool, ) -> Result<()> { let snapshot = read_serving_snapshot_or_report(cluster).await?; - let registry = registry_from_serving_queries(&snapshot.queries, graph)?; + let registry = registry_from_serving_queries(&snapshot.queries, graph).map_err(|errors| { + color_eyre::eyre::eyre!( + "stored-query registry failed to load:\n {}", + errors + .iter() + .map(|e| e.to_string()) + .collect::>() + .join("\n ") + ) + })?; let output = QueriesListOutput { queries: registry @@ -1295,6 +1308,35 @@ pub(crate) fn rewrite_deprecated_argv(args: Vec) -> Vec { mod tests { use super::*; + #[test] + fn stored_query_parse_failures_keep_diagnostics_in_validation_output() { + let queries = vec![omnigraph_cluster::ServingQuery { + graph_id: "g".to_string(), + name: "broken".to_string(), + source: "query broken { match { $p: Person } return { $p.name } }".to_string(), + }]; + let failures = registry_from_serving_queries(&queries, Some("g")).unwrap_err(); + let output = QueriesValidateOutput { + ok: false, + breakages: registry_load_issues(failures), + warnings: Vec::new(), + }; + let json = serde_json::to_value(output).unwrap(); + let error = &json["breakages"][0]; + assert_eq!(error["query"], "broken"); + assert_eq!(error["diagnostic"]["code"], "Q002"); + assert_eq!(error["diagnostic"]["position"]["byte"], 12); + assert_eq!( + error["diagnostic"]["suggestion"]["edits"][0]["replacement"], + "()" + ); + assert!( + registry_from_serving_queries(&queries, Some("other")) + .unwrap() + .is_empty() + ); + } + #[test] fn graph_resource_id_for_selection_uses_name_or_anonymous_uri() { assert_eq!( diff --git a/crates/omnigraph-cli/src/main.rs b/crates/omnigraph-cli/src/main.rs index 59e79754e..4a21b722c 100644 --- a/crates/omnigraph-cli/src/main.rs +++ b/crates/omnigraph-cli/src/main.rs @@ -140,8 +140,13 @@ fn installed_file_is_current(installed: &fs::File, path: &std::path::Path) -> Re #[tokio::main] async fn main() -> Result<()> { - color_eyre::install()?; - let (cli, json) = { + // No environment or location footer: a refusal is documentation for the + // reader, and a backtrace hint is never its fix. + color_eyre::config::HookBuilder::default() + .display_env_section(false) + .display_location_section(false) + .install()?; + let (cli, machine) = { let raw_args = rewrite_deprecated_argv(std::env::args_os().collect()); let matches = Cli::command() .arg( @@ -156,23 +161,38 @@ async fn main() -> Result<()> { while let Some((_, child)) = command_matches.subcommand() { command_matches = child; } + let format = command_matches + .try_get_one::("format") + .ok() + .flatten() + .copied(); let json = command_matches .try_get_one::("json") .ok() .flatten() .copied() .unwrap_or(false) - || command_matches - .try_get_one::("format") - .ok() - .flatten() - == Some(&ReadOutputFormat::Json); - (Cli::from_arg_matches(&matches)?, json) + || format == Some(ReadOutputFormat::Json); + let machine = if json { + Some(MachineErrors::Json) + } else if format == Some(ReadOutputFormat::Jsonl) { + Some(MachineErrors::Jsonl) + } else { + None + }; + (Cli::from_arg_matches(&matches)?, machine) }; match run(cli).await { - Err(error) if json => { - if let Some(remote) = error.downcast_ref::() { - print_json(&remote.output)?; + Err(error) => { + if let Some(output) = error_output_of(&error) { + match machine { + Some(MachineErrors::Json) => print_json(&output)?, + Some(MachineErrors::Jsonl) => println!("{}", serde_json::to_string(&output)?), + None => match &output.diagnostic { + Some(diagnostic) => eprintln!("{}", render_diagnostic(&output, diagnostic)), + None => return Err(error), + }, + } std::io::stdout().flush()?; std::process::exit(1); } @@ -182,6 +202,46 @@ async fn main() -> Result<()> { } } +/// The machine error format a run's output format asks for: `--json` (and +/// `--format json`) print the error body pretty, `--format jsonl` prints it +/// as one line, like the rows it would have carried. +#[derive(Clone, Copy)] +enum MachineErrors { + Json, + Jsonl, +} + +/// The error body a failed run would carry on the wire: a served refusal's +/// own body, or an embedded compile refusal rendered the way the server +/// renders it, so `--json` output is transport-uniform. +fn error_output_of(error: &color_eyre::Report) -> Option { + if let Some(remote) = error.downcast_ref::() { + return Some(remote.output.clone()); + } + let diagnostic = if let Some(engine) = error.downcast_ref::() { + engine.diagnostic() + } else if let Some(compiler) = error.downcast_ref::() + { + compiler.diagnostic() + } else { + None + }?; + let mut output = ErrorOutput::message(error.to_string()); + output.diagnostic = Some(omnigraph_api_types::DiagnosticOutput::from(diagnostic)); + Some(output) +} + +/// The human rendering of the diagnostics contract: the code and the +/// expectation, where it is, and the one fix. +fn render_diagnostic( + output: &ErrorOutput, + diagnostic: &omnigraph_api_types::DiagnosticOutput, +) -> String { + let mut lines = vec![format!("error[{}]: {}", diagnostic.code, output.error)]; + lines.extend(output::diagnostic_detail_lines(diagnostic)); + lines.join("\n") +} + async fn run(cli: Cli) -> Result<()> { if let Some(result) = managed::dispatch(&cli).await { let code = result.emit()?; diff --git a/crates/omnigraph-cli/src/output.rs b/crates/omnigraph-cli/src/output.rs index 9c38c19c6..02bc0723b 100644 --- a/crates/omnigraph-cli/src/output.rs +++ b/crates/omnigraph-cli/src/output.rs @@ -399,6 +399,27 @@ pub(crate) fn cluster_lock_summary(state: &omnigraph_cluster::StateObservations) format!(" ({})", parts.join(", ")) } +/// Where a refused query fails and its one fix, one indented line each, to +/// print under the line that names the refusal: `--> line, column` for a +/// parse refusal, `--> stage: expression` for a later one, then `fix:`. +pub(crate) fn diagnostic_detail_lines( + diagnostic: &omnigraph_api_types::DiagnosticOutput, +) -> Vec { + let mut lines = Vec::new(); + if let Some(at) = &diagnostic.position { + lines.push(format!(" --> line {}, column {}", at.line, at.column)); + } else if let Some(stage) = &diagnostic.stage { + match &diagnostic.expression { + Some(expression) => lines.push(format!(" --> {stage}: {expression}")), + None => lines.push(format!(" --> {stage}")), + } + } + if let Some(fix) = &diagnostic.fix { + lines.push(format!(" fix: {fix}")); + } + lines +} + pub(crate) fn print_cluster_diagnostics(diagnostics: &[omnigraph_cluster::Diagnostic]) { for diagnostic in diagnostics { let label = match diagnostic.severity { @@ -409,6 +430,12 @@ pub(crate) fn print_cluster_diagnostics(diagnostics: &[omnigraph_cluster::Diagno "{label} {} {}: {}", diagnostic.code, diagnostic.path, diagnostic.message ); + if let Some(detail) = &diagnostic.detail { + let detail = omnigraph_api_types::DiagnosticOutput::from(detail.as_ref()); + for line in diagnostic_detail_lines(&detail) { + println!("{line}"); + } + } } } @@ -940,6 +967,8 @@ pub(crate) fn print_policy_explain( pub(crate) struct QueriesIssue { pub(crate) query: String, pub(crate) message: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) diagnostic: Option, } #[derive(serde::Serialize)] diff --git a/crates/omnigraph-cli/tests/cli_queries.rs b/crates/omnigraph-cli/tests/cli_queries.rs index 3475ca26b..48d1f7e72 100644 --- a/crates/omnigraph-cli/tests/cli_queries.rs +++ b/crates/omnigraph-cli/tests/cli_queries.rs @@ -1016,34 +1016,53 @@ fn queries_validate_exits_zero_on_clean_registry() { } #[test] -fn cluster_import_rejects_a_type_broken_query() { - // In the cluster model a stored query is type-checked at the cluster - // boundary (import/apply), so a broken query can never reach the applied - // state `queries validate` reads — the gate is upstream. `Widget` is not in - // the fixture schema, so import must reject it, naming the query. - let temp = tempdir().unwrap(); - let dir = temp.path(); - std::fs::copy(fixture("test.pg"), dir.join("graph.pg")).unwrap(); - write_query_file( - &dir.join("ghost.gq"), - "query ghost() { match { $w: Widget } return { $w.name } }", - ); - std::fs::write( - dir.join("cluster.yaml"), - "version: 1\nmetadata:\n name: sys\nstate:\n backend: cluster\n lock: true\n\ - graphs:\n knowledge:\n schema: ./graph.pg\n queries:\n ghost:\n file: ./ghost.gq\n", - ) - .unwrap(); - let output = output_failure(cli().arg("cluster").arg("import").arg("--config").arg(dir)); - let combined = format!( - "{}{}", - stdout_string(&output), - String::from_utf8_lossy(&output.stderr) - ); - assert!( - combined.contains("ghost"), - "cluster import must reject the broken query, naming it; got:\n{combined}" - ); +fn cluster_import_rejects_a_broken_query_naming_it_and_where() { + // In the cluster model a stored query is checked at the cluster boundary + // (import/apply), so a broken query can never reach the applied state + // `queries validate` reads — the gate is upstream. `Widget` is not in the + // fixture schema, so import must reject `ghost`, naming it; `broken` does + // not parse, and the human report also says where, from the diagnostic. + let cases: [(&str, &str, &[&str]); 2] = [ + ( + "ghost", + "query ghost() { match { $w: Widget } return { $w.name } }", + &["ghost"], + ), + ( + "broken", + "query broken() { match { $p: Person $p.age > } return { $p.name } }", + &[ + "broken: parse error: expected operand", + " --> line 1, column 46", + ], + ), + ]; + for (name, source, expected) in cases { + let temp = tempdir().unwrap(); + let dir = temp.path(); + std::fs::copy(fixture("test.pg"), dir.join("graph.pg")).unwrap(); + write_query_file(&dir.join(format!("{name}.gq")), source); + std::fs::write( + dir.join("cluster.yaml"), + format!( + "version: 1\nmetadata:\n name: sys\nstate:\n backend: cluster\n lock: true\n\ + graphs:\n knowledge:\n schema: ./graph.pg\n queries:\n {name}:\n file: ./{name}.gq\n" + ), + ) + .unwrap(); + let output = output_failure(cli().arg("cluster").arg("import").arg("--config").arg(dir)); + let combined = format!( + "{}{}", + stdout_string(&output), + String::from_utf8_lossy(&output.stderr) + ); + for needle in expected { + assert!( + combined.contains(needle), + "cluster import must reject `{name}` with {needle:?}; got:\n{combined}" + ); + } + } } #[test] @@ -1231,3 +1250,88 @@ fn queries_validate_graph_filter_selects_one_graph() { ); assert!(stdout_string(&output).contains("OK")); } + +/// RFC 0047's diagnostics contract at the CLI: a refused query reports its +/// code, position, expectation and one fix in every output format, with no +/// colour and no backtrace, on both transports. +#[test] +fn a_query_without_its_parameter_list_reports_q002_in_every_output_format() { + let (_temp, graph) = loaded_graph(); + let refused = "query name { match { $p: Person } return { $p.name } }"; + let suggestion = serde_json::json!({ + "applicability": "machine_applicable", + "edits": [{"start": 10, "end": 10, "replacement": "()"}] + }); + + let json = parse_stdout_json(&output_failure( + embedded("query", &graph) + .arg("-e") + .arg(refused) + .arg("--json"), + )); + assert_eq!( + json["error"], + "parse error: expected `(`: a query declares its parameters even when it has none" + ); + assert_eq!(json["diagnostic"]["code"], "Q002"); + assert_eq!(json["diagnostic"]["fix"], "query name()"); + assert_eq!(json["diagnostic"]["position"]["line"], 1); + assert_eq!(json["diagnostic"]["position"]["column"], 11); + assert_eq!(json["diagnostic"]["suggestion"], suggestion); + assert!( + json.get("code").is_none(), + "an embedded refusal has no HTTP code: {json}" + ); + + let jsonl = output_failure( + embedded("query", &graph) + .arg("-e") + .arg(refused) + .arg("--format") + .arg("jsonl"), + ); + let stdout = stdout_string(&jsonl); + let lines: Vec<&str> = stdout.lines().collect(); + assert_eq!( + lines.len(), + 1, + "jsonl carries the error as one line: {stdout:?}" + ); + let line: Value = serde_json::from_str(lines[0]).unwrap(); + assert_eq!(line["diagnostic"]["code"], "Q002"); + assert_eq!(line["diagnostic"]["suggestion"], suggestion); + + let human = output_failure(embedded("query", &graph).arg("-e").arg(refused)); + let stderr = stderr_string(&human); + assert!( + stderr.contains("error[Q002]: parse error: expected `(`"), + "{stderr}" + ); + assert!(stderr.contains("--> line 1, column 11"), "{stderr}"); + assert!(stderr.contains("fix: query name()"), "{stderr}"); + assert!(!stderr.contains("\u{1b}["), "no colour codes: {stderr:?}"); + assert!( + !stderr.contains("Backtrace") && !stderr.contains("Location:"), + "no backtrace footer: {stderr}" + ); + assert!( + stdout_string(&human).is_empty(), + "the human lane writes nothing to stdout" + ); + + let (_cluster, server) = served_graph(); + let json = parse_stdout_json(&output_failure( + served("query", &server) + .arg("-e") + .arg(refused) + .arg("--json"), + )); + assert_eq!(json["code"], "bad_request"); + assert_eq!(json["diagnostic"]["code"], "Q002"); + assert_eq!(json["diagnostic"]["suggestion"], suggestion); + assert_eq!(json["diagnostic"]["fix"], "query name()"); + let served_human = output_failure(served("query", &server).arg("-e").arg(refused)); + let stderr = stderr_string(&served_human); + assert!(stderr.contains("error[Q002]:"), "{stderr}"); + assert!(stderr.contains("fix: query name()"), "{stderr}"); +} diff --git a/crates/omnigraph-cluster/src/config.rs b/crates/omnigraph-cluster/src/config.rs index 920766e13..5a40ad635 100644 --- a/crates/omnigraph-cluster/src/config.rs +++ b/crates/omnigraph-cluster/src/config.rs @@ -160,11 +160,14 @@ pub(crate) fn resolve_query_decls( } }, Err(err) => { - diagnostics.push(Diagnostic::error( - "query_parse_error", - format!("graphs.{graph_id}.queries"), - format!("'{}' does not parse: {err}", resolved.display()), - )); + diagnostics.push( + Diagnostic::error( + "query_parse_error", + format!("graphs.{graph_id}.queries"), + format!("'{}' does not parse: {err}", resolved.display()), + ) + .with_detail(err.diagnostic().cloned()), + ); continue; } }; @@ -1091,42 +1094,48 @@ pub(crate) fn validate_query_source( diagnostics: &mut Vec, ) { let path = format!("graphs.{graph_id}.queries.{query_name}"); - match parse_query(source) - .map_err(|err| err.to_string()) - .and_then(stored_declarations) - { - Ok(queries) => { - let Some(query_decl) = queries.iter().find(|q| q.name == query_name) else { - diagnostics.push(Diagnostic::error( - "query_key_mismatch", - path, - format!("no `query {query_name}` declaration found in the referenced .gq file"), - )); - return; - }; - if let Some(catalog) = catalog { - if let Err(err) = typecheck_query_decl(catalog, query_decl) { - diagnostics.push(Diagnostic::error( - "query_typecheck_error", - format!("graphs.{graph_id}.queries.{query_name}"), - err.to_string(), - )); - } - } else { - diagnostics.push(Diagnostic::warning( - "query_typecheck_skipped", - format!("graphs.{graph_id}.queries.{query_name}"), - "query parsed, but type-check was skipped because the graph schema is invalid", - )); + let file = match parse_query(source) { + Ok(file) => file, + Err(err) => { + let detail = err.diagnostic().cloned(); + diagnostics.push( + Diagnostic::error("query_parse_error", path, err.to_string()).with_detail(detail), + ); + return; + } + }; + let queries = match stored_declarations(file) { + Ok(queries) => queries, + Err(message) => { + diagnostics.push(Diagnostic::error("query_parse_error", path, message)); + return; + } + }; + let Some(query_decl) = queries.iter().find(|q| q.name == query_name) else { + diagnostics.push(Diagnostic::error( + "query_key_mismatch", + path, + format!("no `query {query_name}` declaration found in the referenced .gq file"), + )); + return; + }; + match catalog { + Some(catalog) => { + if let Err(err) = typecheck_query_decl(catalog, query_decl) { + diagnostics.push( + Diagnostic::error("query_typecheck_error", path, err.to_string()) + .with_detail(err.diagnostic().cloned()), + ); } } - Err(message) => diagnostics.push(Diagnostic::error("query_parse_error", path, message)), + None => diagnostics.push(Diagnostic::warning( + "query_typecheck_skipped", + path, + "query parsed, but type-check was skipped because the graph schema is invalid", + )), } } -/// The declarations of a stored `.gq` source. A settings prefix, a branch -/// statement and a `show` statement are refused by name: a stored query runs -/// under the process defaults and is always a declaration. fn stored_declarations( file: QueryFile, ) -> Result, String> { diff --git a/crates/omnigraph-cluster/src/tests.rs b/crates/omnigraph-cluster/src/tests.rs index 4b784e231..951424a97 100644 --- a/crates/omnigraph-cluster/src/tests.rs +++ b/crates/omnigraph-cluster/src/tests.rs @@ -5357,7 +5357,11 @@ fn query_discovery_rejects_duplicates_and_parse_errors() { out.diagnostics ); - fs::write(dir.path().join("broken.gq"), "query {{{ nope").unwrap(); + fs::write( + dir.path().join("broken.gq"), + "query broken { match { $p: Person } return { $p.name } }", + ) + .unwrap(); fs::write( dir.path().join("cluster.yaml"), "version: 1\ngraphs:\n knowledge:\n schema: ./people.pg\n queries: ./broken.gq\n", @@ -5365,12 +5369,18 @@ fn query_discovery_rejects_duplicates_and_parse_errors() { .unwrap(); let out = validate_config_dir(dir.path()); assert!(!out.ok); - assert!( - out.diagnostics - .iter() - .any(|diagnostic| diagnostic.code == "query_parse_error"), - "{:?}", - out.diagnostics + let diagnostic = out + .diagnostics + .iter() + .find(|d| d.code == "query_parse_error") + .unwrap(); + let detail = diagnostic.detail.as_ref().unwrap(); + assert_eq!(detail.code.as_str(), "Q002"); + assert_eq!(detail.position.unwrap().byte, 12); + let json = serde_json::to_value(diagnostic).unwrap(); + assert_eq!( + json["detail"]["suggestion"]["edits"][0]["replacement"], + "()" ); } diff --git a/crates/omnigraph-cluster/src/types.rs b/crates/omnigraph-cluster/src/types.rs index c40266ce8..2a8daa567 100644 --- a/crates/omnigraph-cluster/src/types.rs +++ b/crates/omnigraph-cluster/src/types.rs @@ -17,6 +17,10 @@ pub struct Diagnostic { pub severity: DiagnosticSeverity, pub path: String, pub message: String, + /// The compiler's own diagnostic (code, position or stage, expectation, + /// fix) when the subject is a stored query the compiler refused. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub detail: Option>, } impl Diagnostic { @@ -30,9 +34,18 @@ impl Diagnostic { severity: DiagnosticSeverity::Error, path: path.into(), message: message.into(), + detail: None, } } + pub(crate) fn with_detail( + mut self, + detail: Option, + ) -> Self { + self.detail = detail.map(Box::new); + self + } + pub(crate) fn warning( code: impl Into, path: impl Into, @@ -43,6 +56,7 @@ impl Diagnostic { severity: DiagnosticSeverity::Warning, path: path.into(), message: message.into(), + detail: None, } } } diff --git a/crates/omnigraph-compiler/src/error.rs b/crates/omnigraph-compiler/src/error.rs index 4db03e992..8a476780b 100644 --- a/crates/omnigraph-compiler/src/error.rs +++ b/crates/omnigraph-compiler/src/error.rs @@ -1,5 +1,7 @@ use thiserror::Error; +pub use crate::query::diagnostic::{QueryCode, QueryDiagnostic}; + #[derive(Debug, Error)] pub enum SchemaIdentityError { #[error("invalid schema identity domain: {0}")] @@ -94,14 +96,19 @@ pub fn decode_string_literal(raw: &str) -> Result { #[derive(Debug, Error)] pub enum CompilerError { + /// A schema (`.pg`) parse refusal, or a string helper's; query parse + /// refusals are `Query` diagnostics. #[error("parse error: {0}")] Parse(String), #[error("catalog error: {0}")] Catalog(String), - #[error("type error: {0}")] - Type(String), + /// Every query compile diagnostic, parse (`Q…`) or typecheck (`T…`), + /// with its code, position or stage, message and fix; `Display` is the + /// diagnostic's legacy one-line form. + #[error("{0}")] + Query(Box), #[error("storage error: {0}")] Storage(String), @@ -139,6 +146,28 @@ pub enum CompilerError { SchemaIdentity(#[from] SchemaIdentityError), } +impl CompilerError { + /// A typecheck diagnostic under `code`; `message` states what was + /// expected or violated without the code prefix, which `Display` adds. + pub fn typed(code: QueryCode, message: impl Into) -> Self { + Self::Query(Box::new(QueryDiagnostic::typecheck(code, message))) + } + + /// A query compile error carrying `diagnostic`. + pub fn query(diagnostic: QueryDiagnostic) -> Self { + Self::Query(Box::new(diagnostic)) + } + + /// The structured diagnostic behind a query compile error, when this is + /// one. + pub fn diagnostic(&self) -> Option<&QueryDiagnostic> { + match self { + Self::Query(diagnostic) => Some(diagnostic), + _ => None, + } + } +} + #[deprecated(note = "use CompilerError")] pub type NanoError = CompilerError; diff --git a/crates/omnigraph-compiler/src/lib.rs b/crates/omnigraph-compiler/src/lib.rs index acc28346a..d4985fa40 100644 --- a/crates/omnigraph-compiler/src/lib.rs +++ b/crates/omnigraph-compiler/src/lib.rs @@ -44,6 +44,10 @@ pub use query::descriptor::{ QueryGraphFact, QueryGraphFactKind, QueryOperationDescriptor, QueryResultFieldDescriptor, QueryValueKind, describe_query_operation, }; +pub use query::diagnostic::{ + Applicability, Position, QueryCode, QueryDiagnostic, QueryDiagnosticKind, Stage, Suggestion, + TextEdit, +}; pub use query::lint::{ QueryLintFinding, QueryLintOutput, QueryLintQueryKind, QueryLintQueryResult, QueryLintSchemaSource, QueryLintSchemaSourceKind, QueryLintSeverity, QueryLintStatus, diff --git a/crates/omnigraph-compiler/src/query/codes.rs b/crates/omnigraph-compiler/src/query/codes.rs new file mode 100644 index 000000000..957c72716 --- /dev/null +++ b/crates/omnigraph-compiler/src/query/codes.rs @@ -0,0 +1,93 @@ +//! The catalog of query compile diagnostic codes (RFC 0047). A code is +//! stable once published: its meaning is frozen, its message text may +//! improve. `Q…` codes are the parser's, `T…` codes the typechecker's; the +//! numbering has gaps where earlier stages were withdrawn, and a withdrawn +//! number is never reused. + +use super::diagnostic::{QueryCode, QueryCodeSpec}; + +macro_rules! codes { + ($($name:ident = $short:literal;)*) => { + $(pub const $name: QueryCode = QueryCode(&QueryCodeSpec { code: stringify!($name), short: $short });)* + /// Every code in the catalog, for uniqueness and documentation checks. + pub const ALL: &[QueryCode] = &[$($name),*]; + }; +} + +codes! { + // Parser. + Q001 = "the source does not match the grammar at this position"; + Q002 = "a query declaration is missing its parameter list"; + Q003 = "a settings statement is refused"; + Q004 = "a branch, show or explain statement is misplaced or malformed"; + Q005 = "a declaration body is refused"; + + // Typechecker. + T1 = "unknown node type"; + T2 = "type has no such property"; + T3 = "a literal or binding-match value is invalid for its property, or names an undeclared parameter"; + T4 = "unknown edge type"; + T5 = "variable is not an endpoint of the traversed edge"; + T6 = "a read variable, type or property cannot be resolved"; + T7 = "assignment or comparison operands are invalid"; + T8 = "an aggregate argument is invalid for the aggregate function"; + T9 = "grouping is invalid or a negation block is uncorrelated"; + T10 = "insert has no assignment"; + T11 = "a mutation property or meta-field is unavailable in this context"; + T12 = "insert omits a required property"; + T13 = "duplicate assignment"; + T14 = "mutation variable is not a declared parameter"; + T15 = "traversal hop bounds or nearest operands are invalid"; + T16 = "update is not supported for edge types"; + T17 = "nearest ordering requires a limit"; + T18 = "alias ordering cannot be combined with nearest"; + T19 = "search or fuzzy operands are invalid"; + T20 = "match_text or bm25 operands are invalid"; + T21 = "rrf operands are invalid or its ordering lacks a limit"; + T22 = "undirected traversal needs a same-endpoint-type edge"; + T23 = "a node or edge binding is used in an incompatible role"; + T24 = "a Blob property is not a read value"; + T25 = "two projections produce the same result column"; + T32 = "a retrieval cannot sit under an aggregate"; + T33 = "a projected rank must repeat the executed retrieval"; + T35 = "a search predicate cannot be projected"; + T36 = "an alias cannot be projected"; + T37 = "rrf cannot be projected"; + T38 = "a search predicate must stand alone or be compared with true"; + T39 = "an exists or aggregate block must reference an outer-bound variable"; + T40 = "an aggregate block argument or comparison value is invalid"; + T41 = "a filter or Boolean expression has invalid operands"; + T42 = "an order key must appear in return or name an alias"; + T43 = "a Boolean expression in return needs an alias"; + T44 = "a call is not allowed in a mutation condition, assignment or binding match"; + T45 = "a property value cannot appear where a constant is required"; + T46 = "a Boolean literal where a bare property name was meant"; + T47 = "parameter name is reserved"; + T48 = "unknown parameter type"; + T49 = "variable name is reserved for the compiler"; + T50 = "variable is rebound to another type"; + T51 = "unknown type"; + T52 = "nested list literals are not supported"; + T53 = "list literal elements must share one scalar type"; +} + +#[cfg(test)] +mod tests { + use super::ALL; + use std::collections::BTreeSet; + + #[test] + fn codes_are_unique_and_well_formed() { + let mut seen = BTreeSet::new(); + for code in ALL { + assert!(seen.insert(code.as_str()), "duplicate code {code}"); + let (prefix, number) = code.as_str().split_at(1); + assert!(matches!(prefix, "Q" | "T"), "{code}: unknown namespace"); + assert!( + number.parse::().is_ok(), + "{code}: the number must parse" + ); + assert!(!code.short().is_empty(), "{code}: empty short"); + } + } +} diff --git a/crates/omnigraph-compiler/src/query/descriptor.rs b/crates/omnigraph-compiler/src/query/descriptor.rs index c3de874be..1ce67cc18 100644 --- a/crates/omnigraph-compiler/src/query/descriptor.rs +++ b/crates/omnigraph-compiler/src/query/descriptor.rs @@ -101,7 +101,7 @@ fn describe_checked_query_operation( } CheckedQuery::Mutation(ctx) => { if ctx.targets.len() != query.mutations.len() { - return Err(CompilerError::Type( + return Err(CompilerError::Plan( "typechecked mutation target count does not match the query".to_string(), )); } @@ -113,7 +113,7 @@ fn describe_checked_query_operation( match (mutation, target) { (Mutation::Insert(_), MutationTarget::Edge { type_name }) => { let edge = catalog.edge_types.get(type_name).ok_or_else(|| { - CompilerError::Type(format!( + CompilerError::Plan(format!( "typechecked edge type `{type_name}` is absent from the catalog" )) })?; @@ -160,7 +160,7 @@ fn collect_clause_reads( let edge = catalog .lookup_edge_by_name(&traversal.edge_name) .ok_or_else(|| { - CompilerError::Type(format!( + CompilerError::Plan(format!( "typechecked edge type `{}` is absent from the catalog", traversal.edge_name )) @@ -213,7 +213,7 @@ fn result_value_shape( match data_type { DataType::FixedSizeList(field, dim) if field.data_type() == &DataType::Float32 => { let dim = u32::try_from(*dim).map_err(|_| { - CompilerError::Type(format!( + CompilerError::Plan(format!( "query result vector dimension `{dim}` cannot be represented" )) })?; @@ -222,7 +222,7 @@ fn result_value_shape( DataType::List(field) => { let (item_kind, nested_item, vector_dim) = result_value_shape(field.data_type())?; if nested_item.is_some() || item_kind == QueryValueKind::List { - return Err(CompilerError::Type( + return Err(CompilerError::Plan( "nested query result lists are not representable".to_string(), )); } @@ -243,7 +243,7 @@ fn scalar_result_kind(data_type: &DataType) -> Result { DataType::Date32 => Ok(QueryValueKind::Date), DataType::Date64 => Ok(QueryValueKind::DateTime), DataType::LargeBinary => Ok(QueryValueKind::Blob), - other => Err(CompilerError::Type(format!( + other => Err(CompilerError::Plan(format!( "query result data type `{other}` is not representable" ))), } diff --git a/crates/omnigraph-compiler/src/query/diagnostic.rs b/crates/omnigraph-compiler/src/query/diagnostic.rs new file mode 100644 index 000000000..b5165d29c --- /dev/null +++ b/crates/omnigraph-compiler/src/query/diagnostic.rs @@ -0,0 +1,251 @@ +//! The one shape every query compile diagnostic takes (RFC 0047, "Diagnostics +//! contract"): a stable code, where the failure is (a source position, or the +//! stage and expression when it is post-parse), what was expected or +//! violated, and one concrete fix. The reader is an agent that treats an +//! error as the documentation it acts on, so the fix names the construct to +//! use, not the rule that was broken; a diagnostic with no fix names the +//! decision in its message instead. +//! +//! `Display` renders the legacy one-line form (`parse error: …`, +//! `type error: T33: …`) and nothing else, so every existing assertion on the +//! rendered text keeps holding; the other fields travel as data. + +use serde::Serialize; + +/// The catalog entry behind a [`QueryCode`]: the stable code string and its +/// one-line meaning. +#[derive(Debug, PartialEq, Eq)] +pub struct QueryCodeSpec { + pub code: &'static str, + pub short: &'static str, +} + +/// A stable diagnostic code, a pointer-sized handle to its catalog entry so +/// a diagnostic stays small enough to travel in a `Result`. Parse codes are +/// `Q…`, typecheck codes are `T…`; the catalog is `super::codes`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct QueryCode(pub &'static QueryCodeSpec); + +impl QueryCode { + /// The code string, e.g. `T33`. + pub fn as_str(self) -> &'static str { + self.0.code + } + + /// The code's one-line meaning. + pub fn short(self) -> &'static str { + self.0.short + } +} + +impl Serialize for QueryCode { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(self.as_str()) + } +} + +impl std::fmt::Display for QueryCode { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +/// Which compiler phase refused the source; decides the rendered prefix. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum QueryDiagnosticKind { + Parse, + Type, +} + +/// A source position: 1-based line and column (in characters) plus the byte +/// offset the parser reported (saturated; a query source is never that long). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +pub struct Position { + pub line: u32, + pub column: u32, + pub byte: u32, +} + +impl Position { + /// The line and column of byte offset `byte` in `source`; an offset past + /// the end positions at the end. + pub fn at(source: &str, byte: usize) -> Self { + let byte = byte.min(source.len()); + let prefix = source.get(..byte).unwrap_or(source); + let line = prefix.matches('\n').count() + 1; + let column = prefix + .rsplit('\n') + .next() + .map_or(0, |last| last.chars().count()) + + 1; + Self { + line: u32::try_from(line).unwrap_or(u32::MAX), + column: u32::try_from(column).unwrap_or(u32::MAX), + byte: u32::try_from(byte).unwrap_or(u32::MAX), + } + } +} + +/// Where a post-parse failure sits: the stage that refused and, when the +/// site can render it, the expression it refused. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct Stage { + pub name: &'static str, + #[serde(skip_serializing_if = "Option::is_none")] + pub expression: Option, +} + +/// Whether the compiler has enough evidence to apply the edits mechanically. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum Applicability { + MachineApplicable, + NeedsReview, +} + +/// Replace `start..end` in the original UTF-8 source; an empty range inserts. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct TextEdit { + pub start: usize, + pub end: usize, + pub replacement: String, +} + +/// Edits use non-overlapping byte ranges in the same original source. +/// Applying a suggestion may reveal further errors; it is not query validation. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct Suggestion { + pub applicability: Applicability, + pub edits: Vec, +} + +/// One query compile diagnostic. See the module documentation for the +/// contract each field carries. +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct QueryDiagnostic { + pub kind: QueryDiagnosticKind, + pub code: QueryCode, + /// What was expected or violated, one line, without a position. + pub message: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub position: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub stage: Option>, + /// One concrete fix, naming the construct to use. + #[serde(skip_serializing_if = "Option::is_none")] + pub fix: Option, + /// Optional edits for the original source, separate from human guidance. + #[serde(skip_serializing_if = "Option::is_none")] + pub suggestion: Option>, +} + +impl QueryDiagnostic { + /// A parse diagnostic at `position` (the caller resolves the byte offset + /// against the source it parsed). + pub fn parse(code: QueryCode, message: impl Into, position: Option) -> Self { + Self { + kind: QueryDiagnosticKind::Parse, + code, + message: message.into(), + position, + stage: None, + fix: None, + suggestion: None, + } + } + + /// A typecheck diagnostic: post-parse, so it carries the stage and no + /// position. + pub fn typecheck(code: QueryCode, message: impl Into) -> Self { + Self { + kind: QueryDiagnosticKind::Type, + code, + message: message.into(), + position: None, + stage: Some(Box::new(Stage { + name: "typecheck", + expression: None, + })), + fix: None, + suggestion: None, + } + } + + pub fn with_fix(mut self, fix: impl Into) -> Self { + self.fix = Some(fix.into()); + self + } + + pub fn with_suggestion(mut self, suggestion: Suggestion) -> Self { + self.suggestion = Some(Box::new(suggestion)); + self + } + + pub fn with_expression(mut self, expression: impl Into) -> Self { + let expression = expression.into(); + match &mut self.stage { + Some(stage) => stage.expression = Some(expression), + None => { + self.stage = Some(Box::new(Stage { + name: "typecheck", + expression: Some(expression), + })) + } + } + self + } +} + +impl std::fmt::Display for QueryDiagnostic { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self.kind { + QueryDiagnosticKind::Parse => write!(f, "parse error: {}", self.message), + QueryDiagnosticKind::Type => { + write!(f, "type error: {}: {}", self.code, self.message) + } + } + } +} + +impl std::error::Error for QueryDiagnostic {} + +#[cfg(test)] +mod tests { + use super::*; + use crate::query::codes::{Q001, T1}; + + #[test] + fn position_counts_lines_and_character_columns() { + let source = "set a = 1;\nquery é() {"; + let at = Position::at(source, source.find("()").unwrap()); + assert_eq!((at.line, at.column), (2, 8)); + let end = Position::at(source, usize::MAX); + assert_eq!(end.byte as usize, source.len()); + } + + #[test] + fn display_keeps_the_legacy_one_line_forms() { + let parse = QueryDiagnostic::parse(Q001, "expected `(`", Some(Position::at("x", 0))); + assert_eq!(parse.to_string(), "parse error: expected `(`"); + let typed = QueryDiagnostic::typecheck(T1, "unknown node type `X`").with_fix("declare X"); + assert_eq!(typed.to_string(), "type error: T1: unknown node type `X`"); + } + + #[test] + fn a_diagnostic_fits_a_result_without_boxing() { + // Clippy's `result_large_err` threshold; the parser returns this type + // from every helper. + assert!(std::mem::size_of::() < 128); + } + + #[test] + fn serializes_the_code_as_its_string_and_omits_absent_fields() { + let value = serde_json::to_value(QueryDiagnostic::typecheck(T1, "m")).unwrap(); + assert_eq!(value["code"], "T1"); + assert_eq!(value["stage"]["name"], "typecheck"); + assert!(value.get("position").is_none()); + assert!(value.get("fix").is_none()); + assert!(value.get("suggestion").is_none()); + } +} diff --git a/crates/omnigraph-compiler/src/query/lint.rs b/crates/omnigraph-compiler/src/query/lint.rs index b146e2462..ca7e82131 100644 --- a/crates/omnigraph-compiler/src/query/lint.rs +++ b/crates/omnigraph-compiler/src/query/lint.rs @@ -3,9 +3,9 @@ use std::collections::{BTreeMap, BTreeSet}; use serde::Serialize; use crate::catalog::Catalog; -use crate::error::ParseDiagnostic; use crate::query::ast::{Mutation, QueryDecl}; use crate::query::descriptor::{QueryOperationDescriptor, describe_query_operation}; +use crate::query::diagnostic::QueryDiagnostic; use crate::query::parser::parse_query_diagnostic; const PARSE_ERROR_CODE: &str = "Q000"; @@ -123,7 +123,7 @@ pub fn lint_query_file( let query_path = query_path.into(); let parsed = match parse_query_diagnostic(query_source) { Ok(file) => file.body.into_read_declarations(), - Err(diagnostic) => Err(parse_error_message(query_source, diagnostic)), + Err(diagnostic) => Err(parse_error_message(diagnostic)), }; match parsed { Ok(queries) => { @@ -223,19 +223,14 @@ pub fn lint_query_file( /// The parse message with its position, `line , column : `, /// so a refused `set` line is reported where it stands; a spanless /// diagnostic keeps its message. -fn parse_error_message(query_source: &str, diagnostic: ParseDiagnostic) -> String { - let ParseDiagnostic { message, span } = diagnostic; - let Some(span) = span else { - return message; - }; - let prefix = query_source.get(..span.start).unwrap_or(query_source); - let line = prefix.matches('\n').count() + 1; - let column = prefix - .rsplit('\n') - .next() - .map_or(0, |last| last.chars().count()) - + 1; - format!("line {line}, column {column}: {message}") +fn parse_error_message(diagnostic: QueryDiagnostic) -> String { + let QueryDiagnostic { + message, position, .. + } = diagnostic; + match position { + Some(at) => format!("line {}, column {}: {message}", at.line, at.column), + None => message, + } } fn query_kind(query: &QueryDecl) -> QueryLintQueryKind { diff --git a/crates/omnigraph-compiler/src/query/mod.rs b/crates/omnigraph-compiler/src/query/mod.rs index fdd33d639..b8c2e246b 100644 --- a/crates/omnigraph-compiler/src/query/mod.rs +++ b/crates/omnigraph-compiler/src/query/mod.rs @@ -1,5 +1,7 @@ pub mod ast; +pub mod codes; pub mod descriptor; +pub mod diagnostic; pub mod lint; pub mod parser; pub mod typecheck; diff --git a/crates/omnigraph-compiler/src/query/parser.rs b/crates/omnigraph-compiler/src/query/parser.rs index 566e56432..d45d30d86 100644 --- a/crates/omnigraph-compiler/src/query/parser.rs +++ b/crates/omnigraph-compiler/src/query/parser.rs @@ -1,13 +1,13 @@ use pest::Parser; -use pest::error::InputLocation; +use pest::error::{ErrorVariant, InputLocation}; use pest_derive::Parser; -use crate::error::{ - CompilerError, ParseDiagnostic, Result, SourceSpan, decode_string_literal, render_span, -}; +use crate::error::{CompilerError, Result, decode_string_literal}; use crate::settings::{SessionSettings, SessionSettingsError, SettingId, SettingValue}; use super::ast::*; +use super::codes::{Q001, Q002, Q003, Q004, Q005}; +use super::diagnostic::{Applicability, Position, QueryDiagnostic, Suggestion, TextEdit}; #[derive(Parser)] #[grammar = "query/query.pest"] @@ -32,20 +32,26 @@ impl NameScope<'_> { } } -fn reserved_property_error(word: &str) -> CompilerError { - CompilerError::Parse(format!( - "`{word}` is a reserved word; a property of that name is written `$p.{word}` in a read and cannot be named bare in a mutation `where`" - )) +fn reserved_property_error(at: &pest::iterators::Pair) -> CompilerError { + let word = at.as_str(); + query_error_at( + format!( + "`{word}` is a reserved word; a property of that name is written `$p.{word}` in a read and cannot be named bare in a mutation `where`" + ), + at, + ) } -fn reserved_alias_error(word: &str) -> CompilerError { - CompilerError::Parse(format!( - "`{word}` is a reserved word and cannot be a return alias" - )) +fn reserved_alias_error(at: &pest::iterators::Pair) -> CompilerError { + let word = at.as_str(); + query_error_at( + format!("`{word}` is a reserved word and cannot be a return alias"), + at, + ) } pub fn parse_query(input: &str) -> Result { - parse_query_diagnostic(input).map_err(|e| CompilerError::Parse(e.to_string())) + parse_query_diagnostic(input).map_err(CompilerError::query) } /// Whether `input` opens with a settings statement: `set` or `reset` as the @@ -77,8 +83,10 @@ fn skip_trivia(mut input: &str) -> &str { } } -pub fn parse_query_diagnostic(input: &str) -> std::result::Result { - let pairs = QueryParser::parse(Rule::query_file, input).map_err(pest_error_to_diagnostic)?; +/// Parse `input` into its file body, refusing with a positioned diagnostic. +pub fn parse_query_diagnostic(input: &str) -> std::result::Result { + let pairs = QueryParser::parse(Rule::query_file, input) + .map_err(|error| pest_error_to_diagnostic(input, error))?; let mut settings = Vec::new(); let mut queries = Vec::new(); @@ -103,14 +111,49 @@ pub fn parse_query_diagnostic(input: &str) -> std::result::Result "an `explain` statement", _ => "a branch statement", }; - return Err(ParseDiagnostic::new( + return Err(diagnostic_at( + Q004, format!("{subject} stands alone in its file"), - Some(pair_span(&inner)), + &inner, )); } + Rule::missing_param_list => { + let name = inner + .clone() + .into_inner() + .find(|part| part.as_rule() == Rule::ident) + .expect("grammar: missing_param_list holds an ident"); + let byte = name.as_span().end(); + let mut diagnostic = QueryDiagnostic::parse( + Q002, + "expected `(`: a query declares its parameters even when it has none", + Some(Position::at(input, byte)), + ) + .with_fix(format!("query {}()", name.as_str())); + let mut repaired = input.to_string(); + repaired.insert_str(byte, "()"); + if QueryParser::parse(Rule::query_file, &repaired).is_ok_and(|pairs| { + !pairs + .flatten() + .any(|p| p.as_rule() == Rule::missing_param_list) + }) { + diagnostic = diagnostic.with_suggestion(Suggestion { + applicability: Applicability::MachineApplicable, + edits: vec![TextEdit { + start: byte, + end: byte, + replacement: "()".to_string(), + }], + }); + } + return Err(diagnostic); + } Rule::query_decl => { - queries - .push(parse_query_decl(inner).map_err(compiler_error_to_diagnostic)?); + let position = position_of(&inner); + queries.push( + parse_query_decl(inner) + .map_err(|error| compiler_error_to_diagnostic(error, position))?, + ); } _ => {} } @@ -125,22 +168,36 @@ pub fn parse_query_diagnostic(input: &str) -> std::result::Result, -) -> std::result::Result { +) -> std::result::Result { let decl = pair .into_inner() .find(|inner| inner.as_rule() == Rule::query_decl) .expect("grammar: explain_stmt holds one query_decl after kw_explain"); - parse_query_decl(decl).map_err(compiler_error_to_diagnostic) + let position = position_of(&decl); + parse_query_decl(decl).map_err(|error| compiler_error_to_diagnostic(error, position)) } -fn pair_span(pair: &pest::iterators::Pair) -> SourceSpan { +/// The position of `pair`'s start in the source it was parsed from. +fn position_of(pair: &pest::iterators::Pair) -> Position { let span = pair.as_span(); - render_span(SourceSpan::new(span.start(), span.end())) + Position::at(span.get_input(), span.start()) +} + +fn diagnostic_at( + code: super::diagnostic::QueryCode, + message: impl Into, + at: &pest::iterators::Pair, +) -> QueryDiagnostic { + QueryDiagnostic::parse(code, message, Some(position_of(at))) +} + +fn query_error_at(message: impl Into, at: &pest::iterators::Pair) -> CompilerError { + CompilerError::query(diagnostic_at(Q005, message, at)) } fn parse_setting_stmt( pair: pest::iterators::Pair, -) -> std::result::Result { +) -> std::result::Result { let form = pair .into_inner() .next() @@ -173,7 +230,7 @@ fn parse_setting_stmt( /// The `` or `all` after `reset` or `show`. fn parse_setting_target( pair: pest::iterators::Pair, -) -> std::result::Result, ParseDiagnostic> { +) -> std::result::Result, QueryDiagnostic> { let target = pair .into_inner() .find(|inner| matches!(inner.as_rule(), Rule::kw_all | Rule::setting_name)) @@ -187,14 +244,14 @@ fn parse_setting_target( fn parse_setting_id( name: &pest::iterators::Pair, -) -> std::result::Result { +) -> std::result::Result { SettingId::parse(name.as_str()).map_err(|error| setting_diagnostic(error, name)) } fn parse_setting_value( id: SettingId, pair: &pest::iterators::Pair, -) -> std::result::Result { +) -> std::result::Result { let token = pair .clone() .into_inner() @@ -215,9 +272,9 @@ fn parse_setting_value( ) }), Rule::ident => Ok(SettingValue::Ident(token.as_str().to_string())), - Rule::string_lit => parse_string_lit(token.as_str()) + Rule::string_lit => decode_string_literal(token.as_str()) .map(SettingValue::Str) - .map_err(compiler_error_to_diagnostic), + .map_err(|error| diagnostic_at(Q003, parse_error_message(error), &token)), other => unreachable!("grammar: setting_value admits no {other:?}"), } } @@ -225,13 +282,13 @@ fn parse_setting_value( fn setting_diagnostic( error: SessionSettingsError, at: &pest::iterators::Pair, -) -> ParseDiagnostic { - ParseDiagnostic::new(error.to_string(), Some(pair_span(at))) +) -> QueryDiagnostic { + diagnostic_at(Q003, error.to_string(), at) } fn parse_branch_stmt( pair: pest::iterators::Pair, -) -> std::result::Result { +) -> std::result::Result { let form = pair .into_inner() .find(|inner| inner.as_rule() != Rule::kw_branch) @@ -241,7 +298,7 @@ fn parse_branch_stmt( .into_inner() .filter(|inner| inner.as_rule() == Rule::branch_name) .map(parse_branch_name) - .collect::, ParseDiagnostic>>()?; + .collect::, QueryDiagnostic>>()?; let mut names = names.into_iter(); match (rule, names.next(), names.next()) { (Rule::branch_create, Some(name), from) => { @@ -262,56 +319,88 @@ fn parse_branch_stmt( fn parse_branch_name( pair: pest::iterators::Pair, -) -> std::result::Result { - let span = pair_span(&pair); +) -> std::result::Result { + let position = position_of(&pair); let token = pair .into_inner() .next() .expect("grammar: branch_name wraps an ident or a string_lit"); let name = match token.as_rule() { - Rule::string_lit => { - parse_string_lit(token.as_str()).map_err(compiler_error_to_diagnostic)? - } + Rule::string_lit => decode_string_literal(token.as_str()) + .map_err(|error| diagnostic_at(Q004, parse_error_message(error), &token))?, Rule::ident => token.as_str().to_string(), other => unreachable!("grammar: branch_name admits no {other:?}"), }; if name.chars().any(char::is_control) { - return Err(ParseDiagnostic::new( + return Err(QueryDiagnostic::parse( + Q004, format!("branch name {name:?} contains a control character"), - Some(span), + Some(position), )); } if name.trim() != name { - return Err(ParseDiagnostic::new( + return Err(QueryDiagnostic::parse( + Q004, format!("branch name {name:?} has leading or trailing whitespace"), - Some(span), + Some(position), )); } if name.is_empty() { - return Err(ParseDiagnostic::new( + return Err(QueryDiagnostic::parse( + Q004, format!("branch name {name:?} cannot be empty"), - Some(span), + Some(position), )); } Ok(name) } -fn pest_error_to_diagnostic(err: pest::error::Error) -> ParseDiagnostic { - let span = match err.location { - InputLocation::Pos(pos) => Some(render_span(SourceSpan::new(pos, pos))), - InputLocation::Span((start, end)) => Some(render_span(SourceSpan::new(start, end))), +/// A grammar mismatch: positioned at pest's deepest failure, naming the +/// rules it expected there (pest tracks attempts per rule, so the names are +/// the grammar's, not tokens). +fn pest_error_to_diagnostic(input: &str, err: pest::error::Error) -> QueryDiagnostic { + let byte = match err.location { + InputLocation::Pos(pos) => pos, + InputLocation::Span((start, _)) => start, }; - ParseDiagnostic::new(err.to_string(), span) + let message = match &err.variant { + ErrorVariant::ParsingError { + positives, + negatives, + } => { + let names = |rules: &[Rule]| { + rules + .iter() + .map(|rule| format!("{rule:?}")) + .collect::>() + .join(", ") + }; + match (positives.is_empty(), negatives.is_empty()) { + (false, _) => format!("expected {}", names(positives)), + (true, false) => format!("unexpected {}", names(negatives)), + (true, true) => "unexpected input".to_string(), + } + } + ErrorVariant::CustomError { message } => message.clone(), + }; + QueryDiagnostic::parse(Q001, message, Some(Position::at(input, byte))) } -/// A parse refusal keeps its bare message: `parse_query` adds the one -/// `parse error:` prefix when it wraps the diagnostic. -fn compiler_error_to_diagnostic(err: CompilerError) -> ParseDiagnostic { - let message = match err { +/// A declaration body the hand-written parser refused: its bare message +/// names the construct and `Display` adds the one `parse error:` prefix; a +/// diagnostic it already carries passes through. +fn compiler_error_to_diagnostic(err: CompilerError, position: Position) -> QueryDiagnostic { + match err { + CompilerError::Query(diagnostic) => *diagnostic, + other => QueryDiagnostic::parse(Q005, parse_error_message(other), Some(position)), + } +} + +fn parse_error_message(error: CompilerError) -> String { + match error { CompilerError::Parse(message) => message, other => other.to_string(), - }; - ParseDiagnostic::new(message, None) + } } fn parse_query_decl(pair: pest::iterators::Pair) -> Result { @@ -337,22 +426,29 @@ fn parse_query_decl(pair: pest::iterators::Pair) -> Result { } } Rule::query_annotation => { + let at = item.clone(); let (annotation_name, value) = parse_query_annotation(item)?; match annotation_name { "description" => { if description.replace(value).is_some() { - return Err(CompilerError::Parse(format!( - "query `{}` cannot include duplicate @description annotations", - name - ))); + return Err(query_error_at( + format!( + "query `{}` cannot include duplicate @description annotations", + name + ), + &at, + )); } } "instruction" => { if instruction.replace(value).is_some() { - return Err(CompilerError::Parse(format!( - "query `{}` cannot include duplicate @instruction annotations", - name - ))); + return Err(query_error_at( + format!( + "query `{}` cannot include duplicate @instruction annotations", + name + ), + &at, + )); } } other => { @@ -396,7 +492,10 @@ fn parse_query_decl(pair: pest::iterators::Pair) -> Result { let int_pair = section.into_inner().next().unwrap(); limit = Some(int_pair.as_str().parse::().map_err(|e| { - CompilerError::Parse(format!("invalid limit: {}", e)) + query_error_at( + format!("invalid limit: {}", e), + &int_pair, + ) })?); } _ => {} @@ -448,7 +547,7 @@ fn parse_query_annotation(pair: pest::iterators::Pair) -> Result<(&'static .ok_or_else(|| { CompilerError::Parse("@description requires a string literal".to_string()) }) - .map(|value| parse_string_lit(value.as_str()))??; + .map(|value| parse_string_lit(&value))??; Ok(("description", value)) } Rule::instruction_annotation => { @@ -458,7 +557,7 @@ fn parse_query_annotation(pair: pest::iterators::Pair) -> Result<(&'static .ok_or_else(|| { CompilerError::Parse("@instruction requires a string literal".to_string()) }) - .map(|value| parse_string_lit(value.as_str()))??; + .map(|value| parse_string_lit(&value))??; Ok(("instruction", value)) } other => Err(CompilerError::Parse(format!( @@ -775,7 +874,7 @@ fn parse_edge_name(pair: pest::iterators::Pair) -> Result { if pair.as_rule() != Rule::string_lit { return Ok(pair.as_str().to_string()); } - let name = parse_string_lit(pair.as_str())?; + let name = parse_string_lit(&pair)?; let mut chars = name.chars(); let starts = chars .next() @@ -783,27 +882,31 @@ fn parse_edge_name(pair: pest::iterators::Pair) -> Result { if starts && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') { Ok(name) } else { - Err(CompilerError::Parse(format!( - "`{}` is not an edge name; a quoted edge is its bare name in quotes, as in `$a \"in\" $b`", - pair.as_str() - ))) + Err(query_error_at( + format!( + "`{}` is not an edge name; a quoted edge is its bare name in quotes, as in `$a \"in\" $b`", + pair.as_str() + ), + &pair, + )) } } fn parse_traversal_bounds(pair: pest::iterators::Pair) -> Result<(u32, Option)> { let mut inner = pair.into_inner(); - let min = inner + let min_pair = inner .next() - .ok_or_else(|| CompilerError::Parse("traversal bound missing min hop".to_string()))? + .ok_or_else(|| CompilerError::Parse("traversal bound missing min hop".to_string()))?; + let min = min_pair .as_str() .parse::() - .map_err(|e| CompilerError::Parse(format!("invalid traversal min bound: {}", e)))?; + .map_err(|e| query_error_at(format!("invalid traversal min bound: {}", e), &min_pair))?; let max = inner .next() .map(|p| { p.as_str() .parse::() - .map_err(|e| CompilerError::Parse(format!("invalid traversal max bound: {}", e))) + .map_err(|e| query_error_at(format!("invalid traversal max bound: {}", e), &p)) }) .transpose()?; Ok((min, max)) @@ -926,7 +1029,7 @@ fn parse_operand(pair: pest::iterators::Pair, scope: NameScope<'_>) -> Res Rule::bm25_call => parse_bm25_call(inner, scope), Rule::rrf_call => parse_rrf_call(inner, scope), Rule::meta_field | Rule::expr_ident => Ok(scope.bare_name(inner.as_str())), - Rule::reserved_property => Err(reserved_property_error(inner.as_str())), + Rule::reserved_property => Err(reserved_property_error(&inner)), _ => Err(CompilerError::Parse(format!( "unexpected operand rule: {:?}", inner.as_rule() @@ -1082,19 +1185,19 @@ fn parse_filter_op(pair: pest::iterators::Pair) -> Result { fn parse_literal(pair: pest::iterators::Pair) -> Result { let inner = pair.into_inner().next().unwrap(); match inner.as_rule() { - Rule::string_lit => Ok(Literal::String(parse_string_lit(inner.as_str())?)), + Rule::string_lit => Ok(Literal::String(parse_string_lit(&inner)?)), Rule::integer => { let n: i64 = inner .as_str() .parse() - .map_err(|e| CompilerError::Parse(format!("invalid integer: {}", e)))?; + .map_err(|e| query_error_at(format!("invalid integer: {}", e), &inner))?; Ok(Literal::Integer(n)) } Rule::float_lit => { let f: f64 = inner .as_str() .parse() - .map_err(|e| CompilerError::Parse(format!("invalid float: {}", e)))?; + .map_err(|e| query_error_at(format!("invalid float: {}", e), &inner))?; Ok(Literal::Float(f)) } Rule::bool_lit => { @@ -1114,7 +1217,7 @@ fn parse_literal(pair: pest::iterators::Pair) -> Result { let date_str = inner .into_inner() .next() - .map(|s| parse_string_lit(s.as_str())) + .map(|s| parse_string_lit(&s)) .ok_or_else(|| { CompilerError::Parse("date literal requires a string".to_string()) })?; @@ -1124,7 +1227,7 @@ fn parse_literal(pair: pest::iterators::Pair) -> Result { let dt_str = inner .into_inner() .next() - .map(|s| parse_string_lit(s.as_str())) + .map(|s| parse_string_lit(&s)) .ok_or_else(|| { CompilerError::Parse("datetime literal requires a string".to_string()) })?; @@ -1146,8 +1249,9 @@ fn parse_literal(pair: pest::iterators::Pair) -> Result { } } -fn parse_string_lit(raw: &str) -> Result { - decode_string_literal(raw) +fn parse_string_lit(at: &pest::iterators::Pair) -> Result { + decode_string_literal(at.as_str()) + .map_err(|error| query_error_at(parse_error_message(error), at)) } fn parse_projection(pair: pest::iterators::Pair) -> Result { @@ -1156,7 +1260,7 @@ fn parse_projection(pair: pest::iterators::Pair) -> Result { let alias = match inner.next() { None => None, Some(alias) if alias.as_rule() == Rule::reserved_alias => { - return Err(reserved_alias_error(alias.as_str())); + return Err(reserved_alias_error(&alias)); } Some(alias) => Some(alias.as_str().to_string()), }; @@ -1173,13 +1277,16 @@ fn parse_ordering(pair: pest::iterators::Pair) -> Result { Rule::nearest_ordering => (parse_nearest_ordering(first, NameScope::Alias)?, false), Rule::expr => { let expr = parse_expr(first, NameScope::Alias)?; - let direction = inner.next().map(|p| p.as_str().to_string()); - if matches!(expr, Expr::Nearest { .. }) && direction.is_some() { - return Err(CompilerError::Parse( - "nearest() ordering does not accept asc/desc modifiers".to_string(), + let direction = inner.next(); + if matches!(expr, Expr::Nearest { .. }) + && let Some(direction) = &direction + { + return Err(query_error_at( + "nearest() ordering does not accept asc/desc modifiers", + direction, )); } - let descending = matches!(direction.as_deref(), Some("desc")); + let descending = direction.is_some_and(|p| p.as_str() == "desc"); (expr, descending) } other => { diff --git a/crates/omnigraph-compiler/src/query/parser_tests.rs b/crates/omnigraph-compiler/src/query/parser_tests.rs index 065fc5630..f7ea1fe4b 100644 --- a/crates/omnigraph-compiler/src/query/parser_tests.rs +++ b/crates/omnigraph-compiler/src/query/parser_tests.rs @@ -1420,7 +1420,7 @@ return { $p.name } "#; let err = parse_query_diagnostic(input).unwrap_err(); - assert!(err.span.is_some()); + assert!(err.position.is_some()); } #[test] @@ -1657,7 +1657,12 @@ fn explain_statement_never_shares_a_file_with_a_declaration() { format!("branch list\nexplain {decl}"), ] { let error = parse_query(&input).unwrap_err(); - assert!(matches!(error, CompilerError::Parse(_)), "{input}: {error}"); + assert!( + error.diagnostic().is_some_and(|diagnostic| { + diagnostic.kind == crate::query::diagnostic::QueryDiagnosticKind::Parse + }), + "{input}: {error}" + ); } } @@ -1672,7 +1677,12 @@ fn explain_keyword_ends_at_a_word_boundary_and_needs_a_declaration() { format!("explain explain {decl}"), ] { let error = parse_query(&input).unwrap_err(); - assert!(matches!(error, CompilerError::Parse(_)), "{input}: {error}"); + assert!( + error.diagnostic().is_some_and(|diagnostic| { + diagnostic.kind == crate::query::diagnostic::QueryDiagnosticKind::Parse + }), + "{input}: {error}" + ); } } @@ -1845,7 +1855,7 @@ fn settings_statements_are_checked_against_the_definition() { "{input}: {} (the full text is `settings::tests::messages_follow_the_definition`'s)", err.message ); - assert!(err.span.is_some(), "{input} carries a position"); + assert!(err.position.is_some(), "{input} carries a position"); } let rendered = parse_query("set merge_lineage = v3;") .unwrap_err() @@ -1940,3 +1950,138 @@ return { $p.set, $q.traversal as all } assert_eq!(parse_branch("branch create set"), create("set", None)); assert_eq!(parse_branch("branch create all"), create("all", None)); } + +#[test] +fn query_without_parameter_list_reports_q002_at_the_name_end_with_fix() { + let err = + parse_query_diagnostic("query name {\n match { $p: Person }\n return { $p.name }\n}") + .unwrap_err(); + assert_eq!(err.code.as_str(), "Q002"); + assert_eq!( + err.message, + "expected `(`: a query declares its parameters even when it has none" + ); + assert_eq!(err.fix.as_deref(), Some("query name()")); + let at = err.position.expect("positioned at the name's end"); + assert_eq!((at.line, at.column, at.byte), (1, 11, 10)); + assert!(err.stage.is_none()); + let rendered = parse_query("query name {").unwrap_err().to_string(); + assert_eq!( + rendered, + "parse error: expected `(`: a query declares its parameters even when it has none" + ); + // A well-formed declaration before the broken one leaves the fix's + // subject the broken one. + let err = + parse_query_diagnostic("query a() { match { $p: Person } return { $p.name } }\nquery b {") + .unwrap_err(); + assert_eq!(err.fix.as_deref(), Some("query b()")); + assert_eq!(err.position.unwrap().line, 2); + // The declaration keeps its parameter list when it has one. + assert!(parse_query("query a() { match { $p: Person } return { $p.name } }").is_ok()); +} + +#[test] +fn query_diagnostic_suggestion_edits_preserve_source_and_reparse() { + for source in [ + "query name { match { $p: Person } return { $p.name } }", + "/* café */\nquery name /* keep */ { match { $p: Person } return { $p.name } }", + "query name // keep\n{ match { $p: Person } return { $p.name } }", + "query first() { match { $p: Person } return { $p.name } }\nquery name { match { $p: Person } return { $p.name } }", + "query name @description(\"hello\") { match { $p: Person } return { $p.name } }", + ] { + let diagnostic = parse_query_diagnostic(source).unwrap_err(); + assert_eq!(diagnostic.code, Q002, "{source}"); + let json = serde_json::to_value(&diagnostic).unwrap(); + let suggestion = &json["suggestion"]; + assert_eq!( + suggestion["applicability"], "machine_applicable", + "{source}" + ); + let edits = suggestion["edits"].as_array().unwrap(); + assert_eq!(edits.len(), 1); + let edit = &edits[0]; + let byte = source.find("query name").unwrap() + "query name".len(); + assert_eq!(edit["start"], byte); + assert_eq!(edit["end"], byte); + assert_eq!(edit["replacement"], "()"); + let mut repaired = source.to_string(); + repaired.replace_range(byte..byte, edit["replacement"].as_str().unwrap()); + parse_query_diagnostic(&repaired).unwrap(); + } + + for source in [ + "query name garbage", + "query café {", + "query name {", + "query name { match { $p: Person } return { $p.name } } query second { match { $p: Person } return { $p.name } }", + ] { + let json = serde_json::to_value(parse_query_diagnostic(source).unwrap_err()).unwrap(); + assert!(json.get("suggestion").is_none(), "{source}: {json}"); + } + let source = "queryname { match { $p: Person } return { $p.name } }"; + assert_eq!(parse_query_diagnostic(source).unwrap_err().code, Q001); + parse_query_diagnostic("query name /* keep */ () { match { $p: Person } return { $p.name } }") + .unwrap(); +} + +#[test] +fn handwritten_query_diagnostics_locate_the_refused_token() { + for (source, token, code) in [ + ( + "/* café */\nquery q() { delete Person where in = 1 }", + "in =", + Q005, + ), + ( + "query q() { match { $p: Person } return { $p.name as in } }", + "in }", + Q005, + ), + ( + "query q() { match { $p: Person } return { 99999999999999999999999 as n } }", + "99999999999999999999999", + Q005, + ), + ( + "query q() { match { $p: Person } return { $p.name } limit 99999999999999999999999 }", + "99999999999999999999999", + Q005, + ), + ( + r#"query q() { match { $p: Person } return { "bad\x" as n } }"#, + r#""bad\x""#, + Q005, + ), + (r#"set engine = "bad\x";"#, r#""bad\x""#, Q003), + (r#"branch create "bad\x""#, r#""bad\x""#, Q004), + ( + "query q() { match { $p: Person } return { $p.name } order { (nearest($p.embedding, [1.0])) desc } }", + "desc", + Q005, + ), + ] { + let diagnostic = parse_query_diagnostic(source).unwrap_err(); + assert_eq!(diagnostic.code, code, "{source}: {diagnostic}"); + assert_eq!( + diagnostic.position, + Some(Position::at(source, source.find(token).unwrap())), + "{source}" + ); + assert!(diagnostic.stage.is_none()); + } +} + +#[test] +fn grammar_mismatch_reports_q001_at_the_deepest_failure() { + let err = parse_query_diagnostic("mutation { insert Person { name: \"a\" } }").unwrap_err(); + assert_eq!(err.code.as_str(), "Q001"); + assert!(err.message.starts_with("expected "), "{}", err.message); + assert_eq!(err.position.unwrap().byte, 0); + assert!(err.fix.is_none()); + // A settings refusal is positioned at the offending token. + let err = parse_query_diagnostic("set merge_lineage = v3;").unwrap_err(); + assert_eq!(err.code.as_str(), "Q003"); + let at = err.position.unwrap(); + assert_eq!((at.line, at.column), (1, 21)); +} diff --git a/crates/omnigraph-compiler/src/query/query.pest b/crates/omnigraph-compiler/src/query/query.pest index 4aec7b868..d8d36d5ca 100644 --- a/crates/omnigraph-compiler/src/query/query.pest +++ b/crates/omnigraph-compiler/src/query/query.pest @@ -5,7 +5,14 @@ COMMENT = _{ LINE_COMMENT | BLOCK_COMMENT } LINE_COMMENT = _{ "//" ~ (!"\n" ~ ANY)* } BLOCK_COMMENT = _{ "/*" ~ (!"*/" ~ ANY)* ~ "*/" } -query_file = { SOI ~ setting_stmt* ~ (show_stmt ~ statement_trailer? | branch_stmt ~ ";"? ~ statement_trailer? | explain_stmt ~ statement_trailer? | query_decl*) ~ EOI } +query_file = { SOI ~ setting_stmt* ~ (show_stmt ~ statement_trailer? | branch_stmt ~ ";"? ~ statement_trailer? | explain_stmt ~ statement_trailer? | query_decl* ~ missing_param_list?) ~ EOI } +// A declaration whose name is not followed by `(`, matched after the well +// formed declarations so the parser refuses it at the name's end with the +// one fix (`query name()`) instead of reporting the file's first position; +// pest tracks attempts per rule, never per token, so the missing `(` is +// otherwise not an attempt it can name. +missing_param_list = { missing_query_keyword ~ ident ~ !"(" ~ ANY* } +missing_query_keyword = @{ "query" ~ !(ASCII_ALPHANUMERIC | "_") } // Settings statements stand at the head of a file, before its one statement // or its declarations; `show` is itself the one statement. Names are looked diff --git a/crates/omnigraph-compiler/src/query/typecheck.rs b/crates/omnigraph-compiler/src/query/typecheck.rs index 8ab5186f5..b6a66a484 100644 --- a/crates/omnigraph-compiler/src/query/typecheck.rs +++ b/crates/omnigraph-compiler/src/query/typecheck.rs @@ -9,6 +9,7 @@ use crate::error::{CompilerError, Result}; use crate::types::{Direction, PropType, ScalarType}; use super::ast::*; +use super::codes::*; /// A variable in the query's single namespace, tagged by what it binds. /// @@ -31,9 +32,12 @@ impl BoundVariable { fn require_traversal_endpoint(&self, var: &str) -> Result<&str> { match self { Self::Node { type_name } => Ok(type_name), - Self::Edge { .. } => Err(CompilerError::Type(format!( - "T23: edge binding `${var}` cannot be used as a traversal endpoint; traversal endpoints must be node bindings" - ))), + Self::Edge { .. } => Err(CompilerError::typed( + T23, + format!( + "edge binding `${var}` cannot be used as a traversal endpoint; traversal endpoints must be node bindings" + ), + )), } } } @@ -86,12 +90,18 @@ impl Scope<'_> { fn call_refusal(self, keyword: &str) -> Option { match self { Scope::Read => None, - Scope::MutationWhere(_) => Some(CompilerError::Type(format!( - "T44: `{keyword}` cannot appear in a mutation where; a where compares the row's own properties, parameters and now()" - ))), - Scope::Constant { .. } => Some(CompilerError::Type(format!( - "T44: `{keyword}` cannot appear in an assignment value; assignments and binding matches are constants per invocation" - ))), + Scope::MutationWhere(_) => Some(CompilerError::typed( + T44, + format!( + "`{keyword}` cannot appear in a mutation where; a where compares the row's own properties, parameters and now()" + ), + )), + Scope::Constant { .. } => Some(CompilerError::typed( + T44, + format!( + "`{keyword}` cannot appear in an assignment value; assignments and binding matches are constants per invocation" + ), + )), } } @@ -102,12 +112,14 @@ impl Scope<'_> { Scope::Constant { clause: ConstantClause::BindingMatch, .. - } => CompilerError::Type(format!( - "T3: match variable `${name}` must be a declared query parameter" - )), - _ => CompilerError::Type(format!( - "T14: mutation variable `${name}` must be a declared query parameter" - )), + } => CompilerError::typed( + T3, + format!("match variable `${name}` must be a declared query parameter"), + ), + _ => CompilerError::typed( + T14, + format!("mutation variable `${name}` must be a declared query parameter"), + ), } } } @@ -120,9 +132,12 @@ fn constant_leaf_refusal(type_name: &str, variable: &str, property: &str) -> Com } else { format!("${variable}.{property}") }; - CompilerError::Type(format!( - "T45: `{leaf}` cannot appear in an assignment value; assignments and binding matches are constants per invocation" - )) + CompilerError::typed( + T45, + format!( + "`{leaf}` cannot appear in an assignment value; assignments and binding matches are constants per invocation" + ), + ) } /// The keyword of an aggregate, search or ranking call; `None` for every @@ -217,7 +232,7 @@ pub fn typecheck_query_decl(catalog: &Catalog, query: &QueryDecl) -> Result Result { if !query.mutations.is_empty() { - return Err(CompilerError::Type( + return Err(CompilerError::Plan( "mutation query cannot be typechecked with read-query API".to_string(), )); } @@ -251,17 +266,20 @@ fn parse_declared_param_types(params: &[Param]) -> Result Result Result<()> { fn check(name: &str) -> Result<()> { if name.starts_with("__") { - return Err(CompilerError::Type(format!( - "variable `${name}`: names beginning with `__` are reserved for the compiler" - ))); + return Err(CompilerError::typed( + T49, + format!( + "variable `${name}`: names beginning with `__` are reserved for the compiler" + ), + )); } Ok(()) } @@ -325,9 +346,12 @@ fn typecheck_read_query(catalog: &Catalog, query: &QueryDecl) -> Result Result Result Result {} _ => { - return Err(CompilerError::Type( - "T9: non-aggregate expressions in an aggregate query must be \ + return Err(CompilerError::typed( + T9, + "non-aggregate expressions in an aggregate query must be \ property accesses or variables" .to_string(), )); @@ -421,9 +448,12 @@ fn bind_order_key( if bound { return Ok(()); } - Err(CompilerError::Type(format!( - "T42: order key `{key}` does not appear in return; add it to return or order by its alias" - ))) + Err(CompilerError::typed( + T42, + format!( + "order key `{key}` does not appear in return; add it to return or order by its alias" + ), + )) } fn typecheck_mutation( @@ -436,8 +466,9 @@ fn typecheck_mutation( match mutation { Mutation::Insert(insert) => { if insert.assignments.is_empty() { - return Err(CompilerError::Type( - "T10: insert mutation requires at least one assignment".to_string(), + return Err(CompilerError::typed( + T10, + "insert mutation requires at least one assignment".to_string(), )); } @@ -450,12 +481,15 @@ fn typecheck_mutation( .properties .get(&assignment.property) .ok_or_else(|| { - CompilerError::Type(format!( - "T11: type `{}` has no property `{}`{}", - insert.type_name, - assignment.property, - identity_assignment_hint(&assignment.property) - )) + CompilerError::typed( + T11, + format!( + "type `{}` has no property `{}`{}", + insert.type_name, + assignment.property, + identity_assignment_hint(&assignment.property) + ), + ) })?; typecheck_assignment( catalog, @@ -483,16 +517,22 @@ fn typecheck_mutation( if assigned_props.contains(embed.source.as_str()) { continue; } - return Err(CompilerError::Type(format!( - "T12: insert for `{}` must provide non-nullable property `{}` or @embed source `{}`", - insert.type_name, prop_name, embed.source - ))); + return Err(CompilerError::typed( + T12, + format!( + "insert for `{}` must provide non-nullable property `{}` or @embed source `{}`", + insert.type_name, prop_name, embed.source + ), + )); } - return Err(CompilerError::Type(format!( - "T12: insert for `{}` must provide non-nullable property `{}`", - insert.type_name, prop_name - ))); + return Err(CompilerError::typed( + T12, + format!( + "insert for `{}` must provide non-nullable property `{}`", + insert.type_name, prop_name + ), + )); } return Ok(MutationTarget::Node { type_name: insert.type_name.clone(), @@ -517,12 +557,15 @@ fn typecheck_mutation( .properties .get(&assignment.property) .ok_or_else(|| { - CompilerError::Type(format!( - "T11: type `{}` has no property `{}`{}", - insert.type_name, - assignment.property, - identity_assignment_hint(&assignment.property) - )) + CompilerError::typed( + T11, + format!( + "type `{}` has no property `{}`{}", + insert.type_name, + assignment.property, + identity_assignment_hint(&assignment.property) + ), + ) })? .clone(), }; @@ -536,16 +579,22 @@ fn typecheck_mutation( } if !has_from { - return Err(CompilerError::Type(format!( - "T12: insert for `{}` must provide required endpoint `from`", - insert.type_name - ))); + return Err(CompilerError::typed( + T12, + format!( + "insert for `{}` must provide required endpoint `from`", + insert.type_name + ), + )); } if !has_to { - return Err(CompilerError::Type(format!( - "T12: insert for `{}` must provide required endpoint `to`", - insert.type_name - ))); + return Err(CompilerError::typed( + T12, + format!( + "insert for `{}` must provide required endpoint `to`", + insert.type_name + ), + )); } for (prop_name, prop_type) in &edge_type.properties { @@ -553,10 +602,13 @@ fn typecheck_mutation( continue; } if !insert.assignments.iter().any(|a| &a.property == prop_name) { - return Err(CompilerError::Type(format!( - "T12: insert for `{}` must provide non-nullable property `{}`", - insert.type_name, prop_name - ))); + return Err(CompilerError::typed( + T12, + format!( + "insert for `{}` must provide non-nullable property `{}`", + insert.type_name, prop_name + ), + )); } } return Ok(MutationTarget::Edge { @@ -564,29 +616,33 @@ fn typecheck_mutation( }); } - Err(CompilerError::Type(format!( - "T10: unknown node/edge type `{}`", - insert.type_name - ))) + Err(CompilerError::typed( + T10, + format!("unknown node/edge type `{}`", insert.type_name), + )) } Mutation::Update(update) => { let node_type = if let Some(node_type) = catalog.node_types.get(&update.type_name) { node_type } else if catalog.edge_types.contains_key(&update.type_name) { - return Err(CompilerError::Type(format!( - "T16: update mutation for edge type `{}` is not supported", - update.type_name - ))); + return Err(CompilerError::typed( + T16, + format!( + "update mutation for edge type `{}` is not supported", + update.type_name + ), + )); } else { - return Err(CompilerError::Type(format!( - "T10: unknown node/edge type `{}`", - update.type_name - ))); + return Err(CompilerError::typed( + T10, + format!("unknown node/edge type `{}`", update.type_name), + )); }; if update.assignments.is_empty() { - return Err(CompilerError::Type( - "T10: update mutation requires at least one assignment".to_string(), + return Err(CompilerError::typed( + T10, + "update mutation requires at least one assignment".to_string(), )); } ensure_no_duplicate_assignment_names(&update.assignments)?; @@ -597,12 +653,15 @@ fn typecheck_mutation( .properties .get(&assignment.property) .ok_or_else(|| { - CompilerError::Type(format!( - "T11: type `{}` has no property `{}`{}", - update.type_name, - assignment.property, - identity_assignment_hint(&assignment.property) - )) + CompilerError::typed( + T11, + format!( + "type `{}` has no property `{}`{}", + update.type_name, + assignment.property, + identity_assignment_hint(&assignment.property) + ), + ) })?; typecheck_assignment( catalog, @@ -629,10 +688,10 @@ fn typecheck_mutation( type_name: delete.type_name.clone(), } } else { - return Err(CompilerError::Type(format!( - "T10: unknown node/edge type `{}`", - delete.type_name - ))); + return Err(CompilerError::typed( + T10, + format!("unknown node/edge type `{}`", delete.type_name), + )); }; typecheck_mutation_where(catalog, &target, &delete.predicate, ¶m_types)?; Ok(target) @@ -644,10 +703,13 @@ fn ensure_no_duplicate_assignment_names(assignments: &[MutationAssignment]) -> R let mut seen = std::collections::HashSet::new(); for assignment in assignments { if !seen.insert(&assignment.property) { - return Err(CompilerError::Type(format!( - "T13: duplicate assignment for property `{}`", - assignment.property - ))); + return Err(CompilerError::typed( + T13, + format!( + "duplicate assignment for property `{}`", + assignment.property + ), + )); } } Ok(()) @@ -707,9 +769,10 @@ fn mutation_property_type( ) -> Result { let type_name = target.type_name(); if variable != type_name { - return Err(CompilerError::Type(format!( - "T14: mutation variable `${variable}` must be a declared query parameter" - ))); + return Err(CompilerError::typed( + T14, + format!("mutation variable `${variable}` must be a declared query parameter"), + )); } let is_edge = target.is_edge(); if is_edge && (property == "from" || property == "to") { @@ -727,22 +790,29 @@ fn mutation_property_type( } else { "`@id`" }; - return Err(CompilerError::Type(format!( - "T11: type `{type_name}` has no meta-field `{property}`; the meta-fields of this type are {known}" - ))); + return Err(CompilerError::typed( + T11, + format!( + "type `{type_name}` has no meta-field `{property}`; the meta-fields of this type are {known}" + ), + )); } return Ok(meta_field_type()); } let prop_type = declared_property(catalog, target, property).ok_or_else(|| { - CompilerError::Type(format!( - "T11: type `{type_name}` has no property `{property}`{}", - system_field_hint(property, None, is_edge) - )) + CompilerError::typed( + T11, + format!( + "type `{type_name}` has no property `{property}`{}", + system_field_hint(property, None, is_edge) + ), + ) })?; if matches!(prop_type.scalar, ScalarType::Blob) { - return Err(CompilerError::Type(format!( - "T11: blob property `{property}` cannot be used in WHERE predicates" - ))); + return Err(CompilerError::typed( + T11, + format!("blob property `{property}` cannot be used in WHERE predicates"), + )); } Ok(prop_type.clone()) } @@ -784,10 +854,13 @@ fn typecheck_mutation_where( { let word = if literal { "true" } else { "false" }; let property = shadowed.iter().find(|name| **name == word).unwrap_or(first); - return Err(CompilerError::Type(format!( - "T46: `{word}` is a Boolean literal here; the property named `{property}` of `{}` cannot be named bare in a mutation `where`; rename it in the schema (`@rename_from`)", - target.type_name() - ))); + return Err(CompilerError::typed( + T46, + format!( + "`{word}` is a Boolean literal here; the property named `{property}` of `{}` cannot be named bare in a mutation `where`; rename it in the schema (`@rename_from`)", + target.type_name() + ), + )); } let resolved = resolve_expr_type( catalog, @@ -797,10 +870,13 @@ fn typecheck_mutation_where( Scope::MutationWhere(target), )?; if boolean_scalar(&resolved).is_none() { - return Err(CompilerError::Type(format!( - "T41: a mutation `where` must be Boolean, got {}", - resolved.display_name() - ))); + return Err(CompilerError::typed( + T41, + format!( + "a mutation `where` must be Boolean, got {}", + resolved.display_name() + ), + )); } Ok(()) } @@ -853,10 +929,13 @@ fn typecheck_constant( let scope = Scope::Constant { clause, type_name }; let resolved = resolve_expr_type(catalog, value, &TypeContext::empty(), params, scope)?; let ResolvedType::Scalar(actual) = &resolved else { - return Err(CompilerError::Type(format!( - "T7: the value for property `{property}` must be a scalar, got {}", - resolved.display_name() - ))); + return Err(CompilerError::typed( + T7, + format!( + "the value for property `{property}` must be a scalar, got {}", + resolved.display_name() + ), + )); }; match (clause, value) { (ConstantClause::Assignment, Expr::Literal(lit)) => { @@ -868,12 +947,15 @@ fn typecheck_constant( (_, Expr::Now) => check_now_match_value_type(expected, property), (ConstantClause::Assignment, _) => { if !assignment_compatible(actual, expected) { - return Err(CompilerError::Type(format!( - "T7: cannot assign/compare {} with {} for property `{}`", - actual.display_name(), - expected.display_name(), - property - ))); + return Err(CompilerError::typed( + T7, + format!( + "cannot assign/compare {} with {} for property `{}`", + actual.display_name(), + expected.display_name(), + property + ), + )); } Ok(()) } @@ -885,11 +967,14 @@ fn typecheck_constant( fn check_now_match_value_type(expected: &PropType, property: &str) -> Result<()> { if expected.list || expected.scalar != ScalarType::DateTime { - return Err(CompilerError::Type(format!( - "T7: cannot assign/compare DateTime with {} for property `{}`", - expected.display_name(), - property - ))); + return Err(CompilerError::typed( + T7, + format!( + "cannot assign/compare DateTime with {} for property `{}`", + expected.display_name(), + property + ), + )); } Ok(()) } @@ -912,13 +997,16 @@ fn typecheck_clauses( typecheck_clauses(catalog, &subquery.clauses, &mut inner_ctx, params, true)?; if !block_references_outer(&subquery.clauses, &outer_vars) { let rule = match subquery.keyword { - BlockKeyword::Not => "T9", - BlockKeyword::Exists | BlockKeyword::Aggregate => "T39", + BlockKeyword::Not => T9, + BlockKeyword::Exists | BlockKeyword::Aggregate => T39, }; - return Err(CompilerError::Type(format!( - "{rule}: {} block must reference at least one outer-bound variable", - subquery.block_name() - ))); + return Err(CompilerError::typed( + rule, + format!( + "{} block must reference at least one outer-bound variable", + subquery.block_name() + ), + )); } typecheck_subquery_predicate(catalog, subquery, &inner_ctx, ctx, params)?; } @@ -962,10 +1050,13 @@ fn typecheck_subquery_predicate( PropType::scalar(s.scalar, false) } (_, other) => { - return Err(CompilerError::Type(format!( - "T40: {func} over a block requires a scalar argument, got {}", - other.display_name() - ))); + return Err(CompilerError::typed( + T40, + format!( + "{func} over a block requires a scalar argument, got {}", + other.display_name() + ), + )); } } } @@ -976,23 +1067,30 @@ fn typecheck_subquery_predicate( _ => false, }; if !bound { - return Err(CompilerError::Type(format!( - "T40: {func} over a block compares with a literal, now() or a parameter" - ))); + return Err(CompilerError::typed( + T40, + format!("{func} over a block compares with a literal, now() or a parameter"), + )); } let right = resolve_expr_type(catalog, &subquery.right, ctx, params, Scope::Read)?; let ResolvedType::Scalar(r) = &right else { - return Err(CompilerError::Type(format!( - "T40: {func} over a block compares with a scalar, got {}", - right.display_name() - ))); + return Err(CompilerError::typed( + T40, + format!( + "{func} over a block compares with a scalar, got {}", + right.display_name() + ), + )); }; if !types_compatible(&result, r) { - return Err(CompilerError::Type(format!( - "T40: cannot compare {func} over a block ({}) with {}", - result.display_name(), - r.display_name() - ))); + return Err(CompilerError::typed( + T40, + format!( + "cannot compare {func} over a block ({}) with {}", + result.display_name(), + r.display_name() + ), + )); } Ok(()) } @@ -1005,10 +1103,10 @@ fn typecheck_binding( ) -> Result<()> { // T1: binding type must exist in catalog if !catalog.node_types.contains_key(&binding.type_name) { - return Err(CompilerError::Type(format!( - "T1: unknown node type `{}`", - binding.type_name - ))); + return Err(CompilerError::typed( + T1, + format!("unknown node type `{}`", binding.type_name), + )); } let node_type = &catalog.node_types[&binding.type_name]; @@ -1016,26 +1114,32 @@ fn typecheck_binding( // T2 + T3: property match fields must exist and have correct types for pm in &binding.prop_matches { let prop = node_type.properties.get(&pm.prop_name).ok_or_else(|| { - CompilerError::Type(format!( - "T2: type `{}` has no property `{}`{}", - binding.type_name, - pm.prop_name, - if pm.prop_name == "id" { - format!( - "; filter the system identity with `${}.@id = ...` in the match block", - binding.variable - ) - } else { - String::new() - } - )) + CompilerError::typed( + T2, + format!( + "type `{}` has no property `{}`{}", + binding.type_name, + pm.prop_name, + if pm.prop_name == "id" { + format!( + "; filter the system identity with `${}.@id = ...` in the match block", + binding.variable + ) + } else { + String::new() + } + ), + ) })?; if matches!(prop.scalar, ScalarType::Blob) { - return Err(CompilerError::Type(format!( - "T3: blob property `{}.{}` cannot be used in match patterns", - binding.type_name, pm.prop_name - ))); + return Err(CompilerError::typed( + T3, + format!( + "blob property `{}.{}` cannot be used in match patterns", + binding.type_name, pm.prop_name + ), + )); } typecheck_constant( @@ -1055,17 +1159,23 @@ fn typecheck_binding( if let Some(existing) = ctx.bindings.get(&binding.variable) { match existing { BoundVariable::Edge { type_name } => { - return Err(CompilerError::Type(format!( - "T23: variable `${}` is bound to edge type `{}` and cannot be rebound as node type `{}`", - binding.variable, type_name, binding.type_name - ))); + return Err(CompilerError::typed( + T23, + format!( + "variable `${}` is bound to edge type `{}` and cannot be rebound as node type `{}`", + binding.variable, type_name, binding.type_name + ), + )); } BoundVariable::Node { type_name } => { if *type_name != binding.type_name { - return Err(CompilerError::Type(format!( - "variable `${}` already bound to type `{}`, cannot rebind to `{}`", - binding.variable, type_name, binding.type_name - ))); + return Err(CompilerError::typed( + T50, + format!( + "variable `${}` already bound to type `{}`, cannot rebind to `{}`", + binding.variable, type_name, binding.type_name + ), + )); } } } @@ -1085,20 +1195,26 @@ fn check_binding_literal_type(lit: &Literal, expected: &PropType, property: &str if expected.list { let lit_type = literal_type(lit)?; if lit_type.list { - return Err(CompilerError::Type(format!( - "T3: list equality is not supported for property `{}`; use a scalar value to match list membership", - property - ))); + return Err(CompilerError::typed( + T3, + format!( + "list equality is not supported for property `{}`; use a scalar value to match list membership", + property + ), + )); } let expected_member = PropType::scalar(expected.scalar, expected.nullable); if !types_compatible(&lit_type, &expected_member) { - return Err(CompilerError::Type(format!( - "T3: property `{}` has type {} but membership match got {}", - property, - expected.display_name(), - lit_type.display_name() - ))); + return Err(CompilerError::typed( + T3, + format!( + "property `{}` has type {} but membership match got {}", + property, + expected.display_name(), + lit_type.display_name() + ), + )); } return Ok(()); } @@ -1113,31 +1229,40 @@ fn check_binding_variable_type( ) -> Result<()> { if expected.list { if actual.list { - return Err(CompilerError::Type(format!( - "T7: list equality is not supported for property `{}`; use a scalar parameter for membership matching", - property - ))); + return Err(CompilerError::typed( + T7, + format!( + "list equality is not supported for property `{}`; use a scalar parameter for membership matching", + property + ), + )); } let expected_member = PropType::scalar(expected.scalar, expected.nullable); if !types_compatible(actual, &expected_member) { - return Err(CompilerError::Type(format!( - "T7: cannot compare {} membership against {} for property `{}`", - actual.display_name(), - expected.display_name(), - property - ))); + return Err(CompilerError::typed( + T7, + format!( + "cannot compare {} membership against {} for property `{}`", + actual.display_name(), + expected.display_name(), + property + ), + )); } return Ok(()); } if !types_compatible(actual, expected) { - return Err(CompilerError::Type(format!( - "T7: cannot assign/compare {} with {} for property `{}`", - actual.display_name(), - expected.display_name(), - property - ))); + return Err(CompilerError::typed( + T7, + format!( + "cannot assign/compare {} with {} for property `{}`", + actual.display_name(), + expected.display_name(), + property + ), + )); } Ok(()) } @@ -1151,24 +1276,29 @@ fn typecheck_traversal( let edge = catalog .lookup_edge_by_name(&traversal.edge_name) .ok_or_else(|| { - CompilerError::Type(format!("T4: unknown edge type `{}`", traversal.edge_name)) + CompilerError::typed(T4, format!("unknown edge type `{}`", traversal.edge_name)) })?; if traversal.min_hops == 0 { - return Err(CompilerError::Type( - "T15: traversal min hop bound must be >= 1".to_string(), + return Err(CompilerError::typed( + T15, + "traversal min hop bound must be >= 1".to_string(), )); } if let Some(max_hops) = traversal.max_hops { if max_hops < traversal.min_hops { - return Err(CompilerError::Type(format!( - "T15: invalid traversal bounds {{{},{}}}; max must be >= min", - traversal.min_hops, max_hops - ))); + return Err(CompilerError::typed( + T15, + format!( + "invalid traversal bounds {{{},{}}}; max must be >= min", + traversal.min_hops, max_hops + ), + )); } } else { - return Err(CompilerError::Type( - "T15: unbounded traversal is disabled; use bounded traversal {min,max}".to_string(), + return Err(CompilerError::typed( + T15, + "unbounded traversal is disabled; use bounded traversal {min,max}".to_string(), )); } @@ -1177,10 +1307,13 @@ fn typecheck_traversal( // well-typed in at most one direction, so the undirected form is either // pointless or a type error; require the directional form instead. if traversal.undirected && edge.from_type != edge.to_type { - return Err(CompilerError::Type(format!( - "T22: undirected traversal `<{}>` requires a same-endpoint-type edge, but `{}: {} -> {}` is asymmetric; use the directional form", - edge.name, edge.name, edge.from_type, edge.to_type - ))); + return Err(CompilerError::typed( + T22, + format!( + "undirected traversal `<{}>` requires a same-endpoint-type edge, but `{}: {} -> {}` is asymmetric; use the directional form", + edge.name, edge.name, edge.from_type, edge.to_type + ), + )); } // T23: a {min,max} traversal matches a path of edges; there is no single @@ -1193,22 +1326,31 @@ fn typecheck_traversal( if traversal.edge_binding.is_some() && (traversal.min_hops != 1 || traversal.max_hops != Some(1)) { - return Err(CompilerError::Type(format!( - "T23: edge binding `${}` cannot be combined with traversal bounds; a multi-hop traversal matches a path of edges, not one edge", - traversal.edge_binding.as_deref().unwrap_or("_") - ))); + return Err(CompilerError::typed( + T23, + format!( + "edge binding `${}` cannot be combined with traversal bounds; a multi-hop traversal matches a path of edges, not one edge", + traversal.edge_binding.as_deref().unwrap_or("_") + ), + )); } if let Some(binding) = &edge_binding { if binding == &traversal.src || binding == &traversal.dst { - return Err(CompilerError::Type(format!( - "T23: edge binding `${binding}` cannot reuse a traversal endpoint name; edge bindings and node endpoints need distinct names" - ))); + return Err(CompilerError::typed( + T23, + format!( + "edge binding `${binding}` cannot reuse a traversal endpoint name; edge bindings and node endpoints need distinct names" + ), + )); } if ctx.bindings.contains_key(binding) { - return Err(CompilerError::Type(format!( - "T23: variable `${}` is already bound; an edge binding needs a fresh name", - binding - ))); + return Err(CompilerError::typed( + T23, + format!( + "variable `${}` is already bound; an edge binding needs a fresh name", + binding + ), + )); } ctx.bindings.insert( binding.clone(), @@ -1236,10 +1378,13 @@ fn typecheck_traversal( // dst should be edge.from_type bind_traversal_endpoint(ctx, &traversal.dst, &edge.from_type, edge)?; } else { - return Err(CompilerError::Type(format!( - "T5: variable `${}` has type `{}`, which is not an endpoint of edge `{}: {} -> {}`", - traversal.src, src_type, edge.name, edge.from_type, edge.to_type - ))); + return Err(CompilerError::typed( + T5, + format!( + "variable `${}` has type `{}`, which is not an endpoint of edge `{}: {} -> {}`", + traversal.src, src_type, edge.name, edge.from_type, edge.to_type + ), + )); } } else if let Some(dst_bv) = dst_bound { let dst_type = dst_bv.require_traversal_endpoint(&traversal.dst)?; @@ -1251,10 +1396,13 @@ fn typecheck_traversal( direction = Direction::In; bind_traversal_endpoint(ctx, &traversal.src, &edge.to_type, edge)?; } else { - return Err(CompilerError::Type(format!( - "T5: variable `${}` has type `{}`, which is not an endpoint of edge `{}: {} -> {}`", - traversal.dst, dst_type, edge.name, edge.from_type, edge.to_type - ))); + return Err(CompilerError::typed( + T5, + format!( + "variable `${}` has type `{}`, which is not an endpoint of edge `{}: {} -> {}`", + traversal.dst, dst_type, edge.name, edge.from_type, edge.to_type + ), + )); } } else { // Neither bound — default Out direction, bind both @@ -1294,10 +1442,13 @@ fn bind_traversal_endpoint( if let Some(existing) = ctx.bindings.get(var) { let existing_type = existing.require_traversal_endpoint(var)?; if existing_type != expected_type { - return Err(CompilerError::Type(format!( - "T5: variable `${}` has type `{}` but edge `{}` expects `{}`", - var, existing_type, edge.name, expected_type - ))); + return Err(CompilerError::typed( + T5, + format!( + "variable `${}` has type `{}` but edge `{}` expects `{}`", + var, existing_type, edge.name, expected_type + ), + )); } } else { ctx.bindings.insert( @@ -1323,20 +1474,20 @@ fn typecheck_filter( .into_iter() .any(|conjunct| !conjunct.is_search_predicate() && contains_search_call(conjunct)) { - return Err(CompilerError::Type(SEARCH_PREDICATE_SHAPE.to_string())); + return Err(CompilerError::typed(T38, SEARCH_PREDICATE_SHAPE)); } let resolved = resolve_expr_type(catalog, filter, ctx, params, Scope::Read)?; if boolean_scalar(&resolved).is_none() { - return Err(CompilerError::Type(format!( - "T41: a filter must be Boolean, got {}", - resolved.display_name() - ))); + return Err(CompilerError::typed( + T41, + format!("a filter must be Boolean, got {}", resolved.display_name()), + )); } Ok(()) } const SEARCH_PREDICATE_SHAPE: &str = - "T38: search predicates require a standalone call or `= true`, alone or joined by and"; + "search predicates require a standalone call or `= true`, alone or joined by and"; /// Whether a `search`, `fuzzy` or `match_text` call stands anywhere in `expr`. fn contains_search_call(expr: &Expr) -> bool { @@ -1401,7 +1552,7 @@ fn typecheck_comparison( && op == CompOp::Eq && matches!(right, Expr::Literal(Literal::Bool(true)))) { - return Err(CompilerError::Type(SEARCH_PREDICATE_SHAPE.to_string())); + return Err(CompilerError::typed(T38, SEARCH_PREDICATE_SHAPE)); } if let Scope::MutationWhere(target) = scope @@ -1437,8 +1588,9 @@ fn typecheck_comparison( // bypass containment with a list-membership shape such as // `[Blob] contains Blob`. if matches!(l.scalar, ScalarType::Blob) || matches!(r.scalar, ScalarType::Blob) { - return Err(CompilerError::Type( - "T7: blob comparisons in filters are not supported".to_string(), + return Err(CompilerError::typed( + T7, + "blob comparisons in filters are not supported".to_string(), )); } @@ -1448,29 +1600,37 @@ fn typecheck_comparison( // `StringContains` so execution never re-derives the dispatch. if !l.list && matches!(l.scalar, ScalarType::String) { if r.list || !matches!(r.scalar, ScalarType::String) { - return Err(CompilerError::Type(format!( - "T7: string contains requires a String right operand, got {}", - r.display_name() - ))); + return Err(CompilerError::typed( + T7, + format!( + "string contains requires a String right operand, got {}", + r.display_name() + ), + )); } return Ok(result); } if !l.list { - return Err(CompilerError::Type(format!( - "T7: contains requires a list property (membership) or a String property (substring) on the left, got {}", - l.display_name() - ))); + return Err(CompilerError::typed( + T7, + format!( + "contains requires a list property (membership) or a String property (substring) on the left, got {}", + l.display_name() + ), + )); } if r.list { - return Err(CompilerError::Type( - "T7: contains requires a scalar right operand".to_string(), + return Err(CompilerError::typed( + T7, + "contains requires a scalar right operand".to_string(), )); } if matches!(l.scalar, ScalarType::Vector(_)) || matches!(r.scalar, ScalarType::Vector(_)) { - return Err(CompilerError::Type( - "T7: vector membership filters are not supported".to_string(), + return Err(CompilerError::typed( + T7, + "vector membership filters are not supported".to_string(), )); } @@ -1478,11 +1638,14 @@ fn typecheck_comparison( matches!(left, Expr::Literal(Literal::List(items)) if items.is_empty()); let expected_member = PropType::scalar(l.scalar, l.nullable); if !empty_list_literal && !types_compatible(&expected_member, r) { - return Err(CompilerError::Type(format!( - "T7: cannot test membership of {} in {}", - r.display_name(), - l.display_name() - ))); + return Err(CompilerError::typed( + T7, + format!( + "cannot test membership of {} in {}", + r.display_name(), + l.display_name() + ), + )); } return Ok(result); } @@ -1493,47 +1656,62 @@ fn typecheck_comparison( // check is written over both ops so re-typechecking IR-shaped // input stays consistent.) if l.list || !matches!(l.scalar, ScalarType::String) { - return Err(CompilerError::Type(format!( - "T7: {} requires a String property on the left, got {}", - op, - l.display_name() - ))); + return Err(CompilerError::typed( + T7, + format!( + "{} requires a String property on the left, got {}", + op, + l.display_name() + ), + )); } if r.list || !matches!(r.scalar, ScalarType::String) { - return Err(CompilerError::Type(format!( - "T7: {} requires a String right operand, got {}", - op, - r.display_name() - ))); + return Err(CompilerError::typed( + T7, + format!( + "{} requires a String right operand, got {}", + op, + r.display_name() + ), + )); } return Ok(result); } // T7: check type compatibility if l.list || r.list { - return Err(CompilerError::Type( - "T7: list comparisons in filters are not supported; use `contains` for list membership".to_string(), + return Err(CompilerError::typed( + T7, + "list comparisons in filters are not supported; use `contains` for list membership" + .to_string(), )); } if matches!(l.scalar, ScalarType::Vector(_)) || matches!(r.scalar, ScalarType::Vector(_)) { - return Err(CompilerError::Type( - "T7: vector comparisons in filters are not supported".to_string(), + return Err(CompilerError::typed( + T7, + "vector comparisons in filters are not supported".to_string(), )); } if !types_compatible(l, r) { - return Err(CompilerError::Type(format!( - "T7: cannot compare {} with {}", - l.display_name(), - r.display_name() - ))); + return Err(CompilerError::typed( + T7, + format!( + "cannot compare {} with {}", + l.display_name(), + r.display_name() + ), + )); } Ok(result) } else { - Err(CompilerError::Type(format!( - "T7: filter comparisons require scalar operands, got {} and {}", - left_type.display_name(), - right_type.display_name() - ))) + Err(CompilerError::typed( + T7, + format!( + "filter comparisons require scalar operands, got {} and {}", + left_type.display_name(), + right_type.display_name() + ), + )) } } @@ -1547,10 +1725,13 @@ fn reject_edge_binding_search_field(ctx: &TypeContext, field: &Expr, func: &str) match bv { BoundVariable::Node { .. } => {} BoundVariable::Edge { .. } => { - return Err(CompilerError::Type(format!( - "T23: {} cannot target edge property `${}.{}`; text/rank search runs on node properties — edge properties support comparison filters and projection", - func, variable, property - ))); + return Err(CompilerError::typed( + T23, + format!( + "{} cannot target edge property `${}.{}`; text/rank search runs on node properties — edge properties support comparison filters and projection", + func, variable, property + ), + )); } } } @@ -1569,7 +1750,7 @@ fn read_property_type( let bv = ctx .bindings .get(variable) - .ok_or_else(|| CompilerError::Type(format!("T6: variable `${variable}` is not bound")))?; + .ok_or_else(|| CompilerError::typed(T6, format!("variable `${variable}` is not bound")))?; if let Some(role) = meta_field_role(property) { let admitted = match (bv, role) { @@ -1582,9 +1763,12 @@ fn read_property_type( BoundVariable::Node { .. } => "`@id`", BoundVariable::Edge { .. } => "`@id`, `@src`, `@dst`", }; - return Err(CompilerError::Type(format!( - "T6: binding `${variable}` has no meta-field `{property}`; its meta-fields are {known}" - ))); + return Err(CompilerError::typed( + T6, + format!( + "binding `${variable}` has no meta-field `{property}`; its meta-fields are {known}" + ), + )); } return Ok(meta_field_type()); } @@ -1592,31 +1776,37 @@ fn read_property_type( let prop = match bv { BoundVariable::Node { type_name } => { let node_type = catalog.node_types.get(type_name).ok_or_else(|| { - CompilerError::Type(format!("T6: type `{}` not found in catalog", type_name)) + CompilerError::typed(T6, format!("type `{}` not found in catalog", type_name)) })?; node_type.properties.get(property).ok_or_else(|| { - CompilerError::Type(format!( - "T6: type `{}` has no property `{}`{}", - type_name, - property, - system_field_hint(property, Some(variable), false) - )) + CompilerError::typed( + T6, + format!( + "type `{}` has no property `{}`{}", + type_name, + property, + system_field_hint(property, Some(variable), false) + ), + ) })? } BoundVariable::Edge { type_name } => { let edge_type = catalog.lookup_edge_by_name(type_name).ok_or_else(|| { - CompilerError::Type(format!( - "T6: edge type `{}` not found in catalog", - type_name - )) + CompilerError::typed( + T6, + format!("edge type `{}` not found in catalog", type_name), + ) })?; edge_type.properties.get(property).ok_or_else(|| { - CompilerError::Type(format!( - "T6: edge `{}` has no property `{}`{}", - type_name, - property, - system_field_hint(property, Some(variable), true) - )) + CompilerError::typed( + T6, + format!( + "edge `{}` has no property `{}`{}", + type_name, + property, + system_field_hint(property, Some(variable), true) + ), + ) })? } }; @@ -1644,10 +1834,13 @@ fn resolve_expr_type( Scope::Read => { let prop = read_property_type(catalog, ctx, variable, property)?; if matches!(prop.scalar, ScalarType::Blob) { - return Err(CompilerError::Type(format!( - "T24: Blob property `${}.{}` is not available as a .gq read value; Blob values require a dedicated API", - variable, property - ))); + return Err(CompilerError::typed( + T24, + format!( + "Blob property `${}.{}` is not available as a .gq read value; Blob values require a dedicated API", + variable, property + ), + )); } Ok(ResolvedType::Scalar(prop)) } @@ -1667,46 +1860,56 @@ fn resolve_expr_type( let node_type_name = match ctx.bindings.get(variable) { Some(BoundVariable::Node { type_name }) => type_name, Some(BoundVariable::Edge { .. }) => { - return Err(CompilerError::Type(format!( - "T23: nearest cannot target edge binding `${}`; vector search runs on node properties", - variable - ))); + return Err(CompilerError::typed( + T23, + format!( + "nearest cannot target edge binding `${}`; vector search runs on node properties", + variable + ), + )); } None => { - return Err(CompilerError::Type(format!( - "T15: variable `${}` is not bound", - variable - ))); + return Err(CompilerError::typed( + T15, + format!("variable `${}` is not bound", variable), + )); } }; let node_type = catalog.node_types.get(node_type_name).ok_or_else(|| { - CompilerError::Type(format!( - "T15: type `{}` not found in catalog", - node_type_name - )) + CompilerError::typed( + T15, + format!("type `{}` not found in catalog", node_type_name), + ) })?; let prop_type = node_type.properties.get(property).ok_or_else(|| { - CompilerError::Type(format!( - "T15: type `{}` has no property `{}`{}", - node_type_name, - property, - system_field_hint(property, Some(variable), false) - )) + CompilerError::typed( + T15, + format!( + "type `{}` has no property `{}`{}", + node_type_name, + property, + system_field_hint(property, Some(variable), false) + ), + ) })?; let vector_dim = match prop_type.scalar { ScalarType::Vector(dim) => dim, _ => { - return Err(CompilerError::Type(format!( - "T15: nearest requires a Vector property, got {}.{}: {}", - node_type_name, - property, - prop_type.display_name() - ))); + return Err(CompilerError::typed( + T15, + format!( + "nearest requires a Vector property, got {}.{}: {}", + node_type_name, + property, + prop_type.display_name() + ), + )); } }; if prop_type.list { - return Err(CompilerError::Type( - "T15: nearest does not support list-wrapped vectors".to_string(), + return Err(CompilerError::typed( + T15, + "nearest does not support list-wrapped vectors".to_string(), )); } @@ -1714,10 +1917,13 @@ fn resolve_expr_type( && let Some(dim) = numeric_vector_literal_dim(lit) { if dim != vector_dim { - return Err(CompilerError::Type(format!( - "T15: nearest vector dimension mismatch: property is Vector({}), query literal has {} elements", - vector_dim, dim - ))); + return Err(CompilerError::typed( + T15, + format!( + "nearest vector dimension mismatch: property is Vector({}), query literal has {} elements", + vector_dim, dim + ), + )); } return Ok(ResolvedType::Scalar(PropType::scalar( ScalarType::F32, @@ -1733,25 +1939,32 @@ fn resolve_expr_type( _ => unreachable!(), }; if qdim != vector_dim { - return Err(CompilerError::Type(format!( - "T15: nearest vector dimension mismatch: property is Vector({}), query is Vector({})", - vector_dim, qdim - ))); + return Err(CompilerError::typed( + T15, + format!( + "nearest vector dimension mismatch: property is Vector({}), query is Vector({})", + vector_dim, qdim + ), + )); } } ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => { // query-time string embedding is supported by the runtime executor } ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T15: nearest query must be Vector({}) or String, got {}", - vector_dim, - s.display_name() - ))); + return Err(CompilerError::typed( + T15, + format!( + "nearest query must be Vector({}) or String, got {}", + vector_dim, + s.display_name() + ), + )); } _ => { - return Err(CompilerError::Type( - "T15: nearest query must be a scalar expression".to_string(), + return Err(CompilerError::typed( + T15, + "nearest query must be a scalar expression".to_string(), )); } } @@ -1767,14 +1980,15 @@ fn resolve_expr_type( match field_type { ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T19: search field must be String, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T19, + format!("search field must be String, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T19: search field must be a scalar String expression".to_string(), + return Err(CompilerError::typed( + T19, + "search field must be a scalar String expression".to_string(), )); } } @@ -1783,14 +1997,15 @@ fn resolve_expr_type( match query_type { ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T19: search query must be String, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T19, + format!("search query must be String, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T19: search query must be a scalar String expression".to_string(), + return Err(CompilerError::typed( + T19, + "search query must be a scalar String expression".to_string(), )); } } @@ -1810,14 +2025,15 @@ fn resolve_expr_type( match field_type { ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T19: fuzzy field must be String, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T19, + format!("fuzzy field must be String, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T19: fuzzy field must be a scalar String expression".to_string(), + return Err(CompilerError::typed( + T19, + "fuzzy field must be a scalar String expression".to_string(), )); } } @@ -1826,14 +2042,15 @@ fn resolve_expr_type( match query_type { ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T19: fuzzy query must be String, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T19, + format!("fuzzy query must be String, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T19: fuzzy query must be a scalar String expression".to_string(), + return Err(CompilerError::typed( + T19, + "fuzzy query must be a scalar String expression".to_string(), )); } } @@ -1852,14 +2069,18 @@ fn resolve_expr_type( | ScalarType::U64 ) => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T19: fuzzy max_edits must be an integer scalar, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T19, + format!( + "fuzzy max_edits must be an integer scalar, got {}", + s.display_name() + ), + )); } _ => { - return Err(CompilerError::Type( - "T19: fuzzy max_edits must be an integer scalar expression".to_string(), + return Err(CompilerError::typed( + T19, + "fuzzy max_edits must be an integer scalar expression".to_string(), )); } } @@ -1876,14 +2097,15 @@ fn resolve_expr_type( match field_type { ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T20: match_text field must be String, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T20, + format!("match_text field must be String, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T20: match_text field must be a scalar String expression".to_string(), + return Err(CompilerError::typed( + T20, + "match_text field must be a scalar String expression".to_string(), )); } } @@ -1892,14 +2114,15 @@ fn resolve_expr_type( match query_type { ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T20: match_text query must be String, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T20, + format!("match_text query must be String, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T20: match_text query must be a scalar String expression".to_string(), + return Err(CompilerError::typed( + T20, + "match_text query must be a scalar String expression".to_string(), )); } } @@ -1915,14 +2138,15 @@ fn resolve_expr_type( match field_type { ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T20: bm25 field must be String, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T20, + format!("bm25 field must be String, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T20: bm25 field must be a scalar String expression".to_string(), + return Err(CompilerError::typed( + T20, + "bm25 field must be a scalar String expression".to_string(), )); } } @@ -1931,14 +2155,15 @@ fn resolve_expr_type( match query_type { ResolvedType::Scalar(s) if s.scalar == ScalarType::String && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T20: bm25 query must be String, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T20, + format!("bm25 query must be String, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T20: bm25 query must be a scalar String expression".to_string(), + return Err(CompilerError::typed( + T20, + "bm25 query must be a scalar String expression".to_string(), )); } } @@ -1954,13 +2179,15 @@ fn resolve_expr_type( k, } => { if !matches!(primary.as_ref(), Expr::Nearest { .. } | Expr::Bm25 { .. }) { - return Err(CompilerError::Type( - "T21: rrf primary expression must be nearest(...) or bm25(...)".to_string(), + return Err(CompilerError::typed( + T21, + "rrf primary expression must be nearest(...) or bm25(...)".to_string(), )); } if !matches!(secondary.as_ref(), Expr::Nearest { .. } | Expr::Bm25 { .. }) { - return Err(CompilerError::Type( - "T21: rrf secondary expression must be nearest(...) or bm25(...)".to_string(), + return Err(CompilerError::typed( + T21, + "rrf secondary expression must be nearest(...) or bm25(...)".to_string(), )); } @@ -1971,15 +2198,18 @@ fn resolve_expr_type( match ty { ResolvedType::Scalar(s) if s.scalar == ScalarType::F32 && !s.list => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T21: rrf rank expressions must evaluate to F32, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T21, + format!( + "rrf rank expressions must evaluate to F32, got {}", + s.display_name() + ), + )); } _ => { - return Err(CompilerError::Type( - "T21: rrf rank expressions must be scalar numeric expressions" - .to_string(), + return Err(CompilerError::typed( + T21, + "rrf rank expressions must be scalar numeric expressions".to_string(), )); } } @@ -1998,22 +2228,24 @@ fn resolve_expr_type( | ScalarType::U64 ) => {} ResolvedType::Scalar(s) => { - return Err(CompilerError::Type(format!( - "T21: rrf k must be an integer scalar, got {}", - s.display_name() - ))); + return Err(CompilerError::typed( + T21, + format!("rrf k must be an integer scalar, got {}", s.display_name()), + )); } _ => { - return Err(CompilerError::Type( - "T21: rrf k must be an integer scalar expression".to_string(), + return Err(CompilerError::typed( + T21, + "rrf k must be an integer scalar expression".to_string(), )); } } if let Expr::Literal(Literal::Integer(v)) = k_expr.as_ref() && *v <= 0 { - return Err(CompilerError::Type( - "T21: rrf k must be greater than 0".to_string(), + return Err(CompilerError::typed( + T21, + "rrf k must be greater than 0".to_string(), )); } } @@ -2032,16 +2264,19 @@ fn resolve_expr_type( } else if let Some(bv) = ctx.bindings.get(name) { match bv { BoundVariable::Node { type_name } => Ok(ResolvedType::Node(type_name.clone())), - BoundVariable::Edge { .. } => Err(CompilerError::Type(format!( - "T23: edge binding `${}` cannot be used bare; access one of its properties (`${}.{{prop}}`)", - name, name - ))), + BoundVariable::Edge { .. } => Err(CompilerError::typed( + T23, + format!( + "edge binding `${}` cannot be used bare; access one of its properties (`${}.{{prop}}`)", + name, name + ), + )), } } else { - Err(CompilerError::Type(format!( - "variable `${}` is not bound", - name - ))) + Err(CompilerError::typed( + T6, + format!("variable `${}` is not bound", name), + )) } } Expr::Literal(lit) => Ok(ResolvedType::Scalar(literal_type(lit)?)), @@ -2079,10 +2314,13 @@ fn resolve_expr_type( } let list_type = resolve_expr_type(catalog, list, ctx, params, scope)?; if !matches!(&list_type, ResolvedType::Scalar(list) if list.list) { - return Err(CompilerError::Type(format!( - "T7: `in` needs a list on the right, got {}", - list_type.display_name() - ))); + return Err(CompilerError::typed( + T7, + format!( + "`in` needs a list on the right, got {}", + list_type.display_name() + ), + )); } Ok(ResolvedType::Scalar(typecheck_comparison( catalog, @@ -2099,11 +2337,14 @@ fn resolve_expr_type( let right_type = resolve_expr_type(catalog, right, ctx, params, scope)?; let (Some(l), Some(r)) = (boolean_scalar(&left_type), boolean_scalar(&right_type)) else { - return Err(CompilerError::Type(format!( - "T41: `{op}` needs Bool operands, got {} and {}", - left_type.display_name(), - right_type.display_name() - ))); + return Err(CompilerError::typed( + T41, + format!( + "`{op}` needs Bool operands, got {} and {}", + left_type.display_name(), + right_type.display_name() + ), + )); }; Ok(ResolvedType::Scalar(PropType::scalar( ScalarType::Bool, @@ -2113,10 +2354,13 @@ fn resolve_expr_type( Expr::Not(inner) => { let inner_type = resolve_expr_type(catalog, inner, ctx, params, scope)?; let Some(b) = boolean_scalar(&inner_type) else { - return Err(CompilerError::Type(format!( - "T41: `not` needs a Bool operand, got {}", - inner_type.display_name() - ))); + return Err(CompilerError::typed( + T41, + format!( + "`not` needs a Bool operand, got {}", + inner_type.display_name() + ), + )); }; Ok(ResolvedType::Scalar(PropType::scalar( ScalarType::Bool, @@ -2128,16 +2372,20 @@ fn resolve_expr_type( && let Expr::PropAccess { variable, property } = inner.as_ref() && read_property_type(catalog, ctx, variable, property)?.scalar == ScalarType::Blob { - return Err(CompilerError::Type( - "T7: blob comparisons in filters are not supported".to_string(), + return Err(CompilerError::typed( + T7, + "blob comparisons in filters are not supported".to_string(), )); } let inner_type = resolve_expr_type(catalog, inner, ctx, params, scope)?; if !matches!(inner_type, ResolvedType::Scalar(_)) { - return Err(CompilerError::Type(format!( - "T41: `is null` tests a scalar or list value, got {}", - inner_type.display_name() - ))); + return Err(CompilerError::typed( + T41, + format!( + "`is null` tests a scalar or list value, got {}", + inner_type.display_name() + ), + )); } Ok(ResolvedType::Scalar(PropType::scalar( ScalarType::Bool, @@ -2160,9 +2408,12 @@ fn reject_blob_read_value(resolved: &ResolvedType, expr: &Expr) -> Result<()> { Expr::AliasRef(name) => format!("Blob alias `{name}`"), _ => "Blob expression".to_string(), }; - return Err(CompilerError::Type(format!( - "T24: {subject} is not available as a .gq read value; Blob values require a dedicated API" - ))); + return Err(CompilerError::typed( + T24, + format!( + "{subject} is not available as a .gq read value; Blob values require a dedicated API" + ), + )); } Ok(()) } @@ -2175,42 +2426,58 @@ fn check_projection(expr: &Expr, alias: Option<&str>, order_clause: &[Ordering]) Expr::Now | Expr::PropAccess { .. } | Expr::Variable(_) | Expr::Literal(_) => Ok(()), Expr::Aggregate { func, arg } => match arg.as_ref() { Expr::Nearest { .. } | Expr::Bm25 { .. } | Expr::Rrf { .. } => { - Err(CompilerError::Type(format!( - "T32: `{}` under `{func}` in `return`: a retrieval selects the rows an aggregate counts; state the filter instead", - rank_keyword(arg) - ))) + Err(CompilerError::typed( + T32, + format!( + "`{}` under `{func}` in `return`: a retrieval selects the rows an aggregate counts; state the filter instead", + rank_keyword(arg) + ), + )) } inner => check_projection(inner, alias, order_clause), }, Expr::Nearest { .. } | Expr::Bm25 { .. } => { let executed = order_clause.first().is_some_and(|lead| &lead.expr == expr); if !executed { - return Err(CompilerError::Type(format!( - "T33: `{}` in `return` must repeat the retrieval stated as the leading `order` key; the projection reads the score that ordering computed", - rank_keyword(expr) - ))); + return Err(CompilerError::typed( + T33, + format!( + "`{}` in `return` must repeat the retrieval stated as the leading `order` key; the projection reads the score that ordering computed", + rank_keyword(expr) + ), + )); } if expr.score_column().is_none() { - return Err(CompilerError::Type(format!( - "T33: `{}` projects its score only over a property field; name the property the retrieval ranks", - rank_keyword(expr) - ))); + return Err(CompilerError::typed( + T33, + format!( + "`{}` projects its score only over a property field; name the property the retrieval ranks", + rank_keyword(expr) + ), + )); } Ok(()) } - Expr::Rrf { .. } => Err(CompilerError::Type( - "T37: `rrf` cannot be projected in `return`; order by `rrf(...)` and project plain columns" + Expr::Rrf { .. } => Err(CompilerError::typed( + T37, + "`rrf` cannot be projected in `return`; order by `rrf(...)` and project plain columns" .to_string(), )), Expr::Search { .. } | Expr::Fuzzy { .. } | Expr::MatchText { .. } => { - Err(CompilerError::Type(format!( - "T35: `{}` cannot be projected in `return`; a search predicate belongs in `match`", - rank_keyword(expr) - ))) + Err(CompilerError::typed( + T35, + format!( + "`{}` cannot be projected in `return`; a search predicate belongs in `match`", + rank_keyword(expr) + ), + )) } - Expr::AliasRef(name) => Err(CompilerError::Type(format!( - "T36: `{name}` cannot be projected in `return`; an alias is resolved in `order`, not projected again" - ))), + Expr::AliasRef(name) => Err(CompilerError::typed( + T36, + format!( + "`{name}` cannot be projected in `return`; an alias is resolved in `order`, not projected again" + ), + )), Expr::Binary { left, right, .. } | Expr::In { needle: left, @@ -2229,8 +2496,9 @@ fn check_projection(expr: &Expr, alias: Option<&str>, order_clause: &[Ordering]) fn require_projection_alias(alias: Option<&str>) -> Result<()> { if alias.is_none() { - return Err(CompilerError::Type( - "T43: a comparison in return needs an alias; write `… as `".to_string(), + return Err(CompilerError::typed( + T43, + "a comparison in return needs an alias; write `… as `".to_string(), )); } Ok(()) @@ -2354,9 +2622,10 @@ fn projection_name(expr: &Expr, alias: Option<&str>) -> String { /// orderable scalar; none takes an aggregate. fn check_aggregate_argument(func: &AggFunc, arg: &Expr, arg_type: &ResolvedType) -> Result<()> { match (func, arg_type) { - (_, ResolvedType::Aggregate) => Err(CompilerError::Type(format!( - "T8: {func} cannot take an aggregate or a forward alias reference as its argument" - ))), + (_, ResolvedType::Aggregate) => Err(CompilerError::typed( + T8, + format!("{func} cannot take an aggregate or a forward alias reference as its argument"), + )), (AggFunc::Count, _) => Ok(()), (_, ResolvedType::Node(_)) => { let subject = match arg { @@ -2364,27 +2633,32 @@ fn check_aggregate_argument(func: &AggFunc, arg: &Expr, arg_type: &ResolvedType) Expr::AliasRef(alias) => format!("node projection `{alias}`"), other => format!("node value `{other:?}`"), }; - Err(CompilerError::Type(format!( - "T8: {func} cannot take {subject} bare; access one of the node's properties (`$var.{{prop}}`)" - ))) + Err(CompilerError::typed( + T8, + format!( + "{func} cannot take {subject} bare; access one of the node's properties (`$var.{{prop}}`)" + ), + )) } (AggFunc::Sum | AggFunc::Avg, ResolvedType::Scalar(s)) if s.list || !s.scalar.is_numeric() => { - Err(CompilerError::Type(format!( - "T8: {} requires numeric type, got {}", - func, - s.display_name() - ))) + Err(CompilerError::typed( + T8, + format!("{} requires numeric type, got {}", func, s.display_name()), + )) } (AggFunc::Min | AggFunc::Max, ResolvedType::Scalar(s)) if s.list || !s.scalar.is_orderable() => { - Err(CompilerError::Type(format!( - "T8: {} requires a numeric, String, Bool, Date, or DateTime scalar, got {}", - func, - s.display_name() - ))) + Err(CompilerError::typed( + T8, + format!( + "{} requires a numeric, String, Bool, Date, or DateTime scalar, got {}", + func, + s.display_name() + ), + )) } _ => Ok(()), } @@ -2398,7 +2672,7 @@ fn resolved_type_to_field_shape( ResolvedType::Scalar(prop_type) => Ok((prop_type.to_arrow(), prop_type.nullable)), ResolvedType::Node(type_name) => { let node_type = catalog.node_types.get(type_name).ok_or_else(|| { - CompilerError::Type(format!("type `{}` not found in catalog", type_name)) + CompilerError::typed(T51, format!("type `{}` not found in catalog", type_name)) })?; let fields: Vec = node_type .node_object_members() @@ -2435,9 +2709,10 @@ fn membership_over_a_node( ), None => "`in` takes a value on the left and a list on the right".to_string(), }; - Some(CompilerError::Type(format!( - "T7: `${node} in ${other}` tests membership and `${node}` is a node; {advice}" - ))) + Some(CompilerError::typed( + T7, + format!("`${node} in ${other}` tests membership and `${node}` is a node; {advice}"), + )) } fn literal_type(lit: &Literal) -> Result { @@ -2450,7 +2725,7 @@ fn literal_type(lit: &Literal) -> Result { Literal::Bool(_) => Ok(PropType::scalar(ScalarType::Bool, false)), Literal::Date(value) => { crate::types::check_date_literal(value) - .map_err(|reason| CompilerError::Type(format!("T3: {reason}")))?; + .map_err(|reason| CompilerError::typed(T3, reason.to_string()))?; Ok(PropType::scalar(ScalarType::Date, false)) } Literal::DateTime(_) => Ok(PropType::scalar(ScalarType::DateTime, false)), @@ -2460,14 +2735,16 @@ fn literal_type(lit: &Literal) -> Result { } let first = literal_type(&items[0])?; if first.list { - return Err(CompilerError::Type( + return Err(CompilerError::typed( + T52, "nested list literals are not supported".to_string(), )); } for item in items.iter().skip(1) { let item_type = literal_type(item)?; if item_type.list || !types_compatible(&first, &item_type) { - return Err(CompilerError::Type( + return Err(CompilerError::typed( + T53, "list literal elements must share a compatible scalar type".to_string(), )); } @@ -2483,10 +2760,10 @@ fn check_literal_type(lit: &Literal, expected: &PropType, prop_name: &str) -> Re return if expected.nullable { Ok(()) } else { - Err(CompilerError::Type(format!( - "T3: property `{}` is non-nullable but got null", - prop_name - ))) + Err(CompilerError::typed( + T3, + format!("property `{}` is non-nullable but got null", prop_name), + )) }; } @@ -2497,43 +2774,55 @@ fn check_literal_type(lit: &Literal, expected: &PropType, prop_name: &str) -> Re if actual_dim == expected_dim { return Ok(()); } - return Err(CompilerError::Type(format!( - "T3: property `{}` has type Vector({}) but got vector literal with {} elements", - prop_name, expected_dim, actual_dim - ))); + return Err(CompilerError::typed( + T3, + format!( + "property `{}` has type Vector({}) but got vector literal with {} elements", + prop_name, expected_dim, actual_dim + ), + )); } let lit_type = literal_type(lit)?; if !types_compatible(&lit_type, expected) { - return Err(CompilerError::Type(format!( - "T3: property `{}` has type {} but got {}", - prop_name, - expected.display_name(), - lit_type.display_name() - ))); + return Err(CompilerError::typed( + T3, + format!( + "property `{}` has type {} but got {}", + prop_name, + expected.display_name(), + lit_type.display_name() + ), + )); } if expected.is_enum() { let allowed = expected.enum_values.as_ref().cloned().unwrap_or_default(); match lit { Literal::String(v) if !allowed.contains(v) => { - return Err(CompilerError::Type(format!( - "T3: property `{}` expects one of [{}], got '{}'", - prop_name, - allowed.join(", "), - v - ))); + return Err(CompilerError::typed( + T3, + format!( + "property `{}` expects one of [{}], got '{}'", + prop_name, + allowed.join(", "), + v + ), + )); } Literal::List(items) if expected.list => { for item in items { match item { Literal::String(v) if allowed.contains(v) => {} Literal::String(v) => { - return Err(CompilerError::Type(format!( - "T3: property `{}` expects one of [{}], got '{}'", - prop_name, - allowed.join(", "), - v - ))); + return Err(CompilerError::typed( + T3, + format!( + "property `{}` expects one of [{}], got '{}'", + prop_name, + allowed.join(", "), + v + ), + )); } _ => {} } diff --git a/crates/omnigraph-compiler/src/query/typecheck_tests.rs b/crates/omnigraph-compiler/src/query/typecheck_tests.rs index 3f7e46364..d130de1d7 100644 --- a/crates/omnigraph-compiler/src/query/typecheck_tests.rs +++ b/crates/omnigraph-compiler/src/query/typecheck_tests.rs @@ -2253,3 +2253,29 @@ fn test_assignments_and_binding_matches_take_constants() { "type error: T3: match variable `$missing` must be a declared query parameter" ); } + +#[test] +fn a_type_error_exposes_its_diagnostic_with_code_and_stage() { + let catalog = setup(); + let file = parse_query("query q() { match { $x: Nowhere } return { $x.name } }").unwrap(); + let decl = &file.into_declarations().unwrap()[0]; + let err = typecheck_query_decl(&catalog, decl).unwrap_err(); + let diagnostic = err + .diagnostic() + .expect("a typecheck refusal carries its diagnostic"); + assert_eq!(diagnostic.code.as_str(), "T1"); + assert_eq!( + diagnostic.stage.as_ref().map(|stage| stage.name), + Some("typecheck") + ); + assert!(diagnostic.position.is_none()); + assert_eq!( + err.to_string(), + format!("type error: T1: {}", diagnostic.message) + ); + assert!( + crate::query::codes::ALL + .iter() + .any(|code| code.as_str() == "T1") + ); +} diff --git a/crates/omnigraph-core/src/error.rs b/crates/omnigraph-core/src/error.rs index cea12fbec..a92302533 100644 --- a/crates/omnigraph-core/src/error.rs +++ b/crates/omnigraph-core/src/error.rs @@ -622,6 +622,15 @@ impl OmniError { ) } + /// The structured query compile diagnostic behind this error, when it is + /// one (RFC 0047's diagnostics contract). + pub fn diagnostic(&self) -> Option<&omnigraph_compiler::QueryDiagnostic> { + match self { + Self::Compiler(err) => err.diagnostic(), + _ => None, + } + } + pub fn manifest(message: impl Into) -> Self { Self::Manifest(ManifestError::new(ManifestErrorKind::BadRequest, message)) } diff --git a/crates/omnigraph-server/src/handlers/dispatch.rs b/crates/omnigraph-server/src/handlers/dispatch.rs index 45aafefb9..e607d49cb 100644 --- a/crates/omnigraph-server/src/handlers/dispatch.rs +++ b/crates/omnigraph-server/src/handlers/dispatch.rs @@ -114,7 +114,7 @@ pub(super) fn session_with_prefix( /// statement. Runs before target resolution, authorization, and admission at /// every door, so the door's Cedar action is known before it is checked. pub(super) fn classify(query: &str) -> std::result::Result { - parse_query(query).map_err(|err| ApiError::bad_request(err.to_string())) + parse_query(query).map_err(|err| ApiError::from_compiler(&err)) } pub(super) fn control_write_at_read_door(write: &BranchWrite) -> ApiError { diff --git a/crates/omnigraph-server/src/lib.rs b/crates/omnigraph-server/src/lib.rs index 5ccc5ce1d..15240865b 100644 --- a/crates/omnigraph-server/src/lib.rs +++ b/crates/omnigraph-server/src/lib.rs @@ -459,6 +459,7 @@ enum ApiErrorDetails { ChangeFeedGap(api::ChangeFeedGapOutput), ChangeDiffRefusal(api::ChangeDiffRefusalOutput), FullTextIndexRebuildRequired(api::FullTextIndexRebuildRequiredOutput), + Diagnostic(api::DiagnosticOutput), } impl AppState { @@ -1195,9 +1196,21 @@ impl ApiError { } } + /// A refused query: HTTP 400 with the one-line message, plus the + /// compiler's structured diagnostic when the refusal carries one. + fn from_compiler(err: &omnigraph_compiler::error::CompilerError) -> Self { + let mut response = Self::bad_request(err.to_string()); + if let Some(diagnostic) = err.diagnostic() { + response.details = Some(Box::new(ApiErrorDetails::Diagnostic( + api::DiagnosticOutput::from(diagnostic), + ))); + } + response + } + fn from_omni(err: OmniError) -> Self { match err { - OmniError::Compiler(err) => Self::bad_request(err.to_string()), + OmniError::Compiler(err) => Self::from_compiler(&err), OmniError::DataFusion(message) => Self::bad_request(format!("query: {message}")), OmniError::Manifest(err) => match err.kind { ManifestErrorKind::BadRequest => Self::bad_request(err.message), @@ -1452,22 +1465,8 @@ impl IntoResponse for ApiError { axum::http::HeaderValue::from_static(RETRY_AFTER_SECONDS), ); } - let mut output = ErrorOutput { - error: self.message.into(), - code: self.code, - merge_conflicts: Vec::new(), - published_dataset_version_conflict: None, - read_set_conflict: None, - key_conflict: None, - resource_limit: None, - blob_range: None, - external_blob_source: None, - recovery_required: None, - precondition_failure: None, - change_feed_gap: None, - change_diff_refusal: None, - full_text_index_rebuild_required: None, - }; + let mut output = ErrorOutput::message(self.message); + output.code = self.code; if let Some(details) = self.details { match *details { ApiErrorDetails::MergeConflicts(value) => output.merge_conflicts = value, @@ -1492,6 +1491,7 @@ impl IntoResponse for ApiError { ApiErrorDetails::FullTextIndexRebuildRequired(value) => { output.full_text_index_rebuild_required = Some(value) } + ApiErrorDetails::Diagnostic(value) => output.diagnostic = Some(value), } } (self.status, headers, Json(output)).into_response() @@ -1542,6 +1542,38 @@ mod api_error_tests { assert!(ordinary.full_text_index_rebuild_required.is_none()); } + #[tokio::test] + async fn a_refused_query_returns_bad_request_with_its_diagnostic() { + let err = omnigraph_compiler::query::parser::parse_query("query name {").unwrap_err(); + let response = ApiError::from_omni(OmniError::Compiler(err)).into_response(); + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + let body = axum::body::to_bytes(response.into_body(), 8192) + .await + .unwrap(); + let error: ErrorOutput = serde_json::from_slice(&body).unwrap(); + assert_eq!(error.code, Some(ErrorCode::BadRequest)); + assert_eq!( + error.error, + "parse error: expected `(`: a query declares its parameters even when it has none" + ); + let diagnostic = error + .diagnostic + .expect("a refused query carries its diagnostic"); + assert_eq!(diagnostic.code, "Q002"); + assert_eq!(diagnostic.fix.as_deref(), Some("query name()")); + let at = diagnostic.position.unwrap(); + assert_eq!((at.line, at.column), (1, 11)); + assert!(diagnostic.stage.is_none()); + + // An ordinary bad request carries no diagnostic key at all. + let response = ApiError::bad_request("no").into_response(); + let body = axum::body::to_bytes(response.into_body(), 8192) + .await + .unwrap(); + let json: Value = serde_json::from_slice(&body).unwrap(); + assert!(json.get("diagnostic").is_none()); + } + #[test] fn merge_summary_uses_type_and_entity_vocabulary() { let summary = summarize_merge_conflicts(&[api::MergeConflictOutput { diff --git a/crates/omnigraph-server/src/queries.rs b/crates/omnigraph-server/src/queries.rs index b8ccc1bc7..48bdfe382 100644 --- a/crates/omnigraph-server/src/queries.rs +++ b/crates/omnigraph-server/src/queries.rs @@ -85,6 +85,7 @@ pub struct LoadError { /// The offending query name, when the failure is entry-scoped. pub query: Option, pub message: String, + pub diagnostic: Option>, } impl std::fmt::Display for LoadError { @@ -107,13 +108,22 @@ impl QueryRegistry { for spec in specs { let declarations = parse_query(&spec.source) - .map_err(|err| err.to_string()) + .map_err(|err| LoadError { + query: Some(spec.name.clone()), + message: err.to_string(), + diagnostic: err.diagnostic().cloned().map(Box::new), + }) .and_then(|file| { if file.settings.is_empty() { file.into_declarations() } else { Err(STORED_QUERY_CARRIES_NO_SETTINGS.to_string()) } + .map_err(|message| LoadError { + query: Some(spec.name.clone()), + message, + diagnostic: None, + }) }); match declarations { Ok(queries) => match queries.into_iter().find(|q| q.name == spec.name) { @@ -136,12 +146,10 @@ impl QueryRegistry { (the registry key must match the query symbol)", spec.name ), + diagnostic: None, }), }, - Err(message) => errors.push(LoadError { - query: Some(spec.name), - message, - }), + Err(error) => errors.push(error), } } @@ -163,6 +171,7 @@ impl QueryRegistry { message: format!( "MCP tool name '{tool}' already claimed by exposed query '{winner}'" ), + diagnostic: None, }); } } @@ -201,6 +210,9 @@ impl QueryRegistry { pub struct Breakage { pub query: String, pub message: String, + /// The compiler's diagnostic (code, stage, expectation, fix) behind the + /// message, when the refusal carries one. + pub diagnostic: Option, } /// A non-blocking advisory found during validation. Logged at boot; @@ -254,6 +266,7 @@ pub fn check(registry: &QueryRegistry, catalog: &Catalog) -> CheckReport { report.breakages.push(Breakage { query: query.name.clone(), message: err.to_string(), + diagnostic: err.diagnostic().cloned(), }); } if query.expose { @@ -431,13 +444,22 @@ mod tests { #[test] fn parse_error_surfaces_per_entry() { - let errors = - QueryRegistry::from_specs(vec![spec("broken", "query broken( {{ not valid", false)]) - .unwrap_err(); + let source = "query broken { match { $u: User } return { $u.name } }"; + let errors = QueryRegistry::from_specs(vec![spec("broken", source, false)]).unwrap_err(); assert_eq!(errors[0].query.as_deref(), Some("broken")); let message = &errors[0].message; assert!(message.starts_with("parse error: "), "{message}"); assert_eq!(message.matches("parse error:").count(), 1, "{message}"); + let diagnostic = errors[0].diagnostic.as_deref().unwrap(); + assert_eq!( + diagnostic, + parse_query(source).unwrap_err().diagnostic().unwrap() + ); + assert_eq!(diagnostic.code.as_str(), "Q002"); + assert_eq!( + diagnostic.suggestion.as_ref().unwrap().edits[0].replacement, + "()" + ); } #[test] diff --git a/crates/omnigraph-server/tests/data_routes.rs b/crates/omnigraph-server/tests/data_routes.rs index 1cd373b8a..4dbb5aaed 100644 --- a/crates/omnigraph-server/tests/data_routes.rs +++ b/crates/omnigraph-server/tests/data_routes.rs @@ -1846,6 +1846,47 @@ async fn parse_error_precedes_policy_denial_on_every_door() { assert_eq!(status, StatusCode::BAD_REQUEST, "{path}: {out}"); let error: ErrorOutput = serde_json::from_value(out).unwrap(); assert_ne!(error.code, Some(ErrorCode::Forbidden), "{path}"); + let diagnostic = error + .diagnostic + .expect("a refused query carries its diagnostic"); + assert_eq!(diagnostic.code, "Q001", "{path}"); + assert!(diagnostic.position.is_some(), "{path}"); + + // The measured shape, a declaration without its parameter list, is + // refused at the name's end with the one fix at every door. + let field = if path == "/read" { + "query_source" + } else { + "query" + }; + let missing = json!({field: "query name { match { $p: Person } return { $p.name } }", "branch": "main"}); + let (status, out) = json_response(&app, send(path, token, missing, expected_head)).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{path}: {out}"); + let error: ErrorOutput = serde_json::from_value(out).unwrap(); + let diagnostic = error + .diagnostic + .expect("a refused query carries its diagnostic"); + assert_eq!(diagnostic.code, "Q002", "{path}"); + assert_eq!(diagnostic.fix.as_deref(), Some("query name()"), "{path}"); + let suggestion = diagnostic.suggestion.unwrap(); + assert_eq!( + suggestion.applicability, + omnigraph_api_types::ApplicabilityOutput::MachineApplicable + ); + assert_eq!( + suggestion.edits, + vec![omnigraph_api_types::TextEditOutput { + start: 10, + end: 10, + replacement: "()".to_string(), + }], + "{path}" + ); + assert_eq!( + diagnostic.position.map(|at| (at.line, at.column)), + Some((1, 11)), + "{path}" + ); } } diff --git a/crates/omnigraph-server/tests/openapi.rs b/crates/omnigraph-server/tests/openapi.rs index 13036a1c4..5f1c7284f 100644 --- a/crates/omnigraph-server/tests/openapi.rs +++ b/crates/omnigraph-server/tests/openapi.rs @@ -940,6 +940,11 @@ const EXPECTED_SCHEMAS: &[&str] = &[ "ErrorCode", "ErrorOutput", "FullTextIndexRebuildRequiredOutput", + "DiagnosticOutput", + "PositionOutput", + "SuggestionOutput", + "ApplicabilityOutput", + "TextEditOutput", "EntityKindOutput", "ChangeFeedGapOutput", "ChangeOpOutput", @@ -1330,6 +1335,50 @@ fn error_output_schema_has_expected_fields() { .map(|field| field.as_str().unwrap()) .collect(); assert_eq!(required, HashSet::from(["index", "reason"])); + // A refused query's diagnostic is an optional detail with the contract's + // four fields; the code and the expectation are always present. + let diagnostic = &props["diagnostic"]; + assert!( + diagnostic["oneOf"] + .as_array() + .unwrap() + .iter() + .any(|schema| { schema["$ref"] == "#/components/schemas/DiagnosticOutput" }) + ); + assert!( + !schema["required"] + .as_array() + .unwrap() + .iter() + .any(|field| field == "diagnostic") + ); + let details = &doc["components"]["schemas"]["DiagnosticOutput"]; + let required: HashSet<&str> = details["required"] + .as_array() + .unwrap() + .iter() + .map(|field| field.as_str().unwrap()) + .collect(); + assert_eq!(required, HashSet::from(["code", "expected"])); + for field in ["position", "stage", "expression", "fix", "suggestion"] { + assert!(details["properties"].get(field).is_some(), "{field}"); + } + for (name, fields) in [ + ("SuggestionOutput", vec!["applicability", "edits"]), + ("TextEditOutput", vec!["start", "end", "replacement"]), + ] { + let required: HashSet<&str> = doc["components"]["schemas"][name]["required"] + .as_array() + .unwrap() + .iter() + .map(|field| field.as_str().unwrap()) + .collect(); + assert_eq!(required, fields.into_iter().collect(), "{name}"); + } + assert_eq!( + doc["components"]["schemas"]["ApplicabilityOutput"]["enum"], + serde_json::json!(["machine_applicable", "needs_review"]) + ); for path in [ "/graphs/{graph_id}/query", "/graphs/{graph_id}/read", diff --git a/crates/omnigraph/src/exec/mod.rs b/crates/omnigraph/src/exec/mod.rs index 011e21356..d677d9195 100644 --- a/crates/omnigraph/src/exec/mod.rs +++ b/crates/omnigraph/src/exec/mod.rs @@ -38,3 +38,15 @@ pub(crate) mod merge; pub(crate) mod mutation; mod query_doors; pub(crate) mod staging; + +/// A failure to find a named statement in query text, as an engine error: a +/// compile refusal keeps its diagnostic (code, position, fix) on the read and +/// the mutation door alike, and any other lookup failure is a bad request. +pub(crate) fn query_lookup_error(error: omnigraph_compiler::RunInputError) -> OmniError { + match error { + omnigraph_compiler::RunInputError::Core( + query @ omnigraph_compiler::error::CompilerError::Query(_), + ) => OmniError::Compiler(query), + other => OmniError::manifest(other.to_string()), + } +} diff --git a/crates/omnigraph/src/exec/mutation.rs b/crates/omnigraph/src/exec/mutation.rs index 9ef2b2a30..9fc2725c7 100644 --- a/crates/omnigraph/src/exec/mutation.rs +++ b/crates/omnigraph/src/exec/mutation.rs @@ -1054,7 +1054,7 @@ impl Omnigraph { query_name: &str, ) -> Result { let query_decl = omnigraph_compiler::find_named_query(query_source, query_name) - .map_err(|e| OmniError::manifest(e.to_string()))?; + .map_err(query_lookup_error)?; let checked = typecheck_query_decl(catalog, &query_decl)?; match checked { diff --git a/crates/omnigraph/src/exec/query_doors.rs b/crates/omnigraph/src/exec/query_doors.rs index 90bd8b5d5..1d123a59b 100644 --- a/crates/omnigraph/src/exec/query_doors.rs +++ b/crates/omnigraph/src/exec/query_doors.rs @@ -298,7 +298,7 @@ impl Omnigraph { return Ok(compiled); } let statement = omnigraph_compiler::find_read_statement(query_source, query_name) - .map_err(|e| OmniError::manifest(e.to_string()))?; + .map_err(super::query_lookup_error)?; let type_ctx = typecheck_query(catalog, statement.decl())?; let ir = Arc::new(lower_query(catalog, statement.decl(), &type_ctx)?); let compiled = if statement.is_explain() { diff --git a/crates/omnigraph/src/session.rs b/crates/omnigraph/src/session.rs index e34305e50..43610e981 100644 --- a/crates/omnigraph/src/session.rs +++ b/crates/omnigraph/src/session.rs @@ -8,6 +8,8 @@ use std::sync::Arc; use omnigraph_compiler::error::CompilerError; use omnigraph_compiler::query::ast::SettingStmt; +use omnigraph_compiler::query::codes::Q003; +use omnigraph_compiler::query::diagnostic::QueryDiagnostic; use omnigraph_compiler::query::parser::{has_settings_prefix, parse_query}; use omnigraph_compiler::settings::{ DEFINITIONS, SessionSettings, SessionSettingsError, SettingId, SettingRow, SettingValue, @@ -185,9 +187,13 @@ impl Session { return Ok(self.settings.clone()); } let file = parse_query(source).map_err(OmniError::Compiler)?; - let scoped = self - .with_prefix(&file.settings) - .map_err(|error| OmniError::Compiler(CompilerError::Parse(error.to_string())))?; + let scoped = self.with_prefix(&file.settings).map_err(|error| { + OmniError::Compiler(CompilerError::query(QueryDiagnostic::parse( + Q003, + error.to_string(), + None, + ))) + })?; Ok(scoped.settings) } diff --git a/crates/omnigraph/tests/writes.rs b/crates/omnigraph/tests/writes.rs index b6f528954..2e0e90f71 100644 --- a/crates/omnigraph/tests/writes.rs +++ b/crates/omnigraph/tests/writes.rs @@ -161,6 +161,36 @@ async fn failed_mutation_leaves_target_unchanged() { assert_eq!(db.branch_list().await.unwrap(), vec!["main".to_string()]); } +/// Mutation text the parser refuses keeps its diagnostic through the engine +/// door, as read text does: the code and the position survive, and nothing +/// is published. +#[tokio::test] +async fn refused_mutation_text_keeps_its_parse_diagnostic() { + let dir = tempfile::tempdir().unwrap(); + let db = init_and_load(&dir).await; + let uri = dir.path().to_str().unwrap(); + let head = head_commit_id(uri).await; + + let err = db + .mutate( + "main", + "query add() { insert Person { name \"Zed\" } }", + "add", + ¶ms(&[]), + ) + .await + .unwrap_err(); + let diagnostic = err + .diagnostic() + .unwrap_or_else(|| panic!("a parse refusal carries its diagnostic: {err}")); + assert_eq!(diagnostic.code.as_str(), "Q001"); + // The refused assignment starts at `name`, where the parser reports it. + let position = diagnostic.position.expect("a parse refusal has a position"); + assert_eq!((position.line, position.column), (1, 31)); + + assert_eq!(head_commit_id(uri).await, head); +} + /// Multi-statement mutations are atomic at the query boundary. The /// `insert_person_and_friend` query inserts a person and an edge that /// references it; both must land together (read-your-writes within the diff --git a/docs/releases/v0.12.0.md b/docs/releases/v0.12.0.md index 52863a456..360a6dd67 100644 --- a/docs/releases/v0.12.0.md +++ b/docs/releases/v0.12.0.md @@ -481,6 +481,18 @@ Unreleased. text filter on a BM25-ranked binding is preserved by applying its membership set without changing the BM25 scoring query. +- **A refused query carries its diagnostic.** Every parse or type refusal + now reports a stable code (`Q…`, `T…`), where the failure is (line and + column, or the compiler stage), what was expected, and one fix. The HTTP + `400` body gains an additive `diagnostic` object; the CLI prints the four + fields on stderr for human formats and the API's error body for `--json` + and `--format jsonl`, with no colour codes and no backtrace footer. + `queries validate --json` and `cluster plan --json` carry the same detail + beside their messages. A declaration without its parameter list, `query + name {`, is refused at the name's end with the fix `query name()` where it + was reported at the file's first position; the one-line `error` text of a + parse refusal no longer embeds the parser's multi-line rendering. See + [Diagnostics](../user/queries/diagnostics.md). - **Setting environment variables are process defaults, and an invalid value refuses startup.** `OMNIGRAPH_RRF_PLAN`, diff --git a/docs/user/cli/reference.md b/docs/user/cli/reference.md index 11a7b3522..025f927c2 100644 --- a/docs/user/cli/reference.md +++ b/docs/user/cli/reference.md @@ -93,7 +93,7 @@ Read output supports `table`, `json`, `jsonl`, `csv`, and `kv`. `--json` is the stable machine-readable form for commands that do not use `--format`. Result cells use the [JSON result spelling](../queries/index.md#json-result-spelling); `table`, `csv`, and `kv` print strings unquoted. `--format json` prints the -envelope pretty and the `rows` array compact, verbatim. +envelope pretty and the `rows` array compact; a refusal follows [Diagnostics](../queries/diagnostics.md). ### Machine-readable read and write positions diff --git a/docs/user/index.md b/docs/user/index.md index 07e7d2e35..daac5fde4 100644 --- a/docs/user/index.md +++ b/docs/user/index.md @@ -14,6 +14,7 @@ deployment. - [Schema language](schema/index.md) - [Query language](queries/index.md), [session settings](queries/settings.md), and [explain](queries/explain.md) +- [Diagnostics](queries/diagnostics.md) - [Mutations and loading](mutations/index.md) - [Search](search/index.md) and [embeddings](search/embeddings.md) - [Blob values](blobs.md) diff --git a/docs/user/operations/troubleshooting.md b/docs/user/operations/troubleshooting.md index a41425567..dd208a776 100644 --- a/docs/user/operations/troubleshooting.md +++ b/docs/user/operations/troubleshooting.md @@ -31,6 +31,12 @@ A graph-head `412` includes `precondition_failure` with `expected` and, when available, `actual`. A change-feed `410` includes `change_feed_gap`; retrying the same cursor cannot recover its missing history. +A `400` that refuses a query includes `diagnostic`: the stable code (`Q…` +parse, `T…` typecheck), `position` (`line`, `column`, `byte`) for a parse +refusal or `stage` (and `expression` when known) for a later one, `expected`, +and `fix` when one exists. Act on the fix; a retry of the same source fails +the same way. See [Diagnostics](../queries/diagnostics.md). + ## Conflicts A `409` is not one universal retry signal: diff --git a/docs/user/queries/diagnostics.md b/docs/user/queries/diagnostics.md new file mode 100644 index 000000000..85e4a0ee3 --- /dev/null +++ b/docs/user/queries/diagnostics.md @@ -0,0 +1,116 @@ +# Diagnostics + +Every refusal of a query carries four things: a stable code, where the failure +is, what was expected or violated, and one concrete fix. The reader is often +an agent that treats an error as the documentation it acts on, so the fix +names the construct to use, not the rule that was broken. A refusal with no +fix names the decision instead. + +| Field | Meaning | +|---|---| +| `code` | Stable identifier: `Q…` from the parser, `T…` from the type checker. A code's meaning is frozen; its message text may improve. | +| `position` | For a parse refusal: `line` and `column` (1-based, in characters) and the `byte` offset. | +| `stage` and `expression` | For a refusal after parsing: the compiler stage (`typecheck`) and, when the site can render it, the expression it refused. | +| `expected` | What was expected or violated, one line, without a position. | +| `fix` | One concrete fix, absent when `expected` names the decision. | +| `suggestion` | Optional source edits with `applicability`, plus `start`, `end` and `replacement` for each edit. | + +The one-line `error` text is the code and the expectation in the legacy form +(`parse error: …`, `type error: T33: …`); the other fields travel beside it. + +## The measured case + +A declaration without its parameter list: + +```text +query name { +``` + +```text +error[Q002]: parse error: expected `(`: a query declares its parameters even when it has none + --> line 1, column 11 + fix: query name() +``` + +Earlier releases reported this at the file's first position as `expected +query_file`. + +## Typed suggestions + +The `fix` remains human-readable guidance. When the compiler can propose a +specific edit, `suggestion` carries it as data. For example, this source: + +```text +query name { match { $p: Person } return { $p.name } } +``` + +receives this suggestion alongside Q002: + +```json +{ + "applicability": "machine_applicable", + "edits": [{ "start": 10, "end": 10, "replacement": "()" }] +} +``` + +Ranges use zero-based UTF-8 byte offsets in the exact original request source, +with an exclusive `end`. Equal offsets insert text. Ranges do not overlap; +apply multiple edits from the highest offset to the lowest. Do not apply an +edit to a source that has changed since the request. + +`machine_applicable` means the compiler has enough evidence for this local +correction. `needs_review` means the caller must review the proposed change. +Neither value guarantees that the corrected query will pass every check; +validate it again. The compiler does not apply suggestions automatically. + +Q002 offers the `()` insertion only when the edited file matches the grammar +without another missing-parameter recovery. Incomplete or ambiguous input can +still receive textual guidance while omitting `suggestion`. Missing suggestions +do not mean there is no possible fix. + +## Where the fields appear + +- **CLI, human formats** (`table`, `kv`, `csv`): the form above on stderr, + exit status 1, with no colour codes and no backtrace footer. +- **CLI, machine formats**: `--json` and `--format json` print the API's error + body pretty, `--format jsonl` prints it as one line, both on stdout with + exit status 1. A served refusal keeps the server's `code` field + (`bad_request`); an embedded one has none. +- **HTTP**: a `400` that refuses a query carries the additive `diagnostic` + object in its error body. See + [HTTP errors](../operations/troubleshooting.md#http-errors). +- **`omnigraph lint`**: a `Q000` finding whose message is the expectation at + `line , column `; type errors report their `T…` code. See + [Linting](index.md#linting). +- **`queries validate --json` and `cluster plan --json`**: each breakage or + query parse/typecheck failure carries the same diagnostic information as + `diagnostic` or `detail`, so a stored query the next release would refuse + is a pre-upgrade finding. Cluster `detail` uses the compiler shape: `message` + and a nested `stage` object. API and validation output use `expected` and + separate `stage`/`expression` fields. + +```json +{ + "error": "parse error: expected `(`: a query declares its parameters even when it has none", + "code": "bad_request", + "diagnostic": { + "code": "Q002", + "position": { "line": 1, "column": 11, "byte": 10 }, + "expected": "expected `(`: a query declares its parameters even when it has none", + "fix": "query name()" + } +} +``` + +## Parse codes + +| Code | Meaning | +|---|---| +| `Q001` | The source does not match the grammar at the reported position; `expected` names the grammar rules the parser could accept there. | +| `Q002` | A query declaration is missing its parameter list. | +| `Q003` | A settings statement is refused: unknown setting, value outside its row, or a process setting in a request. | +| `Q004` | A branch, show or explain statement is misplaced or malformed. | +| `Q005` | A declaration body is refused; `expected` names the construct. | + +Type codes are listed where the construct they guard is described, in +[Query language](index.md). diff --git a/docs/user/queries/index.md b/docs/user/queries/index.md index 3e1bc0a81..4e8fddafd 100644 --- a/docs/user/queries/index.md +++ b/docs/user/queries/index.md @@ -312,20 +312,20 @@ Use `set`, `reset`, and `show` to configure query execution. See ## Linting -Validate queries without running them: +Validate queries without running them; a refusal reports a stable code, its +position or stage, the expectation and one fix ([Diagnostics](diagnostics.md)): ```bash omnigraph lint --query queries.gq --schema schema.pg --json ``` -`Q000` identifies parse errors. A file that holds a [branch -statement](../branching/index.md) where query declarations were expected also -reports `Q000`. A [settings line](#session-settings) that names an unknown -setting or a value outside its row reports `ERROR line , column : -`. `L201` warns when a nullable -property is never set by any update query in the inspected set. Type errors -report the affected query and source location. The command exits nonzero when -the overall status is an error. +`Q000` identifies a file the parser refused, at `line , column `; a +[branch statement](../branching/index.md) where declarations were expected +and a [settings line](#session-settings) naming an unknown setting or a value +outside its row report the same way. `L201` warns when a nullable property is +never set by any update query in the inspected set. Type errors report the +affected query and their `T…` code. The command exits nonzero when the overall +status is an error. For every query that compiles successfully, JSON output includes an `operation` descriptor: diff --git a/openapi.json b/openapi.json index 233fbb486..dcbcba778 100644 --- a/openapi.json +++ b/openapi.json @@ -3661,6 +3661,14 @@ }, "components": { "schemas": { + "ApplicabilityOutput": { + "type": "string", + "description": "Whether a suggested source edit can be applied mechanically.", + "enum": [ + "machine_applicable", + "needs_review" + ] + }, "BlobEntityKind": { "type": "string", "description": "Logical graph entity selected by the Blob delivery surface.\n\nThis is intentionally graph vocabulary: callers select a node or edge.", @@ -4452,6 +4460,60 @@ } } }, + "DiagnosticOutput": { + "type": "object", + "description": "The diagnostics contract for a refused query (RFC 0047): a stable code\n(`Q…` parse, `T…` typecheck); where the failure is, as a source position\nor as the stage and expression when it is post-parse; what was expected or\nviolated; and one concrete fix, absent when `expected` names the decision.\nRides `ErrorOutput.diagnostic` as an additive detail because\n[`ErrorCode`] is a closed compatibility contract.", + "required": [ + "code", + "expected" + ], + "properties": { + "code": { + "type": "string" + }, + "expected": { + "type": "string" + }, + "expression": { + "type": [ + "string", + "null" + ] + }, + "fix": { + "type": [ + "string", + "null" + ] + }, + "position": { + "oneOf": [ + { + "type": "null" + }, + { + "$ref": "#/components/schemas/PositionOutput" + } + ] + }, + "stage": { + "type": [ + "string", + "null" + ] + }, + "suggestion": { + "oneOf": [ + { + "type": "null" + }, + { + "$ref": "#/components/schemas/SuggestionOutput" + } + ] + } + } + }, "EntityChangeOutput": { "type": "object", "description": "One entity change. Cause is stated once on the enclosing block, never here.\nAn insert carries only `after`, an update exact `before` and `after`, a\ndelete only `before`.", @@ -4566,6 +4628,17 @@ } ] }, + "diagnostic": { + "oneOf": [ + { + "type": "null" + }, + { + "$ref": "#/components/schemas/DiagnosticOutput", + "description": "Set for a refused query: the diagnostics contract's code, position or\nstage, expectation and fix. `error` keeps the one-line rendering." + } + ] + }, "error": { "type": "string" }, @@ -5205,6 +5278,32 @@ "list" ] }, + "PositionOutput": { + "type": "object", + "description": "A source position: 1-based line and column (in characters) and the byte\noffset.", + "required": [ + "line", + "column", + "byte" + ], + "properties": { + "byte": { + "type": "integer", + "format": "int32", + "minimum": 0 + }, + "column": { + "type": "integer", + "format": "int32", + "minimum": 0 + }, + "line": { + "type": "integer", + "format": "int32", + "minimum": 0 + } + } + }, "PreconditionFailureOutput": { "type": "object", "description": "Structured details for a caller write-precondition failure: HTTP 412, a\nmutation carried `Omnigraph-If-Graph-Commit: `, and the branch\nhead no longer matches that id. The write had no effect; the caller re-reads\nthe branch and decides again. `actual` is `None` on a branch with no commits.", @@ -5736,6 +5835,25 @@ } } }, + "SuggestionOutput": { + "type": "object", + "description": "Non-overlapping edits against the original request source.", + "required": [ + "applicability", + "edits" + ], + "properties": { + "applicability": { + "$ref": "#/components/schemas/ApplicabilityOutput" + }, + "edits": { + "type": "array", + "items": { + "$ref": "#/components/schemas/TextEditOutput" + } + } + } + }, "SystemColumnsOutput": { "type": "object", "description": "A graph's system column spellings (see `SchemaOutput::system_columns`).", @@ -5755,6 +5873,28 @@ "type": "string" } } + }, + "TextEditOutput": { + "type": "object", + "description": "A UTF-8 byte range in the original source, with an exclusive end.", + "required": [ + "start", + "end", + "replacement" + ], + "properties": { + "end": { + "type": "integer", + "minimum": 0 + }, + "replacement": { + "type": "string" + }, + "start": { + "type": "integer", + "minimum": 0 + } + } } }, "securitySchemes": { diff --git a/tools/omnigraph-vocabulary-guard/graph-vocabulary-inventory.tsv b/tools/omnigraph-vocabulary-guard/graph-vocabulary-inventory.tsv index 67bfd8dce..8a37a3729 100644 --- a/tools/omnigraph-vocabulary-guard/graph-vocabulary-inventory.tsv +++ b/tools/omnigraph-vocabulary-guard/graph-vocabulary-inventory.tsv @@ -4,6 +4,9 @@ schema_version occurrence_id surface source_path boundary site_kind current_term 2 openapi:%2Fcomponents%2Fschemas%2FLegacyReadOutput%2Fproperties%2Frows:property_name:bc51e9e65d79e1c90ba544ab2eae1044ccdf8c1af7a559ba375cba4276f7d99e:rows:1 openapi openapi.json /components/schemas/LegacyReadOutput/properties/rows property_name rows retain result_projection retain exact query-result name none omnigraph-vocabulary-guard::openapi_inventory_matches Query-result rows and columns are canonical tabular projection vocabulary and are intentionally unchanged. 2 openapi:%2Fcomponents%2Fschemas%2FLegacyReadOutput%2Frequired%2F2:required_name:d5a76110022e4737009e0cfd33d38eb9a57de63480648a62b474c248b071abc8:row:1 openapi openapi.json /components/schemas/LegacyReadOutput/required/2 required_name row retain result_projection retain exact query-result name none omnigraph-vocabulary-guard::openapi_inventory_matches Query-result rows and columns are canonical tabular projection vocabulary and are intentionally unchanged. 2 openapi:%2Fcomponents%2Fschemas%2FLegacyReadOutput%2Frequired%2F3:required_name:bc51e9e65d79e1c90ba544ab2eae1044ccdf8c1af7a559ba375cba4276f7d99e:rows:1 openapi openapi.json /components/schemas/LegacyReadOutput/required/3 required_name rows retain result_projection retain exact query-result name none omnigraph-vocabulary-guard::openapi_inventory_matches Query-result rows and columns are canonical tabular projection vocabulary and are intentionally unchanged. +2 openapi:%2Fcomponents%2Fschemas%2FPositionOutput%2Fdescription:prose:388f0164fc5740082dfd4138cdd20acf055873de69aa3f659ab7e53e21667071:column:1 openapi openapi.json /components/schemas/PositionOutput/description prose column retain renderer retain exact source-position name none omnigraph-vocabulary-guard::openapi_inventory_matches A source position's column is the character offset in a line, the diagnostics contract's coordinate, not a tabular or physical column. +2 openapi:%2Fcomponents%2Fschemas%2FPositionOutput%2Fproperties%2Fcolumn:property_name:881d5fb60090f59aabba4f287d7596317ca7091d00c2180b6cbc8ab018b9febd:column:1 openapi openapi.json /components/schemas/PositionOutput/properties/column property_name column retain renderer retain exact source-position name none omnigraph-vocabulary-guard::openapi_inventory_matches A source position's column is the character offset in a line, the diagnostics contract's coordinate, not a tabular or physical column. +2 openapi:%2Fcomponents%2Fschemas%2FPositionOutput%2Frequired%2F1:required_name:881d5fb60090f59aabba4f287d7596317ca7091d00c2180b6cbc8ab018b9febd:column:1 openapi openapi.json /components/schemas/PositionOutput/required/1 required_name column retain renderer retain exact source-position name none omnigraph-vocabulary-guard::openapi_inventory_matches A source position's column is the character offset in a line, the diagnostics contract's coordinate, not a tabular or physical column. 2 openapi:%2Fcomponents%2Fschemas%2FReadOutput%2Fproperties%2Fcolumns:property_name:0e624a1a135e4441e96ebbd5f95bc3df19c3c138df98c61ce684ef311d6e8b1a:columns:1 openapi openapi.json /components/schemas/ReadOutput/properties/columns property_name columns retain result_projection retain exact query-result name none omnigraph-vocabulary-guard::openapi_inventory_matches Query-result rows and columns are canonical tabular projection vocabulary and are intentionally unchanged. 2 openapi:%2Fcomponents%2Fschemas%2FReadOutput%2Fproperties%2Fgraph_commit_id%2Fdescription:prose:f5cf98c6fc65d1dd78b092c11bc7c9f7d6494f6392c97847d565a6edb2605e96:rows:1 openapi openapi.json /components/schemas/ReadOutput/properties/graph_commit_id/description prose rows retain result_projection retain query-result vocabulary none omnigraph-vocabulary-guard::openapi_inventory_matches Rows names the tabular query-result projection pinned to the same snapshot as graph_commit_id. 2 openapi:%2Fcomponents%2Fschemas%2FReadOutput%2Fproperties%2Frow_count:property_name:d5a76110022e4737009e0cfd33d38eb9a57de63480648a62b474c248b071abc8:row:1 openapi openapi.json /components/schemas/ReadOutput/properties/row_count property_name row retain result_projection retain exact query-result name none omnigraph-vocabulary-guard::openapi_inventory_matches Query-result rows and columns are canonical tabular projection vocabulary and are intentionally unchanged.