Skip to content
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

Jump to bottom

Postprocessor type does not enable storing state in the postprocessor #175

Open
rsesek opened this issue Sep 22, 2023 · 0 comments · May be fixed by #184
Open

Postprocessor type does not enable storing state in the postprocessor #175

rsesek opened this issue Sep 22, 2023 · 0 comments · May be fixed by #184

Comments

@rsesek
Copy link
Contributor

rsesek commented Sep 22, 2023

The type definition for Postprocessor is:

pub type Postprocessor =
    dyn Fn(&mut Context, &mut MarkdownEvents) -> PostprocessorResult + Send + Sync;

When using obsidian-export as a library, this makes it impossible to create a postprocessor that accumulates state as notes are processed. For example, let's say you wanted to collect all the directory parents containing markdown files to produce an index page. The following code:

    let parents: Mutex<HashSet<PathBuf>> = Default::default();
    let callback = |ctx: &mut Context, _events: &mut MarkdownEvents| -> PostprocessorResult {
        let mut dirs = parents.lock().unwrap();
        dirs.insert(ctx.destination.parent().unwrap().to_path_buf());
        PostprocessorResult::Continue
    };
    exporter.add_postprocessor(&callback);

… results in:

error[E0597]: `parents` does not live long enough
  --> src/main.rs:14:28
   |
13 |         |ctx: &mut Context, _events: &mut MarkdownEvents| -> PostprocessorResult {
   |         ------------------------------------------------------------------------ value captured here
14 |             let mut dirs = parents.lock().unwrap();
   |                            ^^^^^^^ borrowed value does not live long enough
...
18 |     exporter.add_postprocessor(&callback);
   |                                --------- cast requires that `parents` is borrowed for `'static`
19 | }
   | - `parents` dropped here while still borrowed

This can be fixed by adding a lifetime annotation to Postprocessor so that 'static is not assumed by the compiler.

But the callback can be somewhat clunky, if the client code is storing state on a struct (as opposed to the example above just capturing a local). E.g., given something like this:

struct State {
    parents: Mutex<HashSet<PathBuf>>,
}

impl State {
    fn new() -> State {
        State {
            parents: Default::default(),
        }
    }
    fn postprocess(&self, ctx: &mut Context, _events: &mut MarkdownEvents) -> PostprocessorResult {
        let mut dirs = self.parents.lock().unwrap();
        dirs.insert(ctx.destination.parent().unwrap().to_path_buf());
        PostprocessorResult::Continue
    }
}

fn main() {
    let mut exporter = Exporter::new(PathBuf::new(), PathBuf::new());
    let state = State::new();
    exporter.add_postprocessor(&|ctx, events| state.postprocess(ctx, events));
    exporter.run().unwrap();
}

The caller needs to move the closure binding to a local given this error:

error[E0716]: temporary value dropped while borrowed
  --> src/main.rs:28:33
   |
28 |     exporter.add_postprocessor(&|ctx, events| state.postprocess(ctx, events));
   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - temporary value is freed at the end of this statement
   |                                 |
   |                                 creates a temporary value which is freed while still in use
29 |
30 |     exporter.run().unwrap();
   |     -------------- borrow later used here
   |
help: consider using a `let` binding to create a longer lived value
   |
28 ~     let binding = |ctx, events| state.postprocess(ctx, events);
29 ~     exporter.add_postprocessor(&binding);

It seems like it would be more ergonomic still to provide a trait for postprocessing. I propose addressing both of these: add a lifetime to the Postprocessor type, and define traits for postprocessing.
rsesek added a commit to rsesek/obsidian-export that referenced this issue Sep 22, 2023
@rsesek
Add a lifetime annotation to the Postprocesor type
1ab12c4 
This lets the compiler reason about the lifetimes of objects used by the
postprocessor, if the callback captures variables.

See zoni#175
rsesek added a commit to rsesek/obsidian-export that referenced this issue Sep 22, 2023
@rsesek
Define new Postprocess and EmbedPostprocess traits
99ed866 
These are like the Postprocessor callback function type, but they can
be implemented on types for more ergonomic stateful postprocessing.

Fixes zoni#175
@rsesek rsesek mentioned this issue Sep 22, 2023
Merged
rsesek added a commit to rsesek/obsidian-export that referenced this issue Sep 22, 2023
@rsesek
Add a lifetime annotation to the Postprocesor type
011701a 
This lets the compiler reason about the lifetimes of objects used by the
postprocessor, if the callback captures variables.

See zoni#175
rsesek added a commit to rsesek/obsidian-export that referenced this issue Sep 22, 2023
@rsesek
Add a lifetime annotation to the Postprocesor type
c04b697 
This lets the compiler reason about the lifetimes of objects used by the
postprocessor, if the callback captures variables.

See zoni#175
rsesek added a commit to rsesek/obsidian-export that referenced this issue Sep 22, 2023
@rsesek
Define new Postprocess and EmbedPostprocess traits
e64c766 
These are like the Postprocessor callback function type, but they can
be implemented on types for more ergonomic stateful postprocessing.

Fixes zoni#175
rsesek added a commit to rsesek/obsidian-export that referenced this issue Sep 24, 2023
@rsesek
Add a lifetime annotation to the Postprocesor type
5862433 
This lets the compiler reason about the lifetimes of objects used by the
postprocessor, if the callback captures variables.

See zoni#175
zoni pushed a commit that referenced this issue Sep 25, 2023
@rsesek @zoni
Add a lifetime annotation to the Postprocesor type
cd5dbf6 
This lets the compiler reason about the lifetimes of objects used by the
postprocessor, if the callback captures variables.

See #175
rsesek added a commit to rsesek/obsidian-export that referenced this issue Sep 25, 2023
@rsesek
Define new Postprocess and EmbedPostprocess traits
d572428 
These are like the Postprocessor callback function type, but they can
be implemented on types for more ergonomic stateful postprocessing.

Fixes zoni#175
rsesek added a commit to rsesek/obsidian-export that referenced this issue Oct 8, 2023
@rsesek
Define new Postprocess and EmbedPostprocess traits
7988b21 
These are like the Postprocessor callback function type, but they can
be implemented on types for more ergonomic stateful postprocessing.

Fixes zoni#175
@rsesek rsesek linked a pull request Oct 8, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implement postprocessors using traits rsesek/obsidian-export
1 participant
@rsesek