From eaeba15eba0344a00faa3c8053d4431359bd0f30 Mon Sep 17 00:00:00 2001 From: Austin Henriksen Date: Fri, 28 Aug 2026 15:21:27 -0400 Subject: [PATCH 1/5] Merged the 2 'find_node' functions as well. --- slicec/src/ast/mod.rs | 121 +++++++++------------------- slicec/src/parsers/slice/grammar.rs | 7 +- 2 files changed, 41 insertions(+), 87 deletions(-) diff --git a/slicec/src/ast/mod.rs b/slicec/src/ast/mod.rs index 2519e0aa..8fc38931 100644 --- a/slicec/src/ast/mod.rs +++ b/slicec/src/ast/mod.rs @@ -13,7 +13,7 @@ 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 +/// nodes (see [`find_node_with_scope`](Ast::find_node_with_scope)) 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 @@ -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,28 +86,17 @@ 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 @@ -160,42 +118,35 @@ 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); - } - - // Split the provided scope into an iterator of scope segments. - let mut scopes = scope.split("::").collect::>(); + // 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::>(); - // 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]); + // 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]); + } + // 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); + let Some(index) = self.lookup_table.get(stripped_identifier) else { + let identifier = stripped_identifier.to_owned(); + return Err(LookupError::DoesNotExist { identifier }); + }; + Ok(&self.elements[*index]) } /// Returns a reference to a Slice symbol (user-defined element) with the provided identifier and specified type, @@ -212,7 +163,11 @@ impl Ast { where &'a T: TryFrom<&'a Node, Error = LookupError>, { - self.find_node(identifier).and_then(|x| x.try_into()) + let Some(index) = self.lookup_table.get(identifier) else { + let identifier = identifier.to_owned(); + return Err(LookupError::DoesNotExist { identifier }); + }; + (&self.elements[*index]).try_into() } /// Returns an immutable slice of all the [nodes](Node) contained in this AST. diff --git a/slicec/src/parsers/slice/grammar.rs b/slicec/src/parsers/slice/grammar.rs index cf1fd105..529d78a7 100644 --- a/slicec/src/parsers/slice/grammar.rs +++ b/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 = 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 = node.try_into().unwrap(); + TypeRefDefinition::Patched(upcast_weak_as!(primitive_ptr, dyn Type)) } fn anonymous_type_to_type_ref_definition(parser: &mut Parser, ptr: OwnedPtr) -> TypeRefDefinition From 8625b6194d8ece63b7d2f4feee47acfef3f43964 Mon Sep 17 00:00:00 2001 From: Austin Henriksen Date: Mon, 31 Aug 2026 11:55:56 -0400 Subject: [PATCH 2/5] Pull out common code to helper function. --- slicec/src/ast/mod.rs | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/slicec/src/ast/mod.rs b/slicec/src/ast/mod.rs index 8fc38931..b864e098 100644 --- a/slicec/src/ast/mod.rs +++ b/slicec/src/ast/mod.rs @@ -103,12 +103,12 @@ impl Ast { /// 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. /// @@ -129,8 +129,8 @@ impl Ast { // 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 Ok(node) = self.lookup_node_by_id(&candidate) { + return Ok(node); } // Pop the last scope segment off to get to the next highest scope. scopes.pop(); @@ -142,11 +142,7 @@ impl Ast { // 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); - let Some(index) = self.lookup_table.get(stripped_identifier) else { - let identifier = stripped_identifier.to_owned(); - return Err(LookupError::DoesNotExist { identifier }); - }; - Ok(&self.elements[*index]) + self.lookup_node_by_id(stripped_identifier) } /// Returns a reference to a Slice symbol (user-defined element) with the provided identifier and specified type, @@ -163,11 +159,7 @@ impl Ast { where &'a T: TryFrom<&'a Node, Error = LookupError>, { - let Some(index) = self.lookup_table.get(identifier) else { - let identifier = identifier.to_owned(); - return Err(LookupError::DoesNotExist { identifier }); - }; - (&self.elements[*index]).try_into() + self.lookup_node_by_id(identifier)?.try_into() } /// Returns an immutable slice of all the [nodes](Node) contained in this AST. @@ -226,6 +218,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 { From d93dd3973127fa94f3f405253dcbc3c576c963da Mon Sep 17 00:00:00 2001 From: Austin Henriksen Date: Mon, 31 Aug 2026 12:02:14 -0400 Subject: [PATCH 3/5] Added a test and other cleanups. --- slicec/src/ast/mod.rs | 4 +-- slicec/src/patchers/comment_link_patcher.rs | 4 +-- slicec/src/patchers/type_ref_patcher.rs | 4 +-- slicec/tests/primitives/mod.rs | 31 +++++++++++++++++++++ 4 files changed, 37 insertions(+), 6 deletions(-) diff --git a/slicec/src/ast/mod.rs b/slicec/src/ast/mod.rs index b864e098..cd6478ea 100644 --- a/slicec/src/ast/mod.rs +++ b/slicec/src/ast/mod.rs @@ -13,7 +13,7 @@ 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_with_scope`](Ast::find_node_with_scope)) and their +/// 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 @@ -118,7 +118,7 @@ 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. - pub fn find_node_with_scope<'a>(&'a self, identifier: &str, scope: &str) -> Result<&'a Node, LookupError> { + 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("::") { diff --git a/slicec/src/patchers/comment_link_patcher.rs b/slicec/src/patchers/comment_link_patcher.rs index 0a537d47..d64c36d5 100644 --- a/slicec/src/patchers/comment_link_patcher.rs +++ b/slicec/src/patchers/comment_link_patcher.rs @@ -104,10 +104,10 @@ impl CommentLinkPatcher<'_> { // Look up the linked-to entity in the AST. let result = ast - .find_node_with_scope(&identifier.value, &commentable.parser_scoped_identifier()) + .find_node_by_id(&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`"), + _ => unreachable!("`find_node_by_id` reported an error other than `DoesNotExist`"), }) .and_then(convert_node_to_entity_ptr); diff --git a/slicec/src/patchers/type_ref_patcher.rs b/slicec/src/patchers/type_ref_patcher.rs index cadf1166..4cb45c5a 100644 --- a/slicec/src/patchers/type_ref_patcher.rs +++ b/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(); diff --git a/slicec/tests/primitives/mod.rs b/slicec/tests/primitives/mod.rs index acaefb08..f69f1cc6 100644 --- a/slicec/tests/primitives/mod.rs +++ b/slicec/tests/primitives/mod.rs @@ -44,3 +44,34 @@ fn type_parses(slice_component: &str, expected: Primitive) { panic!("type alias was unpatched"); } } + +#[test_case(Primitive::Bool; "bool")] +#[test_case(Primitive::Int8; "int8")] +#[test_case(Primitive::UInt8; "uint8")] +#[test_case(Primitive::Int16; "int16")] +#[test_case(Primitive::UInt16; "uint16")] +#[test_case(Primitive::Int32; "int32")] +#[test_case(Primitive::UInt32; "uint32")] +#[test_case(Primitive::VarInt32; "varint32")] +#[test_case(Primitive::VarUInt32; "varuint32")] +#[test_case(Primitive::Int64; "int64")] +#[test_case(Primitive::UInt64; "uint64")] +#[test_case(Primitive::VarInt62; "varint62")] +#[test_case(Primitive::VarUInt62; "varuint62")] +#[test_case(Primitive::Float32; "float32")] +#[test_case(Primitive::Float64; "float64")] +#[test_case(Primitive::String; "string")] +fn find_primitive_node_returns_the_correct_node(primitive: Primitive) { + // `find_primitive_node` indexes into the AST's elements by the primitive's discriminant, + // which relies on the ordering of the `Primitive` enum. This test ensures this ordering is consistent. + + // Arrange + let ast = slicec::ast::Ast::create(); + let expected_kind = primitive.kind(); + + // Act + let element: &dyn Element = ast.find_primitive_node(primitive).into(); + + // Assert + assert_eq!(element.kind(), expected_kind); +} From 99dc77d57522085adf3d11cf61e4b633966442e6 Mon Sep 17 00:00:00 2001 From: Austin Henriksen Date: Mon, 31 Aug 2026 15:54:51 -0400 Subject: [PATCH 4/5] Tests and doc-comment handling. --- slicec/src/ast/mod.rs | 4 +- slicec/src/grammar/elements/primitive.rs | 28 +++++ slicec/src/patchers/comment_link_patcher.rs | 15 +-- slicec/tests/comment_tests.rs | 24 +++++ slicec/tests/identifier_tests.rs | 108 +++++++++++++++++++- slicec/tests/primitives/mod.rs | 2 +- 6 files changed, 171 insertions(+), 10 deletions(-) diff --git a/slicec/src/ast/mod.rs b/slicec/src/ast/mod.rs index cd6478ea..2b27f91d 100644 --- a/slicec/src/ast/mod.rs +++ b/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_by_id`](Ast::find_node_by_id)) 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`. diff --git a/slicec/src/grammar/elements/primitive.rs b/slicec/src/grammar/elements/primitive.rs index 29803f5a..9fba99a9 100644 --- a/slicec/src/grammar/elements/primitive.rs +++ b/slicec/src/grammar/elements/primitive.rs @@ -1,5 +1,7 @@ // Copyright (c) ZeroC, Inc. +use std::str::FromStr; + use super::super::*; #[derive(Debug, Eq, PartialEq)] @@ -92,3 +94,29 @@ impl Element for Primitive { } } } + +impl FromStr for Primitive { + type Err = (); + + fn from_str(s: &str) -> Result { + 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(()), + } + } +} diff --git a/slicec/src/patchers/comment_link_patcher.rs b/slicec/src/patchers/comment_link_patcher.rs index d64c36d5..0f89f294 100644 --- a/slicec/src/patchers/comment_link_patcher.rs +++ b/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_by_id(&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_by_id` reported an error other than `DoesNotExist`"), + .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, 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!"), } } diff --git a/slicec/tests/comment_tests.rs b/slicec/tests/comment_tests.rs index e4d56424..1eda10a9 100644 --- a/slicec/tests/comment_tests.rs +++ b/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::("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 diff --git a/slicec/tests/identifier_tests.rs b/slicec/tests/identifier_tests.rs index 8f4d70e8..5c73a785 100644 --- a/slicec/tests/identifier_tests.rs +++ b/slicec/tests/identifier_tests.rs @@ -4,7 +4,7 @@ mod test_helpers; use crate::test_helpers::*; use slicec::diagnostics::{Diagnostic, Error}; -use slicec::grammar::{CustomType, Interface, Struct}; +use slicec::grammar::{CustomType, Field, Interface, Module, NamedSymbol, Primitive, Struct, Types}; #[test] fn escaped_keywords() { @@ -44,6 +44,112 @@ fn escaped_identifiers() { assert!(ast.find_symbol_by_id::("MyModule::MyCustom").is_ok()); } +#[test] +fn module_named_after_primitive_keyword_is_allowed() { + // Arrange + let slice = r#" + module \int32 + + struct Foo { + x: int32 + } + "#; + + // Act + let ast = parse_for_ast(slice); + + // Assert + // The module parses correctly, and has an identifier of "int32". + assert!(ast.find_symbol_by_id::("int32").is_ok()); + + // The field in `Foo` still resolves to the `int32` primitive type, not the module. + let field = ast.find_symbol_by_id::("int32::Foo::x").unwrap(); + assert!(matches!( + field.data_type.concrete_type(), + Types::Primitive(Primitive::Int32) + )); +} + +#[test] +fn top_level_element_named_after_primitive_keyword_is_allowed() { + // Arrange + let slice = r#" + module Test + + struct \string {} + + struct Foo { + a: string + b: \string + } + "#; + + // Act + let ast = parse_for_ast(slice); + + // Assert + // The struct parses correctly, and has an identifier of "string". + let string_struct = ast.find_symbol_by_id::("Test::string").unwrap(); + assert!(string_struct.identifier() == "string"); + + // The fields in `Foo` correctly resolve to the primitive type, and the struct, depending on escaping. + let field_a = ast.find_symbol_by_id::("Test::Foo::a").unwrap(); + assert!(matches!( + field_a.data_type.concrete_type(), + Types::Primitive(Primitive::String) + )); + let field_b = ast.find_symbol_by_id::("Test::Foo::b").unwrap(); + assert!(matches!(field_b.data_type.concrete_type(), Types::Struct(_))); +} + +#[test] +fn keyword_named_types_without_a_module_do_not_panic() { + // Arrange + let slice = r#" + struct \int32 {} + + struct Foo { + f: int32 + } + "#; + + // Act + let diagnostics = parse_for_diagnostics(slice); + + // Assert + let expected = Diagnostic::from_error(Error::Syntax { + message: "module declaration is required".to_owned(), + }); + check_diagnostics(diagnostics, [expected]); +} + +#[test] +fn elements_in_modules_named_after_primitive_keywords_are_referenceable() { + // Arrange + let slice1 = r#" + module Hello::\int32 + + struct Foo {} + "#; + let slice2 = r#" + module Test + + struct S { + a: Hello::\int32::Foo + } + "#; + + // Act + let ast = parse_multiple_for_ast(&[slice1, slice2]); + + // Assert + let field = ast.find_symbol_by_id::("Test::S::a").unwrap(); + let Types::Struct(struct_def) = field.data_type.concrete_type() else { + panic!("field type was not a struct"); + }; + assert_eq!(struct_def.module_scoped_identifier(), "Hello::int32::Foo"); +} + #[test] fn must_start_with_a_letter() { // Arrange diff --git a/slicec/tests/primitives/mod.rs b/slicec/tests/primitives/mod.rs index f69f1cc6..383a2397 100644 --- a/slicec/tests/primitives/mod.rs +++ b/slicec/tests/primitives/mod.rs @@ -62,7 +62,7 @@ fn type_parses(slice_component: &str, expected: Primitive) { #[test_case(Primitive::Float64; "float64")] #[test_case(Primitive::String; "string")] fn find_primitive_node_returns_the_correct_node(primitive: Primitive) { - // `find_primitive_node` indexes into the AST's elements by the primitive's discriminant, + // `find_primitive_node` indexes into the AST's elements by the primitive's discriminant, // which relies on the ordering of the `Primitive` enum. This test ensures this ordering is consistent. // Arrange From 31d6a2aef356ad1eec9762b5afbb2ed3aeab3ab9 Mon Sep 17 00:00:00 2001 From: Austin Henriksen Date: Mon, 31 Aug 2026 16:29:03 -0400 Subject: [PATCH 5/5] Review fixes. --- slicec/src/ast/mod.rs | 5 +++-- slicec/src/grammar/elements/primitive.rs | 3 +-- slicec/tests/identifier_tests.rs | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/slicec/src/ast/mod.rs b/slicec/src/ast/mod.rs index 2b27f91d..95472f86 100644 --- a/slicec/src/ast/mod.rs +++ b/slicec/src/ast/mod.rs @@ -129,8 +129,9 @@ impl Ast { // 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 Ok(node) = self.lookup_node_by_id(&candidate) { - return Ok(node); + + 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(); diff --git a/slicec/src/grammar/elements/primitive.rs b/slicec/src/grammar/elements/primitive.rs index 9fba99a9..193f3f6a 100644 --- a/slicec/src/grammar/elements/primitive.rs +++ b/slicec/src/grammar/elements/primitive.rs @@ -1,8 +1,7 @@ // Copyright (c) ZeroC, Inc. -use std::str::FromStr; - use super::super::*; +use std::str::FromStr; #[derive(Debug, Eq, PartialEq)] pub enum Primitive { diff --git a/slicec/tests/identifier_tests.rs b/slicec/tests/identifier_tests.rs index 5c73a785..3db88963 100644 --- a/slicec/tests/identifier_tests.rs +++ b/slicec/tests/identifier_tests.rs @@ -90,7 +90,7 @@ fn top_level_element_named_after_primitive_keyword_is_allowed() { // Assert // The struct parses correctly, and has an identifier of "string". let string_struct = ast.find_symbol_by_id::("Test::string").unwrap(); - assert!(string_struct.identifier() == "string"); + assert_eq!(string_struct.identifier(), "string"); // The fields in `Foo` correctly resolve to the primitive type, and the struct, depending on escaping. let field_a = ast.find_symbol_by_id::("Test::Foo::a").unwrap();