From 4bec64d8881ef6e1ca424e13b3fc3f668bee9adf Mon Sep 17 00:00:00 2001
From: Daniel Burgener <Daniel.Burgener@microsoft.com>
Date: Thu, 16 Mar 2023 16:18:01 -0400
Subject: [PATCH] Improve the error message when an annotation is both
 inherited and explicitly declared.

This is mostly done, but still a bit of a WIP:

1. The todo!() is waiting on the merge of #128
2. This all got quite verbose, and should become its own function to
improve readability
3. The test is written, but not added in lib.rs
---
 data/error_policies/duplicate_association.cas | 10 +++
 src/compile.rs                                | 62 ++++++++++++++++++-
 src/internal_rep.rs                           | 17 +++++
 3 files changed, 87 insertions(+), 2 deletions(-)
 create mode 100644 data/error_policies/duplicate_association.cas

diff --git a/data/error_policies/duplicate_association.cas b/data/error_policies/duplicate_association.cas
new file mode 100644
index 00000000..c199d4c7
--- /dev/null
+++ b/data/error_policies/duplicate_association.cas
@@ -0,0 +1,10 @@
+virtual resource tmp {}
+
+@associate([tmp])
+virtual domain bar {}
+
+@associate([tmp])
+domain foo inherits bar {
+	allow(this, this.tmp, file, write);
+}
+
diff --git a/src/compile.rs b/src/compile.rs
index 2ab7af92..269111fa 100644
--- a/src/compile.rs
+++ b/src/compile.rs
@@ -1252,12 +1252,70 @@ fn create_synthetic_resource(
         .expressions
         // If dup_res_decl is concrete, do not inherit virtual functions
         .retain(|e| dup_res_is_virtual || !e.is_virtual_function());
-    if !global_exprs.insert(Expression::Decl(Declaration::Type(Box::new(dup_res_decl)))) {
-        return Err(InternalError::new().into());
+    if !global_exprs.insert(Expression::Decl(Declaration::Type(Box::new(
+        dup_res_decl.clone(),
+    )))) {
+        // The callers should be handling the situation where the same resource was declared at the
+        // same level of inheritance, but this can arise if a parent associated a resource and a
+        // child associated the same resource.  We should find them and return and error message
+        return match make_duplicate_associate_error(types, dom_info, &res_name) {
+            Some(e) => Err(e.into()),
+            None => Err(InternalError::new().into()),
+        };
     }
     Ok(res_name)
 }
 
+fn make_duplicate_associate_error(
+    types: &TypeMap,
+    child_domain: &TypeInfo,
+    res_name: &CascadeString,
+) -> Option<CompileError> {
+    let mut parent_ti = None;
+    let mut parent_associate_range = None;
+    for p in child_domain.get_all_parent_names(types) {
+        if let Some(p_ti) = types.get(p.as_ref()) {
+            parent_associate_range = p_ti.explicitly_associates(res_name.as_ref());
+            if parent_associate_range.is_some() {
+                parent_ti = Some(p_ti);
+                break;
+            }
+        }
+    }
+    let current_associate_range = match child_domain.explicitly_associates(res_name.as_ref()) {
+        Some(r) => r,
+        None => {
+            // This is an association via nested declaration, so find that
+            child_domain.decl.as_ref()?.expressions.iter().find_map(|e|
+                if let Expression::Decl(Declaration::Type(td)) = e {
+                    if &td.name == res_name {
+                        td.name.get_range()
+                    } else {
+                        None
+                    }
+                } else {
+                    None
+                })?
+        }
+    };
+
+    // If anything we need for a real error is None, all we can do is InternalError, so unwrap
+    // everything, returning InternalError on failure
+    let child_file = child_domain.declaration_file.as_ref()?;
+    let parent_file = parent_ti?.declaration_file.as_ref()?;
+    let parent_associate_range = parent_associate_range?;
+
+    Some(CompileError::new(
+                "This resource is explicitly associated to both the parent and child.  (Perhaps you meant to extend the existing resource in the child?)",
+                child_file,
+                current_associate_range,
+                "Associated in the child here")
+            .add_additional_message(
+                parent_file,
+                parent_associate_range,
+                "But it was already associated in this parent"))
+}
+
 fn interpret_associate(
     global_exprs: &mut HashSet<Expression>,
     local_exprs: &mut HashSet<Expression>,
diff --git a/src/internal_rep.rs b/src/internal_rep.rs
index 653b71fc..5a8571f5 100644
--- a/src/internal_rep.rs
+++ b/src/internal_rep.rs
@@ -315,6 +315,23 @@ impl TypeInfo {
         }
         ret
     }
+
+    // If this TI associates a type named associate_name, return its range
+    // Note that if the association is synthetic, the range will be None, so this finds
+    // specifically associations specifically in source, rather than somehow derived by the
+    // compiler
+    pub fn explicitly_associates(&self, associate_name: &str) -> Option<Range<usize>> {
+        for ann in &self.annotations {
+            if let AnnotationInfo::Associate(associations) = ann {
+                for res in &associations.resources {
+                    if res.as_ref() == associate_name && res.get_range().is_some() {
+                        return res.get_range();
+                    }
+                }
+            }
+        }
+        None
+    }
 }
 
 // This is the sexp for *declaring* the type