Context for FileLoading errors (#1420)

languages/python/oso/polar/errors.py

@@ -7,7 +7,6 @@
     UnrecognizedToken,
     PolarTypeError,
     StackOverflowError,
-    FileLoadingError,
     PolarRuntimeError,
     UnknownError,
     OperationalError,
@@ -70,7 +69,6 @@ def _runtime_error(subkind, message, details):
         "Unsupported": UnsupportedError(message, details),
         "TypeError": PolarTypeError(message, details),
         "StackOverflow": StackOverflowError(message, details),
-        "FileLoading": FileLoadingError(message, details),
     }
     return runtime_errors.get(subkind, PolarRuntimeError(message, details))
 

languages/python/oso/polar/exceptions.py

@@ -57,12 +57,6 @@ class StackOverflowError(PolarRuntimeError):
     pass
 
 
-class FileLoadingError(PolarRuntimeError):
-    """Error loading a Polar file"""
-
-    pass
-
-
 class UnregisteredClassError(PolarRuntimeError):
     """Raised on attempts to reference unregistered Python classes from a Polar policy."""
 

languages/python/oso/tests/test_polar.py

@@ -49,14 +49,14 @@ def test_data_conversions(polar, qvar):
 def test_load_function(polar, query, qvar):
     """Make sure the load function works."""
     filename = Path(__file__).parent / "test_file.polar"
-    with pytest.raises(exceptions.PolarRuntimeError) as e:
+    with pytest.raises(exceptions.ValidationError) as e:
         polar.load_files([filename, filename])
     assert str(e.value).startswith(
         f"Problem loading file: File {filename} has already been loaded."
     )
 
     renamed = Path(__file__).parent / "test_file_renamed.polar"
-    with pytest.raises(exceptions.PolarRuntimeError) as e:
+    with pytest.raises(exceptions.ValidationError) as e:
         polar.load_files([filename, renamed])
     expected = f"Problem loading file: A file with the same contents as {renamed} named {filename} has already been loaded."
     assert str(e.value).startswith(expected)

languages/ruby/lib/oso/polar/errors.rb

@@ -26,7 +26,6 @@ class PolarRuntimeError < Error; end
     class UnsupportedError < PolarRuntimeError; end
     class PolarTypeError < PolarRuntimeError; end
     class StackOverflowError < PolarRuntimeError; end
-    class FileLoadingError < PolarRuntimeError; end
 
     # Errors originating from this side of the FFI boundary.
 

languages/ruby/lib/oso/polar/ffi/error.rb

@@ -74,16 +74,14 @@ def self.get(error_str, enrich_message) # rubocop:disable Metrics/AbcSize, Metri
         # @param msg [String]
         # @param details [Hash<String, Object>]
         # @return [::Oso::Polar::PolarRuntimeError] the object converted into the expected format.
-        private_class_method def self.runtime_error(kind, msg:, details:) # rubocop:disable Metrics/MethodLength
+        private_class_method def self.runtime_error(kind, msg:, details:)
           case kind
           when 'Unsupported'
             ::Oso::Polar::UnsupportedError.new(msg, details: details)
           when 'TypeError'
             ::Oso::Polar::PolarTypeError.new(msg, details: details)
           when 'StackOverflow'
             ::Oso::Polar::StackOverflowError.new(msg, details: details)
-          when 'FileLoading'
-            ::Oso::Polar::FileLoadingError.new(msg, details: details)
           else
             ::Oso::Polar::PolarRuntimeError.new(msg, details: details)
           end

languages/ruby/spec/oso/polar/polar_spec.rb

@@ -131,8 +131,8 @@ def initialize
     # in the core. We should test this once in the core.
     it 'errors if file is already loaded' do
       expect { subject.load_files([test_file, test_file]) }.to raise_error do |e|
-        expect(e).to be_an Oso::Polar::FileLoadingError
-        expect(e.message).to eq("Problem loading file: File #{test_file} has already been loaded.")
+        expect(e).to be_an Oso::Polar::ValidationError
+        expect(e.message).to start_with "Problem loading file: File #{test_file} has already been loaded."
       end
     end
 

languages/rust/oso/tests/test_polar.rs

@@ -245,15 +245,12 @@ fn test_already_loaded_file_error() -> oso::Result<()> {
     let err = oso.oso.load_files(vec![&path, &path]).unwrap_err();
 
     assert!(
-        matches!(&err,
-        OsoError::Polar(polar_error::PolarError {
-            kind:
-                polar_error::ErrorKind::Runtime(polar_error::RuntimeError::FileLoading { .. }),
+        matches!(&err, OsoError::Polar(polar_error::PolarError {
+            kind: polar_error::ErrorKind::Validation(polar_error::ValidationError::FileLoading { .. }),
             ..
-        })
-    if err.to_string() == format!("Problem loading file: File {} has already been loaded.", path.to_string_lossy())),
-        "Error was {:?}",
-        &err
+        }) if err.to_string().starts_with(&format!("Problem loading file: File {} has already been loaded.", path.to_string_lossy()))),
+        "Error was {}",
+        err
     );
 
     Ok(())

polar-core/src/diagnostic/mod.rs

@@ -24,14 +24,13 @@ impl Diagnostic {
     /// second `ResourceBlock` error when rewriting a shorthand rule involving the relation.
     pub fn is_unrecoverable(&self) -> bool {
         use super::error::{
-            ErrorKind::{Parse, Runtime, Validation},
-            RuntimeError::FileLoading,
-            ValidationError::ResourceBlock,
+            ErrorKind::{Parse, Validation},
+            ValidationError::{FileLoading, ResourceBlock},
         };
         matches!(
             self,
             Diagnostic::Error(PolarError {
-                kind: Parse(_) | Runtime(FileLoading { .. }) | Validation(ResourceBlock { .. }),
+                kind: Parse(_) | Validation(FileLoading { .. }) | Validation(ResourceBlock { .. }),
                 ..
             })
         )

polar-core/src/error.rs

@@ -41,7 +41,6 @@ impl PolarError {
             Parse(DuplicateKey { .. }) => "ParseError::DuplicateKey",
             Runtime(Application { .. }) => "RuntimeError::Application",
             Runtime(ArithmeticError { .. }) => "RuntimeError::ArithmeticError",
-            Runtime(FileLoading { .. }) => "RuntimeError::FileLoading",
             Runtime(IncompatibleBindings { .. }) => "RuntimeError::IncompatibleBindings",
             Runtime(QueryTimeout { .. }) => "RuntimeError::QueryTimeout",
             Runtime(StackOverflow { .. }) => "RuntimeError::StackOverflow",
@@ -54,8 +53,10 @@ impl PolarError {
             }
             Runtime(InvalidRegistration { .. }) => "RuntimeError::InvalidRegistration",
             Runtime(InvalidState { .. }) => "RuntimeError::InvalidState",
+            Runtime(MultipleLoadError) => "RuntimeError::MultipleLoadError",
             Operational(Serialization { .. }) => "OperationalError::Serialization",
             Operational(Unknown) => "OperationalError::Unknown",
+            Validation(FileLoading { .. }) => "ValidationError::FileLoading",
             Validation(InvalidRule { .. }) => "ValidationError::InvalidRule",
             Validation(InvalidRuleType { .. }) => "ValidationError::InvalidRuleType",
             Validation(ResourceBlock { .. }) => "ValidationError::ResourceBlock",
@@ -273,10 +274,6 @@ pub enum RuntimeError {
         /// Option<Term> where the error arose, tracked for lexical context.
         term: Option<Term>,
     },
-    // TODO(gj): consider moving to ValidationError.
-    FileLoading {
-        msg: String,
-    },
     IncompatibleBindings {
         msg: String,
     },
@@ -301,6 +298,7 @@ pub enum RuntimeError {
     InvalidState {
         msg: String,
     },
+    MultipleLoadError,
 }
 
 impl RuntimeError {
@@ -323,12 +321,12 @@ impl RuntimeError {
             // These errors never have context.
             StackOverflow { .. }
             | QueryTimeout { .. }
-            | FileLoading { .. }
             | IncompatibleBindings { .. }
             | DataFilteringFieldMissing { .. }
             | DataFilteringUnsupportedOp { .. }
             | InvalidRegistration { .. }
-            | InvalidState { .. } => None,
+            | InvalidState { .. }
+            | MultipleLoadError => None,
         };
 
         let context = context.map(|(span, source)| Context {
@@ -368,7 +366,6 @@ impl fmt::Display for RuntimeError {
                 writeln!(f, "{}", stack_trace)?;
                 write!(f, "Application error: {}", msg)
             }
-            Self::FileLoading { msg } => write!(f, "Problem loading file: {}", msg),
             Self::IncompatibleBindings { msg } => {
                 write!(f, "Attempted binding was incompatible: {}", msg)
             }
@@ -429,6 +426,7 @@ The expression is: {expr}
             // TODO(gj): move this back to `OperationalError` during The Next Great Diagnostic
             // Refactor.
             Self::InvalidState { msg } => write!(f, "Invalid state: {}", msg),
+            Self::MultipleLoadError => write!(f, "Cannot load additional Polar code -- all Polar code must be loaded at the same time."),
         }
     }
 }
@@ -467,6 +465,10 @@ impl fmt::Display for OperationalError {
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub enum ValidationError {
+    FileLoading {
+        source: Source,
+        msg: String,
+    },
     MissingRequiredRule {
         rule_type: Rule,
     },
@@ -541,6 +543,9 @@ impl ValidationError {
                     None
                 }
             }
+
+            // These errors always pertain to a specific file but not to a specific place therein.
+            FileLoading { source, .. } => Some(((0, 0), source.to_owned())),
         };
 
         let context = context.map(|(span, source)| Context {
@@ -558,6 +563,7 @@ impl ValidationError {
 impl fmt::Display for ValidationError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
+            Self::FileLoading { msg, .. } => write!(f, "Problem loading file: {}", msg),
             Self::InvalidRule { rule, msg } => {
                 write!(f, "Invalid rule: {} {}", rule, msg)
             }

polar-core/src/kb.rs

@@ -664,26 +664,29 @@ impl KnowledgeBase {
         self.resource_blocks.clear();
     }
 
-    fn check_file(&self, src: &str, filename: &str) -> Result<(), RuntimeError> {
+    fn check_file(&self, src: &str, filename: &str) -> Result<(), ValidationError> {
         match (
             self.loaded_content.get(src),
             self.loaded_files.get(filename).is_some(),
         ) {
             (Some(other_file), true) if other_file == filename => {
-                return Err(RuntimeError::FileLoading {
+                return Err(ValidationError::FileLoading {
+                    source: Source::new(Some(filename), src),
                     msg: format!("File {} has already been loaded.", filename),
                 })
             }
             (_, true) => {
-                return Err(RuntimeError::FileLoading {
+                return Err(ValidationError::FileLoading {
+                    source: Source::new(Some(filename), src),
                     msg: format!(
                         "A file with the name {}, but different contents has already been loaded.",
                         filename
                     ),
                 })
             }
             (Some(other_file), _) => {
-                return Err(RuntimeError::FileLoading {
+                return Err(ValidationError::FileLoading {
+                    source: Source::new(Some(filename), src),
                     msg: format!(
                         "A file with the same contents as {} named {} has already been loaded.",
                         filename, other_file
@@ -878,10 +881,7 @@ impl KnowledgeBase {
 mod tests {
     use super::*;
 
-    use crate::error::{
-        ErrorKind::{Runtime, Validation},
-        PolarError,
-    };
+    use crate::error::{ErrorKind::Validation, PolarError};
 
     #[test]
     /// Test validation implemented in `check_file()`.
@@ -900,7 +900,7 @@ mod tests {
         // Cannot load source1 a second time.
         let msg = match kb.add_source(source1).unwrap_err() {
             PolarError {
-                kind: Runtime(RuntimeError::FileLoading { msg }),
+                kind: Validation(ValidationError::FileLoading { msg, .. }),
                 ..
             } => msg,
             e => panic!("{}", e),
@@ -914,7 +914,7 @@ mod tests {
         };
         let msg = match kb.add_source(source2).unwrap_err() {
             PolarError {
-                kind: Runtime(RuntimeError::FileLoading { msg }),
+                kind: Validation(ValidationError::FileLoading { msg, .. }),
                 ..
             } => msg,
             e => panic!("{}", e),
@@ -935,7 +935,7 @@ mod tests {
         };
         let msg = match kb.add_source(source3).unwrap_err() {
             PolarError {
-                kind: Runtime(RuntimeError::FileLoading { msg }),
+                kind: Validation(ValidationError::FileLoading { msg, .. }),
                 ..
             } => msg,
             e => panic!("{}", e),

polar-core/src/polar.rs

@@ -29,9 +29,6 @@ impl Default for Polar {
     }
 }
 
-const MULTIPLE_LOAD_ERROR_MSG: &str =
-    "Cannot load additional Polar code -- all Polar code must be loaded at the same time.";
-
 impl Polar {
     pub fn new() -> Self {
         // TODO(@gkaemmer): pulling this from an environment variable is a hack
@@ -185,8 +182,7 @@ impl Polar {
     pub fn load(&self, sources: Vec<Source>) -> PolarResult<()> {
         if let Ok(kb) = self.kb.read() {
             if kb.has_rules() {
-                let msg = MULTIPLE_LOAD_ERROR_MSG.to_owned();
-                return Err(RuntimeError::FileLoading { msg }.with_context(&*kb));
+                return Err(RuntimeError::MultipleLoadError.with_context(&*kb));
             }
         }
 
@@ -311,7 +307,10 @@ impl Polar {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use crate::error::{ErrorKind::Runtime, PolarError};
+    use crate::error::{
+        ErrorKind::{Runtime, Validation},
+        PolarError,
+    };
 
     #[test]
     fn can_load_and_query() {
@@ -333,24 +332,22 @@ mod tests {
         polar.load(vec![source.clone()]).unwrap();
 
         // Loading twice is not.
-        let msg = match polar.load(vec![source]).unwrap_err() {
+        assert!(matches!(
+            polar.load(vec![source.clone()]).unwrap_err(),
             PolarError {
-                kind: Runtime(RuntimeError::FileLoading { msg }),
+                kind: Runtime(RuntimeError::MultipleLoadError),
                 ..
-            } => msg,
-            e => panic!("{}", e),
-        };
-        assert_eq!(msg, MULTIPLE_LOAD_ERROR_MSG);
+            }
+        ));
 
         // Even with load_str().
-        let msg = match polar.load_str(src).unwrap_err() {
+        assert!(matches!(
+            polar.load(vec![source]).unwrap_err(),
             PolarError {
-                kind: Runtime(RuntimeError::FileLoading { msg }),
+                kind: Runtime(RuntimeError::MultipleLoadError),
                 ..
-            } => msg,
-            e => panic!("{}", e),
-        };
-        assert_eq!(msg, MULTIPLE_LOAD_ERROR_MSG);
+            }
+        ));
     }
 
     #[test]
@@ -363,7 +360,7 @@ mod tests {
 
         let msg = match polar.load(vec![source.clone(), source]).unwrap_err() {
             PolarError {
-                kind: Runtime(RuntimeError::FileLoading { msg }),
+                kind: Validation(ValidationError::FileLoading { msg, .. }),
                 ..
             } => msg,
             e => panic!("{}", e),

polar-core/src/sources.rs

@@ -35,6 +35,15 @@ pub struct Source {
     pub src: String,
 }
 
+impl Source {
+    pub(crate) fn new(filename: Option<&str>, src: &str) -> Self {
+        Self {
+            filename: filename.map(Into::into),
+            src: src.into(),
+        }
+    }
+}
+
 pub struct Sources {
     /// Map from term ID to `Source`.
     sources: HashMap<u64, Source>,