Fix Ambiguity in Elements using Escaped Primitive Keywords by InsertCreativityHere · Pull Request #813 · icerpc/slicec

InsertCreativityHere · 2026-08-31T16:08:36Z

This PR fixes #808, which made it possible to 'overwrite' primitive types

module \int32
typealias Foo = int32 // This references the module now, not the primitive type!

It also fixes #815, which let users reference a primitive type as ::int32 in doc-comments, instead of treating that as a globally-scoped user-defined element.

But it does not use the proposed solution. It turns out there is a much simpler solution thanks to the fact:
"the Slice Lexer immediately patches primitives types". TypeRef can be in 2 states, either Patched or Unpatched. For user-defined types, the parser creates them Unpatched, and then after we've parsed everything, we lookup the names and patch all the types. But for primitives, we patch them immediately, since we already know what they are.

This means: We never need to lookup a primitive type by name. So, this PR drops the primitives from the lookup table entirely.
Making it absolutely impossible for a conflict to ever happen. Instead, we now have a dedicated function for looking up the primitive types, which bypasses all the lookup machinery.

What's Changed

This PR changes the following behaviors:

\xxx can no longer be used to reference the primitive type xxx. Now placing a backslash in front of a keyword means the escaped identifier will always be resolved against user-defined types and never treated as a primitive.
Fixed a bug where defining a module whose identifier was an escaped primitive type, would cause future resolutions of that primitive type to fail. For example:

module \int32
struct Foo { f: int32 } // `f` would be referencing the module, not the primitive here!

Fixed a bug where primitive types could be referenced with a leading scope in doc-comments.

InsertCreativityHere · 2026-08-31T19:57:00Z

    }
 }
+
+impl FromStr for Primitive {


Lets you convert a string into a Primitive. Added for convenience.

InsertCreativityHere · 2026-08-31T19:57:58Z

@@ -415,10 +415,9 @@ fn construct_type_ref(
 }

 fn primitive_to_type_ref_definition(parser: &Parser, primitive: Primitive) -> TypeRefDefinition {


The one place we were looking up primitive types.
Now it uses the dedicated find_primitive_node, instead of the old find_node.

InsertCreativityHere · 2026-08-31T19:58:45Z


        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()),


These are matching against the result of find_node, which now can never return a Primitive. So this branch is dead code.

InsertCreativityHere · 2026-08-31T19:59:35Z

-                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| {


This lookup will now fail for either: undefined types, or primitive types.
Good to have dedicated error messages for both cases.

InsertCreativityHere · 2026-08-31T20:01:31Z

 /// 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


We used to have 2 functions: find_node and find_node_with_scope.

find_node was only called to lookup primitives, so it was renamed find_primitive_node.
find_node_with_scope was renamed to find_node_by_id to be symmetric with find_symbol_by_id.

InsertCreativityHere · 2026-08-31T20:02:29Z

            Node::Primitive(OwnedPtr::new(Primitive::String)),
        ];

-        let lookup_table = HashMap::from([


We no longer seed the primitive types in the lookup table, since they are never looked up anymore.
This is the core thing that fixes the problem where module \int32 would overwrite this table's entries.

InsertCreativityHere · 2026-08-31T20:03:10Z

-                identifier: identifier.to_owned(),
-            })
+    pub fn find_primitive_node(&self, primitive: Primitive) -> &Node {
+        self.elements.get(primitive as usize).expect("Missing primitive node!")


The order of the primitives is guaranteed. We know their exact indices.

InsertCreativityHere · 2026-08-31T20:04:13Z

        self.add_element(element)
    }
+
+    fn lookup_node_by_id<'a>(&'a self, identifier: &str) -> Result<&'a Node, LookupError> {


Helper function to pull some common logic out of our find functions.

Copilot

🟡 Changes recommended

Doc-link primitive detection regresses for globally-scoped links like {@link ::bool}, producing the wrong lint message compared to previous behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR resolves ambiguity between primitive type keywords (e.g., int32) and user-defined modules/types that are keyword-escaped (e.g., module \int32) by removing primitives from the AST name lookup table and introducing dedicated primitive lookup paths.

Changes:

Removed primitive keywords from AST identifier lookup and added Ast::find_primitive_node for direct primitive access.
Updated type patching and doc-link patching to use the new scoped lookup API (find_node_by_id) and handle primitives without AST-name lookup.
Added/expanded tests covering modules/elements named after primitive keywords and ensuring primitive-node ordering assumptions remain valid.

File summaries

File	Description
slicec/tests/primitives/mod.rs	Adds a test to ensure `find_primitive_node`’s primitive-indexing remains consistent with `Primitive` ordering.
slicec/tests/identifier_tests.rs	Adds coverage for escaped primitive-keyword identifiers (modules/types) and correct type resolution.
slicec/tests/comment_tests.rs	Adds coverage ensuring doc links prefer user-defined elements over primitives when names collide.
slicec/src/patchers/type_ref_patcher.rs	Switches scoped lookups to `find_node_by_id` to align with new AST lookup behavior.
slicec/src/patchers/comment_link_patcher.rs	Updates doc-link resolution for the new lookup API and primitive handling.
slicec/src/parsers/slice/grammar.rs	Switches primitive type-ref construction to use `find_primitive_node`.
slicec/src/grammar/elements/primitive.rs	Adds `FromStr` for `Primitive` to support primitive detection in patchers.
slicec/src/ast/mod.rs	Removes primitives from the lookup table, adds `find_primitive_node`, and introduces `find_node_by_id` + internal `lookup_node_by_id`.

Review details

Files reviewed: 8/8 changed files
Comments generated: 1
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

pepone

I think this can be simplified:

Use a single lookup mechanism. Splitting find_primitive_node from find_node_by_id gives primitives special lookup semantics. Primitives should participate in the same scoped lookup as other types.
Don’t use \ to select between a primitive and a user-defined type. Treat primitives as predefined types in the global type namespace and apply normal shadowing. Under the current lexer, a declaration may still need to be written as struct \string {}, but an unqualified string reference in that scope should resolve to the struct, while ::string should resolve to the global primitive.
Return Option from the low-level find. Name lookup itself has only one failure condition: nothing was found. A typed conversion may still return Result when distinguishing a type mismatch is useful, but DoesNotExist is better represented by None.

This would also give type references and documentation links the same resolution semantics and avoid the enum-index-based primitive lookup introduced by this PR.

InsertCreativityHere added 4 commits August 31, 2026 11:46

Merged the 2 'find_node' functions as well.

eaeba15

Pull out common code to helper function.

8625b61

Added a test and other cleanups.

d93dd39

Tests and doc-comment handling.

99dc77d

InsertCreativityHere commented Aug 31, 2026

View reviewed changes

InsertCreativityHere requested a lite review from Copilot August 31, 2026 20:04

Copilot started reviewing on behalf of InsertCreativityHere August 31, 2026 20:04 View session

InsertCreativityHere added this to the 0.4.1 milestone Aug 31, 2026

Copilot AI reviewed Aug 31, 2026

View reviewed changes

Comment thread slicec/src/patchers/comment_link_patcher.rs

Review fixes.

31d6a2a

InsertCreativityHere changed the title ~~Fix Ambiguity between Primitives and Modules Named After Them~~ Fix Ambiguity in Elements using Escaped Primitive Keywords Aug 31, 2026

InsertCreativityHere requested review from externl and pepone August 31, 2026 20:30

pepone reviewed Sep 1, 2026

View reviewed changes

InsertCreativityHere mentioned this pull request Sep 1, 2026

Treat primitive type names as predefined identifiers instead of keywords #817

Open

Navigation Menu

Uh oh!

Fix Ambiguity in Elements using Escaped Primitive Keywords - #813

Fix Ambiguity in Elements using Escaped Primitive Keywords#813
InsertCreativityHere wants to merge 5 commits into
icerpc:mainfrom
InsertCreativityHere:fix-primitive-clobbering

InsertCreativityHere commented Aug 31, 2026 •

edited

Loading

Uh oh!

InsertCreativityHere Aug 31, 2026

Uh oh!

InsertCreativityHere Aug 31, 2026

Uh oh!

InsertCreativityHere Aug 31, 2026

Uh oh!

InsertCreativityHere Aug 31, 2026

Uh oh!

InsertCreativityHere Aug 31, 2026

Uh oh!

InsertCreativityHere Aug 31, 2026

Uh oh!

InsertCreativityHere Aug 31, 2026

Uh oh!

InsertCreativityHere Aug 31, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

pepone left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -415,10 +415,9 @@ fn construct_type_ref(
		}

		fn primitive_to_type_ref_definition(parser: &Parser, primitive: Primitive) -> TypeRefDefinition {

Uh oh!

Conversation

InsertCreativityHere commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's Changed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

pepone left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

InsertCreativityHere commented Aug 31, 2026 •

edited

Loading