Skip to content

Conversation

@eyalleshem
Copy link
Contributor

refactor: Make Parser methods immutable using interior mutability

Changed all parsing methods to take &self instead of &mut self.
Mutable parser state (token index and parser state) now uses Cell
for interior mutability.

This refactoring is preparation for the borrowed tokenizer work. When
holding borrowed tokens from the parser (with lifetime tied to &self),
we cannot call methods requiring &mut self due to Rust's borrowing
rules. Using interior mutability resolves this conflict by allowing
state mutations through shared references.

@eyalleshem eyalleshem force-pushed the immutable_parser branch 2 times, most recently from 31adc0e to e9fb987 Compare December 7, 2025 18:27
@eyalleshem eyalleshem changed the base branch from main to reduce-string-copying December 7, 2025 18:52
@eyalleshem eyalleshem marked this pull request as ready for review December 7, 2025 18:52
Changed all parsing methods to take '&self' instead of '\&mut self'.
Mutable parser state (token index and parser state) now uses
for interior mutability.

This refactoring is preparation for the borrowed tokenizer work. When
holding borrowed tokens from the parser (with lifetime tied to '\&self'),
we cannot call methods requiring '\&mut self' due to Rust's borrowing
rules. Using interior mutability resolves this conflict by allowing
state mutations through shared references.
@eyalleshem
Copy link
Contributor Author

@iffyio @alamb - Heads up that I've moved this PR to ready for review! Would appreciate your feedback when you get a chance.

Copy link
Contributor

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @eyalleshem!

@iffyio iffyio merged commit c6933bb into apache:reduce-string-copying Dec 18, 2025
10 checks passed
@@ -331,9 +332,9 @@ pub struct Parser<'a> {
/// The tokens
tokens: Vec<TokenWithSpan>,
/// The index of the first unprocessed token in [`Parser::tokens`].
index: usize,
index: Cell<usize>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a clever way to go about reducing the need for mutable borrows. Thank you @eyalleshem

One thing that might be worth doing (perhaps as a follow on PR) is documenting how this works (notable that Parser has interior mutability so functions like next_token() actually do change state, even though they are immutable

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.

4 participants