jonascarpay / calligraphy

haskell source code visualizer
BSD 3-Clause "New" or "Revised" License
98 stars 13 forks source link

Incorporating TH #7

Closed PPKFS closed 2 years ago

PPKFS commented 2 years ago

So the library looks wonderful, and I have indeed read the part of the README explaining why TH expansion causes an issue. However, it appears the only times that OverlappingBounds can occur is if:

I recompiled by making OverlappingBounds on insert to silently fail and return the tree unchanged, and as far as I can tell it's still fine (or at least, it generated some nice incredibly massive graphs!). Is there some other issue with this method, perhaps with more oversight/logging/whatever than silently ignoring some things, that I'm missing?

jonascarpay commented 2 years ago

Hmm, I think you're right, I think we should be able to safely swallow OverlappingBounds errors. We could do it like this, we just explicitly keep the old definition:

diff --git a/src/Calligraphy/Phases/Parse.hs b/src/Calligraphy/Phases/Parse.hs
index 903f936..bb9296f 100644
--- a/src/Calligraphy/Phases/Parse.hs
+++ b/src/Calligraphy/Phases/Parse.hs
@@ -186,9 +186,9 @@ instance (Ord k, Semigroup v) => Semigroup (Dedup k v) where
 structure :: [RawDecl] -> Either (TreeError Loc RawDecl) (LexTree Loc RawDecl)
 structure = foldM (\ !t decl -> LT.insertWith f (rdStart decl) decl (rdEnd decl) t) LT.emptyLexTree
   where
-    f (RawDecl na ka ta sa ea) (RawDecl nb kb tb _ _)
+    f (RawDecl na ka ta sa ea) prev@(RawDecl nb kb tb _ _)
       | ta == tb && na == nb = Just (RawDecl na (ka <> kb) ta sa ea)
-      | otherwise = Nothing
+      | otherwise = Just prev

 -- | This is the best way I can find of checking whether the name was written by a programmer or not.
 -- GHC internally classifies names extensively, but none of those mechanisms seem to allow to distinguish GHC-generated names.

Do you have a snippet that currently throws OverlappingBounds errors? I'd like to add it to the test suite before introducing this fix.

jonascarpay commented 2 years ago

Also, and I think I should elaborate on this in the readme, there's nothing actually rejecting TH splices, it's just that it's not feasible to handle them correctly. What tends to reject them is that TH splices in the HIE file tend to be so messy that they trigger some rejection heuristic. For example, a makeLenses splice will expand to a single node containing both value definitions and type signatures, and we currently ignore anything that looks like a type signature.

If this fix is a way to turn some TH splices from errors into incorrect graphs, that seems like a net win to me.