Cyclic and multiple include errors (#2278)

This PR adds cyclic and multiple include errors to the qasm parser.

Author: orpuente-MS

compiler/qsc_qasm3/src/io.rs

@@ -17,8 +17,10 @@ use rustc_hash::FxHashMap;
 /// Implementations of this trait can be provided to the parser
 /// to customize how include files are resolved.
 pub trait SourceResolver {
+    fn ctx(&mut self) -> &mut SourceResolverContext;
+
     #[cfg(feature = "fs")]
-    fn resolve<P>(&self, path: P) -> miette::Result<(PathBuf, String), Error>
+    fn resolve<P>(&mut self, path: P) -> miette::Result<(PathBuf, String), Error>
     where
         P: AsRef<Path>,
     {
@@ -27,8 +29,14 @@ pub trait SourceResolver {
                 "Could not resolve include file path: {e}"
             )))
         })?;
+
+        self.ctx().check_include_errors(&path)?;
+
         match std::fs::read_to_string(&path) {
-            Ok(source) => Ok((path, source)),
+            Ok(source) => {
+                self.ctx().add_path_to_include_graph(path.clone());
+                Ok((path, source))
+            }
             Err(_) => Err(Error(ErrorKind::NotFound(format!(
                 "Could not resolve include file: {}",
                 path.display()
@@ -41,6 +49,137 @@ pub trait SourceResolver {
         P: AsRef<Path>;
 }
 
+pub struct IncludeGraphNode {
+    parent: Option<PathBuf>,
+    children: Vec<PathBuf>,
+}
+
+#[derive(Default)]
+pub struct SourceResolverContext {
+    /// A graph representation of the include chain.
+    include_graph: FxHashMap<PathBuf, IncludeGraphNode>,
+    /// Path being resolved.
+    current_file: Option<PathBuf>,
+}
+
+impl SourceResolverContext {
+    pub fn check_include_errors(&mut self, path: &PathBuf) -> miette::Result<(), Error> {
+        // If the new path makes a cycle in the include graph, we return
+        // an error showing the cycle to the user.
+        if let Some(cycle) = self.cycle_made_by_including_path(path) {
+            return Err(Error(ErrorKind::CyclicInclude(cycle)));
+        }
+
+        // If the new path doesn't make a cycle but it was already
+        // included before, we return a `MultipleInclude`
+        // error saying "<FILE> was already included in <FILE>".
+        if let Some(parent_file) = self.path_was_already_included(path) {
+            return Err(Error(ErrorKind::MultipleInclude(
+                path.display().to_string(),
+                parent_file.display().to_string(),
+            )));
+        }
+
+        self.add_path_to_include_graph(path.clone());
+
+        Ok(())
+    }
+
+    /// Changes `current_path` to its parent in the `include_graph`.
+    pub fn pop_current_file(&mut self) {
+        let parent = self
+            .current_file
+            .as_ref()
+            .and_then(|file| self.include_graph.get(file).map(|node| node.parent.clone()))
+            .flatten();
+        self.current_file = parent;
+    }
+
+    /// If including the path makes a cycle, returns a vector of the paths
+    /// that make the cycle. Else, returns None.
+    ///
+    /// To check if adding `path` to the include graph creates a cycle we just
+    /// need to verify if path is an ancestor of the current file.
+    fn cycle_made_by_including_path(&self, path: &PathBuf) -> Option<Cycle> {
+        let mut current_file = self.current_file.as_ref();
+        let mut paths = Vec::new();
+
+        while let Some(file) = current_file {
+            paths.push(file.clone());
+            current_file = self.get_parent(file);
+            if file == path {
+                paths.reverse();
+                paths.push(path.clone());
+                return Some(Cycle { paths });
+            }
+        }
+
+        None
+    }
+
+    /// Returns the file that included `path`.
+    /// Returns `None` if `path` is the "main" file.
+    fn get_parent(&self, path: &PathBuf) -> Option<&PathBuf> {
+        self.include_graph
+            .get(path)
+            .and_then(|node| node.parent.as_ref())
+    }
+
+    /// If the path was already included, returns the path of the file that
+    /// included it. Else, returns None.
+    fn path_was_already_included(&self, path: &PathBuf) -> Option<PathBuf> {
+        // SAFETY: The call to expect should be unreachable, since the parent
+        //         will only be None for the "main" file. But including the
+        //         main file will trigger a cyclic include error before this
+        //         function is called.
+        self.include_graph
+            .get(path)
+            .map(|node| node.parent.clone().expect("unreachable"))
+    }
+
+    /// Adds `path` as a child of `current_path`, and then changes
+    /// the `current_path` to `path`.
+    fn add_path_to_include_graph(&mut self, path: PathBuf) {
+        // 1. Add path to the current file children.
+        self.current_file.as_ref().and_then(|file| {
+            self.include_graph
+                .get_mut(file)
+                .map(|node| node.children.push(path.clone()))
+        });
+
+        // 2. Add path to the include graph.
+        self.include_graph.insert(
+            path.clone(),
+            IncludeGraphNode {
+                parent: self.current_file.clone(),
+                children: Vec::new(),
+            },
+        );
+
+        // 3. Update the current file.
+        self.current_file = Some(path);
+    }
+}
+
+/// We use this struct to print a nice error message when we find a cycle.
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct Cycle {
+    paths: Vec<PathBuf>,
+}
+
+impl std::fmt::Display for Cycle {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let parents = self.paths[0..(self.paths.len() - 1)].iter();
+        let children = self.paths[1..].iter();
+
+        for (parent, child) in parents.zip(children) {
+            write!(f, "\n  {} includes {}", parent.display(), child.display())?;
+        }
+
+        Ok(())
+    }
+}
+
 /// A source resolver that resolves include files from an in-memory map.
 /// This is useful for testing or environments in which file system access
 /// is not available.
@@ -49,6 +188,7 @@ pub trait SourceResolver {
 /// contents prior to parsing.
 pub struct InMemorySourceResolver {
     sources: FxHashMap<PathBuf, String>,
+    ctx: SourceResolverContext,
 }
 
 impl FromIterator<(Arc<str>, Arc<str>)> for InMemorySourceResolver {
@@ -58,16 +198,24 @@ impl FromIterator<(Arc<str>, Arc<str>)> for InMemorySourceResolver {
             map.insert(PathBuf::from(path.to_string()), source.to_string());
         }
 
-        InMemorySourceResolver { sources: map }
+        InMemorySourceResolver {
+            sources: map,
+            ctx: Default::default(),
+        }
     }
 }
 
 impl SourceResolver for InMemorySourceResolver {
-    fn resolve<P>(&self, path: P) -> miette::Result<(PathBuf, String), Error>
+    fn ctx(&mut self) -> &mut SourceResolverContext {
+        &mut self.ctx
+    }
+
+    fn resolve<P>(&mut self, path: P) -> miette::Result<(PathBuf, String), Error>
     where
         P: AsRef<Path>,
     {
         let path = path.as_ref();
+        self.ctx().check_include_errors(&path.to_path_buf())?;
         match self.sources.get(path) {
             Some(source) => Ok((path.to_owned(), source.clone())),
             None => Err(Error(ErrorKind::NotFound(format!(

compiler/qsc_qasm3/src/io/error.rs

@@ -4,6 +4,8 @@
 use miette::Diagnostic;
 use thiserror::Error;
 
+use super::Cycle;
+
 #[derive(Clone, Debug, Diagnostic, Eq, Error, PartialEq)]
 #[error(transparent)]
 #[diagnostic(transparent)]
@@ -15,6 +17,10 @@ pub enum ErrorKind {
     NotFound(String),
     #[error("IO Error: {0}")]
     IO(String),
+    #[error("{0} was already included in: {1}")]
+    MultipleInclude(String, String),
+    #[error("Cyclic include:{0}")]
+    CyclicInclude(Cycle),
 }
 
 impl From<Error> for crate::Error {

compiler/qsc_qasm3/src/parse.rs

@@ -67,7 +67,11 @@ impl QasmParseResult {
 /// This function will resolve includes using the provided resolver.
 /// If an include file cannot be resolved, an error will be returned.
 /// If a file is included recursively, a stack overflow occurs.
-pub fn parse_source<S, P, R>(source: S, path: P, resolver: &R) -> miette::Result<QasmParseResult>
+pub fn parse_source<S, P, R>(
+    source: S,
+    path: P,
+    resolver: &mut R,
+) -> miette::Result<QasmParseResult>
 where
     S: AsRef<str>,
     P: AsRef<Path>,
@@ -203,7 +207,7 @@ impl QasmSource {
 /// This function is the start of a recursive process that will resolve all
 /// includes in the QASM file. Any includes are parsed as if their contents
 /// were defined where the include statement is.
-fn parse_qasm_file<P, R>(path: P, resolver: &R) -> miette::Result<QasmSource>
+fn parse_qasm_file<P, R>(path: P, resolver: &mut R) -> miette::Result<QasmSource>
 where
     P: AsRef<Path>,
     R: SourceResolver,
@@ -212,7 +216,7 @@ where
     parse_qasm_source(source, path, resolver)
 }
 
-fn parse_qasm_source<S, P, R>(source: S, path: P, resolver: &R) -> miette::Result<QasmSource>
+fn parse_qasm_source<S, P, R>(source: S, path: P, resolver: &mut R) -> miette::Result<QasmSource>
 where
     S: AsRef<str>,
     P: AsRef<Path>,
@@ -224,7 +228,7 @@ where
 
 fn parse_source_and_includes<P: AsRef<str>, R>(
     source: P,
-    resolver: &R,
+    resolver: &mut R,
 ) -> miette::Result<(ParseOrErrors<SourceFile>, Vec<QasmSource>)>
 where
     R: SourceResolver,
@@ -240,7 +244,7 @@ where
 
 fn parse_includes<R>(
     syntax_ast: &ParseOrErrors<oq3_syntax::SourceFile>,
-    resolver: &R,
+    resolver: &mut R,
 ) -> miette::Result<Vec<QasmSource>>
 where
     R: SourceResolver,

compiler/qsc_qasm3/src/parser.rs

@@ -110,7 +110,7 @@ fn update_offsets(source_map: &SourceMap, source: &mut QasmSource) {
 /// This function will resolve includes using the provided resolver.
 /// If an include file cannot be resolved, an error will be returned.
 /// If a file is included recursively, a stack overflow occurs.
-pub fn parse_source<S, P, R>(source: S, path: P, resolver: &R) -> QasmParseResult
+pub fn parse_source<S, P, R>(source: S, path: P, resolver: &mut R) -> QasmParseResult
 where
     S: AsRef<str>,
     P: AsRef<Path>,
@@ -230,13 +230,22 @@ impl QasmSource {
 /// This function is the start of a recursive process that will resolve all
 /// includes in the QASM file. Any includes are parsed as if their contents
 /// were defined where the include statement is.
-fn parse_qasm_file<P, R>(path: P, resolver: &R) -> QasmSource
+fn parse_qasm_file<P, R>(path: P, resolver: &mut R) -> QasmSource
 where
     P: AsRef<Path>,
     R: SourceResolver,
 {
     match resolver.resolve(&path) {
-        Ok((path, source)) => parse_qasm_source(source, path, resolver),
+        Ok((path, source)) => {
+            let parse_result = parse_qasm_source(source, path, resolver);
+
+            // Once we finish parsing the source, we pop the file from the
+            // resolver. This is needed to keep track of multiple includes
+            // and cyclic includes.
+            resolver.ctx().pop_current_file();
+
+            parse_result
+        }
         Err(e) => {
             let error = crate::parser::error::ErrorKind::IO(e);
             let error = crate::parser::Error(error, None);
@@ -255,7 +264,7 @@ where
     }
 }
 
-fn parse_qasm_source<S, P, R>(source: S, path: P, resolver: &R) -> QasmSource
+fn parse_qasm_source<S, P, R>(source: S, path: P, resolver: &mut R) -> QasmSource
 where
     S: AsRef<str>,
     P: AsRef<Path>,
@@ -267,7 +276,7 @@ where
 
 fn parse_source_and_includes<P: AsRef<str>, R>(
     source: P,
-    resolver: &R,
+    resolver: &mut R,
 ) -> (Program, Vec<Error>, Vec<QasmSource>)
 where
     R: SourceResolver,
@@ -277,7 +286,7 @@ where
     (program, errors, included)
 }
 
-fn parse_includes<R>(program: &Program, resolver: &R) -> Vec<QasmSource>
+fn parse_includes<R>(program: &Program, resolver: &mut R) -> Vec<QasmSource>
 where
     R: SourceResolver,
 {

compiler/qsc_qasm3/src/parser/tests.rs

@@ -23,11 +23,11 @@ pub(crate) fn parse_all<P>(
 where
     P: AsRef<Path>,
 {
-    let resolver = InMemorySourceResolver::from_iter(sources);
+    let mut resolver = InMemorySourceResolver::from_iter(sources);
     let (path, source) = resolver
         .resolve(path.as_ref())
         .map_err(|e| vec![Report::new(e)])?;
-    let res = crate::parser::parse_source(source, path, &resolver);
+    let res = crate::parser::parse_source(source, path, &mut resolver);
     if res.source.has_errors() {
         let errors = res
             .errors()
@@ -44,8 +44,8 @@ pub(crate) fn parse<S>(source: S) -> miette::Result<QasmParseResult, Vec<Report>
 where
     S: AsRef<str>,
 {
-    let resolver = InMemorySourceResolver::from_iter([("test".into(), source.as_ref().into())]);
-    let res = parse_source(source, "test", &resolver);
+    let mut resolver = InMemorySourceResolver::from_iter([("test".into(), source.as_ref().into())]);
+    let res = parse_source(source, "test", &mut resolver);
     if res.source.has_errors() {
         let errors = res
             .errors()

compiler/qsc_qasm3/src/semantic.rs

@@ -96,18 +96,18 @@ where
     S: AsRef<str>,
     P: AsRef<Path>,
 {
-    let resolver = InMemorySourceResolver::from_iter([(
+    let mut resolver = InMemorySourceResolver::from_iter([(
         path.as_ref().display().to_string().into(),
         source.as_ref().into(),
     )]);
-    parse_source(source, path, &resolver)
+    parse_source(source, path, &mut resolver)
 }
 
 /// Parse a QASM file and return the parse result.
 /// This function will resolve includes using the provided resolver.
 /// If an include file cannot be resolved, an error will be returned.
 /// If a file is included recursively, a stack overflow occurs.
-pub fn parse_source<S, P, R>(source: S, path: P, resolver: &R) -> QasmSemanticParseResult
+pub fn parse_source<S, P, R>(source: S, path: P, resolver: &mut R) -> QasmSemanticParseResult
 where
     S: AsRef<str>,
     P: AsRef<Path>,