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

Generate dot graph to visualize import graph #698

Merged
merged 5 commits into from
Nov 26, 2018

Conversation

basile-henry
Copy link
Collaborator

This PR adds a small option to dhall resolve that outputs the dependency graph at the import level in dot format.

It is a similar feature to stack's dependency visualization but at the module/import level as opposed to the package level.

If the feature is of any interest, I'd love to improve it and make it more user friendly! 😄

Here is what it currently looks like on ./Prelude/package.dhall (in .png as GitHub doesn't seem to support .svg uploads):

dhall resolve --dot <<< "./Prelude/package.dhall" |
  dot -Tpng > dhall-prelude.png

dhall-prelude

There is currently a small issue with diamond dependencies:

diamond

./c.dhall is pointed at twice. I have some idea on how to fix that, and I'll try to implement that, but if you have some advice on how I should go about it, I'm all ears!
I'm also open to hearing all your suggestions to make the graph look better! 😄

Copy link
Collaborator

@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.

Looks great to me! Just one tiny suggestion

dhall/src/Dhall/Import.hs Outdated Show resolved Hide resolved
@basile-henry
Copy link
Collaborator Author

@Gabriel439 Is it possible the caching mechanism for import doesn't work as it should?

With:

diff --git a/dhall/src/Dhall/Import.hs b/dhall/src/Dhall/Import.hs
index 15760b5..75d0372 100644
--- a/dhall/src/Dhall/Import.hs
+++ b/dhall/src/Dhall/Import.hs
@@ -198,6 +198,8 @@ import qualified Text.Megaparsec
 import qualified Text.Parser.Combinators
 import qualified Text.Parser.Token
 
+import Debug.Trace
+
 -- | An import failed because of a cycle in the import graph
 newtype Cycle = Cycle
     { cyclicImport :: Import  -- ^ The offending cyclic import
@@ -732,7 +734,7 @@ loadWith expr₀ = case expr₀ of
     expr <- if here `elem` canonicalizeAll imports
         then throwMissingImport (Imported imports (Cycle import_))
         else do
-            case Map.lookup here _cache of
+            case Map.lookup here $ traceShowId _cache of
                 Just expr -> return expr
                 Nothing   -> do
                     -- Here we have to match and unwrap the @MissingImports@
➜ tail -n +1 -- *.dhall
==> a.dhall <==
./c.dhall

==> b.dhall <==
./c.dhall

==> c.dhall <==
./d.dhall

==> d.dhall <==
42

==> diamond.dhall <==
{ a = ./a.dhall
, b = ./b.dhall
}

➜ cabal new-run -- dhall <<< "./diamond.dhall"
Up to date
fromList []
fromList []
fromList []
fromList []
fromList [(Import {importHashed = ImportHashed {hash = Nothing, importType = Local Here (File {directory = Directory {components = []}, file = "a.dhall"})}, importMode = Code},NaturalLit 42)]                     
fromList [(Import {importHashed = ImportHashed {hash = Nothing, importType = Local Here (File {directory = Directory {components = []}, file = "a.dhall"})}, importMode = Code},NaturalLit 42)]                     
fromList [(Import {importHashed = ImportHashed {hash = Nothing, importType = Local Here (File {directory = Directory {components = []}, file = "a.dhall"})}, importMode = Code},NaturalLit 42)]                     
{ a = 42, b = 42 }

I would expect to see other files than just a.dhall in the cache. The way the imports are being traversed (depth first) isn't making it easy, but this looks like a bug to me.

What do you think?

@Gabriella439
Copy link
Collaborator

@basile-henry: Yes, it is! I think you may have found the long-sought cause of Dhall's inefficient import resolution. I have the fix up here: #702

@f-f
Copy link
Member

f-f commented Nov 23, 2018

Oh wow, nice :)

@Gabriel439 could this be the cause of #642? (I recall there was another issue like "we do a suspiciously big amount of operations when resolving imports", but cannot find it anymore)

@Gabriella439
Copy link
Collaborator

@f-f: You might be thinking of #580

@f-f
Copy link
Member

f-f commented Nov 23, 2018

No, I think it was older than that, and I think we closed it when fixing some other bug related to imports. So it looks like we don't have any issue open for inefficient imports, which is good :)

@basile-henry
Copy link
Collaborator Author

Alright, diamond dependencies handled! 😄

diamond

The code might be a bit messy as I am using the cache to store the node id I need. Any suggestions on how to improve that and keep the code maintainable?

@basile-henry
Copy link
Collaborator Author

Bonus graph dhall-to-cabal:

dhall-to-cabal

Do you think I should use relative imports in the graph? Would that make it more readable? 😄

@basile-henry
Copy link
Collaborator Author

I can do relative imports but the result is not good. The relative paths is different depending on where it's relative to which is an issue when multiple arrows point to the same import. I'll leave the PR as is for now!

Signed-off-by: Basile Henry <bjm.henry@gmail.com>
Signed-off-by: Basile Henry <bjm.henry@gmail.com>
Signed-off-by: Basile Henry <bjm.henry@gmail.com>
@basile-henry basile-henry merged commit c8dc585 into dhall-lang:master Nov 26, 2018
@basile-henry basile-henry deleted the resolve-dot branch November 26, 2018 05:24
@basile-henry basile-henry mentioned this pull request Nov 26, 2018
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

3 participants