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

not(expr) broken #2

Closed
leostera opened this issue Feb 27, 2024 · 3 comments
Closed

not(expr) broken #2

leostera opened this issue Feb 27, 2024 · 3 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Milestone

Comments

@leostera
Copy link
Collaborator

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:

  • not(expr) is equals to false if the expression is true, or true if it is false
  • not(varName) is equals to true if the varName is not defined in the environment or has no value, or false if varName has a value
@leostera leostera added bug Something isn't working help wanted Extra attention is needed good first issue Good for newcomers labels Feb 27, 2024
@leostera leostera added this to the riot/phase-2 milestone Feb 28, 2024
@KFoxder
Copy link
Contributor

KFoxder commented Mar 1, 2024

@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: #7

@KFoxder
Copy link
Contributor

KFoxder commented Mar 3, 2024

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

@leostera
Copy link
Collaborator Author

leostera commented Mar 4, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed
Projects
Status: Done
Development

No branches or pull requests

2 participants