Skip to content

[rust] add node_ext module with full_name methods for constant nodes#3887

Open
sei40kr wants to merge 2 commits intoruby:mainfrom
sei40kr:feat/rust-node-ext
Open

[rust] add node_ext module with full_name methods for constant nodes#3887
sei40kr wants to merge 2 commits intoruby:mainfrom
sei40kr:feat/rust-node-ext

Conversation

@sei40kr
Copy link
Contributor

@sei40kr sei40kr commented Jan 29, 2026

Summary

  • Port Ruby's node_ext.rb functionality to Rust bindings
  • Add full_name and full_name_parts methods for ConstantReadNode, ConstantWriteNode, ConstantTargetNode, ConstantPathNode, and ConstantPathTargetNode
  • Add ConstantPathError enum for error handling

Test Coverage

Rust bindings include 11 unit tests compared to Ruby's 8 tests. Additional coverage includes:

  • ConstantWriteNode (not tested in Ruby)
  • ConstantTargetNode (not tested in Ruby)
  • MissingNodes error case (not tested in Ruby)

Public API Changes (cargo-public-api)

Added items to the public API
=============================
+pub enum ruby_prism::ConstantPathError
+pub ruby_prism::ConstantPathError::DynamicParts
+pub ruby_prism::ConstantPathError::MissingNodes
+impl core::clone::Clone for ruby_prism::ConstantPathError
+impl core::cmp::Eq for ruby_prism::ConstantPathError
+impl core::cmp::PartialEq for ruby_prism::ConstantPathError
+impl core::error::Error for ruby_prism::ConstantPathError
+impl core::fmt::Debug for ruby_prism::ConstantPathError
+impl core::fmt::Display for ruby_prism::ConstantPathError
+impl<'pr> ruby_prism::ConstantPathNode<'pr>
+pub fn ruby_prism::ConstantPathNode<'pr>::full_name(&self) -> Result
+pub fn ruby_prism::ConstantPathNode<'pr>::full_name_parts(&self) -> Result>, ConstantPathError>
+impl<'pr> ruby_prism::ConstantPathTargetNode<'pr>
+pub fn ruby_prism::ConstantPathTargetNode<'pr>::full_name(&self) -> Result
+pub fn ruby_prism::ConstantPathTargetNode<'pr>::full_name_parts(&self) -> Result>, ConstantPathError>
+impl<'pr> ruby_prism::ConstantReadNode<'pr>
+pub fn ruby_prism::ConstantReadNode<'pr>::full_name(&self) -> Cow<'pr, str>
+pub fn ruby_prism::ConstantReadNode<'pr>::full_name_parts(&self) -> Vec>
+impl<'pr> ruby_prism::ConstantTargetNode<'pr>
+pub fn ruby_prism::ConstantTargetNode<'pr>::full_name(&self) -> Cow<'pr, str>
+pub fn ruby_prism::ConstantTargetNode<'pr>::full_name_parts(&self) -> Vec>
+impl<'pr> ruby_prism::ConstantWriteNode<'pr>
+pub fn ruby_prism::ConstantWriteNode<'pr>::full_name(&self) -> Cow<'pr, str>
+pub fn ruby_prism::ConstantWriteNode<'pr>::full_name_parts(&self) -> Vec>

Test plan

  • bundle exec rake cargo:test passes
  • bundle exec rake cargo:lint passes
  • Doctests pass

🤖 Generated with Claude Code

@sei40kr sei40kr marked this pull request as draft January 29, 2026 04:41
@sei40kr sei40kr changed the title feat: add node_ext module with full_name methods for constant nodes [rust] add node_ext module with full_name methods for constant nodes Jan 29, 2026
Port Ruby's `node_ext.rb` functionality to Rust bindings, providing
`full_name` and `full_name_parts` methods for `ConstantReadNode`,
`ConstantWriteNode`, `ConstantTargetNode`, `ConstantPathNode`, and
`ConstantPathTargetNode`.

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@sei40kr sei40kr force-pushed the feat/rust-node-ext branch from ed95c4e to f3f9590 Compare January 29, 2026 05:33
@sei40kr sei40kr marked this pull request as ready for review January 29, 2026 05:33
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why copy on write here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel great about utf8 here, it's not guaranteed by the parser (there are ~90 encodings supported).

Address review feedback: stop assuming UTF-8 encoding since the parser
supports ~90 encodings. Return &[u8] / Vec<u8> instead of Cow<str> /
String to avoid silently corrupting non-UTF-8 data via from_utf8_lossy.

Co-Authored-By: Claude Opus 4.6 <[email protected]>
@sei40kr
Copy link
Contributor Author

sei40kr commented Feb 18, 2026

Thanks for the review!

Why copy on write here?

The Cow<'pr, str> was not an intentional API choice — it was just the return type of String::from_utf8_lossy() leaking through. When the bytes are valid UTF-8 it returns Cow::Borrowed(&str), and when they're not it returns Cow::Owned(String) with replacement characters.

I don't feel great about utf8 here, it's not guaranteed by the parser (there are ~90 encodings supported).

Agreed. I've updated the PR to return raw bytes (&'pr [u8] / Vec<u8>) instead, removing all from_utf8_lossy calls. This is consistent with the existing ConstantId::as_slice() API and avoids silently corrupting non-UTF-8 data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments