This refactoring is useful to make sure that we use the same logic to prefix rules with the failure discovered in the expansion

This clause was the cause of the error. It's too soon to raise the error at this stage, we should instead prefix the rule itself with the error.

Makes sense

The user should only see this error if they used some unknown percent form. Because the errors discovered before the expansion are prefixed.

I don't understand why we need to go through partial expansion here. The aim of partial expansion for actions is to be able to collect targets before the action is fully expanded. However, we never need this for the deps field given that it doesn't contain targets.

We need to interpret deps before expanding targets. For example, consider how we evaluate something like this:

(action
 (deps (:foo %{bin:bar}))
 (target %{foo}))

Agreed, but I still don't follow why we need to go through partial.

The errors are discovered as we're doing the expansion. When such an error is discovered, the expansion of that variable is aborted and hence the only way to recover is through a partial expansion.

I suppose it's possible to make this error some other exception. It could be caught and handled instead of handling partial expansions.

Ok, I feel like I'll need to go over all this code again at some point. I'm finding it difficult to follow everything. When you have left the airport chaos, we can maybe go over this together via videoconf.

Though, if someone else can review this don't wait for me.

In case someone else is curious, here's the pseudo-code of the current code works:

String_with_vars.expand ~f:(fun var ->
  if binary_vailable var then
    add_failure resolved_forms;
    None (* This raises because we are in an [expand] *)
  else
    Some (binary_path)
)

My proposal is to change the above to use partial_expand over expand. This will prevent the None case from raising and we'll be able to extract the error from the resolved form.

To avoid partial expansion, I suggested changing add_failure resolved_forms to raise an exception. The caller can catch it and propagate it via Build.fail.

Oh ok, I get it now!

In other places where we want to delay error reporting, such as for library name resolution, we are using the Or_exn.t monad. What do you think of doing the same here? That would make the intent explicit.

I was finding the code confusing because in my head partial is related to static vs dynamic. While here we are talking about delaying error reporting.

Sure, I can do that. It would also make things a bit faster as we'd stop trying to expand the other variables.

This wrapping is necessary so that the exception is caught by Expander.Or_error. We want to make sure that we don't accidentally delay exceptions that weren't raised by add_fail.

I'm wondering if that's really necessary. I tried removing the wrapping and nothing broke in the testsuite. I'm also thinking that we could remove add_fail entirely and just add a catch all around expand_and_record. That would make the code simpler and easier to reason about. WDYT?

I'm wondering if that's really necessary. I tried removing the wrapping and nothing broke in the testsuite.

Indeed. Another option is to just catch & delay User_error.E. I think we can reasonably assume that all errors can be delayed.

I'm also thinking that we could remove add_fail entirely and just add a catch all around expand_and_record. That would make the code simpler and easier to reason about. WDYT?

Seems fine to me. Just to confirm, you would then have to handle this expansion based on the expansion_kind. I.e.

...
with User_error.E _ as exn ->
  match exception_kind with
  | Dynamic -> Resolved_forms.add acc exn
  | Static -> reraise exn

Yep. But I don't even think we need to match on User_error.E here. Catching all exceptions seems fine. We are just making the behaviour lazy, that's all.

I think it would be quite bad to swallow Code_error here. See the last commit, I've simplified the code further but I'm still not catching only User_errors. I think that's a safe compromise.

Seems reasonable

This error is now created more eagerly than necessary. I don't think this is worth worrying about however.