From 6edfb7a2f4624bf9f907142602c382400174c525 Mon Sep 17 00:00:00 2001 From: Tpt Date: Mon, 4 Sep 2023 21:32:58 +0200 Subject: [PATCH] Python: Adds location data to SyntaxError --- .cargo/build_config.py | 12 +++++---- python/src/io.rs | 56 +++++++++++++++++++++++++++++++++++++---- python/src/sparql.rs | 7 ++---- python/src/store.rs | 18 +++++++------ python/tests/test_io.py | 15 +++++++++++ 5 files changed, 85 insertions(+), 23 deletions(-) diff --git a/.cargo/build_config.py b/.cargo/build_config.py index 97dd9667..44079d9c 100644 --- a/.cargo/build_config.py +++ b/.cargo/build_config.py @@ -53,17 +53,19 @@ FLAGS_BLACKLIST = { "-Wclippy::std-instead-of-core", "-Wclippy::shadow-reuse", "-Wclippy::shadow-unrelated", - "-Wclippy::string-slice", # TODO: might be nice + "-Wclippy::string-slice", # TODO: might be nice "-Wclippy::too-many-lines", "-Wclippy::separated-literal-suffix", - "-Wclippy::unreachable", # TODO: might be nice - "-Wclippy::unwrap-used", # TODO: might be nice to use expect instead + "-Wclippy::unreachable", # TODO: might be nice + "-Wclippy::unwrap-used", # TODO: might be nice to use expect instead "-Wclippy::wildcard-enum-match-arm", # TODO: might be nice "-Wclippy::wildcard-imports", # TODO: might be nice } build_flags = set(DEFAULT_BUILD_FLAGS) -with urlopen(f"https://rust-lang.github.io/rust-clippy/rust-{MSRV}/lints.json") as response: +with urlopen( + f"https://rust-lang.github.io/rust-clippy/rust-{MSRV}/lints.json" +) as response: for lint in json.load(response): if lint["level"] == "allow" and lint["group"] != "nursery": build_flags.add(f"-Wclippy::{lint['id'].replace('_', '-')}") @@ -78,5 +80,5 @@ with open("./config.toml", "wt") as fp: fp.write("[build]\n") fp.write("rustflags = [\n") for flag in sorted(build_flags): - fp.write(f" \"{flag}\",\n") + fp.write(f' "{flag}",\n') fp.write("]\n") diff --git a/python/src/io.rs b/python/src/io.rs index 0b705c11..86f39992 100644 --- a/python/src/io.rs +++ b/python/src/io.rs @@ -12,6 +12,7 @@ use std::error::Error; use std::fs::File; use std::io::{self, BufWriter, Cursor, Read, Write}; use std::path::{Path, PathBuf}; +use std::sync::OnceLock; pub fn add_to_module(module: &PyModule) -> PyResult<()> { module.add_wrapped(wrap_pyfunction!(parse))?; @@ -62,8 +63,9 @@ pub fn parse( py: Python<'_>, ) -> PyResult { let format = rdf_format(format)?; - let input = if let Ok(path) = input.extract::(py) { - PyReadable::from_file(&path, py).map_err(map_io_err)? + let file_path = input.extract::(py).ok(); + let input = if let Some(file_path) = &file_path { + PyReadable::from_file(file_path, py).map_err(map_io_err)? } else { PyReadable::from_data(input, py) }; @@ -81,6 +83,7 @@ pub fn parse( } Ok(PyQuadReader { inner: parser.parse_read(input), + file_path, } .into_py(py)) } @@ -151,6 +154,7 @@ pub fn serialize(input: &PyAny, output: PyObject, format: &str, py: Python<'_>) #[pyclass(name = "QuadReader", module = "pyoxigraph")] pub struct PyQuadReader { inner: FromReadQuadReader, + file_path: Option, } #[pymethods] @@ -163,7 +167,10 @@ impl PyQuadReader { py.allow_threads(|| { self.inner .next() - .map(|q| Ok(q.map_err(map_parse_error)?.into())) + .map(|q| { + Ok(q.map_err(|e| map_parse_error(e, self.file_path.clone()))? + .into()) + }) .transpose() }) } @@ -314,9 +321,38 @@ pub fn map_io_err(error: io::Error) -> PyErr { } } -pub fn map_parse_error(error: ParseError) -> PyErr { +pub fn map_parse_error(error: ParseError, file_path: Option) -> PyErr { match error { - ParseError::Syntax(error) => PySyntaxError::new_err(error.to_string()), + ParseError::Syntax(error) => { + // Python 3.9 does not support end line and end column + if python_version() >= (3, 10, 0) { + let params = if let Some(location) = error.location() { + ( + file_path, + Some(location.start.line + 1), + Some(location.start.column + 1), + None::>, + Some(location.end.line + 1), + Some(location.end.column + 1), + ) + } else { + (None, None, None, None, None, None) + }; + PySyntaxError::new_err((error.to_string(), params)) + } else { + let params = if let Some(location) = error.location() { + ( + file_path, + Some(location.start.line + 1), + Some(location.start.column + 1), + None::>, + ) + } else { + (None, None, None, None) + }; + PySyntaxError::new_err((error.to_string(), params)) + } + } ParseError::Io(error) => map_io_err(error), } } @@ -345,3 +381,13 @@ pub fn allow_threads_unsafe(f: impl FnOnce() -> T) -> T { let _guard = RestoreGuard { tstate }; f() } + +fn python_version() -> (u8, u8, u8) { + static VERSION: OnceLock<(u8, u8, u8)> = OnceLock::new(); + *VERSION.get_or_init(|| { + Python::with_gil(|py| { + let v = py.version_info(); + (v.major, v.minor, v.patch) + }) + }) +} diff --git a/python/src/sparql.rs b/python/src/sparql.rs index 7c99707b..c7ad693b 100644 --- a/python/src/sparql.rs +++ b/python/src/sparql.rs @@ -1,4 +1,4 @@ -use crate::io::{allow_threads_unsafe, map_io_err}; +use crate::io::{allow_threads_unsafe, map_io_err, map_parse_error}; use crate::map_storage_error; use crate::model::*; use oxigraph::model::Term; @@ -233,10 +233,7 @@ pub fn map_evaluation_error(error: EvaluationError) -> PyErr { match error { EvaluationError::Parsing(error) => PySyntaxError::new_err(error.to_string()), EvaluationError::Storage(error) => map_storage_error(error), - EvaluationError::GraphParsing(error) => match error { - oxigraph::io::ParseError::Syntax(error) => PySyntaxError::new_err(error.to_string()), - oxigraph::io::ParseError::Io(error) => map_io_err(error), - }, + EvaluationError::GraphParsing(error) => map_parse_error(error, None), EvaluationError::ResultsParsing(error) => match error { oxigraph::sparql::results::ParseError::Syntax(error) => { PySyntaxError::new_err(error.to_string()) diff --git a/python/src/store.rs b/python/src/store.rs index d71d4936..70dda4cd 100644 --- a/python/src/store.rs +++ b/python/src/store.rs @@ -392,8 +392,9 @@ impl PyStore { } else { None }; - let input = if let Ok(path) = input.extract::(py) { - PyReadable::from_file(&path, py).map_err(map_io_err)? + let file_path = input.extract::(py).ok(); + let input = if let Some(file_path) = &file_path { + PyReadable::from_file(file_path, py).map_err(map_io_err)? } else { PyReadable::from_data(input, py) }; @@ -404,7 +405,7 @@ impl PyStore { } else { self.inner.load_dataset(input, format, base_iri) } - .map_err(map_loader_error) + .map_err(|e| map_loader_error(e, file_path)) }) } @@ -460,8 +461,9 @@ impl PyStore { } else { None }; - let input = if let Ok(path) = input.extract::(py) { - PyReadable::from_file(&path, py).map_err(map_io_err)? + let file_path = input.extract::(py).ok(); + let input = if let Some(file_path) = &file_path { + PyReadable::from_file(file_path, py).map_err(map_io_err)? } else { PyReadable::from_data(input, py) }; @@ -475,7 +477,7 @@ impl PyStore { .bulk_loader() .load_dataset(input, format, base_iri) } - .map_err(map_loader_error) + .map_err(|e| map_loader_error(e, file_path)) }) } @@ -833,10 +835,10 @@ pub fn map_storage_error(error: StorageError) -> PyErr { } } -pub fn map_loader_error(error: LoaderError) -> PyErr { +pub fn map_loader_error(error: LoaderError, file_path: Option) -> PyErr { match error { LoaderError::Storage(error) => map_storage_error(error), - LoaderError::Parsing(error) => map_parse_error(error), + LoaderError::Parsing(error) => map_parse_error(error, file_path), LoaderError::InvalidBaseIri { .. } => PyValueError::new_err(error.to_string()), } } diff --git a/python/tests/test_io.py b/python/tests/test_io.py index e9069e08..d70179e7 100644 --- a/python/tests/test_io.py +++ b/python/tests/test_io.py @@ -1,3 +1,4 @@ +import sys import unittest from io import BytesIO, StringIO, UnsupportedOperation from tempfile import NamedTemporaryFile, TemporaryFile @@ -83,6 +84,20 @@ class TestParse(unittest.TestCase): [EXAMPLE_QUAD], ) + def test_parse_syntax_error(self) -> None: + with NamedTemporaryFile() as fp: + fp.write(b"@base .\n") + fp.write(b' "p" "1"') + fp.flush() + with self.assertRaises(SyntaxError) as ctx: + list(parse(fp.name, "text/turtle")) + self.assertEqual(ctx.exception.filename, fp.name) + self.assertEqual(ctx.exception.lineno, 2) + self.assertEqual(ctx.exception.offset, 7) + if sys.version_info >= (3, 10): + self.assertEqual(ctx.exception.end_lineno, 2) + self.assertEqual(ctx.exception.end_offset, 10) + def test_parse_without_named_graphs(self) -> None: with self.assertRaises(SyntaxError) as _: list(