compiler icon indicating copy to clipboard operation
compiler copied to clipboard

Import cycle error shows odd import cycles

Open jfmengels opened this issue 4 years ago • 3 comments

Quick Summary: The import cycle that the Elm compiler sometimes prints does not correspond to the real import cycle.

Here's an example from one project I work on (closed-source, so I renamed the modules)

    ┌─────┐
    │    ABC
    │     ↓
    │    BCD
    │     ↓
    │    CDE
    │     ↓
    │    DEF
    │     ↓
    │    EFG
    │     ↓
    │    FGH
    │     ↓
    │    GHI
    │     ↓
    │    HIJ
    │     ↓
    │    IJK
    │     ↓
    │    JKL
    │     ↓
    │    KLM
    └─────┘

but the real import cycle is actually

    ┌─────┐
    │    ABC
    │     ↓
    │    BCD
    │     ↓
    │    CDE
    └─────┘

And there are even false "steps" in the reported one, since EFG does not actually import FGH. In this example, the real cycle is the "start" of the reported cycle, but I don't know if this is always the case.

I have seen this problem reported numerous times on Slack, but it doesn't look like anyone ever made an issue for it. Everytime I saw this mentioned on Slack, there was indeed a cycle, so it's not like there is a false positive, but the compiler misreports the details of the cycle.

SSCCE

I have not yet been able to create a SSCCE. I'll try to do so again later. It's hard to construct such a problematic example from scratch without knowing how the compiler might be confused.

  • Elm: 0.19.1
  • Operating System: Linux Ubuntu

Additional details

Import cycles can be tricky to solve, and having a wrong import cycle displayed to you makes it harder to solve the problem because it's not clear where the cycle can be broken.

jfmengels avatar Feb 24 '21 07:02 jfmengels

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.

github-actions[bot] avatar Feb 24 '21 07:02 github-actions[bot]

SSCCE:

Main.elm

module Main exposing (..)
import A
import Html exposing (text)
fun = String.fromInt
main =  text (A.fun 1)

A.elm

module A exposing (fun)
import B
import Main
fun = Main.fun 

B.elm

module B exposing (..)
import Main
fun = Main.fun

Compiling any of the above files reports A -> B -> Main as the cycle.

An interesting thing is that if Main is imported before B in A.elm the reported cycle is A -> Main -> B which does not make sense because Main doesn't import B. The same effect can be obtained by renaming module B to Z so that it end up after Main with automatic elm-format.

LATER EDIT: The reported issue might be either a valid longer cycle OR some wrong cycle like the one reported when switching the import modules above.

One could also argue that the A -> B-> Main cycle is triggered by dead code (import B is not used in module A) and should not be reported but this not relevant to the reported issue.

pdamoc avatar Feb 24 '21 08:02 pdamoc

As I mentioned on Twitter, elm-review now reports a shorter import cycle than the compiler.

@evancz explained in this thread why the compiler may give overly complex cycles, and suggested a way to get the smallest cycle available.

I just implemented that (released in the Elm package jfmengels/elm-review v2.3.10), and it seems to work well. It is a bit hard to test because of the lack of visibility of when a strongly connected components would occur.

This is how we go about detecting import cycle in elm-review:

  • We try to get the get the topologicalSort of the module list, which fails if there is a cycle
  • When we detect a cycle, we only have the problematic edge. We do a cycle search starting from that edge
  • Before: We would report the found cycle
  • After: For every module in that cycle, we do a new breadth-first search for a cycle and we take the shortest one out of all of those, which we then report

I added special check for self-importing modules because of how (I understand) the elm-community/graph API works, but that will likely be implementation-dependent. Also, maybe my implementation is simplifiable but at this point in time I am happy with the result.

I hope this is useful.

jfmengels avatar Mar 06 '21 16:03 jfmengels