Allow variables in pps flags

[mlasson]

This PR propose to add support for variables in the flags of the "pps" (and "pps_staged") kind of preprocessing.

The use-case I have in mind would be to pass "%{profile}" to the preprocessor which would allow it to generate a different code depending on the profile. This would be useful for preprocessors that implement code instrumentation (eg. bisect, or landmarks): they could implement the "identity preprocessor" when profile is "release" (also they could use the profile name as a scoping mechanism).

Here is an example:

(executable
  (name test)
  (preprocess (pps landmarks.ppx -- --profile "%{profile}"))
)

I made this PR to experiment with a profile-based workflow for landmarks and maybe to see if others think it sounds like a good idea. So, do not hesitate to comment or to close the PR if it happens to be a bad idea.

[nojb]

Related to #57

[ghost]

We had a similar discussion in #2020. My idea was that when declaring the ppx rewriter, we would declare what informations it wants. i.e. instead of passing --profile %{profile} everytime you use the ppx rewriter, you would instead declare once and for all when defining the landmarks.ppx rewriter that it wants to know the profile:

(library
 (public_name landmarks.ppx)
 (kind (ppx_rewriter (cookies (profile %{profile})))))

@Khady also mentioned that matching on the profile wasn't great. So I was thinking to add a new variable %{inline-tests} which could be set via env and take 3 values:

• ignored: silently drop inline tests
• disabled: keep them in the code, but make them dead code: if false then <test> to avoid warnings caused by deleting code
• enabled

The default would be enabled except in the release profile where it would be ignored. We could then pass it to ppx_inline_test via (cookies (inline-tests %{inline-tests})). We could do something similar for instrumentation.

However, the current PR still seems good to me. It's nice to be able to use variables in the arguiments of ppx rewriters.

[ghost]

It seems that we are going to do the expansion several times in this file. We should either do the expansion in make, or simply comapre the string unexpanded here. The latter seems simpler and enough.

[mlasson]

@diml Thanks for your input ! It seems indeed nicer to have it defined in the library stanzas.

I try to implement your suggestions. It was a bit boring to write the comparison functions; should I have used String.compare composed with "String_with_vars.to_string" ?

[ghost]

The way you did it is good. Unfortunately, we do have to write these boring functions at the moment. However, we are thinking of setting up something based on [@@deriving_inline] in the future.

Could you add a changelog entry and a test for this feature? You can add some variables in the preprocess field of the test_ppx_args library in test/blackbox-tests/test-cases/dune-ppx-driver-system/driver-tests/dune.

BTW, I just noticed that variables are only supported after the --, however it is possible to pass arguments before that by prefixing them with -. For instance: (pps ppx1 -arg1 -- -arg2 x). It would seem more natural to me to support them in arguments before the -- as well, so that one may write -arg1=%{x}. Could you make tihs change? Elements starting with a variable should be treated as ppx rewriter names, and ppx rewriter names with variables should be rejected.

[mlasson]

Ok, I'll try to do that on the beginning of next week.

[mlasson]

Note to the reviewer: I struggled to find a variable that is actually ... a constant in (almost) all context; that's why I used this "trick" ; the test will fail on machine that define a variable called "SOME_UNDEFINED_VARIABLE".

[ghost]

Ack. I wondered about this actually. To avoid the last pitfall, you can add the following stanza in this dune file:

(env (_ (env-vars (X foo))))

[ghost]

Small nitpick: I would do the check right at the end, on the final more_flags @ flags list. That would make the code slightly shorter.

[mlasson]

Ok !

[ghost]

The false result is slightly ambiguous here, as we don't know whether prefix is really not a prefix or wether we can't answer accurately because the string is not evaluated enough. I suggest to return something else that bool that can express this third state. The case where we don't know should be effectively handled the same way as false, but at least it's explicit when reading the code at the call site. For instance:

type is_prefix_result = Yes | No | Unknown

[mlasson]

Or I could rename the function into "is_prefix_before_expansion" ?

Note that the implementation of is_prefix is not trivial and could be replaced by the simpler

  let is_prefix {parts; _} ~prefix = function Text s :: _ -> String.is_prefix s ~prefix | [] | Var _ :: _ -> String.length prefix = 0

But it assumes that text parts are always interleaved with variables (as does the already existing "text_only" function). I actually think that this simpler implementation is better, what do you think ?

[ghost]

is_prefix_before_expansion makes me think of testing is_prefix on the original string, i.e. is_prefix_before_expansion t ~prefix = String.is_prefix (to_string t) ~prefix.

But it assumes that text parts are always interleaved with variables (as does the already existing "text_only" function). I actually think that this simpler implementation is better, what do you think ?

Agreed. That seems better to me as well. The various functions are also careful about ensuring that we never produce Text x :: Text y or Text "".


[aalekseyev]

I like the 3-way is_prefix_result better than trying to express this in the name.

Regarding canonical representation, I think it's better to not rely on canonical representation if we can help it.

Maybe a nice middle ground could be a function like this:

let get_known_prefix = 
  let rec go t acc = match t with
    | Text s :: rest -> go (s :: acc) rest
    | _ -> String.concat ~sep:"" (List.rev acc)
  in
  fun t -> go t []

(or perhaps a variation that distinguishes fully known string from partially unknown)

In fact I need something like this (for the suffix) in #2073.

[mlasson]

FYI, the is_prefix that assume the canonical representation would be:

type is_prefix_result = Yes | No | Unknown
let is_prefix t ~prefix =
  match t.template_.parts with
  | [Text s] -> if String.is_prefix s ~prefix then Yes else No
  | Text s :: _ ->
    if String.is_prefix s ~prefix then
      Yes
    else
      if String.is_prefix prefix ~prefix:s then
        Unknown
      else No
  | [] -> if String.length prefix = 0 then Yes else No
  | Var _ :: _ -> if String.length prefix = 0 then Yes else Unknown

I think I like the "get_known_prefix" better.

[mlasson]

( Btw, the final flags are "swapped": when we write "-arg1 -arg2 -- arg3 arg4" in the dune file the preprocessor will read "arg3 arg4 -arg1 -arg2" ; was it on purpose ? )

[ghost]

Hmm, no that does look like a bug.

[mlasson]

Sorry, I actually misread the order is right.

[mlasson]

It should be good now.

[ghost]

Looks good, thanks for this PR!

[ghost]

BTW, would you be happy to help with #2020? :)

I started the work in the branch ppx-cookies of github.com/diml/dune, but I have a few other things to finish on my todo list before I come back to this feature. So far I implemented the parsing of the new syntax, what's left to do is collecting the cookies, instantiating them and passing them to the driver in src/preprocessing.ml.

[mlasson]

I'll see what I can do ! (but no promise; as I have my own todo list to go through :)).

[ghost]

Sounds good :)