Skip to content

Commit 33b4e6f

Browse files
sei40krclaude
sei40kr
and
claude
committed
refactor: extract full_name/full_name_parts into FullName trait
Address review feedback: make full_name and full_name_parts into a FullName trait. This simplifies the ConstantPathNode implementation by replacing the iterative while-loop + reverse + is_stovetop pattern with recursive delegation via a helper function. The default full_name implementation in the trait also eliminates duplicate join logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent ec991ae commit 33b4e6f

2 files changed

Lines changed: 90 additions & 153 deletions

File tree

‎rust/ruby-prism/src/lib.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ use std::ptr::NonNull;
2222

2323
pub use self::bindings::*;
2424
pub use self::node::{ConstantId, ConstantList, ConstantListIter, Integer, NodeList, NodeListIter};
25-
pub use self::node_ext::ConstantPathError;
25+
pub use self::node_ext::{ConstantPathError, FullName};
2626
pub use self::parse_result::{Comment, CommentType, Comments, Diagnostic, Diagnostics, Location, MagicComment, MagicComments, ParseResult};
2727

2828
use ruby_prism_sys::{pm_parse, pm_parser_init, pm_parser_t};

‎rust/ruby-prism/src/node_ext.rs‎

Lines changed: 89 additions & 152 deletions
Original file line numberDiff line numberDiff line change
@@ -38,125 +38,159 @@ impl fmt::Display for ConstantPathError {
3838

3939
impl std::error::Error for ConstantPathError {}
4040

41-
impl<'pr> ConstantReadNode<'pr> {
41+
/// Trait for nodes that can compute their full constant name.
42+
///
43+
/// Implemented by constant-related nodes (`ConstantReadNode`,
44+
/// `ConstantWriteNode`, `ConstantTargetNode`, `ConstantPathNode`, and
45+
/// `ConstantPathTargetNode`).
46+
pub trait FullName<'pr> {
47+
/// Returns the list of parts for the full name of this constant.
48+
///
49+
/// # Errors
50+
///
51+
/// Returns [`ConstantPathError`] if the path contains dynamic parts or
52+
/// missing nodes.
53+
fn full_name_parts(&self) -> Result<Vec<&'pr [u8]>, ConstantPathError>;
54+
55+
/// Returns the full name of this constant.
56+
///
57+
/// # Errors
58+
///
59+
/// Returns [`ConstantPathError`] if the path contains dynamic parts or
60+
/// missing nodes.
61+
fn full_name(&self) -> Result<Vec<u8>, ConstantPathError> {
62+
let parts = self.full_name_parts()?;
63+
let mut result = Vec::new();
64+
for (i, part) in parts.iter().enumerate() {
65+
if i > 0 {
66+
result.extend_from_slice(b"::");
67+
}
68+
result.extend_from_slice(part);
69+
}
70+
Ok(result)
71+
}
72+
}
73+
74+
/// Computes `full_name_parts` for a `Node` by dispatching to the appropriate
75+
/// `FullName` implementation.
76+
fn full_name_parts_for_node<'pr>(node: &Node<'pr>) -> Result<Vec<&'pr [u8]>, ConstantPathError> {
77+
node.as_constant_path_node()
78+
.map_or_else(|| node.as_constant_read_node().map_or(Err(ConstantPathError::DynamicParts), |read_node| read_node.full_name_parts()), |path_node| path_node.full_name_parts())
79+
}
80+
81+
impl<'pr> FullName<'pr> for ConstantReadNode<'pr> {
4282
/// Returns the list of parts for the full name of this constant.
4383
///
4484
/// # Examples
4585
///
4686
/// ```
47-
/// # use ruby_prism::parse;
87+
/// # use ruby_prism::{parse, FullName};
4888
/// let result = parse(b"Foo");
4989
/// let stmt = result.node().as_program_node().unwrap()
5090
/// .statements().body().iter().next().unwrap();
5191
/// let constant = stmt.as_constant_read_node().unwrap();
52-
/// assert_eq!(constant.full_name_parts(), vec![b"Foo".as_slice()]);
92+
/// assert_eq!(constant.full_name_parts().unwrap(), vec![b"Foo".as_slice()]);
5393
/// ```
54-
#[must_use]
55-
pub fn full_name_parts(&self) -> Vec<&'pr [u8]> {
56-
vec![self.name().as_slice()]
94+
fn full_name_parts(&self) -> Result<Vec<&'pr [u8]>, ConstantPathError> {
95+
Ok(vec![self.name().as_slice()])
5796
}
5897

5998
/// Returns the full name of this constant.
6099
///
61100
/// # Examples
62101
///
63102
/// ```
64-
/// # use ruby_prism::parse;
103+
/// # use ruby_prism::{parse, FullName};
65104
/// let result = parse(b"Foo");
66105
/// let stmt = result.node().as_program_node().unwrap()
67106
/// .statements().body().iter().next().unwrap();
68107
/// let constant = stmt.as_constant_read_node().unwrap();
69-
/// assert_eq!(constant.full_name(), b"Foo");
108+
/// assert_eq!(constant.full_name().unwrap(), b"Foo");
70109
/// ```
71-
#[must_use]
72-
pub fn full_name(&self) -> &'pr [u8] {
73-
self.name().as_slice()
110+
fn full_name(&self) -> Result<Vec<u8>, ConstantPathError> {
111+
Ok(self.name().as_slice().to_vec())
74112
}
75113
}
76114

77-
impl<'pr> ConstantWriteNode<'pr> {
115+
impl<'pr> FullName<'pr> for ConstantWriteNode<'pr> {
78116
/// Returns the list of parts for the full name of this constant.
79117
///
80118
/// # Examples
81119
///
82120
/// ```
83-
/// # use ruby_prism::parse;
121+
/// # use ruby_prism::{parse, FullName};
84122
/// let result = parse(b"Foo = 1");
85123
/// let stmt = result.node().as_program_node().unwrap()
86124
/// .statements().body().iter().next().unwrap();
87125
/// let constant = stmt.as_constant_write_node().unwrap();
88-
/// assert_eq!(constant.full_name_parts(), vec![b"Foo".as_slice()]);
126+
/// assert_eq!(constant.full_name_parts().unwrap(), vec![b"Foo".as_slice()]);
89127
/// ```
90-
#[must_use]
91-
pub fn full_name_parts(&self) -> Vec<&'pr [u8]> {
92-
vec![self.name().as_slice()]
128+
fn full_name_parts(&self) -> Result<Vec<&'pr [u8]>, ConstantPathError> {
129+
Ok(vec![self.name().as_slice()])
93130
}
94131

95132
/// Returns the full name of this constant.
96133
///
97134
/// # Examples
98135
///
99136
/// ```
100-
/// # use ruby_prism::parse;
137+
/// # use ruby_prism::{parse, FullName};
101138
/// let result = parse(b"Foo = 1");
102139
/// let stmt = result.node().as_program_node().unwrap()
103140
/// .statements().body().iter().next().unwrap();
104141
/// let constant = stmt.as_constant_write_node().unwrap();
105-
/// assert_eq!(constant.full_name(), b"Foo");
142+
/// assert_eq!(constant.full_name().unwrap(), b"Foo");
106143
/// ```
107-
#[must_use]
108-
pub fn full_name(&self) -> &'pr [u8] {
109-
self.name().as_slice()
144+
fn full_name(&self) -> Result<Vec<u8>, ConstantPathError> {
145+
Ok(self.name().as_slice().to_vec())
110146
}
111147
}
112148

113-
impl<'pr> ConstantTargetNode<'pr> {
149+
impl<'pr> FullName<'pr> for ConstantTargetNode<'pr> {
114150
/// Returns the list of parts for the full name of this constant.
115151
///
116152
/// # Examples
117153
///
118154
/// ```
119-
/// # use ruby_prism::parse;
155+
/// # use ruby_prism::{parse, FullName};
120156
/// let result = parse(b"Foo, Bar = [1, 2]");
121157
/// let stmt = result.node().as_program_node().unwrap()
122158
/// .statements().body().iter().next().unwrap();
123159
/// let target = stmt.as_multi_write_node().unwrap()
124160
/// .lefts().iter().next().unwrap();
125161
/// let constant = target.as_constant_target_node().unwrap();
126-
/// assert_eq!(constant.full_name_parts(), vec![b"Foo".as_slice()]);
162+
/// assert_eq!(constant.full_name_parts().unwrap(), vec![b"Foo".as_slice()]);
127163
/// ```
128-
#[must_use]
129-
pub fn full_name_parts(&self) -> Vec<&'pr [u8]> {
130-
vec![self.name().as_slice()]
164+
fn full_name_parts(&self) -> Result<Vec<&'pr [u8]>, ConstantPathError> {
165+
Ok(vec![self.name().as_slice()])
131166
}
132167

133168
/// Returns the full name of this constant.
134169
///
135170
/// # Examples
136171
///
137172
/// ```
138-
/// # use ruby_prism::parse;
173+
/// # use ruby_prism::{parse, FullName};
139174
/// let result = parse(b"Foo, Bar = [1, 2]");
140175
/// let stmt = result.node().as_program_node().unwrap()
141176
/// .statements().body().iter().next().unwrap();
142177
/// let target = stmt.as_multi_write_node().unwrap()
143178
/// .lefts().iter().next().unwrap();
144179
/// let constant = target.as_constant_target_node().unwrap();
145-
/// assert_eq!(constant.full_name(), b"Foo");
180+
/// assert_eq!(constant.full_name().unwrap(), b"Foo");
146181
/// ```
147-
#[must_use]
148-
pub fn full_name(&self) -> &'pr [u8] {
149-
self.name().as_slice()
182+
fn full_name(&self) -> Result<Vec<u8>, ConstantPathError> {
183+
Ok(self.name().as_slice().to_vec())
150184
}
151185
}
152186

153-
impl<'pr> ConstantPathNode<'pr> {
187+
impl<'pr> FullName<'pr> for ConstantPathNode<'pr> {
154188
/// Returns the list of parts for the full name of this constant path.
155189
///
156190
/// # Examples
157191
///
158192
/// ```
159-
/// # use ruby_prism::parse;
193+
/// # use ruby_prism::{parse, FullName};
160194
/// let result = parse(b"Foo::Bar");
161195
/// let stmt = result.node().as_program_node().unwrap()
162196
/// .statements().body().iter().next().unwrap();
@@ -169,84 +203,26 @@ impl<'pr> ConstantPathNode<'pr> {
169203
/// Returns [`ConstantPathError::DynamicParts`] if the path contains
170204
/// dynamic parts, or [`ConstantPathError::MissingNodes`] if the path
171205
/// contains missing nodes.
172-
pub fn full_name_parts(&self) -> Result<Vec<&'pr [u8]>, ConstantPathError> {
173-
let mut parts = Vec::new();
174-
let mut current: Option<Node<'pr>> = Some(self.as_node());
175-
176-
while let Some(ref node) = current {
177-
if let Some(path_node) = node.as_constant_path_node() {
178-
let name = path_node.name().ok_or(ConstantPathError::MissingNodes)?;
179-
parts.push(name.as_slice());
180-
current = path_node.parent();
181-
} else if let Some(read_node) = node.as_constant_read_node() {
182-
parts.push(read_node.name().as_slice());
183-
current = None;
184-
} else {
185-
return Err(ConstantPathError::DynamicParts);
186-
}
187-
}
188-
189-
parts.reverse();
206+
fn full_name_parts(&self) -> Result<Vec<&'pr [u8]>, ConstantPathError> {
207+
let name = self.name().ok_or(ConstantPathError::MissingNodes)?;
190208

191-
if self.is_stovetop() {
192-
parts.insert(0, b"".as_slice());
193-
}
209+
let mut parts = match self.parent() {
210+
Some(ref parent) => full_name_parts_for_node(parent)?,
211+
None => vec![b"".as_slice()],
212+
};
194213

214+
parts.push(name.as_slice());
195215
Ok(parts)
196216
}
197-
198-
/// Returns the full name of this constant path.
199-
///
200-
/// # Examples
201-
///
202-
/// ```
203-
/// # use ruby_prism::parse;
204-
/// let result = parse(b"Foo::Bar");
205-
/// let stmt = result.node().as_program_node().unwrap()
206-
/// .statements().body().iter().next().unwrap();
207-
/// let constant_path = stmt.as_constant_path_node().unwrap();
208-
/// assert_eq!(constant_path.full_name().unwrap(), b"Foo::Bar");
209-
/// ```
210-
///
211-
/// # Errors
212-
///
213-
/// Returns [`ConstantPathError::DynamicParts`] if the path contains
214-
/// dynamic parts, or [`ConstantPathError::MissingNodes`] if the path
215-
/// contains missing nodes.
216-
pub fn full_name(&self) -> Result<Vec<u8>, ConstantPathError> {
217-
let parts = self.full_name_parts()?;
218-
let mut result = Vec::new();
219-
for (i, part) in parts.iter().enumerate() {
220-
if i > 0 {
221-
result.extend_from_slice(b"::");
222-
}
223-
result.extend_from_slice(part);
224-
}
225-
Ok(result)
226-
}
227-
228-
fn is_stovetop(&self) -> bool {
229-
let mut current: Option<Node<'pr>> = Some(self.as_node());
230-
231-
while let Some(ref node) = current {
232-
if let Some(path_node) = node.as_constant_path_node() {
233-
current = path_node.parent();
234-
} else {
235-
return false;
236-
}
237-
}
238-
239-
true
240-
}
241217
}
242218

243-
impl<'pr> ConstantPathTargetNode<'pr> {
219+
impl<'pr> FullName<'pr> for ConstantPathTargetNode<'pr> {
244220
/// Returns the list of parts for the full name of this constant path.
245221
///
246222
/// # Examples
247223
///
248224
/// ```
249-
/// # use ruby_prism::parse;
225+
/// # use ruby_prism::{parse, FullName};
250226
/// let result = parse(b"Foo::Bar, Baz = [1, 2]");
251227
/// let stmt = result.node().as_program_node().unwrap()
252228
/// .statements().body().iter().next().unwrap();
@@ -261,61 +237,22 @@ impl<'pr> ConstantPathTargetNode<'pr> {
261237
/// Returns [`ConstantPathError::DynamicParts`] if the path contains
262238
/// dynamic parts, or [`ConstantPathError::MissingNodes`] if the path
263239
/// contains missing nodes.
264-
pub fn full_name_parts(&self) -> Result<Vec<&'pr [u8]>, ConstantPathError> {
240+
fn full_name_parts(&self) -> Result<Vec<&'pr [u8]>, ConstantPathError> {
265241
let name = self.name().ok_or(ConstantPathError::MissingNodes)?;
266242

267-
let mut parts = if let Some(parent) = self.parent() {
268-
if let Some(path_node) = parent.as_constant_path_node() {
269-
path_node.full_name_parts()?
270-
} else if let Some(read_node) = parent.as_constant_read_node() {
271-
read_node.full_name_parts()
272-
} else {
273-
return Err(ConstantPathError::DynamicParts);
274-
}
275-
} else {
276-
vec![b"".as_slice()]
243+
let mut parts = match self.parent() {
244+
Some(ref parent) => full_name_parts_for_node(parent)?,
245+
None => vec![b"".as_slice()],
277246
};
278247

279248
parts.push(name.as_slice());
280249
Ok(parts)
281250
}
282-
283-
/// Returns the full name of this constant path.
284-
///
285-
/// # Examples
286-
///
287-
/// ```
288-
/// # use ruby_prism::parse;
289-
/// let result = parse(b"Foo::Bar, Baz = [1, 2]");
290-
/// let stmt = result.node().as_program_node().unwrap()
291-
/// .statements().body().iter().next().unwrap();
292-
/// let target = stmt.as_multi_write_node().unwrap()
293-
/// .lefts().iter().next().unwrap();
294-
/// let constant_path = target.as_constant_path_target_node().unwrap();
295-
/// assert_eq!(constant_path.full_name().unwrap(), b"Foo::Bar");
296-
/// ```
297-
///
298-
/// # Errors
299-
///
300-
/// Returns [`ConstantPathError::DynamicParts`] if the path contains
301-
/// dynamic parts, or [`ConstantPathError::MissingNodes`] if the path
302-
/// contains missing nodes.
303-
pub fn full_name(&self) -> Result<Vec<u8>, ConstantPathError> {
304-
let parts = self.full_name_parts()?;
305-
let mut result = Vec::new();
306-
for (i, part) in parts.iter().enumerate() {
307-
if i > 0 {
308-
result.extend_from_slice(b"::");
309-
}
310-
result.extend_from_slice(part);
311-
}
312-
Ok(result)
313-
}
314251
}
315252

316253
#[cfg(test)]
317254
mod tests {
318-
use super::ConstantPathError;
255+
use super::{ConstantPathError, FullName};
319256
use crate::parse;
320257

321258
#[test]
@@ -324,8 +261,8 @@ mod tests {
324261
let node = result.node().as_program_node().unwrap().statements().body().iter().next().unwrap();
325262
let constant = node.as_constant_read_node().unwrap();
326263

327-
assert_eq!(constant.full_name_parts(), vec![b"Foo".as_slice()]);
328-
assert_eq!(constant.full_name(), b"Foo");
264+
assert_eq!(constant.full_name_parts().unwrap(), vec![b"Foo".as_slice()]);
265+
assert_eq!(constant.full_name().unwrap(), b"Foo");
329266
}
330267

331268
#[test]
@@ -334,8 +271,8 @@ mod tests {
334271
let node = result.node().as_program_node().unwrap().statements().body().iter().next().unwrap();
335272
let constant = node.as_constant_write_node().unwrap();
336273

337-
assert_eq!(constant.full_name_parts(), vec![b"Foo".as_slice()]);
338-
assert_eq!(constant.full_name(), b"Foo");
274+
assert_eq!(constant.full_name_parts().unwrap(), vec![b"Foo".as_slice()]);
275+
assert_eq!(constant.full_name().unwrap(), b"Foo");
339276
}
340277

341278
#[test]
@@ -346,8 +283,8 @@ mod tests {
346283
let target = multi_write.lefts().iter().next().unwrap();
347284
let constant = target.as_constant_target_node().unwrap();
348285

349-
assert_eq!(constant.full_name_parts(), vec![b"Foo".as_slice()]);
350-
assert_eq!(constant.full_name(), b"Foo");
286+
assert_eq!(constant.full_name_parts().unwrap(), vec![b"Foo".as_slice()]);
287+
assert_eq!(constant.full_name().unwrap(), b"Foo");
351288
}
352289

353290
#[test]

0 commit comments

Comments
 (0)