feat: implement import is list twice error (#90)

protoc will raise an error in those case, which makes a discrepency if
you use protox as a proto validator

protox/src/compile/mod.rs

@@ -1,5 +1,5 @@
 use std::{
-    collections::HashMap,
+    collections::{HashMap, HashSet},
     fmt::{self, Write},
     path::{Path, PathBuf},
 };
@@ -148,11 +148,16 @@ impl Compiler {
         }
 
         let mut import_stack = vec![name.clone()];
+        let mut already_imported = HashSet::new();
         for (i, import) in file.descriptor.dependency.iter().enumerate() {
+            if !already_imported.insert(import) {
+                return Err(Error::from_kind(ErrorKind::DuplicatedThing {
+                    name: import.to_owned(),
+                }).into_import_error(&file, i));
+            }
             self.add_import(import, &mut import_stack)
                 .map_err(|e| e.into_import_error(&file, i))?;
         }
-        drop(import_stack);
 
         let path = self.check_file(file)?;
         self.files.insert(
@@ -270,7 +275,13 @@ impl Compiler {
         let file = self.resolver.open_file(file_name)?;
 
         import_stack.push(file_name.to_owned());
+        let mut already_imported = HashSet::new();
         for (i, import) in file.descriptor.dependency.iter().enumerate() {
+            if !already_imported.insert(import) {
+                return Err(Error::from_kind(ErrorKind::DuplicatedThing {
+                    name: import.to_owned(),
+                }).into_import_error(&file, i));
+            }
             self.add_import(import, import_stack)
                 .map_err(|e| e.into_import_error(&file, i))?;
         }

protox/src/error.rs

@@ -56,6 +56,17 @@ pub(crate) enum ErrorKind {
         path: PathBuf,
         shadow: PathBuf,
     },
+    /// This variant is intermediate and should not be present in the final error.
+    #[error("'{name}' was listed twice")]
+    DuplicatedThing { name: String },
+    #[error("import '{name}' was listed twice")]
+    DuplicateImport {
+        #[label("imported here")]
+        span: Option<SourceSpan>,
+        #[source_code]
+        source_code: NamedSource<String>,
+        name: String,
+    },
     #[error(transparent)]
     Custom(Box<dyn std::error::Error + Send + Sync>),
 }
@@ -83,6 +94,7 @@ impl Error {
         match &*self.kind {
             ErrorKind::Parse { err } => Some(err.file()),
             ErrorKind::Check { err } => err.file(),
+            ErrorKind::DuplicatedThing { .. } => None,
             ErrorKind::OpenFile { name, .. }
             | ErrorKind::FileTooLarge { name }
             | ErrorKind::FileInvalidUtf8 { name }
@@ -91,7 +103,8 @@ impl Error {
             | ErrorKind::FileShadowed { name, .. } => Some(name),
             ErrorKind::FileNotIncluded { .. } => None,
             ErrorKind::Custom(_) => None,
-            ErrorKind::ImportNotFound { source_code, .. } => Some(source_code.name()),
+            ErrorKind::ImportNotFound { source_code, .. }
+            | ErrorKind::DuplicateImport { source_code, .. } => Some(source_code.name()),
         }
     }
 
@@ -157,17 +170,19 @@ impl Error {
             }
             None
         }
+        let source_code =
+            NamedSource::new(file.name(), file.source().unwrap_or_default().to_owned());
         match *self.kind {
-            ErrorKind::FileNotFound { name } => {
-                let source_code: NamedSource<String> =
-                    NamedSource::new(file.name(), file.source().unwrap_or_default().to_owned());
-                let span = find_span(file, import_idx);
-                Error::from_kind(ErrorKind::ImportNotFound {
-                    span,
-                    source_code,
-                    name,
-                })
-            }
+            ErrorKind::FileNotFound { name } => Error::from_kind(ErrorKind::ImportNotFound {
+                span: find_span(file, import_idx),
+                source_code,
+                name,
+            }),
+            ErrorKind::DuplicatedThing { name } => Error::from_kind(ErrorKind::DuplicateImport {
+                span: find_span(file, import_idx),
+                source_code,
+                name,
+            }),
             _ => self,
         }
     }
@@ -202,9 +217,13 @@ impl fmt::Debug for Error {
             | ErrorKind::FileNotFound { .. }
             | ErrorKind::CircularImport { .. }
             | ErrorKind::FileNotIncluded { .. }
+            | ErrorKind::DuplicatedThing { .. }
             | ErrorKind::FileShadowed { .. } => write!(f, "{}", self),
             ErrorKind::Custom(err) => err.fmt(f),
-            ErrorKind::ImportNotFound {
+            ErrorKind::DuplicateImport {
+                span, source_code, ..
+            }
+            | ErrorKind::ImportNotFound {
                 span, source_code, ..
             } => {
                 write!(f, "{}:", source_code.name())?;

protox/tests/compiler.rs

@@ -44,10 +44,9 @@ fn check(files: &'static [(&'static str, &'static str)]) -> Result<Compiler, Err
     }
 
     let mut compiler = Compiler::with_file_resolver(TestFileResolver { files });
-    for (file, _) in &files[..files.len() - 1] {
-        compiler.open_file(file).unwrap();
-    }
 
+    // Only compile last file.
+    // Imports may have errors that must be correctly listed by compilation of root.
     compiler.open_file(files[files.len() - 1].0)?;
     Ok(compiler)
 }
@@ -74,6 +73,37 @@ fn import_error() {
     assert_yaml_snapshot!(check_err(&[("root.proto", "import 'customerror.proto';")]));
 }
 
+#[test]
+fn double_import_error() {
+    assert_yaml_snapshot!(check_err(&[
+        ("existing.proto", ""),
+        (
+            "root.proto",
+            "import 'existing.proto';
+        import 'existing.proto';
+        "
+        ),
+    ]));
+}
+
+#[test]
+fn double_import_branch_error() {
+    assert_yaml_snapshot!(check_err(&[
+        ("existing.proto", ""),
+        (
+            "branch.proto",
+            "import 'existing.proto';
+        import 'existing.proto';
+        "
+        ),
+        (
+            "root.proto",
+            "import 'branch.proto';
+        "
+        ),
+    ]));
+}
+
 #[test]
 fn type_not_found() {
     assert_yaml_snapshot!(check_err(&[(

protox/tests/snapshots/compiler__double_import_branch_error.snap

@@ -0,0 +1,15 @@
+---
+source: protox/tests/compiler.rs
+assertion_line: 91
+expression: "check_err(&[(\"existing.proto\", \"\"),\n(\"branch.proto\",\n\"import 'existing.proto';\n        import 'existing.proto';\n        \"),\n(\"root.proto\", \"import 'branch.proto';\n        \"),])"
+---
+causes: []
+filename: branch.proto
+labels:
+  - label: imported here
+    span:
+      length: 24
+      offset: 33
+message: "import 'existing.proto' was listed twice"
+related: []
+severity: error

protox/tests/snapshots/compiler__double_import_error.snap

@@ -0,0 +1,15 @@
+---
+source: protox/tests/compiler.rs
+assertion_line: 79
+expression: "check_err(&[(\"existing.proto\", \"\"),\n(\"root.proto\",\n\"import 'existing.proto';\n        import 'existing.proto';\n        \"),])"
+---
+causes: []
+filename: root.proto
+labels:
+  - label: imported here
+    span:
+      length: 24
+      offset: 33
+message: "import 'existing.proto' was listed twice"
+related: []
+severity: error