Remove Concat_or_split

[rgrinberg]

Thie property will now be determined from the context

Signed-off-by: Rudi Grinberg rudi.grinberg@gmail.com

[rgrinberg]

Putting this up separately and I'm a bit surprised this doesn't break any tests. I guess we don't have a test suite for this?

[ghost]

shouldn't this be true?

[rgrinberg]

Hmm, why is that? If we only have 1 value then we aren't multivalued.

[ghost]

Currently this function is the same as this one:

let is_multivalued _ = false

[rgrinberg]

Ah yes. Oops.

[ghost]

I think some of the cases we could represent with the previous types didn't make much sense in practice, so there probably never appear.

BTW, the error messages in strings_with_vars need to be updated. What about:

Variable <var> expands to <N> values, however a single value is expected here. Please quote this atom.

[rgrinberg]

@diml I've added the length to the error message, though I'm not sure the boilerplate necessary to do it is worth it.

[ghost]

What about getting rid of is_multivalued and keeping only length? Then it should be the same amount of boilerplate as before.

[rgrinberg]

is_multivalued can be faster than length. I suppose it doesn't matter much here.

[ghost]

I don't think it matters indeed since it's only going to be slow in error cases.

[rgrinberg]

Why's that? we checked for multivalued-ness for every valid substitution.

[rgrinberg]

I've implemented the desired behavior for actions by adding a ~allow_multivalue flag to the expansion function. The alternative is to check if the result is an Expansion. But this is a bit annoying as we don't have access to the same error message anymore.

[ghost]

Why's that? we checked for multivalued-ness for every valid substitution.

the only place where we call is_multivalued is the following:

    if not t.quoted && V.is_multivalued x then
      Loc.fail t.loc ...

We can only go in the error case if the list is not of length 1. And if the list is of length 1, then V.length x is as fast as V.is_multivalued.

[ghost]

String_with_vars.Expand_to is now applied only once, and String_with_vars.{,partial_}expand are never used. At this point it is no longer necessary to keep the separation between string_with_vars and variable expansions.

I propose the following API for String_with_vars (without functors):

module Mode : sig
  type 'a t =
    | Single : Value.t t
    | Many   : Value.t list t
end

val expand
  :  t
  -> mode:'a Mode.t
  -> dir:Path.t
  -> f:(Loc.t -> string -> Value.t list option)
  -> 'a

module Expand_partial : sig
  type nonrec 'a t =
    | Expansion  of 'a
    | Unexpanded of t
end

val partial_expand 
  :  t
  -> mode:'a Mode.t
  -> dir:Path.t
  -> f:(Loc.t -> string -> Value.t list option)
  -> 'a Expand_partial.t

where Value is a new module with this API:

type t =
  | String of string
  | Path of Path.t

and later we'll add Version of string to Value.t.

Overall this will simplify the code and implement the desired semantic in a more obvious way.

[rgrinberg]

@diml that looks good, I will work towards that. Is it desirable that Value.t list supports returning a mix of strings and paths? While I can imagine that being useful, we don't actually have any instances of that in the codebase.

[ghost]

One case I was thinking of is program and arguments: we want the first element to be a path but the rest to be strings.

[rgrinberg]

@diml have a look

[ghost]

what about prog_and_args_of_values?

[ghost]

Should be _::_::_. BTW, I suggest to use the same branches in expand and partial_expand. I was a bit surprised at first that there were different.

[rgrinberg]

Yeah, this is ugly. I'll try to factor it out as well.

[ghost]

I would call this function map

[rgrinberg]

Sure. Though this function is called either in Haskell for example.

[ghost]

@rgrinberg looks good. It's much cleaner than master.

I suggest that we also add Value.Dir, so that we can directly add ROOT and SCOPE_ROOT to the vars field of Super_context.t.

[ghost]

The misc test looks broken, it must have been broken during the renaming of the jbuild file to dune. Looking at it, it seems like once fixed, it is broken by this PR because it expects that (echo ${^}) behaves the same as (echo "${^}"). This case might happen in existing projects as well.

I suggest that we simply change echo to be variadic. I don't think the other actions are affected.

[rgrinberg]

@diml btw, another way to make the code more DRY would be to implement expansion in terms of partial expansion. I'm thinking that we can just try a partial expansion and modify f to simply return string_of_var in the missing cases. The only problem with that is f doesn't have access to the variable's syntax. Perhaps it's worth adding this parameter in an internal implementation of partial_expand?

[rgrinberg]

I suggest that we also add Value.Dir, so that we can directly add ROOT and SCOPE_ROOT to the vars field of Super_context.t.

I might be missing something here, why can't we just add them vars as Value.Path (like other directories).

[rgrinberg]

@diml the misc tests are fixed.

[rgrinberg]

I suggest that we also add Value.Dir, so that we can directly add ROOT and SCOPE_ROOT to the vars field of Super_context.t.

How would this work for SCOPE_ROOT? This variable depends on the scope where the variable is expanded.

[rgrinberg]

@diml done. this PR seems to build bin_prot (and the rest of the workspace) just fine.