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

Implement drop and take builtins #1287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ear7h
Copy link
Contributor

@ear7h ear7h commented Jun 18, 2022

Per #1286

I can implement this for the Haskell, Rust, and Go implementations (although it might take a while)

@ear7h
Copy link
Contributor Author

ear7h commented Jun 27, 2022

@Gabriella439 This is ready for a first pass review. I checked the hydra logs and it failed with

Error: Unbound variable: List/drop

4│       List/drop

Which I think means it's running dhall-haskell on the prelude (instead of the reference impl in this repo). How should I work around this? Also, I didn't update the hashes on the prelude, what is the command to generate them?

Aside: I feel like the answer to these questions could go in the CONTRIBUTING.md, I can add it in a separate PR.

Gabriella439 added a commit to dhall-lang/dhall-haskell that referenced this pull request Jun 28, 2022
Copy link
Contributor

@Gabriella439 Gabriella439 left a comment

Choose a reason for hiding this comment

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

I have a matching PR for the Haskell implementation of Dhall: dhall-lang/dhall-haskell#2430

Also, I'll need you to merge ear7h#1 into your PR

```dhall
List/drop n a ([] : List a) = [] : List a

List/drop n a (xs # ys) =
Copy link
Contributor

Choose a reason for hiding this comment

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

The goal here is to be more declarative, so I'd suggest something like:

List/drop n a (xs # ys) =
    List/drop n a xs # List/drop (Natural/subtract (List/length a xs) n) a ys

```dhall
List/take n a ([] : List a) = [] : List a

List/take n a (xs # ys) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, here I would suggest:

List/take n a (xs # ys) =
    List/take n a xs # List/take (Natural/subtract (List/length a xs) n) a ys

Comment on lines +1380 to +1382
f ⇥ List/drop 0 A₀ as ⇥ [ a, as… ]
────────────────────────────────────
f as ⇥ [ a, as… ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though the previous judgment handles the empty list case, there's no need to explicitly require the list to be non-empty here, or to even be a list:

Suggested change
f ⇥ List/drop 0 A₀ as ⇥ [ a, as… ]
────────────────────────────────────
f as ⇥ [ a, as… ]
f ⇥ List/drop 0 A₀ asas₁
────────────────────────────────────
f asas₁

If you omit the requirement that as₁ is a concrete list then the interpreter can do things like simplify λ(a : Type) → λ(xs : as) → List/drop 0 a xs to λ(a : Type) → λ(xs : as) → xs. In fact, that would be a useful property test to add for List/drop in the Prelude.

Comment on lines +1385 to +1387
f ⇥ List/drop n A₀ List/drop (n - 1) [ as… ] ⇥ bs
───────────────────────────────────────────────────
f [ a, as… ] ⇥ bs
Copy link
Contributor

Choose a reason for hiding this comment

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

The argument to f might not necessarily be a list until we β-reduce the list:

Suggested change
f ⇥ List/drop n A₀ List/drop (n - 1) [ as… ] ⇥ bs
───────────────────────────────────────────────────
f [ a, as… ] ⇥ bs
f ⇥ List/drop n A₀ as ⇥ [ a, as… ] List/drop (n - 1) [ as… ] ⇥ bs
─────────────────────────────────────────────────────────────────────
f as ⇥ bs

f as ⇥ [] : A₁


f ⇥ List/take 0 A₀ as ⇥ [ a, as… ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comments to List/drop

```haskell
betaNormalize (Application f as)
| Application
(Application (Builtin ListDrop) (NaturalLiteral _)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to require the argument to be a concrete Natural literal when the list is empty

Suggested change
(Application (Builtin ListDrop) (NaturalLiteral _)
(Application (Builtin ListDrop) n

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.

None yet

2 participants