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.

Sign up for GitHub

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

Jump to bottom

Let binding cyclic value detection is stronger than top level #2322

Open
HuwCampbell opened this issue Feb 22, 2024 · 1 comment
Open

Let binding cyclic value detection is stronger than top level #2322

HuwCampbell opened this issue Feb 22, 2024 · 1 comment

Comments

@HuwCampbell
Copy link

HuwCampbell commented Feb 22, 2024 •
edited
Loading

Quick Summary:

Top level definitions allow cyclic values to be declared when the recursive call is guarded behind a lambda.

In let bindings however, they're disallowed more strongly, based on whether the binding itself has arguments. This is a different algorithm, and forbids working programs.

SSCCE

This works (as per the bad-recursion document)

import Json.Decode as Decode exposing (Decoder)

decodeComment : Decoder Comment
decodeComment =
  Decode.map4 Comment
    (Decode.field "message" Decode.string)
    (Decode.field "upvotes" Decode.int)
    (Decode.field "downvotes" Decode.int)
    (Decode.field "responses" decodeResponses)


decodeResponses : Decoder Responses
decodeResponses =
  Decode.map Responses (Decode.list (Decode.lazy (\_ -> decodeComment)))

While this doesn't

import Json.Decode as Decode exposing (Decoder)

decodeResponses : Decoder Responses
decodeResponses =
  let
    decodeComment =
      Decode.map4 Comment
        (Decode.field "message" Decode.string)
        (Decode.field "upvotes" Decode.int)
        (Decode.field "downvotes" Decode.int)
        (Decode.field "responses" resulting)

    resulting =
      Decode.map Responses (Decode.list (Decode.lazy (\_ -> decodeComment)))
  in
  resulting

Even though these definitions are semantically identical.

  • Elm: 0.19.1

Additional Details

Code in question is checkCycles in Canonicalize.Expression.

case binding of
  Define def@(Can.Def name args _) ->
    if null args then
      Result.throw (Error.RecursiveLet name (toNames otherBindings defs))
    else
      checkCycle otherBindings (def:defs)

This is overly strict and could instead use the toNodeOne and toNodeTwo approach from Canonicalize.Module.

While the above example is indeed liftable to the top level, not all let bindings are, and the alternative (to put everything in the let behind a function call, means potentially a lot of extra computation may occur.
@github-actions GitHub Actions
Copy link

Thanks for reporting this! To set expectations:

  • Issues are reviewed in batches, so it can take some time to get a response.
  • Ask questions in a community forum. You will get an answer quicker that way!
  • If you experience something similar, open a new issue. We like duplicates.

Finally, please be patient with the core team. They are trying their best with limited resources.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@HuwCampbell