Fix Ambiguity in Elements using Escaped Primitive Keywords

slicec/src/ast/mod.rs

@@ -13,8 +13,8 @@ use std::collections::HashMap;
 /// slice files passed into the compiler.
 ///
 /// The AST is primarily for centralizing ownership of Slice elements, but also features lookup functions for finding
-/// nodes (see [`find_node`](Ast::find_node) and [`find_node_with_scope`](Ast::find_node_with_scope)) and their
-/// elements (see [`find_symbol_by_id`](Ast::find_symbol_by_id)).
+/// nodes (see [`find_node_by_id`](Ast::find_node_by_id)) and their elements
+/// (see [`find_symbol_by_id`](Ast::find_symbol_by_id)).
 ///
 /// In practice, there is a single instance of the AST per compilation, which is [created](Ast::create) during
 /// initialization and lives as long as the program does, making the AST effectively `'static`.
@@ -50,7 +50,6 @@ impl Ast {
     pub fn create() -> Ast {
         // Primitive types are built in to the compiler. Since they aren't defined in Slice, we 'define' them here,
         // when the AST is created, to ensure they're always available.
-
         let elements = vec![
             Node::Primitive(OwnedPtr::new(Primitive::Bool)),
             Node::Primitive(OwnedPtr::new(Primitive::Int8)),
@@ -70,44 +69,14 @@ impl Ast {
             Node::Primitive(OwnedPtr::new(Primitive::String)),
         ];
 
-        let lookup_table = HashMap::from([
-            ("bool".to_owned(), 0),
-            ("int8".to_owned(), 1),
-            ("uint8".to_owned(), 2),
-            ("int16".to_owned(), 3),
-            ("uint16".to_owned(), 4),
-            ("int32".to_owned(), 5),
-            ("uint32".to_owned(), 6),
-            ("varint32".to_owned(), 7),
-            ("varuint32".to_owned(), 8),
-            ("int64".to_owned(), 9),
-            ("uint64".to_owned(), 10),
-            ("varint62".to_owned(), 11),
-            ("varuint62".to_owned(), 12),
-            ("float32".to_owned(), 13),
-            ("float64".to_owned(), 14),
-            ("string".to_owned(), 15),
-        ]);
+        let lookup_table = HashMap::new();
 
         Ast { elements, lookup_table }
     }
 
-    /// Returns a reference to the AST [node](Node) with the provided identifier, if one exists.
-    /// The identifier must be fully qualified, since this performs no scope resolution, but cannot begin with '::'.
-    ///
-    /// Anonymous types (those without identifiers) cannot be looked up. These are results, sequences, and dictionaries.
-    /// Primitive types can be looked up by their Slice keywords. Care should be taken when looking up modules (which
-    /// can be re-opened) or parameters and return members (which share an AST scope), since these may not be unique.
+    /// Returns a reference to the Ast [node](Node) that corresponds to the provided [primitive](Primitive) type.
     ///
     /// This is a low level method used for retrieving nodes from the AST directly.
-    /// Only use this if you need access to the node, or the pointer, holding a slice element.
-    ///
-    /// If you want a reference to the Slice construct itself, use [find_symbol_by_id](Ast::find_symbol_by_id) instead.
-    ///
-    /// # Returns
-    ///
-    /// If a [node](Node) can be found with the provided identifier, this returns a reference to its [node](Node) in
-    /// the AST, wrapped in `Ok`. Otherwise, this returns `Err` with a string describing why the lookup failed.
     ///
     /// # Examples
     ///
@@ -117,40 +86,29 @@ impl Ast {
     /// let ast = Ast::create();
     ///
     /// // Lookup a primitive type.
-    /// let int32_node = ast.find_node("int32");
-    /// assert!(int32_node.is_ok());
-    ///
-    /// // TODO add more examples once parsing is easier.
-    ///
-    /// // If an element doesn't exist with the specified identifier, `Err` is returned.
-    /// let fake_node = ast.find_node("foo::bar");
-    /// assert!(fake_node.is_err());
+    /// let int32: &dyn Element = ast.find_primitive_node(Primitive::Int32).into();
+    /// assert_eq!(int32.kind(), "int32");
     /// ```
-    pub fn find_node<'a>(&'a self, identifier: &str) -> Result<&'a Node, LookupError> {
-        self.lookup_table
-            .get(identifier)
-            .map(|i| &self.elements[*i])
-            .ok_or_else(|| LookupError::DoesNotExist {
-                identifier: identifier.to_owned(),
-            })
+    pub fn find_primitive_node(&self, primitive: Primitive) -> &Node {
+        self.elements.get(primitive as usize).expect("Missing primitive node!")
     }
 
     /// Returns a reference to the AST [node](Node) with the provided identifier, if one exists.
     ///
-    /// If the identifier begins with '::' it is treated as globally scoped, and this function just forwards to
-    /// [`find_node`](Ast::find_node). Otherwise the identifier is treated as being relatively scoped.
+    /// If the identifier starts with '::' it is treated as globally scoped, otherwise it is treated as relatively
+    /// scoped.
     ///
     /// For relative identifiers, this method first checks if the identifier is defined in the provided scope. If so, a
     /// reference is returned to it. Otherwise each enclosing scope is checked, starting from the provided scope, and
     /// working outwards through each of its parent scopes until reaching global scope.
     ///
     /// This returns the first matching AST node it can find. If another node in a more outward scope also has the
-    /// specified identifier, it is shadowed, and will not be returned.
-    ///
-    /// Anonymous types (those without identifiers) cannot be looked up. These are results, sequences, and dictionaries.
-    /// Primitive types can be looked up by their Slice keywords. Care should be taken when looking up modules (which
+    /// specified identifier, it is shadowed, and will not be returned. Exercise care when looking up modules (which
     /// can be re-opened) or parameters and return members (which share an AST scope), since these may not be unique.
     ///
+    /// Primitive types (`int32`, `string`, etc.) and anonymous types (results, sequences, and dictionaries)
+    /// cannot be looked up with this method.
+    ///
     /// This is a low level method used for retrieving nodes from the AST directly.
     /// Only use this if you need access to the node, or the pointer, holding a slice element.
     ///
@@ -160,42 +118,32 @@ impl Ast {
     ///
     /// If a node can be found with the provided identifier, this returns a reference to its [node](Node) in the AST
     /// wrapped in `Ok`. Otherwise, this returns `Err` with a string describing why the lookup failed.
-    ///
-    /// # Examples
-    ///
-    /// ```
-    /// # use slicec::ast::Ast;
-    /// # use slicec::grammar::*;
-    /// let ast = Ast::create();
-    ///
-    /// // TODO add more examples once parsing is easier.
-    ///
-    /// // If an element doesn't exist with the specified identifier, `Err` is returned.
-    /// let fake_node = ast.find_node_with_scope("hello", "foo::bar");
-    /// assert!(fake_node.is_err());
-    /// ```
-    pub fn find_node_with_scope<'a>(&'a self, identifier: &str, scope: &str) -> Result<&'a Node, LookupError> {
-        // If the identifier is globally scoped (starts with '::'), find the node without scoping.
-        if let Some(unprefixed_identifier) = identifier.strip_prefix("::") {
-            return self.find_node(unprefixed_identifier);
-        }
+    pub fn find_node_by_id<'a>(&'a self, identifier: &str, scope: &str) -> Result<&'a Node, LookupError> {
+        // If the identifier isn't globally scoped, we check for it in the provided scope,
+        // followed by each of its parent scopes, until finally landing at global scope.
+        if !identifier.starts_with("::") {
+            // Split the provided scope into an iterator of scope segments.
+            let mut scopes = scope.split("::").collect::<Vec<_>>();
 
-        // Split the provided scope into an iterator of scope segments.
-        let mut scopes = scope.split("::").collect::<Vec<_>>();
+            // Check for the identifier with the full scope first.
+            // If it doesn't exist, keep checking for it in parent scopes until all enclosing scopes have been checked.
+            while !scopes.is_empty() {
+                let candidate = scopes.join("::") + "::" + identifier;
 
-        // Check for the identifier with the full scope first.
-        // If it doesn't exist, keep checking for it in parent scopes until all enclosing scopes have been checked.
-        while !scopes.is_empty() {
-            let candidate = scopes.join("::") + "::" + identifier;
-            if let Some(i) = self.lookup_table.get(&candidate) {
-                return Ok(&self.elements[*i]);
+                if let Some(index) = self.lookup_table.get(&candidate) {
+                    return Ok(&self.elements[*index]);
+                }
+                // Pop the last scope segment off to get to the next highest scope.
+                scopes.pop();
             }
-            // Pop the last scope segment off to get to the next highest scope.
-            scopes.pop();
+
+            // If the identifier wasn't defined in any of the scopes, fallback to checking for it at global scope.
         }
 
-        // If the identifier wasn't defined in any of the scopes, check for it at global scope.
-        self.find_node(identifier)
+        // Remove any leading '::' from the identifier, since the lookup table doesn't store them.
+        // TODO switch to 'trim_prefix' (https://github.com/rust-lang/rust/issues/142312) when it's stabilized.
+        let stripped_identifier = identifier.strip_prefix("::").unwrap_or(identifier);
+        self.lookup_node_by_id(stripped_identifier)
     }
 
     /// Returns a reference to a Slice symbol (user-defined element) with the provided identifier and specified type,
@@ -212,7 +160,7 @@ impl Ast {
     where
         &'a T: TryFrom<&'a Node, Error = LookupError>,
     {
-        self.find_node(identifier).and_then(|x| x.try_into())
+        self.lookup_node_by_id(identifier)?.try_into()
     }
 
     /// Returns an immutable slice of all the [nodes](Node) contained in this AST.
@@ -271,6 +219,15 @@ impl Ast {
         // Add the element to this AST.
         self.add_element(element)
     }
+
+    fn lookup_node_by_id<'a>(&'a self, identifier: &str) -> Result<&'a Node, LookupError> {
+        match self.lookup_table.get(identifier) {
+            Some(index) => Ok(&self.elements[*index]),
+            None => Err(LookupError::DoesNotExist {
+                identifier: identifier.to_owned(),
+            }),
+        }
+    }
 }
 
 impl Default for Ast {

slicec/src/grammar/elements/primitive.rs

@@ -1,6 +1,7 @@
 // Copyright (c) ZeroC, Inc.
 
 use super::super::*;
+use std::str::FromStr;
 
 #[derive(Debug, Eq, PartialEq)]
 pub enum Primitive {
@@ -92,3 +93,29 @@ impl Element for Primitive {
         }
     }
 }
+
+impl FromStr for Primitive {
+    type Err = ();
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        match s {
+            "bool" => Ok(Self::Bool),
+            "int8" => Ok(Self::Int8),
+            "uint8" => Ok(Self::UInt8),
+            "int16" => Ok(Self::Int16),
+            "uint16" => Ok(Self::UInt16),
+            "int32" => Ok(Self::Int32),
+            "uint32" => Ok(Self::UInt32),
+            "varint32" => Ok(Self::VarInt32),
+            "varuint32" => Ok(Self::VarUInt32),
+            "int64" => Ok(Self::Int64),
+            "uint64" => Ok(Self::UInt64),
+            "varint62" => Ok(Self::VarInt62),
+            "varuint62" => Ok(Self::VarUInt62),
+            "float32" => Ok(Self::Float32),
+            "float64" => Ok(Self::Float64),
+            "string" => Ok(Self::String),
+            _ => Err(()),
+        }
+    }
+}

slicec/src/parsers/slice/grammar.rs

@@ -415,10 +415,9 @@ fn construct_type_ref(
 }
 
 fn primitive_to_type_ref_definition(parser: &Parser, primitive: Primitive) -> TypeRefDefinition {
-    // These unwraps are safe because the primitive types are always defined in the AST.
-    let node = parser.ast.find_node(primitive.kind()).unwrap();
-    let weak_ptr: WeakPtr<Primitive> = node.try_into().unwrap();
-    TypeRefDefinition::Patched(upcast_weak_as!(weak_ptr, dyn Type))
+    let node = parser.ast.find_primitive_node(primitive);
+    let primitive_ptr: WeakPtr<Primitive> = node.try_into().unwrap();
+    TypeRefDefinition::Patched(upcast_weak_as!(primitive_ptr, dyn Type))
 }
 
 fn anonymous_type_to_type_ref_definition<T>(parser: &mut Parser, ptr: OwnedPtr<T>) -> TypeRefDefinition

slicec/src/patchers/comment_link_patcher.rs

@@ -7,6 +7,7 @@ use crate::diagnostics::{Diagnostic, Diagnostics, Lint};
 use crate::grammar::*;
 use crate::utils::ptr_util::{downgrade_as, WeakPtr};
 use std::collections::VecDeque;
+use std::str::FromStr;
 
 macro_rules! patch_link {
     ($self:ident, $tag:expr) => {
@@ -101,13 +102,16 @@ impl CommentLinkPatcher<'_> {
         let TypeRefDefinition::Unpatched(identifier) = link else {
             panic!("encountered comment link that was already patched");
         };
-
         // Look up the linked-to entity in the AST.
         let result = ast
-            .find_node_with_scope(&identifier.value, &commentable.parser_scoped_identifier())
-            .map_err(|lookup_error| match lookup_error {
-                LookupError::DoesNotExist { identifier } => format!("no element named '{identifier}' exists in scope"),
-                _ => unreachable!("`find_node_with_scope` reported an error other than `DoesNotExist`"),
+            .find_node_by_id(&identifier.value, &commentable.parser_scoped_identifier())
+            .map_err(|lookup_error| {
+                if Primitive::from_str(&identifier.value).is_ok() {
+                    "primitive types cannot be linked to".to_owned()
+                } else {
+                    assert!(matches!(lookup_error, LookupError::DoesNotExist { .. }));
+                    format!("no element named '{}' exists in scope", identifier.value)
+                }
             })
             .and_then(convert_node_to_entity_ptr);
 
@@ -163,8 +167,7 @@ fn convert_node_to_entity_ptr(node: &Node) -> Result<WeakPtr<dyn Entity>, String
 
         Node::Module(_) => Err("modules cannot be linked to".to_owned()),
         Node::Parameter(_) => Err("parameters cannot be linked to".to_owned()), // TODO improve for return members.
-        Node::Primitive(_) => Err("primitive types cannot be linked to".to_owned()),
 
-        _ => unreachable!("`convert_node_to_entity_ptr` was called on an anonymous type or attribute"),
+        _ => unreachable!("`convert_node_to_entity_ptr` was called on a non-user-defined element!"),
     }
 }

slicec/src/patchers/type_ref_patcher.rs

@@ -161,7 +161,7 @@ impl TypeRefPatcher<'_> {
         // Second, handle the case where the type is an alias (by resolving down to its concrete underlying type).
         // Third, get the type's pointer from its node and attempt to cast it to `T` (the required Slice type).
         let lookup_result = ast
-            .find_node_with_scope(&identifier.value, type_ref.module_scope())
+            .find_node_by_id(&identifier.value, type_ref.module_scope())
             .and_then(|node| {
                 // We perform the deprecation check here instead of the validators since we need to check type-aliases
                 // which are resolved and erased after TypeRef patching is completed.
@@ -266,7 +266,7 @@ impl TypeRefPatcher<'_> {
             };
 
             // We hit another unpatched alias; try to resolve its underlying type's identifier in the AST.
-            let node = ast.find_node_with_scope(&identifier.value, underlying_type.module_scope())?;
+            let node = ast.find_node_by_id(&identifier.value, underlying_type.module_scope())?;
             // If the resolved node is another type alias, push it onto the chain and loop again, otherwise return it.
             if let Node::TypeAlias(next_type_alias) = node {
                 current_type_alias = next_type_alias.borrow();

slicec/tests/comment_tests.rs

@@ -395,6 +395,30 @@ mod comments {
         check_diagnostics(diagnostics, [expected]);
     }
 
+    #[test]
+    fn doc_comment_links_preferentially_resolve_to_user_defined_elements() {
+        // Arrange
+        let slice = "
+            module tests
+
+            struct \\int32 {}
+
+            /// A test struct, should probably use {@link int32}.
+            struct TestStruct {}
+        ";
+
+        // Act
+        let ast = parse_for_ast(slice);
+
+        // Assert
+        let struct_def = ast.find_symbol_by_id::<Struct>("tests::TestStruct").unwrap();
+        let message = &struct_def.comment().unwrap().overview.as_ref().unwrap().value;
+
+        assert_eq!(message.len(), 4);
+        let MessageComponent::Link(link) = &message[1] else { panic!() };
+        assert_eq!(link.linked_entity().unwrap().parser_scoped_identifier(), "tests::int32");
+    }
+
     #[test]
     fn param_tag_is_rejected_for_operations_with_no_parameters() {
         // Arrange