Ast_builder.Default.pexp_tuple on empty list

[sim642]

Ast_builder.Default.pexp_tuple, etc. currently have a special case for the one-element list:

ppxlib/src/ast_builder.ml

Lines 91 to 99 in 6baf858

	let pexp_tuple ~loc l = match l with [ x ] -> x | _ -> pexp_tuple ~loc l
	let ppat_tuple ~loc l = match l with [ x ] -> x | _ -> ppat_tuple ~loc l
	let ptyp_tuple ~loc l = match l with [ x ] -> x | _ -> ptyp_tuple ~loc l
	let pexp_tuple_opt ~loc l =
	match l with [] -> None | _ :: _ -> Some (pexp_tuple ~loc l)
	let ppat_tuple_opt ~loc l =
	match l with [] -> None | _ :: _ -> Some (ppat_tuple ~loc l)

I wonder if it makes sense to also have a special case for the empty list, degrading to () instead. Right now calling them with the empty list yields an error from deeper down that a tuple needs at least 2 elements.

Or maybe there's a reason this generalization hasn't been made? The existence _opt versions of these functions hints at the need to distinguish the empty list case.

[pitag-ha]

Thanks for the issue! I wasn't sure myself, so I've just had a look at the compiler Parsetree module. There are few Parsetree invariants that aren't enforced by the type checker. They're written down as comments in the compiler Parsetree module. There's one for Pexp_tuple: n>=2. That explains the behavior of ppat_tuple and why ppat_tuple_opt exists.

It would be quite valuable to add documentation about that in Ast_builder though. Do you want to do that?