Location.none is different than the compiler's

[ncik-roberts]

Both the compiler and ppxlib define Location.none as let pos = { dummy_pos with pos_fname = "_none_" } in { loc_start = pos; loc_end = pos; loc_ghost = true }.

However, ppxlib defines dummy_pos as { pos_fname = ...; pos_lnum = 1; pos_bol = 0; pos_cnum = -1 } ¹, whereas the compiler defines dummy_pos as { pos_fname = ...; pos_lnum = 0; pos_bol = 0; pos_cnum = -1 } ². pos_lnum is different.

This mismatch causes at least 1 bug in ppxlib. For example, this code that attempts to show a reasonable location in the event that an attribute's location is none will fail to detect the case where the compiler inserted the none-locationed attribute (e.g. a doc comment):

ppxlib/src/common.ml

Lines 146 to 157 in 8a0cb71

	let loc_of_attribute { attr_name; attr_payload; attr_loc = _ } =
	(* TODO: fix this in the compiler, and move the logic to omp when converting
	from older asts. *)
	(* "ocaml.doc" attributes are generated with [Location.none], which is not helpful for
	error messages. *)
	if Poly.( = ) attr_name.loc Location.none then
	loc_of_name_and_payload attr_name attr_payload
	else
	{
	attr_name.loc with
	loc_end = (loc_of_name_and_payload attr_name attr_payload).loc_end;
	}

At Jane Street, we've patched ppxlib's Location.none to be the same as the compiler's with no issues.

Footnotes

1. https://github.com/ocaml-ppx/ppxlib/blob/8a0cb7122d7d454c20d732621795d910018d1b66/src/location.ml#L10-L19 ↩

2. https://github.com/ocaml/ocaml/blob/b8f23dd5af09a3d9e6f891e2ac7f0d4baee81e1b/stdlib/lexing.ml#L25-L30 ↩

[patricoferris]

Thanks @ncik-roberts !

I'll run this through some reverse dependency checks to see if anything comes up but I think we could align with the compiler's values.