ocaml-sys / config.ml

conditional compilation via attributes for OCaml
Other
27 stars 3 forks source link

not(expr) broken #2

Closed leostera closed 6 months ago

leostera commented 6 months ago

It looks like annotating [@config (not(a = 1))] actually breaks at evaluation time with:

   $ dune clean
   $ dune build
+  File "cond_type_var_constructor.ml", line 16, characters 17-48:
+  16 |   | `KingCrimson [@config (not(made_up = true))]
+                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+  Error: Not expressions must have a single parameter
+         
+  [1]

And using a variable to test for it being unset like [@config (not(a))], failed with:

   $ dune clean
   $ dune build
+  File "cond_type_var_constructor.ml", line 16, characters 17-41:
+  16 |   | `KingCrimson [@config (not(made_up))]
+                        ^^^^^^^^^^^^^^^^^^^^^^^^
+  Error: Forms any/all/not must parenthesize its arguments
+         
+  [1]

The expected behavior is that both of these parse correctly and evaluate as follows:

KFoxder commented 6 months ago

@leostera I have been looking at this bug and noticed a few things. I am also very new to OCaml so apologies if I am off on any of it.

  1. The test in the cfg_lang_test.ml file is using an extra set of parens as compared to what is in the github issue above which is why that unit tests passes but the actual test in the module fails
  2. Even with the added parens in the code, they get stripped out when the payload reaches the Eval.eval function. Meaning [@config (not((made_up = true)))] -> not (made_up = true). I would have expected it to be not ((made_up = false)). My limited knowledge leaves me feeling like this is something more "core". Maybe perhaps because not is part of Stdlib?

I have attempted to fix this with the PR: https://github.com/ocaml-sys/config.ml/pull/7

KFoxder commented 6 months ago

This is fixed thanks to @gpetiot in #11 and added test for it in #12

leostera commented 6 months ago

Thanks a million @gpetiot and @KFoxder! Closing this now :)