Delay expansion errors for rules #3261

rgrinberg · 2020-03-14T10:50:24Z

Previosuly, a failed expansion would immediately fail generating a rule.
Now, we delay the error using a [Build.fail].

I've left some inline comments to make it easy for reviewers.

rgrinberg · 2020-03-14T10:52:36Z

src/dune/expander.ml

+  let prefix_failures t b =
+    match t.failures with
+    | [] -> b
+    | fail :: _ -> Build.O.( >>> ) (Build.fail fail) b


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

rgrinberg · 2020-03-14T10:53:21Z

src/dune/expander.ml

@@ -322,11 +342,6 @@ let expand_and_record acc ~map_exe ~dep_kind ~expansion_kind
    | Static -> fun _ -> User_error.raise ~loc [ cannot_be_used_here pform ]
    | Dynamic -> Resolved_forms.add_ddep acc expansion
  in
-  let add_fail =
-    match expansion_kind with
-    | Static -> fun _ (f : fail) -> f.fail ()


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

rgrinberg · 2020-03-14T10:55:12Z

src/dune/super_context.ml

+                (fun () ->
+                  let loc = String_with_vars.loc sw in
+                  User_error.raise ~loc
+                    [ Pp.text "failed to expand percent form" ])


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

ghost · 2020-03-16T11:46:44Z

src/dune/super_context.ml

+      Expander.Partial.expand_path expander s
+      |> String_with_vars.Partial.map ~f:(fun path ->
+             let+ () = Build.path path in
+             [ path ])


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.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg · 2020-03-17T13:39:55Z

@diml should be ready now.

rgrinberg · 2020-03-17T13:40:55Z

src/dune/expander.ml

@@ -324,8 +351,9 @@ let expand_and_record acc ~map_exe ~dep_kind ~expansion_kind
  in
  let add_fail =
    match expansion_kind with
-    | Static -> fun _ (f : fail) -> f.fail ()
-    | Dynamic -> Resolved_forms.add_fail
+    | Static -> Or_exn.raise


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

rgrinberg · 2020-03-17T13:41:33Z

src/dune/expander.ml

+        | false ->
+          if Lib.DB.available (Scope.libs t.scope) lib then
+            User_error.E
+              (User_error.make ~loc


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

ghost · 2020-03-17T19:33:03Z

I did a pass of refactoring and made the API of Expander a bit higher level. For instance, Resolved_forms is no longer exposed and is just an implementation detail. I feel like expander.ml could be simplified further, but that'll be for another time.

ghost

Looks good

rgrinberg · 2020-03-17T19:56:21Z

@diml I removed the type variable from expander_result. Was it added by accident?

I agree this looks much better.

Previosuly, a failed expansion would immediately fail generating a rule. Now, we delay the error using a [Build.fail]. Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

In expand_and_record, instead of receiving an argument describing how the function should behave, we always do the same thing and add two wrappers that decide what to do based on what behavior the caller wanted. Make [Resolved_forms] hidden inside [Expander] Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

ghost · 2020-03-18T08:12:59Z

@diml I removed the type variable from expander_result. Was it added by accident?

Indeed. It was a temporary state while I was rediscovering how all this works.

@diml

…lugin, dune-private-libs and dune-glob (2.5.0) CHANGES: - Add a `--release` option meaning the same as `-p` but without the package filtering. This is useful for custom `dune` invocation in opam files where we don't want `-p` (ocaml/dune#3260, @diml) - Fix a bug introduced in 2.4.0 causing `.bc` programs to be built with `-custom` by default (ocaml/dune#3269, fixes ocaml/dune#3262, @diml) - Allow contexts to be defined with local switches in workspace files (ocaml/dune#3265, fix ocaml/dune#3264, @rgrinberg) - Delay expansion errors until the rule is used to build something (ocaml/dune#3261, fix ocaml/dune#3252, @rgrinberg, @diml) - [coq] Support for theory dependencies and compositional builds using new field `(theories ...)` (ocaml/dune#2053, @ejgallego, @rgrinberg) - From now on, each version of a syntax extension must be explicitely tied to a minimum version of the dune language. Inconsistent versions in a `dune-project` will trigger a warning for version <=2.4 and an error for versions >2.4 of the dune language. (ocaml/dune#3270, fixes ocaml/dune#2957, @voodoos) - [coq] Bump coq lang version to 0.2. New coq features presented this release require this version of the coq lang. (ocaml/dune#3283, @ejgallego) - Prevent installation of public executables disabled using the `enabled_if` field. Installation will now simply skip such executables instead of raising an error. (ocaml/dune#3195, @voodoos) - `dune upgrade` will now try to upgrade projects using versions <2.0 to version 2.0 of the dune language. (ocaml/dune#3174, @voodoos) - Add a `top` command to integrate dune with any toplevel, not just utop. It is meant to be used with the new `#use_output` directive of OCaml 4.11 (ocaml/dune#2952, @mbernat, @diml) - Allow per-package `version` in generated `opam` files (ocaml/dune#3287, @toots) - [coq] Introduce the `coq.extraction` stanza. It can be used to extract OCaml sources (ocaml/dune#3299, fixes ocaml/dune#2178, @rgrinberg) - Load ppx rewriters in dune utop and add pps field to toplevel stanza. Ppx extensions will now be usable in the toplevel (ocaml/dune#3266, fixes ocaml/dune#346, @stephanieyou) - Add a `(subdir ..)` stanza to allow evaluating stanzas in sub directories. (ocaml/dune#3268, @rgrinberg) - Fix a bug preventing one from running inline tests in multiple modes (ocaml/dune#3352, @diml) - Allow the use of the `%{profile}` variable in the `enabled_if` field of the library stanza. (ocaml/dune#3344, @mrmr1993) - Allow the use of `%{ocaml_version}` variable in `enabled_if` field of the library stanza. (ocaml/dune#3339, @voodoos) - Fix dune build freezing on MacOS when cache is enabled. (ocaml/dune#3249, fixes #ocaml/dune#2973, @artempyanykh)

rgrinberg force-pushed the extra-deps branch from 4df13fe to 9ebe184 Compare March 14, 2020 10:50

rgrinberg commented Mar 14, 2020

View reviewed changes

rgrinberg force-pushed the extra-deps branch 2 times, most recently from 62388f4 to 5c8f9a8 Compare March 14, 2020 16:28

ghost reviewed Mar 16, 2020

View reviewed changes

Add reproduction case for ocaml#3252

6b2bb7a

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg force-pushed the extra-deps branch from 5c8f9a8 to 4aa98a3 Compare March 17, 2020 13:38

rgrinberg commented Mar 17, 2020

View reviewed changes

rgrinberg force-pushed the extra-deps branch from 44e1dd5 to d549478 Compare March 17, 2020 17:17

ghost approved these changes Mar 17, 2020

View reviewed changes

Delay expansion errors for rules

b9d9637

Previosuly, a failed expansion would immediately fail generating a rule. Now, we delay the error using a [Build.fail]. Signed-off-by: Rudi Grinberg <me@rgrinberg.com>

rgrinberg force-pushed the extra-deps branch from 06832ec to 8c863da Compare March 17, 2020 19:57

rgrinberg force-pushed the extra-deps branch from 8c863da to 70c1dee Compare March 17, 2020 19:59

rgrinberg merged commit 70c1dee into ocaml:master Mar 17, 2020

rgrinberg mentioned this pull request Apr 10, 2020

[new release] dune-build-info, dune, dune-configurator, dune-action-plugin, dune-private-libs and dune-glob (2.5.0) ocaml/opam-repository#16170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delay expansion errors for rules #3261

Delay expansion errors for rules #3261

rgrinberg commented Mar 14, 2020

rgrinberg Mar 14, 2020

rgrinberg Mar 14, 2020

ghost Mar 16, 2020

rgrinberg Mar 14, 2020

ghost Mar 16, 2020

rgrinberg Mar 16, 2020

ghost Mar 16, 2020

rgrinberg Mar 16, 2020

ghost Mar 16, 2020

rgrinberg Mar 16, 2020

ghost Mar 17, 2020

rgrinberg Mar 17, 2020

rgrinberg commented Mar 17, 2020

rgrinberg Mar 17, 2020

ghost Mar 17, 2020

rgrinberg Mar 17, 2020

ghost Mar 17, 2020

rgrinberg Mar 17, 2020

ghost Mar 17, 2020

rgrinberg Mar 17, 2020

ghost commented Mar 17, 2020

ghost left a comment

rgrinberg commented Mar 17, 2020

ghost commented Mar 18, 2020

Delay expansion errors for rules #3261

Delay expansion errors for rules #3261

Conversation

rgrinberg commented Mar 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rgrinberg commented Mar 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Mar 17, 2020

ghost left a comment

Choose a reason for hiding this comment

rgrinberg commented Mar 17, 2020

ghost commented Mar 18, 2020