Support using `BuildPlanBuilder` without chaining

[edmorley]

Currently the methods on BuildPlanBuilder consume self since they take mut self as the first method argument, eg: https://github.com/Malax/libcnb.rs/blob/39cb9879707e16323e44ff5e14408ba43d2b126c/libcnb-data/src/build_plan.rs#L48-L51

This means one cannot write something like:

    fn detect(&self, context: DetectContext<Self>) -> libcnb::Result<DetectResult, Self::Error> {
        let build_plan_builder = BuildPlanBuilder::new();
        build_plan_builder.provides("python");

        if ["pyproject.toml", "requirements.txt", "setup.py"]
            .iter()
            .any(|f| context.app_dir.join(f).exists())
        {
            build_plan_builder.requires("python");
        }

        // Always pass detect, and instead use the build plan to determine whether this
        // buildpack should be used. This allows other buildpacks to require `python` even
        // if the app source doesn't contain one of the files above.
        DetectResultBuilder::pass()
            .build_plan(build_plan_builder.build())
            .build()
    }

And instead a reassignment has to be performed inside the conditional:

    fn detect(&self, context: DetectContext<Self>) -> libcnb::Result<DetectResult, Self::Error> {
        let mut build_plan_builder = BuildPlanBuilder::new().provides("python");

        if ["pyproject.toml", "requirements.txt", "setup.py"]
            .iter()
            .any(|f| context.app_dir.join(f).exists())
        {
            build_plan_builder = build_plan_builder.requires("python");
        }

        // Always pass detect, and instead use the build plan to determine whether this
        // buildpack should be used. This allows other buildpacks to require `python` even
        // if the app source doesn't contain one of the files above.
        DetectResultBuilder::pass()
            .build_plan(build_plan_builder.build())
            .build()
    }

Unless there is a more idiomatic way to write the above, that I'm missing?

Contrast this to ProcessBuilder, which doesn't consume self: https://github.com/Malax/libcnb.rs/blob/5ffcd9c5ed2c9c95fa73e00930f693f13888ee33/libcnb-data/src/launch.rs#L132

In general we have quite a lot inconsistency between our builder types - cross-ref #166.

[edmorley]

To support the non-chained form, we'd need to

• change the receiver of all of the BuildPlanBuilder methods to be &mut self not mut self
• adjust BuildPlanBuilder::build() to clone self before building it (since it can't move out of a mutable reference)

After that, the example without the additional assignment in the OP will work.

In addition, the fully-chained form will still work, eg:

let build_plan = BuildPlanBuilder::new().provides("python").build();

However this form will no longer work:

let build_plan_builder = BuildPlanBuilder::new().provides("python");
let build_plan = build_plan_builder.build();

...since it will give an error like:

error[E0716]: temporary value dropped while borrowed
 --> src/build_plan.rs:30:26
  |
6 | let build_plan_builder = BuildPlanBuilder::new().provides("python");
  |                          ^^^^^^^^^^^^^^^^^^^^^^^                   - temporary value is freed at the end of this statement
  |                          |
  |                          creates a temporary which is freed while still in use
7 | let build_plan = build_plan_builder.build();
  |                  -------------------------- borrow later used here
  |
  = note: consider using a `let` binding to create a longer lived value

Instead that would have to be switched to either the fully chained form, or else just make sure the return value from new() is assigned first:

let build_plan_builder = BuildPlanBuilder::new();
let build_plan = build_plan_builder.provides("python").build();

To me, this seems fine, and so I'm leaning towards making this change.

@Malax @hone Thoughts?

[joshwlewis]

I really like the chained form and would hate to lose it, but I also had this issue recently.

For more complex modification, what about a mutation function to avoid the mutable reassignment?

let builder = BuildPlanBuilder::new().provides("python").tap(|builder| {
  if ["pyproject.toml", "requirements.txt", "setup.py"]
            .iter()
            .any(|f| context.app_dir.join(f).exists())
        {
            builder.requires("python")
        } else {
             builder
        }
});

Where tap could look like:

fn tap<F>(self, mut f: F) -> Self
where
    F: FnMut(Self) -> Self,
{
    f(Self)
}