-write produces syntax error in OCaml w.r.t. destructive substitution on module-items and tuple pattern-matching

[ELLIOTTCABLE]

Given this input (a destructive substitution of a module-item):

module Frequency : Wrap.S with type u := int = struct
  exception UnknownFrequency of int

  type t =
    | Daily
    | Monthly

  (* TODO: Right now billing only supports 3, 5, and 7. Fix this once billing is updated. *)
  let wrap = function
    | 1 -> Daily
    | 30 -> Monthly
    | n -> raise (UnknownFrequency n)

  let unwrap = function
    | Daily -> 1
    | Monthly -> 30
end

… Reanalyze v2.23.0 invoked with -write produces this (syntactically invalid, and also nonsensical in this case hahaha) output:

module Frequency : Wrap.S with type u := int [@@dead "Frequency.+unwrap"]  = struct
  exception UnknownFrequency of int

  type t =
    | Daily
    | Monthly

  (* TODO: Right now billing only supports 3, 5, and 7. Fix this once billing is updated. *)
  let wrap = function
    | 1 -> Daily
    | 30 -> Monthly
    | n -> raise (UnknownFrequency n) [@@dead "Frequency.+wrap"] 

  let unwrap = function
    | Daily -> 1
    | Monthly -> 30 [@@dead "Frequency.+unwrap"] 
end

Note that [@@dead "Frequency.+unwrap"] appears twice, in both the correct and nonsensical locations. A bug perhaps? (=

---

Similar issues when pattern-matching on a tuple:

let target, _group, filters = populate_mapping target group filters

... becomes ...

let target, _group [@@dead "Mappings.+_group"] , filters = populate_mapping target group filters

---

All told, -write dropped a few dozen syntax-errors across our codebase. Not the biggest deal, except I'm not sure how to correctly annotate these values so the next -write dosen't simply re-add them …

[ELLIOTTCABLE]

An additional one:

module Mappings = struct
  module For_fields = struct
    let all_fields, all_fields_agg =
      let filters = Instances_mapping.Mapping.create () in
      let group = Instances_mapping.Mapping.create () in
      let filters_for_agg = Instances_mapping.Mapping.create () in
      let target = Instances_mapping.Mapping.create () in
      let target_for_agg = Instances_mapping.Mapping.create () in
      let dst_page_info = Instances_mapping.Mapping.create () in
      let dst_page_info_for_agg = Instances_mapping.Mapping.create () in
      let () = Pages_mapping.populate_mapping target dst_page_info group filters in
      let () = Pages_mapping.populate_mapping target_for_agg dst_page_info_for_agg group filters_for_agg in
      let filters = Instances_mapping.Mapping.union filters dst_page_info in
      let all_fields = Instances_mapping.Mapping.(union filters @@ union target dst_page_info) in
      let all_fields_agg = Instances_mapping.Mapping.(union filters_for_agg target_for_agg) in
      all_fields, all_fields_agg
  end

  (* ... *)
end

… becomes the nonsensical (the annotations appear mis-located, oddly?) …

module Mappings = struct
  module For_fields = struct
    let all_fields, all_fields_agg =
      let filters = Instances_mapping.Mapping.create () in
      let group = Instances_mapping.Mapping.create () in
      let filters_for_agg = Instances_mapping.Mapping.create () in
      let target = Instances_mapping.Mapping.create () in
      let target_for_agg = Instances_mapping.Mapping.create () in
      let dst_page_info = Instances_mapping.Mapping.create () in [@@dead "Mappings.For_fields.+all_fields"] 
      let dst_page_info_for_agg = Instances_mapping.Mapping.create () in [@@dead "Mappings.For_fields.+all_fields_agg"] 
      let () = Pages_mapping.populate_mapping target dst_page_info group filters in
      let () = Pages_mapping.populate_mapping target_for_agg dst_page_info_for_agg group filters_for_agg in
      let filters = Instances_mapping.Mapping.union filters dst_page_info in
      let all_fields = Instances_mapping.Mapping.(union filters @@ union target dst_page_info) in
      let all_fields_agg = Instances_mapping.Mapping.(union filters_for_agg target_for_agg) in
      all_fields, all_fields_agg
  end

  (* ... *)
end

Edit: In fact, this file reproducibly results in even weirder errors — not just content attached to the wrong node (like the above), but content appearing in the middle of words. Something's badly wrong with -write:

      let () = Pages_mapping.populate_mapping target dst_page_info
        group fi [@@dead "Mappings.For_filters.+filters_for_agg"] lters in

I can't really dump the entire file's contents in public — it's a large one, and we've had recent issues with code-sharing outside the org; pretty sure that'd get me fired. 😅 Let me know if you'd like a full copy as to use as a test-case somewhere private, though.

[cristianoc]

The ocaml annotations are best effort. Not guaranteed to work in all cases. In general, it might help to format the code first.

[ELLIOTTCABLE]

nods we use ocamlformat, if that's helpful.

Some of these seem lower-hanging than others; some of the above I am able to massage with a janky sed, especially the pattern-matching / multi-declarations simply need parenthesis, and to use the single-@ for algebraic categories:

-let target, _group [@@dead "Mappings.+_group"] , filters = populate_mapping target group filters
+let target, (_group [@dead "Mappings.+_group"]), filters = populate_mapping target group filters

The doubling-up of dumping that applies to the last item of a module that uses destructive substitution seems like it'd be similarly easy to catch; but I'm a little more worried about the really weird location-tracking bug in my latest comment. Haven't looked at the implementation yet, but that smells strongly to me of the kind of bug that's sucked down weeks of my life in the past. 😅

(No promises, we're all busy of course; but if this is just simply never going to be a priority for you - are you feeling likely to accept PRs that improve -write?)

[cristianoc]

The -write option is intended as an experiment for automatic dead code elimination together with a specific ppx. Also, the new development of reanalyze has moved into the rescript-vscode extension. What's left here is for existing OCaml users (which includes some of the ReScript tooling), and no big changes are planned for the future. @ELLIOTTCABLE If there's interest in providing PRs for -write on OCaml files, that's great. The logic is pretty much self contained and can be found starting from DeadCommon.WriteDeadAnnotations.write ());.