From af398c28e23f2e2b8e099951eb7ec7325037cd5c Mon Sep 17 00:00:00 2001 From: Elijah Hartvigsen Date: Fri, 28 Jul 2023 18:12:40 +0200 Subject: [PATCH 1/4] recursive-leak: fix leak by using middle-man --- src/lib.rs | 23 ------------- src/recursive.rs | 23 +++++++++---- src/recursive/decl.rs | 79 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 29 deletions(-) create mode 100644 src/recursive/decl.rs diff --git a/src/lib.rs b/src/lib.rs index 2b1ad144..ce955b19 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -3130,29 +3130,6 @@ mod tests { // TODO what about IterConfigure and TryIterConfigure? } - #[test] - #[should_panic] - fn recursive_define_twice() { - let mut expr = Recursive::declare(); - expr.define({ - let atom = any::<&str, extra::Default>() - .filter(|c: &char| c.is_alphabetic()) - .repeated() - .at_least(1) - .collect(); - let sum = expr - .clone() - .then_ignore(just('+')) - .then(expr.clone()) - .map(|(a, b)| format!("{}{}", a, b)); - - sum.or(atom) - }); - expr.define(expr.clone()); - - expr.then_ignore(end()).parse("a+b+c"); - } - #[test] #[should_panic] fn todo_err() { diff --git a/src/recursive.rs b/src/recursive.rs index e0c0e7b5..f78515d7 100644 --- a/src/recursive.rs +++ b/src/recursive.rs @@ -8,6 +8,10 @@ //! definition of parsers more corefully, particularly for mutually-recursive parsers. In such cases, the functions on //! [`Recursive`] allow for this. +mod decl; + +use decl::RecursiveDecl; + use super::*; #[cfg(not(feature = "sync"))] @@ -111,11 +115,11 @@ impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> Recursive>>('+') + /// let chain = Recursive::define(just::<_, _, extra::Err>>('+') /// .then(chain.clone()) /// .map(|(c, chain)| Chain::Link(c, Box::new(chain))) /// .or_not() - /// .map(|chain| chain.unwrap_or(Chain::End))); + /// .map(|chain| chain.unwrap_or(Chain::End)), chain); /// /// assert_eq!(chain.parse("").into_result(), Ok(Chain::End)); /// assert_eq!( @@ -123,8 +127,8 @@ impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> Recursive Self { - Recursive { + pub fn declare() -> RecursiveDecl> { + RecursiveDecl { inner: RecursiveInner::Owned(RefC::new(Indirect { inner: OnceCell::new(), })), @@ -134,14 +138,21 @@ impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> Recursive + Clone + MaybeSync + 'a + 'b>(&mut self, parser: P) { + pub fn define + Clone + MaybeSync + 'a + 'b>( + parser: P, + declaration: RecursiveDecl>, + ) -> super::Recursive> { let location = *Location::caller(); - self.parser() + declaration.parser() .inner .set(Box::new(parser)) .unwrap_or_else(|_| { panic!("recursive parsers can only be defined once, trying to redefine it at {location}") }); + + Recursive { + inner: declaration.inner, + } } } diff --git a/src/recursive/decl.rs b/src/recursive/decl.rs new file mode 100644 index 00000000..7bf9092f --- /dev/null +++ b/src/recursive/decl.rs @@ -0,0 +1,79 @@ +//! Recursive parsers (parser that include themselves within their patterns). +//! +//! *“It's unpleasantly like being drunk." +//! "What's so unpleasant about being drunk?" +//! "You ask a glass of water.”* +//! +//! The [`recursive()`] function covers most cases, but sometimes it's necessary to manually control the declaration and +//! definition of parsers more corefully, particularly for mutually-recursive parsers. In such cases, the functions on +//! [`Recursive`] allow for this. + +use super::*; + +/// A parser that can be defined in terms of itself by separating its [declaration](Recursive::declare) from its +/// [definition](Recursive::define). +/// +/// Prefer to use [`recursive()`], which exists as a convenient wrapper around both operations, if possible. +pub struct RecursiveDecl { + pub(super) inner: RecursiveInner

, +} + +impl RecursiveDecl

{ + #[inline] + pub(super) fn parser(&self) -> RefC

{ + match &self.inner { + RecursiveInner::Owned(x) => x.clone(), + RecursiveInner::Unowned(x) => x + .upgrade() + .expect("Recursive parser used before being defined"), + } + } +} + +// The decl should only pass weak references so that it can be used in itself +// without creating a memory leak +impl Clone for RecursiveDecl

{ + fn clone(&self) -> Self { + Self { + inner: match &self.inner { + RecursiveInner::Owned(x) => RecursiveInner::Unowned(RefC::

::downgrade(&x)), + RecursiveInner::Unowned(x) => RecursiveInner::Unowned(x.clone()), + }, + } + } +} + +impl<'a, 'b, I, O, E> ParserSealed<'a, I, O, E> for RecursiveDecl> +where + I: Input<'a>, + E: ParserExtra<'a, I>, +{ + #[inline] + fn go(&self, inp: &mut InputRef<'a, '_, I, E>) -> PResult { + recurse(move || { + M::invoke( + self.parser() + .inner + .get() + .expect("Recursive parser used before being defined") + .as_ref(), + inp, + ) + }) + } + + go_extra!(O); +} + +impl<'a, 'b, I, O, E> ParserSealed<'a, I, O, E> for RecursiveDecl> +where + I: Input<'a>, + E: ParserExtra<'a, I>, +{ + #[inline] + fn go(&self, inp: &mut InputRef<'a, '_, I, E>) -> PResult { + recurse(move || M::invoke(&*self.parser(), inp)) + } + + go_extra!(O); +} From 440dbc3f4f9f9c5e395a3f6360c5ed860bb01453 Mon Sep 17 00:00:00 2001 From: Elijah Hartvigsen Date: Sat, 29 Jul 2023 11:46:25 +0200 Subject: [PATCH 2/4] recursive-leak: clean-up code and use type-state --- src/recursive.rs | 66 ++++++++++++++++++++++++++++-------- src/recursive/decl.rs | 79 ------------------------------------------- 2 files changed, 51 insertions(+), 94 deletions(-) delete mode 100644 src/recursive/decl.rs diff --git a/src/recursive.rs b/src/recursive.rs index f78515d7..290cb035 100644 --- a/src/recursive.rs +++ b/src/recursive.rs @@ -8,11 +8,22 @@ //! definition of parsers more corefully, particularly for mutually-recursive parsers. In such cases, the functions on //! [`Recursive`] allow for this. -mod decl; +use super::*; -use decl::RecursiveDecl; +mod private { + pub trait RecursiveState {} +} -use super::*; +use private::RecursiveState; + +/// A State which represents when a [`Recursive`](Recursive) has been declared, but not defined +pub struct Declared; + +/// A State which represents when a [`Recursive`](Recursive) has been both defined and declared +pub struct Defined; + +impl RecursiveState for Declared {} +impl RecursiveState for Defined {} #[cfg(not(feature = "sync"))] struct OnceCell(core::cell::Cell>); @@ -84,11 +95,15 @@ pub struct Indirect<'a, 'b, I: Input<'a>, O, Extra: ParserExtra<'a, I>> { /// [definition](Recursive::define). /// /// Prefer to use [`recursive()`], which exists as a convenient wrapper around both operations, if possible. -pub struct Recursive { +pub struct Recursive { inner: RecursiveInner

, + #[allow(dead_code)] + state: EmptyPhantom, } -impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> Recursive> { +impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> + Recursive, Declared> +{ /// Declare the existence of a recursive parser, allowing it to be used to construct parser combinators before /// being fulled defined. /// @@ -127,21 +142,24 @@ impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> Recursive RecursiveDecl> { - RecursiveDecl { + pub fn declare() -> Self { + Recursive { inner: RecursiveInner::Owned(RefC::new(Indirect { inner: OnceCell::new(), })), + state: EmptyPhantom::new(), } } +} +impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> Recursive, Defined> { /// Defines the parser after declaring it, allowing it to be used for parsing. // INFO: Clone bound not actually needed, but good to be safe for future compat #[track_caller] pub fn define + Clone + MaybeSync + 'a + 'b>( parser: P, - declaration: RecursiveDecl>, - ) -> super::Recursive> { + declaration: Recursive, Declared>, + ) -> Self { let location = *Location::caller(); declaration.parser() .inner @@ -151,12 +169,13 @@ impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> Recursive Recursive

{ +impl Recursive { #[inline] fn parser(&self) -> RefC

{ match &self.inner { @@ -168,9 +187,22 @@ impl Recursive

{ } } -impl Clone for Recursive

{ +impl Clone for Recursive { + fn clone(&self) -> Self { + Self { + state: EmptyPhantom::new(), + inner: match &self.inner { + RecursiveInner::Owned(x) => RecursiveInner::Unowned(RefC::downgrade(x)), + RecursiveInner::Unowned(x) => RecursiveInner::Unowned(x.clone()), + }, + } + } +} + +impl Clone for Recursive { fn clone(&self) -> Self { Self { + state: EmptyPhantom::new(), inner: match &self.inner { RecursiveInner::Owned(x) => RecursiveInner::Owned(x.clone()), RecursiveInner::Unowned(x) => RecursiveInner::Unowned(x.clone()), @@ -190,10 +222,11 @@ fn recurse R>(f: F) -> R { f() } -impl<'a, 'b, I, O, E> ParserSealed<'a, I, O, E> for Recursive> +impl<'a, 'b, I, O, E, S> ParserSealed<'a, I, O, E> for Recursive, S> where I: Input<'a>, E: ParserExtra<'a, I>, + S: RecursiveState, { #[inline] fn go(&self, inp: &mut InputRef<'a, '_, I, E>) -> PResult { @@ -212,10 +245,11 @@ where go_extra!(O); } -impl<'a, 'b, I, O, E> ParserSealed<'a, I, O, E> for Recursive> +impl<'a, 'b, I, O, E, S> ParserSealed<'a, I, O, E> for Recursive, S> where I: Input<'a>, E: ParserExtra<'a, I>, + S: RecursiveState, { #[inline] fn go(&self, inp: &mut InputRef<'a, '_, I, E>) -> PResult { @@ -275,17 +309,18 @@ where /// ]))); /// ``` // INFO: Clone bound not actually needed, but good to be safe for future compat -pub fn recursive<'a, 'b, I, O, E, A, F>(f: F) -> Recursive> +pub fn recursive<'a, 'b, I, O, E, A, F>(f: F) -> Recursive, Defined> where I: Input<'a>, E: ParserExtra<'a, I>, A: Parser<'a, I, O, E> + Clone + MaybeSync + 'b, - F: FnOnce(Recursive>) -> A, + F: FnOnce(Recursive, Declared>) -> A, { let rc = RefC::new_cyclic(|rc| { let rc: RefW> = rc.clone() as _; let parser = Recursive { inner: RecursiveInner::Unowned(rc.clone()), + state: EmptyPhantom::::new(), }; f(parser) @@ -293,5 +328,6 @@ where Recursive { inner: RecursiveInner::Owned(rc), + state: EmptyPhantom::new(), } } diff --git a/src/recursive/decl.rs b/src/recursive/decl.rs deleted file mode 100644 index 7bf9092f..00000000 --- a/src/recursive/decl.rs +++ /dev/null @@ -1,79 +0,0 @@ -//! Recursive parsers (parser that include themselves within their patterns). -//! -//! *“It's unpleasantly like being drunk." -//! "What's so unpleasant about being drunk?" -//! "You ask a glass of water.”* -//! -//! The [`recursive()`] function covers most cases, but sometimes it's necessary to manually control the declaration and -//! definition of parsers more corefully, particularly for mutually-recursive parsers. In such cases, the functions on -//! [`Recursive`] allow for this. - -use super::*; - -/// A parser that can be defined in terms of itself by separating its [declaration](Recursive::declare) from its -/// [definition](Recursive::define). -/// -/// Prefer to use [`recursive()`], which exists as a convenient wrapper around both operations, if possible. -pub struct RecursiveDecl { - pub(super) inner: RecursiveInner

, -} - -impl RecursiveDecl

{ - #[inline] - pub(super) fn parser(&self) -> RefC

{ - match &self.inner { - RecursiveInner::Owned(x) => x.clone(), - RecursiveInner::Unowned(x) => x - .upgrade() - .expect("Recursive parser used before being defined"), - } - } -} - -// The decl should only pass weak references so that it can be used in itself -// without creating a memory leak -impl Clone for RecursiveDecl

{ - fn clone(&self) -> Self { - Self { - inner: match &self.inner { - RecursiveInner::Owned(x) => RecursiveInner::Unowned(RefC::

::downgrade(&x)), - RecursiveInner::Unowned(x) => RecursiveInner::Unowned(x.clone()), - }, - } - } -} - -impl<'a, 'b, I, O, E> ParserSealed<'a, I, O, E> for RecursiveDecl> -where - I: Input<'a>, - E: ParserExtra<'a, I>, -{ - #[inline] - fn go(&self, inp: &mut InputRef<'a, '_, I, E>) -> PResult { - recurse(move || { - M::invoke( - self.parser() - .inner - .get() - .expect("Recursive parser used before being defined") - .as_ref(), - inp, - ) - }) - } - - go_extra!(O); -} - -impl<'a, 'b, I, O, E> ParserSealed<'a, I, O, E> for RecursiveDecl> -where - I: Input<'a>, - E: ParserExtra<'a, I>, -{ - #[inline] - fn go(&self, inp: &mut InputRef<'a, '_, I, E>) -> PResult { - recurse(move || M::invoke(&*self.parser(), inp)) - } - - go_extra!(O); -} From c487531a743a52b3735e7d0d8e67b10a9af2487c Mon Sep 17 00:00:00 2001 From: Elijah Hartvigsen Date: Sat, 29 Jul 2023 11:52:26 +0200 Subject: [PATCH 3/4] recursive-leak: fix format --- examples/foo.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/examples/foo.rs b/examples/foo.rs index d5944011..a2ca4b4b 100644 --- a/examples/foo.rs +++ b/examples/foo.rs @@ -31,8 +31,7 @@ fn parser<'a>() -> impl Parser<'a, &'a str, Expr<'a>> { let ident = text::ascii::ident().padded(); let expr = recursive(|expr| { - let int = text::int(10) - .map(|s: &str| Expr::Num(s.parse().unwrap())); + let int = text::int(10).map(|s: &str| Expr::Num(s.parse().unwrap())); let call = ident .then( From 4572bb50278d32c527f322766f7d0eb87a7e7101 Mon Sep 17 00:00:00 2001 From: Zij-IT Date: Mon, 7 Aug 2023 11:10:20 +0200 Subject: [PATCH 4/4] recursive-leak: move define into Recursive<_, .., Declared> --- src/recursive.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/recursive.rs b/src/recursive.rs index c01cfa3d..6e492f0e 100644 --- a/src/recursive.rs +++ b/src/recursive.rs @@ -150,22 +150,23 @@ impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> state: EmptyPhantom::new(), } } -} -impl<'a, 'b, I: Input<'a>, O, E: ParserExtra<'a, I>> Recursive, Defined> { /// Defines the parser after declaring it, allowing it to be used for parsing. // INFO: Clone bound not actually needed, but good to be safe for future compat #[track_caller] pub fn define + Clone + MaybeSync + 'a + 'b>( parser: P, - declaration: Recursive, Declared>, - ) -> Self { + declaration: Self, + ) -> Recursive, Defined> { let location = *Location::caller(); - declaration.parser() + declaration + .parser() .inner .set(Box::new(parser)) .unwrap_or_else(|_| { - panic!("recursive parsers can only be defined once, trying to redefine it at {location}") + panic!( + "recursive parsers can only be defined once, trying to redefine it at {location}" + ) }); Recursive {