Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: cannot handle some functions with optional module parameters #1201

Closed
cbarcenas opened this issue Dec 27, 2019 · 1 comment · Fixed by #1212
Closed

Bug: cannot handle some functions with optional module parameters #1201

cbarcenas opened this issue Dec 27, 2019 · 1 comment · Fixed by #1212
Assignees

Comments

@cbarcenas
Copy link

ocamlformat seems to have an issue processing certain let-bindings that contain first-class modules as optional parameters.

Specifically, it fails when encountering a let-binding that takes an optional module argument with a default value, such as in scenario 2 below.

Let-bindings containing optional module parameters without a default value, such a scenario 1, are unaffected. Let-bindings containing non-optional module parameters are also fine.

Error output

ocamlformat version 0.12 with default configuration

ocamlformat: Cannot process "example.ml".
  Please report this bug at https://github.com/ocaml-ppx/ocamlformat/issues.
  BUG: generating invalid ocaml syntax.

Reproduction steps

module type X_typ = sig
  val f : int -> int
end

module X_add_one : X_typ = struct
  let f = (+) 1
end

(* Scenario 1: No issue with no default value *)
let helper ?x = match x with
  | Some (module X:X_typ) -> X.f
  | None                  -> X_add_one.f

(* Scenario 2: Fails with "BUG: generating invalid ocaml syntax." *)
let helper ?x:((module X)=(module X_add_one:X_typ)) = X.f

let () = Printf.printf "%d\n" (helper 41)
@cbarcenas
Copy link
Author

I patched ocamlformat to print the malformed formatted code and found the offending line:

-let helper ?x:((module X)=(module X_add_one:X_typ)) = X.f
+let helper ?x:(module X = (module X_add_one : X_typ)) = X.f

ocamlformat should not be removing the parentheses (...) surrounding module X, as these are required.

cbarcenas added a commit to cbarcenas/ocamlformat that referenced this issue Jan 3, 2020
Previously, some module patterns would not be parenthesized.
This was a bug because module patterns must always be
parenthesized per the language spec (8.6).

Fixes ocaml-ppx#1201
cbarcenas added a commit to cbarcenas/ocamlformat that referenced this issue Jan 3, 2020
Always use Ast.parenze_pat to decide whether to
parenthesize patterns appearing in function arguments.

Previously, patterns appearing in optional arguments with
default values would never be parenthesized. This was a bug
because some patterns (e.g. module patterns) must always be
parenthesized per the language spec.

Fixes ocaml-ppx#1201
@gpetiot gpetiot self-assigned this Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants