WIP: Adapt to simplifed subsumption proposal

[bgamari]

Cabal's IO a = HasCallStack => IO a type synonym causes quite some
trouble under the simplified subsumption proposal. Fix these cases
by eta-expanding or adding type signatures where necessary.

---

Please include the following checklist in your PR:

• Patches conform to the coding conventions.
• Any changes that could be relevant to users have been recorded in the changelog.
• The documentation has been updated, if necessary.
• If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

Please also shortly describe how you tested your change. Bonus points for added tests!

[phadej]

[244 of 244] Compiling Distribution.Simple ( Distribution/Simple.hs, /__w/cabal/cabal/dist-newstyle/build/x86_64-linux/ghc-8.6.5/Cabal-3.3.0.0/build/Distribution/Simple.o )

Distribution/Simple.hs:673:48: error:
    Not in scope: type constructor or class ‘Prelude.IO’
    Perhaps you want to add ‘IO’ to the import list in the import of
    ‘Prelude’ (Distribution/Simple.hs:60:1-17).
    |
673 |                           -> LocalBuildInfo -> Prelude.IO ()
    |                                                ^^^^^^^^^^

Distribution/Simple.hs:695:31: error:
    Not in scope: type constructor or class ‘Prelude.IO’
    Perhaps you want to add ‘IO’ to the import list in the import of
    ‘Prelude’ (Distribution/Simple.hs:60:1-17).
    |
695 |                            -> Prelude.IO HookedBuildInfo
    |                               ^^^^^^^^^^

Distribution/Simple.hs:704:36: error:
    Not in scope: type constructor or class ‘Prelude.IO’
    Perhaps you want to add ‘IO’ to the import list in the import of
    ‘Prelude’ (Distribution/Simple.hs:60:1-17).
    |
704 |                    -> Args -> a -> Prelude.IO HookedBuildInfo
    |                                    ^^^^^^^^^^

[phadej]

There are few issues I can spot, but in general this is something for @ezyang to review.

[phadej]

This changes (WithCallStack)IO to Prelude.IO, and I guess this is the place where we'd like the CallStacks to propagate.

[phadej]

We have

type NoCallStackIO a = OrigPrelude.IO a

defined in Distribution.Compat.Prelude, please don't import Prelude (IO).

[phadej]

Link to proposal in description doesn't work/show.

[bgamari]

Yes, my apologies, this is still a WIP.

[phadej]

(FYI, GitHub has draft pull requests, yet the feature is a bit hidden)

[bgamari]

@ezyang, frankly, given the amount of churn this has caused I do wonder whether these CallStacks are pulling their weight. Thoughts?

[ezyang]

Well, the original introduction of CallStack was always predicated on the idea that it was actually really easy to add them, without having to modify any code at all. So if simplified subsumption means a lot of these patterns stop working, it might mean that the cost-benefit analysis has swung the other way. That being said, in the past, I have greatly appreciated having call stacks available default from IO code; it makes it a lot easier to understand what was going on when an error occurred.

[ezyang]

So, to put it more explicitly, I'd accept the version of this patch that just removes the type synonym entirely. It's cool, but it seems like more trouble than it's worth, based on this patch.

[phadej]

I removed the alias in #6552

[simonpj]

Thanks for being resonsive to this.

Well, the original introduction of CallStack was always predicated on the idea that it was actually really easy to add them, without having to modify any code at all.

I agree with this, and for the most part it remains the case. But for the use that Cabal makes of it you are really piggy-backing on GHC's implicit eta-expansion, with all the implications that has for work duplication. Often it doesn't matter, but sometimes it'll bite you. Perhaps the ice was thinner than we realised!

Anyway, all good now.

[phadej]

I think that as I removed the IO alias in #6552, this PR is better redone from scratch.

There are few non-IO related needed eta-expansions. For example

-fieldDescrParse (F m) fn = pParse <$> Map.lookup fn m
+fieldDescrParse (F m) fn = (\f -> pParse f) <$> Map.lookup fn m

but I'm not sure if there are more.

[RyanGlScott]

Here is the patch that I ended up with after building Cabal in the master branch (commit 89396ec) using the simplified subsumption branch:

diff --git a/Cabal/Distribution/FieldGrammar/FieldDescrs.hs b/Cabal/Distribution/FieldGrammar/FieldDescrs.hs
index 803ce603c..f58a918af 100644
--- a/Cabal/Distribution/FieldGrammar/FieldDescrs.hs
+++ b/Cabal/Distribution/FieldGrammar/FieldDescrs.hs
@@ -45,7 +45,7 @@ fieldDescrPretty (F m) fn = pPretty <$> Map.lookup fn m
 
 -- | Lookup a field value parser.
 fieldDescrParse :: P.CabalParsing m => FieldDescrs s a -> P.FieldName -> Maybe (s -> m s)
-fieldDescrParse (F m) fn = pParse <$> Map.lookup fn m
+fieldDescrParse (F m) fn = (\f -> pParse f) <$> Map.lookup fn m
 
 fieldDescrsToList
     :: P.CabalParsing m
diff --git a/Cabal/Distribution/Simple/Utils.hs b/Cabal/Distribution/Simple/Utils.hs
index d6e8384f1..deeaef243 100644
--- a/Cabal/Distribution/Simple/Utils.hs
+++ b/Cabal/Distribution/Simple/Utils.hs
@@ -1341,7 +1341,7 @@ withTempFileEx opts tmpDir template action =
     (\(name, handle) -> do hClose handle
                            unless (optKeepTempFiles opts) $
                              handleDoesNotExist () . removeFile $ name)
-    (withLexicalCallStack (uncurry action))
+    (withLexicalCallStack (\x -> uncurry action x))
 
 -- | Create and use a temporary directory.
 --
@@ -1356,7 +1356,7 @@ withTempFileEx opts tmpDir template action =
 withTempDirectory :: Verbosity -> FilePath -> String -> (FilePath -> IO a) -> IO a
 withTempDirectory verbosity targetDir template f = withFrozenCallStack $
   withTempDirectoryEx verbosity defaultTempFileOptions targetDir template
-    (withLexicalCallStack f)
+    (withLexicalCallStack (\x -> f x))
 
 -- | A version of 'withTempDirectory' that additionally takes a
 -- 'TempFileOptions' argument.
@@ -1367,7 +1367,7 @@ withTempDirectoryEx _verbosity opts targetDir template f = withFrozenCallStack $
     (createTempDirectory targetDir template)
     (unless (optKeepTempFiles opts)
      . handleDoesNotExist () . removeDirectoryRecursive)
-    (withLexicalCallStack f)
+    (withLexicalCallStack (\x -> f x))
 
 -----------------------------------
 -- Safely reading and writing files

I don't have the power to push this to the branch in this PR, so I leave this to you @bgamari.

[RyanGlScott]

It looks like the changes in #6545 (comment) have been packaged into a separate PR at #6563, making that PR supersede this one. I'll close this PR in favor of #6563 accordingly.