upenn-cis1xx / camelot

A fully-modular OCaml style linter
https://opam.ocaml.org/packages/camelot/
Apache License 2.0
44 stars 5 forks source link

False positive warning on nested if statements #85

Open ksromanov opened 2 years ago

ksromanov commented 2 years ago

Describe the bug Camelot does not recognize else if ... pattern, which is widely used to chain conditional expressions. This pattern does not reduce readability of the code.

To Reproduce

let name_of_the_winter_month month =
   if month == 0 then
      "January"
   else if month == 1 then
      "February"
   else if month == 11 then
      "December"
   else if month < 5 && month > 0 then
      "It is spring!"
   else
      "Unknown"

Expected behavior Camelot should recognize this pattern (chained else if) and issue no warning.

Desktop (please complete the following information):

ksromanov commented 2 years ago

Perhaps something like

        | Pexp_ifthenelse (_, bthen, Some (Pexp_ifthenelse  _) as belse) ->
          if depth = 1 then true else
            find_nesting ((skip_seq_let bthen).pexp_desc) (depth - 1) ||
            find_nesting ((skip_seq_let belse).pexp_desc) depth

before https://github.com/upenn-cis1xx/camelot/blob/master/lib/style/verbose.ml#L47 should work.

hongchangwu commented 2 years ago

On a related note, sometimes it's desirable to write cascading match statements, such as

let output =
  match do_something input with
  | Some output -> output
  | None ->
  match fallback1 input with
  | Some output -> ouput
  | None ->
  match fallback2 input with
  | Some output -> output
  | None -> failwith "Giving up"

I consider this to be idiomatic and more readable than alternatives such as using higher-order functions or introducing helper functions. Unfortunately Camelot flags this as match statements nested too deep.