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

Support for list recombinations #568

Merged
merged 3 commits into from
Jan 26, 2024
Merged

Support for list recombinations #568

merged 3 commits into from
Jan 26, 2024

Conversation

AltGr
Copy link
Contributor

@AltGr AltGr commented Jan 25, 2024

The primary use-case for this was to be able to run computations on a list
of structures, then return an updated list with some fields in the
structures modified : that is what we need for distribution of tax amounts
among household members, for example.

This patch has a few components:

  • Addition of a test as an example for tax distributions (and tests for various
    cases of the below desugaring)

  • Internally added a Map2 operator that is used as backend for the following

  • Added a transformation, performed during desugaring, that -- where lists
    are syntactically expected, i.e. after the among keyword -- turns a
    (syntactic) tuple of lists into a list of tuples ("zipping" the lists)

  • Arg-extremum transformation was also fixed to use an intermediate list
    instead of computing the predicate twice

  • For convenience, allow to bind multiple variables in most* list
    operations (previously only let in and functions allowed it)

  • Fixed the printer for tuples to differentiate them from lists

*Note: tuples are not yet allowed on the left-hand side of filters and
arg-extremums for annoying syntax conflict reasons.

@AltGr
Copy link
Contributor Author

AltGr commented Jan 26, 2024

(note: CI shows red because of a quirk that made it attempt to publish its artifacts, but tests actually passed)

The primary use-case for this was to be able to run computations on a list of
structures, then return an updated list with some fields in the structures
modified : that is what we need for distribution of tax amounts among household
members, for example.

This patch has a few components:

- Addition of a test as an example for tax distributions

- Added a transformation, performed during desugaring, that -- where lists are
  syntactically expected, i.e. after the `among` keyword -- turns a (syntactic)
  tuple of lists into a list of tuples ("zipping" the lists)

- Arg-extremum transformation was also fixed to use an intermediate list instead
  of computing the predicate twice

- For convenience, allow to bind multiple variables in most* list
  operations (previously only `let in` and functions allowed it)

- Fixed the printer for tuples to differentiate them from lists

*Note: tuples are not yet allowed on the left-hand side of filters and
arg-extremums for annoying syntax conflict reasons.
Copy link
Contributor

@denismerigoux denismerigoux left a comment

Choose a reason for hiding this comment

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

Thank you @AltGr !!

m
in
zip ls
| e -> rec_helper e
Copy link
Contributor

Choose a reason for hiding this comment

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

No error message at this point ? Does this mean that the error will be raised later during typing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No it's the correct case for non-tuple arguments

Var.make (String.concat "_" (List.map Mark.remove param_names))
in
let x = Expr.evar v emark in
let tys = List.map (fun _ -> TAny, pos) param_names in
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way to be smarter than TAny here or in the other instances of changes in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, remember this is desugaring so there is very little type information available, except for top-level definitions. We do enforce tuples of the correct length though.

@@ -353,6 +353,7 @@ module Oper : sig
val o_xor : bool -> bool -> bool
val o_eq : 'a -> 'a -> bool
val o_map : ('a -> 'b) -> 'a array -> 'b array
val o_map2 : ('a -> 'b -> 'c) -> 'a array -> 'b array -> 'c array
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing comment : raises NotSameLength

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed!

@@ -0,0 +1,65 @@
> Using Prorata_external as Ext
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is Prorata_external defined ? Is this really how you're supposed to use this builtin? Will it be the same for other builtins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a built-in at the moment, it's an external defined in tests/test_modules/prorata_external.{catala_en,ml}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(far from perfect but there is an HOWTO about how to handle it in its current state: https://github.com/CatalaLang/catala/blob/master/doc/devel/externals.md)

@AltGr AltGr merged commit 2d8bdca into CatalaLang:master Jan 26, 2024
5 checks passed
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.

2 participants