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

Make matchList lazy and add a strict version alongside #6059

Closed
effectfully opened this issue May 20, 2024 · 3 comments
Closed

Make matchList lazy and add a strict version alongside #6059

effectfully opened this issue May 20, 2024 · 3 comments

Comments

@effectfully
Copy link
Contributor

On current master in the builtinListIndexing.pir.golden test we have the following:

        chooseList
          {data}
          {Unit -> Unit -> data}
          xs
          (\(ds : Unit) -> error {Unit -> data})
          (\(ds : Unit) (ds : Unit) ->
             let
               !hd : data = headList {data} xs
               !tl : list data = tailList {data} xs
             in <...>

Clearly we don't need two Unit arguments, how do we get them? This code is a compiled version of

    go :: BI.BuiltinList a -> Integer -> a
    go xs i =
      Builtins.matchList
        xs
        (\_ -> traceError builtinListIndexTooLargeError)
        ( \hd tl _ -> <...>)
        ()

which makes sense, we don't want to evaluate the nil case and fail at the very first element.

But why two Units? That's because matchList has a Unit inside of it to make pattern matching on lists lazy:

matchList :: forall a r . BI.BuiltinList a -> r -> (a -> BI.BuiltinList a -> r) -> r
matchList l nilCase consCase = BI.chooseList l (const nilCase) (\_ -> consCase (BI.head l) (BI.tail l)) ()

But it doesn't make the matching actually lazy! matchList always forces nilCase, so putting () inside of matchList doesn't help with laziness as that point nilCase is already forced. So we have to do the () dance again and end up with two unit arguments, which is kinda ridiculous.

I think we should have two matchList functions instead:

matchList :: forall a r . BI.BuiltinList a -> (() -> r) -> (a -> BI.BuiltinList a -> r) -> r
matchList' :: forall a r . BI.BuiltinList a -> r -> (a -> BI.BuiltinList a -> r) -> r

one for lazy matching and one for strict matching.

@github-actions github-actions bot added the status: needs triage GH issues that requires triage label May 20, 2024
@effectfully effectfully added Good first issue and removed status: needs triage GH issues that requires triage labels May 20, 2024
@effectfully
Copy link
Contributor Author

Oh, that's already done apparently, sorry for the noise.

@effectfully
Copy link
Contributor Author

Actually, we do have the pointless double Units, so I'm reopening the issue until that is fixed.

@effectfully effectfully reopened this May 20, 2024
@github-actions github-actions bot added the status: needs triage GH issues that requires triage label May 20, 2024
@effectfully effectfully removed the status: needs triage GH issues that requires triage label Aug 8, 2024
@effectfully
Copy link
Contributor Author

Closing in favor of a subtask here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant