Ast_pattern: augment API with ebool, pbool helper, and a new map.

[Burnleydev1]

fixes #399

• Write a function map_value
• Use the map_value function to turn a (bool, 'c, 'd) Ast_pattern.t into a (string, c, d) Ast_pattern.t
• Combine pexp_construct, lident and the value previously created to define ebool and pbool

[panglesd]

The map_value and map_value' look good!

The second bullet point is almost good: bool_of_string raises Invalid_argument when the given string is not "true" or "false", and that is supposed to happen everytime the pattern fails to match.
You can take inspiration from the some function for instance, for what happens when a pattern fails to match.

You don't need to "implement" Lident: it is already implemented in ast_pattern_generated.ml. You just need to combine pexp_construct, lident, the value you've called ebool in this PR, and none, to create the ebool we want.

[Burnleydev1]

Hello @panglesd, what if I use the bool_of_string_opt since it has the option which is what I think is what is added by the some function, will it do the job, because bool_of_string_opt says that it does this ( Convert the given string to a boolean. Return None if the string is not "true" or "false")

[Burnleydev1]

But it just hit me that ebool is supposed to be (bool, 'a, 'b) t -> (expression, 'c, 'd) t and not (bool option, 'a, 'b) t -> (expression, 'c, 'd) t

[panglesd]

You could have used bool_of_string_opt and matched on the result to know if it has worked or not. However, this function is only available since OCaml 4.05, and ppxlib supports OCaml starting from 4.04!

However, those functions are easy to reimplement.

[Burnleydev1]

Hello @panglesd,please I looked at some and came did something like this

let bool' (T f0) =
  T
    (fun ctx loc x k ->
      match x with
      | Some x0 ->
          ctx.matched <- ctx.matched + 1;
          let k = f0 ctx loc (bool_of_string x0) k in
          k
      | _ -> fail loc "true")

which has the signature (bool, 'a, 'b) t -> (label option, 'a, 'b) t
Please I wish to ask if I am in the right direction.
I'm sorry for asking too many questions.

[panglesd]

You don't need to take so much of the some function: just look at the branch where the given value is not a Some _.

[Burnleydev1]

Hello @panglesd, please i am having issues identifying the branch where the given value is not a Some _
please is it the | _ -> fail loc "Some")?

[Burnleydev1]

please is there documentation on this, please I would like to read it and understand some and other functions.

[panglesd]

let some (T f0) =           (* some takes as input a pattern [T f0] (supposed to match the inside of the [Some _]*)
  T
    (fun ctx loc x k ->     (* it returns a pattern which: *)
      match x with          (* match whether [x] is [Some _] or [None]: *)
      | Some x0 ->                   (* - in the [Some _] case it *)
          ctx.matched <- ctx.matched + 1;
          let k = f0 ctx loc x0 k in (*    applies the pattern given as input to the value enclosed in the [Some] *)
          k
      | _ -> fail loc "Some"         (* - otherwise, it fails with location [loc], saying that it expects a [Some] (see the definition of [fail] *))

[Burnleydev1]

Hello @panglesd, thanks for explaining how some functions, I tried using the method where we explicitly contain what happens when its "true" and "false", describing what should do when fails. The string I passed in fail is not correct yet but I just wanted you to checkout this new definition of bool'

[panglesd]

Yes, that's perfect! You now have a function turning a (bool, 'c, 'd) Ast_pattern.t into a (string, c, d) Ast_pattern.t, the task 2. in your first post. (note that you did not use the map function, but that's not a problem of course)

[Burnleydev1]

Hello @panglesd, I am trying to carry out step 3 but I'm facing some difficulties, I took inspiration from this function below since it is the only place in Ast_pattern when pexp_construct and Lident are used

let rec parse_elist (e : Parsetree.expression) acc =
  Common.assert_no_attributes e.pexp_attributes;
  match e.pexp_desc with
  | Pexp_construct ({ txt = Lident "[]"; _ }, None) -> List.rev acc
  | Pexp_construct ({ txt = Lident "::"; _ }, Some arg) -> (
      Common.assert_no_attributes arg.pexp_attributes;
      match arg.pexp_desc with
      | Pexp_tuple [ hd; tl ] -> parse_elist tl (hd :: acc)
      | _ -> fail arg.pexp_loc "list")
  | _ -> fail e.pexp_loc "list"

and started with something like this

let ebool  = Pexp_construct ({txt = Lident "true"; _} ,None) 

but I am getting errors like Some record fields are undefined: loc
so I changed it to

let ebool  = Pexp_construct ({txt = Lident "true"; loc = Location.none} ,None) 

which gives no errors
but then ebool here has just the expression_desc signature instead of (bool, 'a, 'b) t -> (expression, 'c, 'd) t that is expected
I am lost on how to proceed here.

[Burnleydev1]

I tried using the method from this documentation as well to get something like this

let ebool (parse_res : (bool, 'a, 'b) t) : (expression, 'a, 'b) t   = 
  Pexp_construct ({txt = Lident bool'; loc = Location.none} ,None) 

but it did not work either.
as I got the errors Error (warning 27): unused variable parse_res. and
longident_loc * expression option -> expression_desc This variant expression is expected to have type (expression, 'a, 'b) t There is no constructor Pexp_construct within type t

[Burnleydev1]

Lident converts (label, 'a, 'b) t -> (longident, 'a, 'b) t
pexp_construct converts (longident, 'a, 'b) t -> (expression option, 'b, 'c) t -> (expression, 'a, 'c) t

and bool' converts (bool, 'a, 'b) t -> (label, 'a, 'b) t
ebool needs to convert (bool, 'a, 'b) t -> (expression, 'a, 'b) t
I am currently working on trying to link them

[Burnleydev1]

Hello @panglesd, I think I finally got it, I have pushed the changes here.

[panglesd]

Very nice! Thank you @Burnleydev1!

Why did you decide to include bool' in the .mli file? and why naming it like this?

[Burnleydev1]

@panglesd, I noticed that other variables in the .ml had their signature in the .mli file and I named it like that because I noticed that each variable eg map has a map’ and int32 also has int32’.I don’t know why that’s the case but I just decide that bool should also have a bool
’

[Burnleydev1]

And I noticed that the map’ was the map but with location

[Burnleydev1]

I just saw how other vals were defined and used that as a template to define these ones but I can remove bool’ if it is not needed

[Burnleydev1]

Or change it to something more conventional

[Burnleydev1]

But the main reason is that bool already existed and I did not know a better name, I removed it from .mli and it still built without any errors, so I will push a commit for the changes

[Burnleydev1]

@panglesd, could we name it bool_l indicating that it converts bool to label?

[ceastlund]

If the existing helpers are called <type>', then I think it's sensible for this feature to continue that, rather than introduce a competing idiom. I agree the idiom is not great, but I think we should fix it wholesale in a separate PR rather than make it this PR's responsibility.

[panglesd]

Yes, bool' is coherent with other naming!
For some reason I missed the int32', ... convention, I only knew the map', ... convention (which is for "with location" variants).

With the removal of ebool' from the mli, It's now perfect, and ready to be merged!

[Burnleydev1]

Thanks @panglesd, @ceastlund for the review.