Merge pull request #282 from Shopify/at-ref-ids

Use unique IDs for unresolved references

Author: Morriar

rust/saturn/src/indexing/ruby_indexer.rs

@@ -980,9 +980,27 @@ mod tests {
         };
     }
 
+    fn collect_unresolved_references(graph: &Graph) -> Vec<&UnresolvedReference> {
+        let mut unresolved_references = graph.unresolved_references().values().collect::<Vec<_>>();
+
+        unresolved_references.sort_by_key(|r| match r {
+            UnresolvedReference::Constant(constant) => (
+                graph.documents().get(&constant.uri_id()).unwrap().uri().to_string(),
+                constant.offset().start(),
+                graph.names().get(constant.name_id()).unwrap(),
+            ),
+            UnresolvedReference::Method(method) => (
+                graph.documents().get(&method.uri_id()).unwrap().uri().to_string(),
+                method.offset().start(),
+                graph.names().get(method.name_id()).unwrap(),
+            ),
+        });
+
+        unresolved_references
+    }
+
     fn collect_constant_reference_names(graph: &Graph) -> Vec<&String> {
-        graph
-            .unresolved_references()
+        collect_unresolved_references(graph)
             .iter()
             .filter_map(|r| match r {
                 UnresolvedReference::Constant(constant) => Some(graph.names().get(constant.name_id()).unwrap()),
@@ -992,8 +1010,7 @@ mod tests {
     }
 
     fn collect_method_reference_names(graph: &Graph) -> Vec<&String> {
-        graph
-            .unresolved_references()
+        collect_unresolved_references(graph)
             .iter()
             .filter_map(|r| match r {
                 UnresolvedReference::Constant(_) => None,
@@ -2037,7 +2054,7 @@ mod tests {
             "
         });
 
-        let refs = context.graph.unresolved_references();
+        let refs = context.graph.unresolved_references().values().collect::<Vec<_>>();
         assert_eq!(refs.len(), 2);
 
         let reference = &refs[0];
@@ -2089,7 +2106,7 @@ mod tests {
             "
         });
 
-        let refs = context.graph.unresolved_references();
+        let refs = context.graph.unresolved_references().values().collect::<Vec<_>>();
         assert_eq!(refs.len(), 1);
 
         let reference = &refs[0];
@@ -2124,7 +2141,7 @@ mod tests {
             "
         });
 
-        let refs = context.graph.unresolved_references();
+        let refs = context.graph.unresolved_references().values().collect::<Vec<_>>();
         assert_eq!(refs.len(), 2);
 
         let reference = &refs[0];
@@ -2350,7 +2367,7 @@ mod tests {
             collect_method_reference_names(&context.graph),
             vec![
                 "m1", "m2", "m3", "m4", "m5", "m6", "m7", "m8", "m9", "m10", "m11", "m12", "m13", "m14", "m15", "m16",
-                "m17", "m18", "m19", "m20", "m21", "m22", "m23", "m24", "m25", "m26", "m27", "!", "m28", "m29", "m30",
+                "m17", "m18", "m19", "m20", "m21", "m22", "m23", "m24", "m25", "m26", "!", "m27", "m28", "m29", "m30",
                 "m31", "m32", "m33", "m34", "m35", "m36", "[]", "m37", "m38", "m39", "m40", "m41", "m42", "m43", "m44",
                 "m45", "m46", "m47", "m48", "m49", "m50"
             ]
@@ -2488,7 +2505,7 @@ mod tests {
 
         assert_eq!(
             collect_method_reference_names(&context.graph),
-            vec!["x", ">", "<=>", "y"]
+            vec!["x", "<=>", ">", "y"]
         );
     }
 
@@ -2504,7 +2521,7 @@ mod tests {
 
         assert_eq!(
             collect_method_reference_names(&context.graph),
-            vec!["x", ">=", "<=>", "y"]
+            vec!["x", "<=>", ">=", "y"]
         );
     }
 

rust/saturn/src/model/graph.rs

@@ -4,7 +4,7 @@ use crate::model::declaration::Declaration;
 use crate::model::definitions::Definition;
 use crate::model::document::Document;
 use crate::model::identity_maps::IdentityHashMap;
-use crate::model::ids::{DeclarationId, DefinitionId, NameId, UriId};
+use crate::model::ids::{DeclarationId, DefinitionId, NameId, ReferenceId, UriId};
 use crate::model::integrity::IntegrityChecker;
 use crate::model::references::{ResolvedReference, UnresolvedReference};
 use crate::stats;
@@ -28,8 +28,8 @@ pub struct Graph {
     definitions: IdentityHashMap<DefinitionId, Definition>,
     // Map of unqualified names
     names: IdentityHashMap<NameId, String>,
-    // List of references that still need to be resolved
-    unresolved_references: Vec<UnresolvedReference>,
+    // Map of references that still need to be resolved
+    unresolved_references: IdentityHashMap<ReferenceId, UnresolvedReference>,
     db: Db,
 }
 
@@ -47,7 +47,7 @@ impl Graph {
             definitions: IdentityHashMap::default(),
             documents: IdentityHashMap::default(),
             names: IdentityHashMap::default(),
-            unresolved_references: Vec::new(),
+            unresolved_references: IdentityHashMap::default(),
             db: Db::new(),
         }
     }
@@ -76,9 +76,9 @@ impl Graph {
         &self.documents
     }
 
-    // Returns an immutable reference to the list of unresolved references
+    // Returns an immutable reference to the unresolved references map
     #[must_use]
-    pub fn unresolved_references(&self) -> &Vec<UnresolvedReference> {
+    pub fn unresolved_references(&self) -> &IdentityHashMap<ReferenceId, UnresolvedReference> {
         &self.unresolved_references
     }
 
@@ -182,8 +182,10 @@ impl Graph {
     }
 
     // Register an unresolved reference to something (e.g.: constant, method, variable), which has to be resolved later
-    pub fn add_unresolved_reference(&mut self, reference: UnresolvedReference) {
-        self.unresolved_references.push(reference);
+    pub fn add_unresolved_reference(&mut self, reference: UnresolvedReference) -> ReferenceId {
+        let reference_id = reference.id();
+        self.unresolved_references.insert(reference_id, reference);
+        reference_id
     }
 
     /// Attempts to resolve a reference against the graph. Returns the fully qualified declaration ID that the reference
@@ -469,7 +471,7 @@ impl Graph {
         self.definitions = IdentityHashMap::default();
         self.documents = IdentityHashMap::default();
         self.names = IdentityHashMap::default();
-        self.unresolved_references = Vec::new();
+        self.unresolved_references = IdentityHashMap::default();
     }
 
     #[allow(clippy::cast_precision_loss)]
@@ -539,7 +541,7 @@ impl Graph {
     pub fn resolve_references(&mut self) {
         let unresolved = std::mem::take(&mut self.unresolved_references);
 
-        for reference in unresolved {
+        for (id, reference) in unresolved {
             match &reference {
                 UnresolvedReference::Constant(_) => {
                     if let Some(declaration) = self.resolve_reference(&reference) {
@@ -562,12 +564,12 @@ impl Graph {
                         }
                     } else {
                         // Keep unresolved references
-                        self.unresolved_references.push(reference);
+                        self.unresolved_references.insert(id, reference);
                     }
                 }
                 UnresolvedReference::Method(_) => {
                     // TODO: Implement method reference resolution
-                    self.unresolved_references.push(reference);
+                    self.unresolved_references.insert(id, reference);
                 }
             }
         }
@@ -594,7 +596,7 @@ impl Graph {
         // Count unresolved references by type
         let mut unresolved_constants = 0;
         let mut unresolved_methods = 0;
-        for reference in &self.unresolved_references {
+        for reference in self.unresolved_references.values() {
             match reference {
                 UnresolvedReference::Constant(_) => unresolved_constants += 1,
                 UnresolvedReference::Method(_) => unresolved_methods += 1,
@@ -705,7 +707,7 @@ mod tests {
             let constant = $context
                 .graph
                 .unresolved_references
-                .iter()
+                .values()
                 .filter_map(|r| {
                     if let UnresolvedReference::Constant(c) = r {
                         Some(c)
@@ -975,6 +977,14 @@ mod tests {
     fn resolving_constants() {
         let mut context = GraphTest::new();
 
+        context.index_uri("file:///bar.rb", {
+            r"
+            class Bar
+              ::Foo::Bar # should be resolved to Foo::Bar from file:///foo.rb
+            end
+            "
+        });
+
         context.index_uri("file:///foo.rb", {
             r"
             module Foo
@@ -992,12 +1002,31 @@ mod tests {
             "
         });
 
-        context.index_uri("file:///bar.rb", {
-            r"
-            class Bar
-              ::Foo::Bar # should be resolved to Foo::Bar from file:///foo.rb
-            end
-            "
+        let mut unresolved_references = context.graph.unresolved_references().values().collect::<Vec<_>>();
+
+        unresolved_references.sort_by_key(|r| match r {
+            UnresolvedReference::Constant(constant) => (
+                context
+                    .graph
+                    .documents()
+                    .get(&constant.uri_id())
+                    .unwrap()
+                    .uri()
+                    .to_string(),
+                constant.offset().start(),
+                constant.offset().end(),
+            ),
+            UnresolvedReference::Method(method) => (
+                context
+                    .graph
+                    .documents()
+                    .get(&method.uri_id())
+                    .unwrap()
+                    .uri()
+                    .to_string(),
+                method.offset().start(),
+                method.offset().end(),
+            ),
         });
 
         context.graph.resolve_references();

rust/saturn/src/model/ids.rs

@@ -29,3 +29,9 @@ pub struct NameMarker;
 ///
 /// In here, the `NameId` for `Bar` is just the hashed `Bar` string, not `Foo::Bar`
 pub type NameId = Id<NameMarker>;
+
+#[derive(PartialEq, Eq, Debug, Clone, Copy, Serialize, Deserialize)]
+pub struct ReferenceMarker;
+/// `ReferenceId` represents the ID of a reference occurrence in a file.
+/// It is built from the reference kind, `uri_id` and the reference `offset`.
+pub type ReferenceId = Id<ReferenceMarker>;

rust/saturn/src/model/references.rs

@@ -1,6 +1,6 @@
 use crate::{
     indexing::scope::Nesting,
-    model::ids::{NameId, UriId},
+    model::ids::{NameId, ReferenceId, UriId},
     offset::Offset,
 };
 use serde::{Deserialize, Serialize};
@@ -65,6 +65,36 @@ pub enum ResolvedReference {
     Method(Box<MethodReference>),
 }
 
+impl UnresolvedReference {
+    #[must_use]
+    pub fn id(&self) -> ReferenceId {
+        match self {
+            UnresolvedReference::Constant(constant) => {
+                // C:<uri_id>:<start>-<end>
+                let key = format!(
+                    "C:{}:{}:{}-{}",
+                    constant.name_id(),
+                    constant.uri_id(),
+                    constant.offset().start(),
+                    constant.offset().end()
+                );
+                ReferenceId::from(&key)
+            }
+            UnresolvedReference::Method(method) => {
+                // M:<uri_id>:<start>-<end>
+                let key = format!(
+                    "M:{}:{}:{}-{}",
+                    method.name_id(),
+                    method.uri_id(),
+                    method.offset().start(),
+                    method.offset().end()
+                );
+                ReferenceId::from(&key)
+            }
+        }
+    }
+}
+
 impl ConstantReference {
     #[must_use]
     pub fn new(name_id: NameId, nesting: Option<Arc<Nesting>>, uri_id: UriId, offset: Offset) -> Self {