elm / compiler

Compiler for Elm, a functional language for reliable webapps.
https://elm-lang.org/
BSD 3-Clause "New" or "Revised" License
7.48k stars 658 forks source link

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

Open HuwCampbell opened 4 months ago

HuwCampbell commented 4 months ago

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.

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[bot] commented 4 months ago

Thanks for reporting this! To set expectations:

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