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.

[patricoferris]

Hey @ncik-roberts,

Whilst writing some tests for this portion of code I was wondering under which circumstances you ran into this issue -- it seems to be fairly hard to trigger given most of the checks against Location.none are happening behind the -checks flag? The standard ocaml.doc attributes are then ignored as they are reserved? Maybe some JS libraries are manipulating doc comments? Having a concrete example would be very useful if possible?

[ncik-roberts]

Yes, I think it's simply that a user ppx was calling the loc_of_attribute mentioned in my original post and raising an error (with Location.raise_errorf or equivalent) with the returned location. I think you can repro it on the following OCaml program:

(** my doc comment *)
let x = 3

The attr_name.loc of the forged ocaml.doc attribute is the compiler's Location.none. With the current ppxlib/compiler mismatch, a ppx calling loc_of_attribute on that attribute will get a useless location (the compiler's Location.none) rather than a useful one.

[patricoferris]

Thanks @ncik-roberts ! I hadn't clocked that loc_of_attribute was exposed!