Inline transformations do not remove odoc comments

[jaymody]

The following:

[@@@expand_inline
(** i am bar *)
let bar = 10
]
(** i am foo *)
let foo = 10
[@@@end] 

Would get corrected to:

[@@@expand_inline
(** i am bar *)
let bar = 10
]
(** i am foo *)
let bar = 10 [@@ocaml.doc {| i am bar |}]
[@@@end] 

The problem is that the location of structure/signature items do not include the location of attached odoc comments via (** *). Consequently, when ppxlib is deciding which part of the source code to overwrite in a correction file, this ends up not including odoc comments. Note, this is not a bug with [@@@expand_inline] specifically, but for any transformation that registers a correction file (however, this would be extremely unlikely to occur with [@@deriving_inline] unless the user was doing some really funky stuff). A more likely scenario for how one might encounter this bug:

1. You wrap some existing code inside an [@@@expand_inline] block.
2. You copy that same code into the payload of [@@@expand_inline].
3. You accept the correction and you get something like the above.

Arguably, this is a parser bug. Structure and signature items should include the locations of any attached odoc comments. That being said, this bug is really rare to come across and not urgent to fix imo. I thought I'd just make an issue here to document it.

[NathanReb]

Good catch!

Just to make sure I got this right, what happens here is that we register a replacement with a location that does not include the doc comment location, meaning Reconcile just copies the doc comment from the source and only replace the item. Does that sound correct? A possible fix would then be to merge the location of the item with the doc comment one (which I assume would be available as the attribute's location) in Code_matcher when registering the replacement.

[jaymody]

Yep, that sounds right. One quick fix (which we are using at Jane Street) is to, as you said, merge the location of the item with the doc comment:

let merge_locs loc1 loc2 ~loc_ghost =
  let loc_start = Location.min_pos loc1.loc_start loc2.loc_start in
  let loc_end = Location.max_pos loc1.loc_end loc2.loc_end in
  { loc_start; loc_end; loc_ghost }

let get_doc_comments_loc_bounds =
  object
    inherit [location option] Ast_traverse.fold as super

    method! attribute attr acc =
      let acc = super#attribute attr acc in
      match acc with
      | Some loc when Option.is_some (get_odoc_contents_if_comment attr) ->
        Some (merge_locs loc attr.attr_loc ~loc_ghost:true)
      | _ -> Some attr.attr_loc
  end

let update_locs_to_include_doc_comments =
  object
    inherit Ast_traverse.map as super

    method! structure_item item =
      let item = super#structure_item item in
      match get_doc_comments_loc_bounds#structure_item item None with
      | Some loc ->
        { item with
          pstr_loc =
            merge_locs
              ~loc_ghost:item.pstr_loc.loc_ghost
              item.pstr_loc
              loc
        }
      | None -> item

    method! signature_item item =
      let item = super#signature_item item in
      match get_doc_comments_loc_bounds#signature_item item None with
      | Some loc ->
        { item with
          psig_loc =
            merge_locs
              ~loc_ghost:item.psig_loc.loc_ghost
              item.psig_loc
              loc
        }
      | None -> item
  end

And then in code_matcher.ml, we can run let source = update_locs_to_include_doc_comments#structure source.

I think, however, the actual issue is that I would expect the parser itself to include the location of doc comments in the item location. Ideally, we wouldn't ever need to worry about this in ppxlib.