Syntax for naming dependencies

[rgrinberg]

This PR is WIP but I'd like to make sure we agree on the syntax before I clean things up and implement the rest.

@diml I've ended up not including a general List constructor in Dep_conf.t because I think nesting such a constructor would make things awkward. Instead, I only allow list as a "top level" constructor.

Also, I made (list ..) available to unnamed dependencies for symmetric even though it's fairly useless there. I believe that if we have :x (foo bar baz) and deleting :x causing an error is a bit unnatural.

Refer to the tests for what it currently looks like, as the implementation is still hairy.

[rgrinberg]

Actual, naming dependencies is trickier than I thought. Since you can also name template variables this way: :x %{y} where y is another variable. Should we disallow these kind of bindings? Or check if such bindings don't introduce any recursion?

Btw, since such named deps can be bound to variables. I'm thinking that we might need to do the expansion in stages. First expand user named dependencies, and then the other variables.

[ghost]

The description of the feature looks good to me. Allowing list at toplevel for consistency seems good.

We should allow :x %{y}. It should behave as if %{y} was first completely evaluated and then bound to x. As long as we don't allow things like %{read:...} immediately, it should be fine, we can do the expansion completely statically. The evaluation of dependencies shouldn't be recursive, i.e. considering this environment:

x = a

and the following dependencies:

(deps :x b :y %{x})

we should obtain the following environment:

x = b
y = a

[emillon]

I'm not sure I understand what this does: does (deps :x x1 x2 x3 :y y1 y2) create two bindings x and y? What is their scope? Also I'm curious about a concrete use case (I assume it's about replacing :include).

[rgrinberg]

@diml I've added some validation for duplicate bindings.

Okay, I think what you're describing is similar to what I said.

@emillon In your example, you would create 2 bindings ["x", "x1"; "y", "y1"] and this feature is not related to replacing :include. This feature is what is going to replace %{<} and %{^}. The point is that we can give names to sub lists of the dependencies list to refer to in actions. So you could write actions like:

(action
 (deps :js (glob_files *.js) :css (glob_files *.css))
 (action (run bundle -html %{js} -css %{css})))

Which seems more reasonable than using indices.

[emillon]

Thanks, I see the use case now. Have you considered using an association list syntax?

(named_deps
  (js (glob_files *.js))
  (css (glob_files *.css))
)

Or a dedicated let construct?

(rule
  (let
     ((js (glob_files *.js))
      (css (glob_files *.css)))
  (deps %{js} %{css})
  (action (run bundle -html %{js} -css %{css})
))

I'm not particularly in favor of a particular one, but if it's a binding construct we can consider the alternatives.

[ghost]

I like named_deps. The only downside is that we'll need two variants of each xxx_deps field (named_preprocessor_deps, named_link_deps, ...) but that doesn't seem too bad.

[rgrinberg]

No, I haven't considered any alternatives apart from the :x syntax. Just a note however, :x is also lisp inspired as it's copying the property list syntax.

I think that eventually we'll have to introduce a let construct anyways, but it seems quite heavy weight just for user rules.

I'm not such a fan of named_deps because it's yet another field to remember the way it interacts with deps is not obvious. Also, I think the field duplication is more acute than what Jeremie thinks because it's possible that we might want to introduce such binding in other fields (e.g. targets). I'd hate to have to add yet another set of fields. Maybe a named constructor is more serviceable instead?

[rgrinberg]

And with that last note, I could probably make my implementation more generic by allowing for bindings in any list valued field:

module Named_list : sig
  type 'a t =
    { named: (Loc.t * 'a) String.Map.t
    ; unnamed: 'a list
    }

  val t : 'a Sexp.Of_sexp.t -> 'a t Sexp.Of_sexp.t
end

Of course, the concrete syntax still remains under discussion.

[emillon]

Duplicating every field does not look too great, indeed, so how would the concrete syntax for a named constructor look like?

(deps
  (universe)
  (file something.txt)
  (named
     (js (glob_files *.js))
  )
)

Some questions:

• do we need :js or is js enough?
• there's a small irregularity in the syntax: things that are named cannot include named things (ie (deps (named (a (named (b (universe)))))) does not make sense)
• what would it mean if there are several (named) blocks?

[rgrinberg]

Actually my idea would to simplify have 1 named constructor where you can introduce both named and unnamed deps:

(deps (names (:js glob_files *.js) some-files)))

Although tbh, this doesn't really look convincing anymore and the original syntax suggestion is the same thing just with less clutter.

[ghost]

I'm not a big fan of the nested names or named either as I find it a bit odd that the bindings are deep inside a field.

Looking at this again, I prefer the original syntax. My thinking is as follow: filenames pretty much never start with :, so it's a good indication that it is a special syntax, and because these stanzas are often short, the use of the variable is always close to the binding point so it's easy to make the mental association. @emillon do you find the original syntax very non-intuitive or do you think it's something users could easily get used to?

BTW, I realized that we don't need the list constructor. A variable can simply be bound to everything on the right until the next one: i.e. (deps a :x b c :y d) would produce the following bindings:

x = b c
y = d

If we wanted to exclude c from x, we could simply reorder things: (deps a c :x b :y d).

[emillon]

The problems I see with the short syntax is that it does not look like a binding, and is hard to search documentation for, since there's no keyword. But as long as it's documented in (deps), that should be enough to make it discoverable.

[ghost]

Would it be clearer if we used an equal sign? For instance:

(action
 (deps js:= (glob_files *.js) css:= (glob_files *.css))
 (action (run bundle -html %{js} -css %{css})))

[rgrinberg]

IMO, the original syntax is more clean. It's also fairly intuitive for those who are familiar with elisp or the like where :xxx is a syntax for symbols.

BTW, I realized that we don't need the list constructor. A variable can simply be bound to everything on the right until the next one: i.e. (deps a :x b c :y d) would produce the following bindings:

Sure but I think that this kind of order dependence is a weakness of the syntax. Why can't have parens for grouping again. Looking back at the example you've brought up before:

:x (glob_files foo)

I actually think that it should just be a constructor and an incorrect number of arguments should be an error. The user can easily just quote the constructor name to get around this and we could in fact include that point in the error message: "To use the constructor name literally, surround it with double quotes"

[emillon]

I agree that the original syntax is cleaner. I don't have a particular opinion on the concrete syntax for lists.

[rgrinberg]

Actually, maybe avoiding the parens on the RHS is best. Neither the list constructor nor my suggestion are satisfactory. Doing something like :x (foo bar) is fragile if we ever introduce a new constructor foo and we don't really want the user to quote stuff defensively. I think diml's suggestion is best after all.

[ghost]

Great, that's settled then. @rgrinberg do you consider that this feature is ready, modulo the last discussed changes? If yes, we can review it today and include it in 1.0.0 sine it's a nice addition, otherwise we should just make a PR to reserve the :... syntax.

[rgrinberg]

Yeah, I'd like to get it done today or this weekend. But there are other pre-reqs for 1.0 that need to be merged so let's focus on those before this PR.

[ghost]

BTW, looking at the example I wrote before, we don't even need to re-order dependencies to exclude c from x. We can simply introduce a dummy variable: (deps a :x b :_ c :y d). We could even allow : as a delimiter:

(deps file1 :x file2 : file3 :y file4 file5 : file6)

However I'm not sure that's very clear. Another possibility is to allow parentheses around the bindings. Since we don't have constructors starting with : there is no ambiguity:

(deps file1 (:x file2) file3 (:y file4 file5) file6)

[rgrinberg]

I like your last suggestion the most. Requiring parens around bindings seems acceptable boilerplate to me over adding things like artificial separators. If anything, I'd prefer if we added another form of braces specifically for lists. For example:

(deps :x [foo bar] :y baz)

But I think surrounding the entire pair is a good compromise.

The dummy variable idea is alright too, but I feel that it's too clever to be intuitive for users.

[ghost]

I like your last suggestion the most.

Me too. Should we also allow this: (deps file1 (:x file2 file3 (:y file4) file5)) which would procude:

x = file2 file3 file4 file5
y = file4

? i.e. we allow to bind at different levels. This can be useful to avoid duplicating a dependency.

[rgrinberg]

Yeah, I think it's fine. Though perhaps we should save it post 1.0 as I'd like to think about whether this cleverness will pay off.

Also I'm wondering if we should change the meaning of first-dep to be the first unnamed dep. But actually, I'm thinking that your idea of getting rid of first-dep makes the most sense. Mixing named and unnamed deps with integer indexing is confusing.

[ghost]

Additionally if we get rid of %{first-dep} then the following nice property is true: all the variables that are recognized are either global or appear close to their use site. This is because for %{deps} and %{targets} the names match the field names.

[rgrinberg]

A last minute bug fix was necessary. I accidentally renamed %{<} incorrectly (to Deps rather than First_deps) and this ended breaking downstream packages such as yojson. Fixing this bug revealed yet another bug in this PR: %{input-file} only works for dune files. I added a compatibility fix that makes %{<} be the same as %{input-file} for those cases. This is fudging things a little bit, but I don't think it will matter.