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

Allow arbitrary opam depopt disjunction represntations #11099

Closed
wants to merge 1 commit into from

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Nov 5, 2024

Opam currenly represents lists of depopts packages with disjunctions that "stack" no the left-side, such as: Or(Or(A, B), C). This code was originally written to expect disjunctions to stack on the right-side (Or(A, Or(B, C))). This change generalizes the resolving of depopts so that the representation does not matter, as long as every node is Or, Atom, or Empty.

Since we now traverse both sides of the Or node it's tricky to keep the collect function tail-recursive so the code is simplified to avoid collecting the atoms into an accumulator. Packages are expected to have a small number of depopts so there's little benefit to keeping it tail-recursive given the code complexity it would add.

Fixes #11058

Opam currenly represents lists of depopts packages with disjunctions
that "stack" no the left-side, such as: `Or(Or(A, B), C)`. This code was
originally written to expect disjunctions to stack on the right-side
(`Or(A, Or(B, C))`). This change generalizes the resolving of depopts so
that the representation does not matter, as long as every node is `Or`,
`Atom`, or `Empty`.

Since we now traverse both sides of the `Or` node it's tricky to keep
the `collect` function tail-recursive so the code is simplified to avoid
collecting the atoms into an accumulator. Packages are expected to have
a small number of depopts so there's little benefit to keeping it
tail-recursive given the code complexity it would add.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@rgrinberg
Copy link
Member

Duplicate of #11090

@rgrinberg rgrinberg marked this as a duplicate of #11090 Nov 5, 2024
@rgrinberg rgrinberg closed this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dune crashes on dune pkg lock
2 participants