Lowercase builtin vars #956

rgrinberg · 2018-07-05T17:32:19Z

SCOPE_ROOT and ROOT are still remaining as uppercased but those still need to be renamed.

emillon · 2018-07-06T07:43:38Z

test/blackbox-tests/test-cases/dune-jbuild-var-case/run.t

+  $ dune runtest --root dune-upper
+  Entering directory 'dune-upper'
+  File "dune", line 3, characters 41-46:
+  Error: Uppercase variables are removed in dune files. Use: %{make}


As discussed in #948, what do you think about the following error message:

Error: Uppercase variables are removed in dune files.
Hint: Did you mean %{make} instead?

emillon · 2018-07-06T07:44:28Z

test/blackbox-tests/test-cases/dune-jbuild-var-case/run.t

+  Error: Uppercase variables are removed in dune files. Use: %{make}
+  [1]
+
+jbuilder retains the old names:


Since it's about the file syntax rather than the tool, I think this should say "jbuild files" (and the commands can use dune runtest).

ghost · 2018-07-06T07:50:07Z

@rgrinberg could you make the mechanism a bit more generic? We might want to add, remove or rename more variables in the future and we'll need support to do that. What about replacing vars : Value.t list String.Map.t in Super_context.t by vars : Var.t String.Map.t where:

module Var : sig
  module Info : sig
    type t =
      | Nothing
      | Since of Syntax.t * Syntax.Version.t
      | Renamed_in of Syntax.t * Syntax.Version.t * string
      | Deleted_in of Syntax.t * Syntax.Version.t
  end

  type t =
    { value : Value.t list
    ; info : Info.t
    }
end

rgrinberg · 2018-07-06T07:59:08Z

@diml yeah, I think that's a good call. Can I do this refactor in a separate PR. I'll need to do essentially the same thing for #957 and it will be better to just do this change once for all variables.

rgrinberg · 2018-07-06T08:03:26Z

Actually, I can just introduce this change here and change the other stuff to use this later. It's not as straightforward to change the other stuff as they aren't really using maps yet (I hope to change that).

ghost · 2018-07-06T08:08:05Z

Indeed. One possibility would be to enrich a bit the Var.t type to allow variables that depend on the current directory:

module Var : sig
  module Info : sig
    type t =
      | Nothing
      | Since of Syntax.t * Syntax.Version.t
      | Renamed_in of Syntax.t * Syntax.Version.t * string
      | Deleted_in of Syntax.t * Syntax.Version.t
  end

  module Kind : sig
    type t =
      | Values of Value.t list
      | Root
      | Project_root
  end

  type t =
    { kind : Kind.t
    ; info : Info.t
    }
end

ghost · 2018-07-06T08:08:48Z

Possibly Deps and Targets could even go in Kind.t

rgrinberg · 2018-07-06T14:52:31Z

@diml actually, ROOT can be handled using the old scheme we discussed before. By introducing a Dir constructor that callers will know not to interpret as a dependency. The last 2 commits I just pushed implement that.

I'm not sure we should add deps or targets in there btw. Those expansions aren't available everywhere so it might be confusing to remember not to expand if a corresponding value has been found.

ghost · 2018-07-06T15:31:31Z

@diml actually, ROOT can be handled using the old scheme we discussed before. By introducing a Dir constructor that callers will know not to interpret as a dependency. The last 2 commits I just pushed implement that.

That seems good, yes.

I'm not sure we should add deps or targets in there btw. Those expansions aren't available everywhere so it might be confusing to remember not to expand if a corresponding value has been found.

Well it's a two stage expansion, once we get to the Deps constructor, we either expand it appropriately or fail if we are in a place where %{deps} is not allowed. That would actually lead to slightly better error messages. The advantage of doing so is that then all variables can go in the same map and it's easier to handle renames, etc... We can even push this further and include %{xxx:...} forms as well:

module Var : sig
  module Info : sig
    type t =
      | Nothing
      | Since of Syntax.t * Syntax.Version.t
      | Renamed_in of Syntax.t * Syntax.Version.t * string
      | Deleted_in of Syntax.t * Syntax.Version.t
  end

  module Kind : sig
    type t =
      | Values of Value.t list
      | Project_root
      | Deps
      | Targets
  end

  module Form : sig
     type t =
      | Exe
      | Dep
      | Bin
      | Lib
      | ...
      | Read_lines
      | ...
  end

  type 'a t =
    { kind : 'a
    ; info : Info.t
    }
end

type t =
  { ...
  ; vars  : Var.Kind.t Var.t String.Map.t
  ; forms : Var.Form.t Var.t String.Map.t
  ...
  }

This would allow to completely factorize the versioning code for variables and %{...:...} forms.

rgrinberg · 2018-07-06T23:43:41Z

What is the point of the Nothing constructor? Is the absence of the key in the map enough?

Also, do we need a Syntax.t yet? We're only using the dune syntax for now. But we can indeed add this parameter once we have other syntaxes to consider.

EDIT: Ah ok I see, nvm

rgrinberg · 2018-07-07T02:03:49Z

The last commit implements a rough version of the scheme @diml was talking about. One thing that I decided to change was not to attached values to renamed variables. The reasoning behind this is that it's yet another thing to make sure we sync correctly between the different names. The cost here is of course the extra lookup for these renamed vars. But since users are expected to transition, this cost will not be paid for long. In any case, if we switch the variable maps to be tables, the lookup will be much faster.

Also, there might be some type system game we could play to avoid those assert false for the impossible cases. Open to suggestions here.

rgrinberg · 2018-07-07T07:37:22Z

I added a commit that removes a few assert false clauses at the expense of a lot GADT boilerplate. It's more of a joke than a serious improvement so I'm leaning towards reverting it :)

rgrinberg · 2018-07-07T08:09:32Z

By the way, we could just include the Kind inside the info and all these problems will go away:

module Info : sig
  type 'a t =
    | No_info of 'a
    | Renamed_in of Syntax.Version.t * string
    | Since of Syntax.Version.t * 'a
    | Deleted_in of Syntax.Version.t * 'a
end

rgrinberg · 2018-07-07T17:25:32Z

And this is what I did. The code looks a lot cleaner. Switching to a hash table was fairly easy as well.

@diml I'm wondering if we should make variable names interned. Seems like there will be some performance benefit and we'll have a distinct type for these kinds of strings.

ghost · 2018-07-07T18:56:58Z

src/super_context.ml

@@ -33,6 +33,245 @@ module Env_node = struct
    }
 end

+module Var : sig


This module is big enough, let's put it in its own file.

Yeah, I agree, but this module is really private to super contexts. Can you think of a good name for this? Longer term, I'm thinking of renaming String_with_vars to Template and then it will be possible to have a Var module.

By the way, this really highlights the need to have sub library like modules that we've talked about.

So for now I'll create make var.ml for this module.

ghost · 2018-07-07T19:02:37Z

src/super_context.ml

+          else
+            String_with_vars.Var.fail var ~f:(fun var ->
+              sprintf "Variable %s is available in since version %s. \
+                       Current version is %s"


Could you factorize these error messages between syntax.ml and here? They'll be more consistent this way.

ghost · 2018-07-07T19:05:38Z

src/super_context.ml

+                (Syntax.Version.to_string syntax_version))
+        | Renamed_in (in_version, new_name) -> begin
+            if syntax_version >= in_version then
+              String_with_vars.Var.fail var ~f:(fun old_name ->


I don't find Var.fail particularly clear. I think this code would be clearer if we stick to Loc.fail (String_with_vars.Var.loc var) "..."

ghost · 2018-07-07T19:09:22Z

src/super_context.ml

+
+    let rec expand t ~syntax_version ~var =
+      let name =
+        match String_with_vars.Var.destruct var with


This is complicated version of var.name, let's add Var.name : t -> string

ghost · 2018-07-07T19:17:46Z

src/super_context.ml

+  end
+
+  type 'a t =
+    | Nothing    of 'a


the name Nothing was meant to mean no information. Nothing of 'a looks a bit odd, maybe just T of 'a?

I changed it to No_info. T is already our convention for open types.

ghost · 2018-07-07T19:27:37Z

src/super_context.ml

+  begin match String_with_vars.Var.destruct var with
+  | Single _ -> ()
+  | Pair (_, _) ->
+    Exn.code_error "expand_vars can't expand forms"


I think this error can leak to the user. For instance if they write %{read:...} in (flags ...) or (copy_files ...).

You sure about that? We're making sure to call this on the right map. I.e. the one that contains only forms or only single variables.

Can you add a test with a %{read:...} inside a copy_files just to be sure?

ghost · 2018-07-07T19:28:31Z

src/super_context.ml

@@ -84,26 +324,39 @@ let installed_libs t = t.installed_libs
 let find_scope_by_dir  t dir  = Scope.DB.find_by_dir  t.scopes dir
 let find_scope_by_name t name = Scope.DB.find_by_name t.scopes name

-let expand_var_no_root t var = String.Map.find t.vars var
+let expand_vars t ~syntax_version ~var : Var.Kind.t option =
+  begin match String_with_vars.Var.destruct var with


Let's add Var.is_form : t -> bool. This should make this code slightly clearer.

ghost · 2018-07-07T19:36:43Z

src/super_context.ml

+      match targets_written_by_user with
+      | Infer -> Loc.fail loc "You cannot use %s with inferred rules." var
+      | Alias -> Loc.fail loc "You cannot use %s in aliases." var
+      | Static l -> Some (Value.L.dirs l) (* XXX hack to signal no dep *)


The targets function is used only once, I think it'll be clearer if it's inlined. It took me a while to understand why we needed a hack for instance.

ghost · 2018-07-07T19:44:46Z

test/blackbox-tests/test-cases/findlib-error/run.t

@@ -3,8 +3,7 @@ We are dropping support for findlib in dune
  $ dune build --root in-dune target.txt
  Entering directory 'in-dune'
  File "dune", line 2, characters 25-37:
-  Error: The findlib special variable is not supported in jbuild files, please use lib instead:
-  %{lib:pkg} in dune files
+  Error: Variable %{findlib:pkg} has been renamed to %{lib:pkg} since 1.0


We should keep the term variable for variables without payload. These should be called forms, so:

Error: The %{findlib:...} form has been renamed to %{lib:...} since version 1.0 of the dune language.

Note: we need to make sure to not print the actual payload in such error messages. If we include the payload (pkg in thie case), then we are talking about this instance of the form. If we print ..., we are talking about the form in general, which is what we want here.

ghost · 2018-07-07T19:56:53Z

This is a lot cleaner indeed and it's nice to see this code being cleanup! it dates from the origin of jbuilder...

@rgrinberg I'm not sure we should use a hash table here. In the future we might want to locally extend the environment and using a mutable data structure will make that difficult.

Regarding interning variables, I'm pretty sure the difference wouldn't be noticeable; I can only count about 1500 variables in the jbuild files of the whole dune-universe.

rgrinberg · 2018-07-08T07:07:02Z

I'm not sure we should use a hash table here. In the future we might want to locally extend the environment and using a mutable data structure will make that difficult.

Not necessarily. I was thinking that it should be possible to extend an environment by chaining the lookup. Which of course will also require a different representation. It's just that I thought we're paying for the map keys to be in sorted order, and I don't ever really see that being useful. I'll bring back the maps as it's really simple to swap the implementation.

Regarding interning variables, I'm pretty sure the difference wouldn't be noticeable; I can only count about 1500 variables in the jbuild files of the whole dune-universe.

Yeah, I didn't really expect it to be a huge gain either. We can revisit this if variables become more common place.

rgrinberg · 2018-07-08T08:59:29Z

Have another look @diml. I think I've addressed all your comments, but there were quite a few. So let me know if I missed anything.

ghost · 2018-07-08T09:08:00Z

BTW, some of the terminology (var, form, ...) came up during development but we never really discussed it properly and as a result things are not always consistent. It'd be good to settle on a consistent naming for both user facing things (doc and error messages) and the code. I suggest the following one:

thing	name in documentation and error messages	name in the code
`%{x}`	variable	var
`%{x:y}`	macro	macro
`%{...}` (either a variable or a macro)	percent form	pform

ghost · 2018-07-08T09:08:25Z

I'm having a look now

ghost · 2018-07-08T09:12:52Z

src/var.ml

+    let name = String_with_vars.Var.name var in
+    Option.bind (String.Map.find t name) ~f:(fun v ->
+      let what var =
+        sprintf "Variable %s" (String_with_vars.Var.to_string var) in


We should check here if var is a form or variable In order to display a different error message. If it's a form we should display: The %{<var-name>:...} form, i.e. with the payload replaced by ... so that it's clear we are talking about the form in general and not just this particular instance.

Ah yes. Makes sense. I only made this fix for a particular error message.

ghost · 2018-07-08T09:14:07Z

test/blackbox-tests/test-cases/path-variables/run.t

@@ -51,6 +53,5 @@ This form does not exist, but displays an hint:
  $ dune build --root jbuild-invalid @test-dep
  Entering directory 'jbuild-invalid'
  File "jbuild", line 5, characters 16-37:
-  Error: ${dep:generated-file} is not supported in jbuild files.
-  Hint: Did you mean ${path:generated-file} instead?
+  Error: Variable ${dep:generated-file} is only available since version 0.0 of the dune language


This looks odd. Shouldn't this say since version 1.0 instead?

Whoops, I just substituted the wrong version.

rgrinberg · 2018-07-08T09:33:08Z

Alright fixed those comments. I've renamed a few things to use the new naming convention now. There's probably more at the moment, but I'm not sure if it all needs to be in this PR.