Inconsistent API for `Ast_builder.pconstruct`, `econstruct`

[kwshi]

The other Ast_builder.p* and Ast_builder.e* functions, such as estring and eint and pvar, etc., take in "simple" values, making it easier to construct common patterns/expressions without too much verbosity. For instance, to create the string expression "hello", we can do

val estring : loc:Location.t -> string -> expression
estring ~loc:Location.none "hello"

significantly simplifies

{ pexp_desc = Pexp_constant (Pconst_string ("hello", None))
; pexp_loc = Location.none
; pexp_loc_stack = []
; pexp_attributes = []
}

as well as the `Ast_builder` direct equivalent

```ocaml
pexp_constant ~loc:Location.none (Pconst_string ("hello", None))

It is very nice.

---

Meanwhile, we have pconstruct (resp. econstruct), which, for some reason, takes in a whole constructor declaration, completely defeating the purpose of having a pconstruct function. As an example, create a Some x constructor pattern, the shortest way to call the current pconstruct function is

val pconstruct : constructor_declaration -> pattern option -> pattern
pconstruct
  (constructor_declaration
    ~name:{ txt = "Some"; loc = Location.none }
    ~args:[]
    ~res:None
  )
  (pvar "x")

In fact, _simply calling the explicit ppat_construct function is much more concise:

ppat_construct 
  { txt = "Some"; loc = Location.none }
  (pvar "x")

Here's a few reasons why this current behavior is not good:

• It completely defeats the purpose of having a pconstruct function, since the pconstruct call is twice as long as the direct AST construction via ppat_construct!

• Bad semantics: the ~args and ~res properties of the constructor declaration is irrelevant when constructing the match-pattern--it's simply not the right type to use! The source for Ast_builder confirms that pconstruct only uses name and loc of the constructor:

  let pconstruct cd arg = ppat_construct ~loc:cd.pcd_loc (Located.map_lident cd.pcd_name) arg
  

  hence semantically, it doesn't make sense to take in a whole constructor_declaration.

  Another semantic error here is that constructor-patterns take Longident.t identifier values, such as Http.Request_kind, but constructor-declarations are only supposed to take strings without dots, i.e. Request_kind. One can simply use a string with a . in it and pass that to constructor_declaration, but that again is not a good semantic; furthermore, the current implementation does not parse strings the way evar does, so having strings with . instead of using Longident.t actually can break things in certain cases.

• It uses inconsistent API conventions. All the other functions in Ast_builder.Default are called with a leading ~loc argument to specify the location, but pconstruct doesn't but instead steals it from the constructor_declaration implicitly via .pcd_loc. That not only means sometimes using the incorrect location (if a PPX parses a constructor declaration, stores it, and builds a constructor pattern later, the location referenced should be that of the pattern-matching construct, not the original declaration) but is also just inconsistent API.

---

The proposal: replace the current interface/implementation with

val pconstruct : loc:Location.t -> string -> pattern option -> pattern
let pconstruct ~loc name arg =
  ppat_construct 
    ~loc 
    { txt = Longident.parse name; loc }
    arg

Which would be used like

(pconstruct "Some" @@ Some (pvar "x"))

Which would be much nicer to use, and matches the other p* functions. Something similar would be done to econstruct.