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

feature: case and cond statements #8324

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Aug 2, 2023

We add a (case ...) constructor to dune_lang for actions which when expanded will reduce down to the matching branch.

We also add a (cond ...) constructor for checking blang values which will reduce down to the first true branch condition.

@Alizter Alizter requested review from emillon and rgrinberg and removed request for emillon August 2, 2023 12:26
@Alizter Alizter force-pushed the ps/branch/feature__case_and_cond_statements branch from 1b2736c to cb7c7bc Compare August 2, 2023 12:32
@rgrinberg rgrinberg removed their request for review August 2, 2023 15:17
@Alizter Alizter force-pushed the ps/branch/feature__case_and_cond_statements branch 3 times, most recently from 0029320 to 42ff9d8 Compare August 4, 2023 14:37
@Alizter Alizter requested a review from rgrinberg August 4, 2023 14:38
@Alizter Alizter force-pushed the ps/branch/feature__case_and_cond_statements branch from 42ff9d8 to b43e8b2 Compare August 4, 2023 14:39
test/blackbox-tests/test-cases/actions/cond.t Outdated Show resolved Hide resolved
File "dune", line 8, characters 4-11:
8 | ((< 3 4) (echo "3 is less than 4")))))
^^^^^^^
Error: Only the default case can be at the end.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I were consuming this error message I'd prefer to be told that the default case appears somewhere other than the end of the case statement in this instance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is trickier since we parse the list backwards to check the (reversed) first is a default case. In order to find the default case we would have to parse the rest of them. There is also no guarantee that there is even a default case.

> (alias foo)
> (action
> (case %{read-lines:version}
> (_ (echo "version is 1"))
Copy link
Collaborator

@gridbugs gridbugs Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strings in dune can be unquoted. What happens if the version file contains _? Would the _ case be treated as the default case or matching the _ string?

What if we changed the syntax so the default case doesn't require a condition at all. E.g.

(case %{read-lines:version}
 (2 (echo "version is 2"))
 (echo "version is not 2"))

Copy link
Collaborator Author

@Alizter Alizter Aug 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If version contained _ it would be compared to any other branch with pattern _. You can give such a pattern by quoting the _. For instance, here is a test I have added:


  $ cat > dune << EOF
  > (rule
  >  (alias foo)
  >  (action
  >   (case %{read-lines:version}
  >    ("_" (echo "version is _"))
  >    (_ (echo "version is default")))))
  > EOF

  $ dune build @foo
  version is default

  $ cat > version << EOF
  > _
  > EOF

  $ dune build @foo
  version is _

You second point:

This makes parsing more complicated. In fact it can event be ambiguous. Imagine you had a file called progn. Then

(progn (echo foo))

could mean the default case or the string progn.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the _ string a special value in other places in dune? As a user I would find it confusing for there to be some values which are treated differently when they are quoted since strings without spaces don't usually need to be quoted in dune.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has a precedent as a catchall value in the env stanza.

@gridbugs
Copy link
Collaborator

I'm looking at using this feature to convert opam commands with filters into dune actions as part of package management. Opam lets entire commands be included or excluded based on a boolean predicate.

For example:

build: [
  ["dune" "subst"] {dev}
  ["dune" "build" "-p" name "-j" jobs]
  ["sh" "-ex" "packaging/FreeBSD/create_package.sh"] {os = "freebsd"}
  ["sh" "-ex" "packaging/debian/create_package.sh"] {os-family = "debian"}
]

This could be translated a dune action. Note I'm using the (progn) action with no arguments as a no-op.

(progn
 (cond
  (%{dev} (run dune subst))
  (_ (progn)))
 (run dune build -p %{pkg-self:name} -j %{jobs})
 (cond
  ((= %{os} freebsd) (run sh -ex packaging/FreeBSD/create_package.sh))
  (_ (progn)))
 (cond
  ((= %{os-family} debian) (run sh -ex packaging/debian/create_package.sh))
  (_ (progn))))

For improving the ergonomics of using this feature to support commands in lockfiles translated from opam I think it would be useful to allow users to omit the default case and have the conditional action act like a no-op if none of the conditions are met. Also I think it would be useful to add a no-op action to dune so we don't have to use the (progn) hack to simulate one.

@Alizter
Copy link
Collaborator Author

Alizter commented Aug 17, 2023

@gridbugs I think defaulting to the noop will be a poor choice aimilar to the (optional) field in stanzas. Silent failures are difficult to debug. I would rather a user message get raised when there is no matching branch and allow people to default to a noop if needed.

We add a `(case ...)` constructor to dune_lang for actions which when
expanded will reduce down to the matching branch.

We also add a `(cond ...)` constructor for checking blang values which
will reduce down to the first true branch condition.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/branch/feature__case_and_cond_statements branch from b43e8b2 to 2d856a4 Compare August 19, 2023 00:57
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 19, 2023

@gridbugs I've added a test demonstrating what happens with _ as a case in case.t. I've also improved the wording of the error message you mentioned.

@Alizter
Copy link
Collaborator Author

Alizter commented Aug 22, 2023

@gridbugs Something else we can try is allowing for the omission of the default case when in a lockfile and forcing user written actions to have default cases. That way we can have the lockfile convenience without users shooting themselves in the foot.

@Alizter
Copy link
Collaborator Author

Alizter commented Sep 15, 2023

Closing this as I don't think there is any wish to continue with it. Happy to reopen if needed.

@Alizter Alizter closed this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding some conditionals in the language
2 participants