Skip to content

Add data_push parameter to DataStack::reduce_action #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ehwan merged 2 commits into from
Oct 3, 2025
Merged

Add data_push parameter to DataStack::reduce_action #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ba3044
add push_data parameter to reduce_action
ehwan Oct 3, 2025
c5d2a7e
do not return bool from reduce_action
ehwan Oct 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion rusty_lr_core/src/parser/data_stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ pub trait DataStack: Sized + Default {
// the caller (usually from generated code) must pops all of the tokens used for this reduce_action
data_stack: &mut Self,
location_stack: &mut Vec<Self::Location>,
push_data: bool,

// the index of the production rule to reduce
rule_index: usize,
Expand All @@ -55,5 +56,5 @@ pub trait DataStack: Sized + Default {
userdata: &mut Self::UserData,
// location of this non-terminal, e.g. `@$`
location0: &mut Self::Location,
) -> Result<bool, Self::ReduceActionError>;
) -> Result<(), Self::ReduceActionError>;
}
35 changes: 19 additions & 16 deletions rusty_lr_core/src/parser/deterministic/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ impl<Data: DataStack, StateIndex: Index + Copy> Context<Data, StateIndex> {
) -> Result<Data::StartType, ParseError<Data::Term, Data::Location, Data::ReduceActionError>>
where
Data::Term: Clone,
Data::NonTerm: Hash + Eq + Copy + NonTerminal,
Data::NonTerm: std::fmt::Debug,
P::State: State<StateIndex = StateIndex>,
{
self.feed_eof(parser, userdata)?;
Expand All @@ -80,7 +80,6 @@ impl<Data: DataStack, StateIndex: Index + Copy> Context<Data, StateIndex> {
parser: &P,
) -> bool
where
Data::NonTerm: Hash + Eq + NonTerminal,
P::State: State<StateIndex = StateIndex>,
{
let mut extra_state_stack = Vec::new();
Expand Down Expand Up @@ -230,6 +229,7 @@ impl<Data: DataStack, StateIndex: Index + Copy> Context<Data, StateIndex> {
where
Data::Location: Default,
P::Term: Clone,
P::NonTerm: std::fmt::Debug,
P::State: State<StateIndex = StateIndex>,
{
self.feed_location(parser, term, userdata, Default::default())
Expand All @@ -245,6 +245,7 @@ impl<Data: DataStack, StateIndex: Index + Copy> Context<Data, StateIndex> {
) -> Result<(), ParseError<Data::Term, Data::Location, Data::ReduceActionError>>
where
P::Term: Clone,
P::NonTerm: std::fmt::Debug,
P::State: State<StateIndex = StateIndex>,
{
use crate::Location;
Expand Down Expand Up @@ -362,6 +363,7 @@ impl<Data: DataStack, StateIndex: Index + Copy> Context<Data, StateIndex> {
) -> Result<(), ParseError<Data::Term, Data::Location, Data::ReduceActionError>>
where
P::Term: Clone,
P::NonTerm: std::fmt::Debug,
P::State: State<StateIndex = StateIndex>,
{
debug_assert!(
Expand Down Expand Up @@ -446,17 +448,28 @@ impl<Data: DataStack, StateIndex: Index + Copy> Context<Data, StateIndex> {
let mut new_location =
Data::Location::new(self.location_stack.iter().rev(), tokens_len);

let Some(next_nonterm_shift) = parser.get_states()
[self.state_stack.last().unwrap().into_usize()]
.shift_goto_nonterm(rule.name) else {
unreachable!(
"Failed to shift nonterminal: {:?} in state {}",
rule.name,
self.state_stack.last().unwrap().into_usize()
);
};

// call reduce action
let non_empty_pushed = match Data::reduce_action(
match Data::reduce_action(
&mut self.data_stack,
&mut self.location_stack,
next_nonterm_shift.push,
reduce_rule.into_usize(),
&mut shift,
&term,
userdata,
&mut new_location,
) {
Ok(ret) => ret,
Ok(_) => {}
Err(err) => {
return Err(ParseError::ReduceAction(super::error::ReduceActionError {
term,
Expand Down Expand Up @@ -485,18 +498,7 @@ impl<Data: DataStack, StateIndex: Index + Copy> Context<Data, StateIndex> {
}

// shift with reduced nonterminal
if let Some(next_state_id) = parser.get_states()
[self.state_stack.last().unwrap().into_usize()]
.shift_goto_nonterm(rule.name)
{
self.state_stack.push(next_state_id.state);
if !next_state_id.push && non_empty_pushed {
self.data_stack.pop();
self.data_stack.push_empty();
}
} else {
unreachable!("shift nonterm failed");
}
self.state_stack.push(next_nonterm_shift.state);
} else {
break shift;
}
Expand Down Expand Up @@ -735,6 +737,7 @@ impl<Data: DataStack, StateIndex: Index + Copy> Context<Data, StateIndex> {
) -> Result<(), ParseError<Data::Term, Data::Location, Data::ReduceActionError>>
where
P::Term: Clone,
P::NonTerm: std::fmt::Debug,
P::State: State<StateIndex = StateIndex>,
{
self.feed_location_impl(
Expand Down
38 changes: 17 additions & 21 deletions rusty_lr_core/src/parser/nondeterministic/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,38 +459,34 @@ impl<Data: DataStack, StateIndex: Index, const MAX_REDUCE_RULES: usize>
#[cfg(feature = "tree")]
let trees = node.tree_stack.split_off(node.tree_stack.len() - count);

let Some(next_nonterm_shift) = parser.get_states()[state].shift_goto_nonterm(rule.name)
else {
unreachable!(
"Failed to shift non-terminal: {:?} in state {}",
rule.name, state
);
};

match Data::reduce_action(
&mut node.data_stack,
&mut node.location_stack,
next_nonterm_shift.push,
reduce_rule,
shift,
term,
userdata,
&mut new_location,
) {
Ok(non_empty_pushed) => {
if let Some(nonterm_shift_state) =
parser.get_states()[state].shift_goto_nonterm(rule.name)
Ok(_) => {
node.state_stack.push(next_nonterm_shift.state);
node.location_stack.push(new_location);
node.precedence_stack.push(precedence);
#[cfg(feature = "tree")]
{
node.state_stack.push(nonterm_shift_state.state);
if !nonterm_shift_state.push && non_empty_pushed {
node.data_stack.pop();
node.data_stack.push_empty();
}
node.location_stack.push(new_location);
node.precedence_stack.push(precedence);
#[cfg(feature = "tree")]
{
node.tree_stack
.push(crate::tree::Tree::new_nonterminal(rule.name.clone(), trees));
}
Ok(node_to_shift)
} else {
unreachable!(
"no shift state for non-terminal: {:?}",
parser.get_rules()[reduce_rule].name
);
node.tree_stack
.push(crate::tree::Tree::new_nonterminal(rule.name.clone(), trees));
}
Ok(node_to_shift)
}
Err(err) => {
self.try_remove_node_recursive(node_to_shift);
Expand Down
127 changes: 98 additions & 29 deletions rusty_lr_parser/src/emit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1151,31 +1151,97 @@ impl Grammar {
TokenStream::new()
} else {
if rule.tokens.len() > 0 {
// if first token's tag is equal to new_tag, no need to (pop n tokens -> push new token).
// just pop n-1 tokens
let first_tag_name = token_to_stack_name(rule.tokens[0].token)
.unwrap_or(&empty_tag_name);

if first_tag_name == new_tag_name {
// pop n-1 tokens, no new insertion
let len = rule.tokens.len() - 1;
let truncate_stream = if len > 0 {
quote! {__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #len);}
if new_tag_name == &empty_tag_name {
// if first token's tag is equal to new_tag, no need to (pop n tokens -> push new token).
// just pop n-1 tokens
let first_tag_name = token_to_stack_name(rule.tokens[0].token)
.unwrap_or(&empty_tag_name);

if first_tag_name == new_tag_name {
// pop n-1 tokens, no new insertion
let len = rule.tokens.len() - 1;
let truncate_stream = if len > 0 {
quote! {__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #len);}
} else {
TokenStream::new()
};
truncate_stream
} else {
TokenStream::new()
};
truncate_stream
let len = rule.tokens.len();
// len > 0 here
quote! {
__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #len);
__data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
}
}
} else {
let len = rule.tokens.len();
// len > 0 here
quote! {
__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #len);
__data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
// if first token's tag is equal to new_tag, no need to (pop n tokens -> push new token).
// just pop n-1 tokens
let first_tag_name = token_to_stack_name(rule.tokens[0].token)
.unwrap_or(&empty_tag_name);

if first_tag_name == new_tag_name {
// pop n-1 tokens, no new insertion
let lenm1 = rule.tokens.len() - 1;
let truncate_stream = if lenm1 > 0 {
quote! {__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #lenm1);}
} else {
TokenStream::new()
};

let len = rule.tokens.len();
quote! {
if __push_data {
#truncate_stream
} else {
__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #len);
__data_stack.#tag_stack_name.push(#tag_enum_name::#empty_tag_name);
}
}
} else if first_tag_name == &empty_tag_name {
// pop n-1 tokens, no new insertion
let lenm1 = rule.tokens.len() - 1;
let truncate_stream = if lenm1 > 0 {
quote! {__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #lenm1);}
} else {
TokenStream::new()
};

let len = rule.tokens.len();
quote! {
if __push_data {
__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #len);
__data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
} else {
#truncate_stream
}
}
} else {
let len = rule.tokens.len();
// len > 0 here
quote! {
__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #len);
if __push_data {
__data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
} else {
__data_stack.#tag_stack_name.push(#tag_enum_name::#empty_tag_name);
}
}
}
}
} else {
quote! {
__data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
if new_tag_name == &empty_tag_name {
quote! {
__data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
}
} else {
quote! {
if __push_data {
__data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
} else {
__data_stack.#tag_stack_name.push(#tag_enum_name::#empty_tag_name);
}
}
}
}
Comment on lines 1153 to 1246
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Oct 3, 2025

Choose a reason for hiding this comment

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

medium

The logic for generating tag stack modifications is quite complex and contains a lot of duplicated code. This makes it hard to read and maintain. For example, let first_tag_name = ... and the logic to create truncate_stream are repeated.

Consider refactoring this block to reduce duplication. You could extract common variables and structures to the top of the if rule.tokens.len() > 0 block. This would make the logic for each case clearer and the whole block more maintainable.

This refactoring would also help avoid generating code like if __push_data {} else { ... } which is functionally correct but stylistically awkward.

                        if rule.tokens.len() > 0 {
                            let first_tag_name = token_to_stack_name(rule.tokens[0].token)
                                .unwrap_or(&empty_tag_name);
                            let len = rule.tokens.len();
                            let lenm1 = len - 1;

                            let truncate_lenm1 = if lenm1 > 0 {
                                quote! {__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #lenm1);}
                            } else {
                                TokenStream::new()
                            };
                            let truncate_len = quote! {__data_stack.#tag_stack_name.truncate(__data_stack.#tag_stack_name.len() - #len);};

                            if new_tag_name == &empty_tag_name {
                                if first_tag_name == new_tag_name {
                                    // pop n-1 tokens, no new insertion
                                    truncate_lenm1
                                } else {
                                    // pop n tokens, push empty
                                    quote! {
                                        #truncate_len
                                        __data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
                                    }
                                }
                            } else { // new_tag_name is not empty
                                if first_tag_name == new_tag_name {
                                    // pop n-1 tokens if __push_data, else pop n and push empty
                                    quote! {
                                        if __push_data {
                                            #truncate_lenm1
                                        } else {
                                            #truncate_len
                                            __data_stack.#tag_stack_name.push(#tag_enum_name::#empty_tag_name);
                                        }
                                    }
                                } else if first_tag_name == &empty_tag_name {
                                    // pop n and push new if __push_data, else pop n-1
                                    quote! {
                                        if __push_data {
                                            #truncate_len
                                            __data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
                                        } else {
                                            #truncate_lenm1
                                        }
                                    }
                                } else {
                                    // pop n, and push new or empty based on __push_data
                                    quote! {
                                        #truncate_len
                                        if __push_data {
                                            __data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
                                        } else {
                                            __data_stack.#tag_stack_name.push(#tag_enum_name::#empty_tag_name);
                                        }
                                    }
                                }
                            }
                        } else {
                            if new_tag_name == &empty_tag_name {
                                quote! {
                                    __data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
                                }
                            } else {
                                quote! {
                                    if __push_data {
                                        __data_stack.#tag_stack_name.push(#tag_enum_name::#new_tag_name);
                                    } else {
                                        __data_stack.#tag_stack_name.push(#tag_enum_name::#empty_tag_name);
                                    }
                                }
                            }
                        }

};
Expand Down Expand Up @@ -1286,11 +1352,9 @@ impl Grammar {
}

reduce_action_case_streams.extend(quote! {
#rule_index => Self::#reduce_fn_ident( data_stack, location_stack, shift, lookahead, user_data, location0 ),
#rule_index => Self::#reduce_fn_ident( data_stack, location_stack, push_data, shift, lookahead, user_data, location0 ),
});

let returns_non_empty = stack_names_for_nonterm[nonterm_idx].is_some();

let reduce_action_body = match &rule.reduce_action {
Some(ReduceAction::Custom(custom)) => {
let body = &custom.body;
Expand All @@ -1309,11 +1373,12 @@ impl Grammar {
fn #reduce_fn_ident(
__data_stack: &mut Self,
__location_stack: &mut Vec<#location_typename>,
__push_data: bool,
shift: &mut bool,
lookahead: &#module_prefix::TerminalSymbol<#token_typename>,
#user_data_parameter_name: &mut #user_data_typename,
__rustylr_location0: &mut #location_typename,
) -> Result<bool, #reduce_error_typename> {
) -> Result<(), #reduce_error_typename> {
#[cfg(debug_assertions)]
{
#debug_tag_check_stream
Expand All @@ -1324,9 +1389,11 @@ impl Grammar {
#custom_reduce_action_stream

let __res = #reduce_action_body
__data_stack.#stack_name.push(__res);
if __push_data {
__data_stack.#stack_name.push(__res);
}

Ok(#returns_non_empty)
Ok(())
}
});
} else {
Expand All @@ -1336,11 +1403,12 @@ impl Grammar {
fn #reduce_fn_ident(
__data_stack: &mut Self,
__location_stack: &mut Vec<#location_typename>,
__push_data: bool,
shift: &mut bool,
lookahead: &#module_prefix::TerminalSymbol<#token_typename>,
#user_data_parameter_name: &mut #user_data_typename,
__rustylr_location0: &mut #location_typename,
) -> Result<bool, #reduce_error_typename> {
) -> Result<(), #reduce_error_typename> {
#[cfg(debug_assertions)]
{
#debug_tag_check_stream
Expand All @@ -1352,7 +1420,7 @@ impl Grammar {

#reduce_action_body

Ok(#returns_non_empty)
Ok(())
}
});
}
Expand Down Expand Up @@ -1662,12 +1730,13 @@ impl Grammar {
fn reduce_action(
data_stack: &mut Self,
location_stack: &mut Vec<#location_typename>,
push_data: bool,
rule_index: usize,
shift: &mut bool,
lookahead: &#module_prefix::TerminalSymbol<Self::Term>,
user_data: &mut Self::UserData,
location0: &mut Self::Location,
) -> Result<bool, Self::ReduceActionError> {
) -> Result<(), Self::ReduceActionError> {
match rule_index {
#reduce_action_case_streams
_ => {
Expand Down
Loading